fix(openshift): handle unquoted short max versions

Add special handling for version shorthand commonly used in pipelines for Red Hat operator catalogs. Allows unquoted short versions to be used when specifying an operator's compatibility with versions of OpenShift; e.g. "olm.maxOpenShiftVersion": 4.8. Without this, OLM will only recognize quoted maxOpenShiftVersion properties; e.g. "olm.maxOpenShiftVersion": "4.8". Signed-off-by: Nick Hale <njohnhale@gmail.com>
operator-framework · Jul 20, 2021 · 1ebebdb · 1ebebdb
1 parent d76b75b
commit 1ebebdb
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 29 deletions.
diff --git a/pkg/controller/operators/openshift/clusteroperator_controller_test.go b/pkg/controller/operators/openshift/clusteroperator_controller_test.go
@@ -1,6 +1,8 @@
 package openshift
 
 import (
+	"fmt"
+
 	semver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -161,7 +163,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		withMax := func(version string) map[string]string {
 			maxProperty := &api.Property{
 				Type:  MaxOpenShiftVersionProperty,
-				Value: `"` + version + `"`, // Wrap in quotes so we don't break property marshaling
+				Value: version,
 			}
 			value, err := projection.PropertiesAnnotationFromPropertyList([]*api.Property{maxProperty})
 			Expect(err).ToNot(HaveOccurred())
@@ -170,7 +172,7 @@ var _ = Describe("ClusterOperator controller", func() {
 				projection.PropertiesAnnotationKey: value,
 			}
 		}
-		incompatible.SetAnnotations(withMax(clusterVersion))
+		incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, clusterVersion))) // Wrap in quotes so we don't break property marshaling
 
 		Eventually(func() error {
 			return k8sClient.Create(ctx, incompatible)
@@ -202,7 +204,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		// Set compatibility to the next minor version
 		next := semver.MustParse(clusterVersion)
 		Expect(next.IncrementMinor()).To(Succeed())
-		incompatible.SetAnnotations(withMax(next.String()))
+		incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, next.String())))
 
 		Eventually(func() error {
 			return k8sClient.Update(ctx, incompatible)
@@ -216,5 +218,32 @@ var _ = Describe("ClusterOperator controller", func() {
 			Status:             configv1.ConditionTrue,
 			LastTransitionTime: fixedNow(),
 		}))
+
+		By("understanding unquoted short max versions; e.g. X.Y")
+		// Mimic common pipeline shorthand
+		v := semver.MustParse(clusterVersion)
+		short := fmt.Sprintf("%d.%d", v.Major, v.Minor)
+		incompatible.SetAnnotations(withMax(short))
+
+		Eventually(func() error {
+			return k8sClient.Update(ctx, incompatible)
+		}).Should(Succeed())
+
+		Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
+			err := k8sClient.Get(ctx, clusterOperatorName, co)
+			return co.Status.Conditions, err
+		}, timeout).Should(ContainElement(configv1.ClusterOperatorStatusCondition{
+			Type:   configv1.OperatorUpgradeable,
+			Status: configv1.ConditionFalse,
+			Reason: IncompatibleOperatorsInstalled,
+			Message: skews{
+				{
+					namespace:           ns.GetName(),
+					name:                incompatible.GetName(),
+					maxOpenShiftVersion: short + ".0",
+				},
+			}.String(),
+			LastTransitionTime: fixedNow(),
+		}))
 	})
 })
diff --git a/pkg/controller/operators/openshift/helpers.go b/pkg/controller/operators/openshift/helpers.go
@@ -2,7 +2,6 @@ package openshift
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -190,19 +189,15 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.
 			continue
 		}
 
-		var value string
-		if err := json.Unmarshal([]byte(property.Value), &value); err != nil {
-			errs = append(errs, err)
-			continue
-		}
-
+		value := strings.Trim(property.Value, "\"")
 		if value == "" {
 			continue
 		}
 
 		version, err := semver.ParseTolerant(value)
 		if err != nil {
 			errs = append(errs, err)
+			continue
 		}
 
 		if max == nil {

diff --git a/pkg/controller/operators/openshift/helpers_test.go b/pkg/controller/operators/openshift/helpers_test.go
@@ -2,6 +2,7 @@ package openshift
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	semver "github.com/blang/semver/v4"
@@ -341,6 +342,11 @@ func TestIncompatibleOperators(t *testing.T) {
 					namespace:           "default",
 					maxOpenShiftVersion: "1.0.0",
 				},
+				{
+					name:                "chestnut",
+					namespace:           "default",
+					maxOpenShiftVersion: "1.0",
+				},
 			},
 			expect: expect{
 				err: false,
@@ -350,6 +356,11 @@ func TestIncompatibleOperators(t *testing.T) {
 						namespace:           "default",
 						maxOpenShiftVersion: "1.0.0",
 					},
+					{
+						name:                "chestnut",
+						namespace:           "default",
+						maxOpenShiftVersion: "1.0.0",
+					},
 				},
 			},
 		},
@@ -463,7 +474,10 @@ func TestIncompatibleOperators(t *testing.T) {
 
 func TestMaxOpenShiftVersion(t *testing.T) {
 	mustParse := func(s string) *semver.Version {
-		version := semver.MustParse(s)
+		version, err := semver.ParseTolerant(s)
+		if err != nil {
+			panic(fmt.Sprintf("bad version given for test case: %s", err))
+		}
 		return &version
 	}
 
@@ -485,7 +499,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Nothing",
-			in:          []string{""},
+			in:          []string{`""`},
 			expect: expect{
 				err: false,
 				max: nil,
@@ -494,8 +508,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Nothing/Mixed",
 			in: []string{
-				"",
-				"1.0.0",
+				`""`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -504,7 +518,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Garbage",
-			in:          []string{"bad_version"},
+			in:          []string{`"bad_version"`},
 			expect: expect{
 				err: true,
 				max: nil,
@@ -513,8 +527,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Garbage/Mixed",
 			in: []string{
-				"bad_version",
-				"1.0.0",
+				`"bad_version"`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: true,
@@ -523,7 +537,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Single",
-			in:          []string{"1.0.0"},
+			in:          []string{`"1.0.0"`},
 			expect: expect{
 				err: false,
 				max: mustParse("1.0.0"),
@@ -532,8 +546,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Multiple",
 			in: []string{
-				"1.0.0",
-				"2.0.0",
+				`"1.0.0"`,
+				`"2.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -543,8 +557,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Duplicates",
 			in: []string{
-				"1.0.0",
-				"1.0.0",
+				`"1.0.0"`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -554,9 +568,9 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Duplicates/NonMax",
 			in: []string{
-				"1.0.0",
-				"1.0.0",
-				"2.0.0",
+				`"1.0.0"`,
+				`"1.0.0"`,
+				`"2.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -566,21 +580,30 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Ambiguous",
 			in: []string{
-				"1.0.0",
-				"1.0.0+1",
+				`"1.0.0"`,
+				`"1.0.0+1"`,
 			},
 			expect: expect{
 				err: true,
 				max: nil,
 			},
 		},
+		{
+			// Ensure unquoted short strings are accepted; e.g. X.Y
+			description: "Unquoted/Short",
+			in:          []string{"4.8"},
+			expect: expect{
+				err: false,
+				max: mustParse("4.8"),
+			},
+		},
 	} {
 		t.Run(tt.description, func(t *testing.T) {
 			var properties []*api.Property
 			for _, max := range tt.in {
 				properties = append(properties, &api.Property{
 					Type:  MaxOpenShiftVersionProperty,
-					Value: `"` + max + `"`, // Wrap in quotes so we don't break property marshaling
+					Value: max,
 				})
 			}
 

diff --git a/pkg/controller/registry/resolver/projection/properties_test.go b/pkg/controller/registry/resolver/projection/properties_test.go
@@ -54,6 +54,16 @@ func TestPropertiesAnnotationFromPropertyList(t *testing.T) {
 			},
 			expected: `{"properties":[{"type":"string","value":"hello"},{"type":"number","value":5},{"type":"array","value":[1,"two",3,"four"]},{"type":"object","value":{"hello":{"worl":"d"}}}]}`,
 		},
+		{
+			name: "unquoted string",
+			properties: []*api.Property{
+				{
+					Type:  "version",
+					Value: "4.8",
+				},
+			},
+			expected: `{"properties":[{"type":"version","value":4.8}]}`,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			actual, err := projection.PropertiesAnnotationFromPropertyList(tc.properties)
@@ -120,12 +130,23 @@ func TestPropertyListFromPropertiesAnnotation(t *testing.T) {
 				{
 					Type:  "array",
 					Value: `[1,"two",3,"four"]`,
-				}, {
+				},
+				{
 					Type:  "object",
 					Value: `{"hello":{"worl":"d"}}`,
 				},
 			},
 		},
+		{
+			name:       "unquoted string values",
+			annotation: `{"properties":[{"type": "version","value": 4.8}]}`,
+			expected: []*api.Property{
+				{
+					Type:  "version",
+					Value: "4.8",
+				},
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			actual, err := projection.PropertyListFromPropertiesAnnotation(tc.annotation)