Adjust upgrade valid check (CrunchyData#3223)

The upgradeTagValid was originally checking tags as strings
which led to 12.9=>12.10 upgrades being rejected as invalid.
This changes that validator to check certain parts of the tag
as ints rather than strings, and adds a unit test.

Issue [sc-14575]

Author: benjaminjb

internal/apiserver/upgradeservice/upgradeimpl.go

@@ -230,44 +230,61 @@ func supportedOperatorVersion(version string) bool {
 // upgradeTagValid compares and validates the PostgreSQL version values stored
 // in the image tag of the existing pgcluster CR against the values set in the
 // Postgres Operator's configuration
+// A typical example tag is `ubi8-12.9-4.7.4`, so we want to extract and
+// compare that `12.9` to make sure that we are only allowing minor upgrades.
+// For major upgrades, see PGOv5.1.
 func upgradeTagValid(upgradeFrom, upgradeTo string) bool {
 	log.Debugf("Validating upgrade from %s to %s", upgradeFrom, upgradeTo)
 
-	versionRegex := regexp.MustCompile(`-(\d+)\.(\d+)(\.\d+)?-`)
+	versionRegex := regexp.MustCompile(`-(\d+)\.(\d+)\.?(\d+)?-`)
 
 	// get the PostgreSQL version values
 	upgradeFromValue := versionRegex.FindStringSubmatch(upgradeFrom)
 	upgradeToValue := versionRegex.FindStringSubmatch(upgradeTo)
 
 	// if this regex passes, the returned array should always contain
-	// 4 values. At 0, the full match, then 1-3 are the three defined groups
-	// If this is not true, the upgrade cannot continue (and we won't want to
-	// reference potentially missing array items).
+	// 4 values:
+	// 	-At 0, the full match;
+	// 	-At 1, the major version of PG;
+	//	-At 2, the minor version of PG, which needs to be compared as ints;
+	// 	-At 3, the patch version, which can be null, but if not, should be compared as ints;
+	// (Note the `?` in the regex after the last capture group.)
 	if len(upgradeFromValue) != 4 || len(upgradeToValue) != 4 {
 		return false
 	}
 
-	// if the first group does not match (PG version 9, 10, 11, 12 etc), or if a value is
+	// if the first group does not match (i.e., the PG major version), or if a value is
 	// missing, then the upgrade cannot continue
 	if upgradeFromValue[1] != upgradeToValue[1] && upgradeToValue[1] != "" {
 		return false
 	}
 
-	// if the above check passed, and there is no fourth value, then the PG
-	// version has only two digits (e.g. PG 10, 11 or 12), meaning this is a minor upgrade.
+	// if the above check passed, and there is no patch version value, then the PG
+	// version has only two digits (e.g. PG 12.6, 12.10), meaning this is a minor upgrade.
 	// After validating the second value is at least equal (this is to allow for multiple executions of the
 	// upgrade in case an error occurs), the upgrade can continue
-	if upgradeFromValue[3] == "" && upgradeToValue[3] == "" && upgradeFromValue[2] <= upgradeToValue[2] {
+	// In order to compare correctly, these values have to be ints.
+	// Note: thanks to the regex capture, we know these second values consist of digits,
+	// so we can skip testing the error.
+
+	upgradeFromInt, _ := strconv.Atoi(upgradeFromValue[2])
+	upgradeToInt, _ := strconv.Atoi(upgradeToValue[2])
+	if upgradeFromValue[3] == "" && upgradeToValue[3] == "" && upgradeFromInt <= upgradeToInt {
 		return true
 	}
 
 	// finally, if the second group matches and is not empty, then, based on the
 	// possibilities remaining for Operator container image tags, this is either PG 9.5 or 9.6.
-	// if the second group value matches, and the third group was already validated as not
-	// empty, check that the third value is at least equal (this is to allow for multiple executions of the
+	// if the second group value matches, check that the third value is not empty and
+	// at least equal (this is to allow for multiple executions of the
 	// upgrade in case an error occurs). If so, the upgrade can continue.
-	if upgradeFromValue[2] == upgradeToValue[2] && upgradeToValue[2] != "" && upgradeFromValue[3] <= upgradeToValue[3] {
-		return true
+	if upgradeFromValue[2] == upgradeToValue[2] && upgradeToValue[2] != "" &&
+		upgradeFromValue[3] != "" && upgradeToValue[3] != "" {
+		upgradeFromInt, _ = strconv.Atoi(upgradeFromValue[3])
+		upgradeToInt, _ = strconv.Atoi(upgradeToValue[3])
+		if upgradeFromInt <= upgradeToInt {
+			return true
+		}
 	}
 
 	// if none of the above conditions are met, a two digit Major version upgrade is likely being

internal/apiserver/upgradeservice/upgradeimpl_test.go

@@ -0,0 +1,66 @@
+package upgradeservice
+
+/*
+Copyright 2021 - 2022 Crunchy Data Solutions, Inc.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestUpgradeTagValid(t *testing.T) {
+	tests := []struct {
+		fromTag  string
+		toTag    string
+		expected bool
+	}{
+		// Bad tags
+		// Too short
+		{fromTag: "ubi8-12-4.7.4", toTag: "ubi8-12.10-4.7.5", expected: false},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12-4.7.5", expected: false},
+		// Too long
+		{fromTag: "ubi8-12.9.10.3-4.7.4", toTag: "ubi8-12.10-4.7.5", expected: false},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.10.3.4-4.7.5", expected: false},
+		// Not digits
+		{fromTag: "ubi8-12.9.hello-4.7.4", toTag: "ubi8-12.10-4.7.5", expected: false},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.10.hello-4.7.5", expected: false},
+		{fromTag: "ubi8-12.hello-4.7.4", toTag: "ubi8-12-4.7.5", expected: false},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.hello-4.7.5", expected: false},
+		// Mismatched major version
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-13.10-4.7.5", expected: false},
+		{fromTag: "ubi8-14.9-4.7.4", toTag: "ubi8-13.10-4.7.5", expected: false},
+		// Patch should be absent if comparing minor values
+		{fromTag: "ubi8-12.9.3-4.7.4", toTag: "ubi8-12.10-4.7.5", expected: false},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.10.3-4.7.5", expected: false},
+		// From values higher than to values for minor or patch values
+		// Note values chosen here are 9=>10 to test that int conversion occurs
+		{fromTag: "ubi8-12.10-4.7.4", toTag: "ubi8-12.9-4.7.5", expected: false},
+		{fromTag: "ubi8-12.7.10-4.7.4", toTag: "ubi8-12.7.9-4.7.5", expected: false},
+		// Patch value partially absent
+		{fromTag: "ubi8-12.9.3-4.7.4", toTag: "ubi8-12.9-4.7.5", expected: false},
+		{fromTag: "ubi8-12.10-4.7.4", toTag: "ubi8-12.10.3-4.7.5", expected: false},
+		// Valid
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.10-4.7.5", expected: true},
+		{fromTag: "ubi8-12.9-4.7.4", toTag: "ubi8-12.9-4.7.5", expected: true},
+		{fromTag: "ubi8-12.9.9-4.7.4", toTag: "ubi8-12.9.10-4.7.5", expected: true},
+	}
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%s=>%s", test.fromTag, test.toTag), func(t *testing.T) {
+			if upgradeTagValid(test.fromTag, test.toTag) != test.expected {
+				t.Fatalf("expected %t", test.expected)
+			}
+		})
+	}
+}