From 1ebebdb37af7c64b800ff7ec3a04ddd0cccc2de7 Mon Sep 17 00:00:00 2001 From: Nick Hale Date: Mon, 19 Jul 2021 21:34:24 -0400 Subject: [PATCH] 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 --- .../clusteroperator_controller_test.go | 35 ++++++++++- pkg/controller/operators/openshift/helpers.go | 9 +-- .../operators/openshift/helpers_test.go | 59 +++++++++++++------ .../resolver/projection/properties_test.go | 23 +++++++- 4 files changed, 97 insertions(+), 29 deletions(-) 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)