diff --git a/pkg/controller/operators/openshift/clusteroperator_controller_test.go b/pkg/controller/operators/openshift/clusteroperator_controller_test.go index 9b5c750ff5..6f00839d1b 100644 --- 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 index 4dfd76c904..c7b0a00f2f 100644 --- 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,12 +189,7 @@ 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 } @@ -203,6 +197,7 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver. 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 index 92c7a22f3f..3e6a12dd6f 100644 --- 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 index 6c71a82a7f..acde5acd76 100644 --- 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)