From 8c003ade0face3ee49cdb65395fa43e2fdd13644 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 2 Nov 2026 12:45:30 -0800 Subject: [PATCH] pkg/controller/operators/openshift: Drop hard coded upgradeable 4.23 or 5.0 (#3955) * Sort incompatible CSVs in the Upgradeable condition message incompatibleOperators lists the CSVs from the informer cache. The cache returns them in map order, which changes between reconciles. skews.String() kept that order, so the same set of blocking CSVs produced different messages. conditionsEqual compares the message, so each reorder counted as a condition transition: the controller rewrote the ClusterOperator status and moved lastTransitionTime although nothing had changed. With two blocking CSVs on OCP 4.22 the message alternated between the two orders every one to two minutes. skews.String() now sorts a copy of the skews before it builds the message: skews with a property error first, as before, then by namespace and name. The same set of CSVs gives the same message in any input order, so the condition only transitions when the set changes. The regression test feeds three skews in three orders and expects one message. It fails on the previous implementation, which filled the non-error entries from the end of the slice. Fixes OCPBUGS-129618 Signed-off-by: Harald Klein Co-Authored-By: Claude * Revert "Hard-code version upgrade message for 4.22 upgrade paths (#3748)" This reverts commit 73a2205759278f98a4407bd0d07f166c5445a7bc (Hard-code version upgrade message for 4.22 upgrade paths, 2026-01-26, #3748). That change was an acceptable hack for 4.22 controllers, but should never have landed in the development branch, where it's currently generating output like: ClusterServiceVersions blocking minor version upgrades to 4.23 or major version upgrades to 5.0 in 5.0 clusters [1]. [1]: https://redhat.atlassian.net/browse/OPRUN-4431?focusedCommentId=17613550 * pkg/controller/operators/openshift/helpers: skew.process method The sorting from 52765afcbc (Sort incompatible CSVs in the Upgradeable condition message, 2026-10-28, #3952) relies on skew.err being set. It was being set via incompatibleOperators, but not via the direct skew{...} instantiation in TestSkewsStringIsDeterministic. This commit doesn't update the TestSkewsStringIsDeterministic pathways (that will come in a future commit), but it does refactor to shift some of the error handling and normalization out of both maxOpenShiftVersion() and incompatibleOperators(), and consolidate the logic in a new skew.process() method. I've moved skew references to pointers too, to make it more obvious that methods like process() can mutate the skew properties. * pkg/controller/operators/openshift: Firm skew-error handling While "it is safe to ignore the error here" would be nice, and may work for flows coming from an incompatibleOperators call, it's a brittle assumption. For example, it breaks down in TestSkewsStringIsDeterministic, which had been passing in an invalid entry based on "broken.v1.0.0", and sometimes using that as the initial entry. I'm making sure we've evaluated for errors in any code path that leads to those String() calls by adding .process() calls. process() is a no-op if the skew instance has already been processed, so it should be efficient, although having String mutate structure properties is pretty weird. Hopefully the fact that the mutated properties are idempotent output from the input properties helps reduce the chance that the mutation surprises someone down the road. If all the input versions to skews.String() have .err set, don't worry about claiming a particular x.y target, just say "next major or minor version" instead. --------- Signed-off-by: Harald Klein Co-authored-by: Harald Klein Co-authored-by: Claude Upstream-repository: operator-lifecycle-manager Upstream-commit: 0c5380ac184b9b9f533c99f3783e92a8d4d8660a --- .../clusteroperator_controller_test.go | 10 +- .../controller/operators/openshift/helpers.go | 148 +++++++++------ .../operators/openshift/helpers_test.go | 173 +++++++++++------- .../controller/operators/openshift/helpers.go | 148 +++++++++------ 4 files changed, 303 insertions(+), 176 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller_test.go index 72a0552ff2..9f3e9b6d47 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller_test.go @@ -195,7 +195,6 @@ var _ = Describe("ClusterOperator controller", func() { }).Should(Succeed()) }() - parsedVersion := semver.MustParse(clusterVersion) Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) { err := k8sClient.Get(ctx, clusterOperatorName, co) return co.Status.Conditions, err @@ -207,7 +206,7 @@ var _ = Describe("ClusterOperator controller", func() { { namespace: ns.GetName(), name: incompatible.GetName(), - maxOpenShiftVersion: fmt.Sprintf("%d.%d", parsedVersion.Major, parsedVersion.Minor), + maxOpenShiftVersion: clusterVersion, }, }.String(), LastTransitionTime: fixedNow(), @@ -266,7 +265,6 @@ var _ = Describe("ClusterOperator controller", func() { return k8sClient.Update(ctx, incompatible) }).Should(Succeed()) - _, parseErr := semver.ParseTolerant("garbage") Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) { err := k8sClient.Get(ctx, clusterOperatorName, co) return co.Status.Conditions, err @@ -276,9 +274,9 @@ var _ = Describe("ClusterOperator controller", func() { Reason: IncompatibleOperatorsInstalled, Message: skews{ { - namespace: ns.GetName(), - name: incompatible.GetName(), - err: fmt.Errorf(`failed to parse "garbage" as semver: %w`, parseErr), + namespace: ns.GetName(), + name: incompatible.GetName(), + maxOpenShiftVersion: "garbage", }, }.String(), LastTransitionTime: fixedNow(), diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go index 2e85462db2..66e1e14e8c 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "sort" "strings" "sync" @@ -74,39 +75,97 @@ func versionsMatch(a []configv1.OperandVersion, b []configv1.OperandVersion) boo return len(counts) < 1 } -type skews []skew +type skews []*skew func (s skews) String() string { - msg := make([]string, len(s)) - i, j := 0, len(s)-1 - for _, sk := range s { - m := sk.String() - // Partial order: error skews first - if sk.err != nil { - msg[i] = m - i++ - continue + // Sorting depends on a populated .err, to sort invalid skews + // first. incompatibleOperators sets .err through one call path, + // but check again here, to ensure reliability, regardless of call-chain. + var minVersion *semver.Version + for i := range s { + s[i].process() + if s[i].err == nil && (minVersion == nil || s[i].normalizedMaxOpenShiftVersion.LT(*minVersion)) { + minVersion = s[i].normalizedMaxOpenShiftVersion } - msg[j] = m - j-- } - // TODO Revisit this message after 4.23 release once release schedule is determined for future possible minor versions. - return fmt.Sprintf("ClusterServiceVersions blocking minor version upgrades to 4.23 or major version upgrades to 5.0:\n%s", strings.Join(msg, "\n")) + // Sort so the message does not depend on the (unstable) order in which the + // CSVs were listed: error skews first, then by namespace/name. A different + // order would be a different Message, and so a condition transition. + sorted := make(skews, len(s)) + copy(sorted, s) + sort.SliceStable(sorted, func(i, j int) bool { + a, b := sorted[i], sorted[j] + if (a.err != nil) != (b.err != nil) { + return a.err != nil + } + if a.namespace != b.namespace { + return a.namespace < b.namespace + } + return a.name < b.name + }) + + msg := make([]string, len(sorted)) + for i, sk := range sorted { + msg[i] = sk.String() + } + + var next string + if minVersion == nil { + next = "next major or minor version" + } else { + n := nextY(*minVersion) + next = fmt.Sprintf("%d.%d", n.Major, n.Minor) + } + return fmt.Sprintf("ClusterServiceVersions blocking minor version upgrades to %s or higher:\n%s", next, strings.Join(msg, "\n")) } type skew struct { namespace string name string maxOpenShiftVersion string - err error + + // the following properties are populated by skew.process() + err error + normalizedMaxOpenShiftVersion *semver.Version +} + +func (s *skew) process() { + if s == nil || s.normalizedMaxOpenShiftVersion != nil || s.err != nil { + return + } + + if s.maxOpenShiftVersion == "" { + // Handle "" separately, so parse doesn't treat it as a zero + s.err = fmt.Errorf(`%s value cannot be "" (an empty string)`, MaxOpenShiftVersionProperty) + s.normalizedMaxOpenShiftVersion = &semver.Version{} + return + } + + version, err := semver.ParseTolerant(s.maxOpenShiftVersion) + s.normalizedMaxOpenShiftVersion = &version + if err != nil { + s.err = fmt.Errorf(`failed to parse %s=%q as semver: %w`, MaxOpenShiftVersionProperty, s.maxOpenShiftVersion, err) + return + } + + truncatedVersion := semver.Version{Major: version.Major, Minor: version.Minor} + if !s.normalizedMaxOpenShiftVersion.EQ(truncatedVersion) { + s.err = fmt.Errorf("%s property must specify only . version, got invalid value %q", MaxOpenShiftVersionProperty, s.normalizedMaxOpenShiftVersion) + return + } + + // Normalize the operator's maxOpenShiftVersion: 4.23 and 5.0 are equivalent. + version = normalizeOCPVersion(*s.normalizedMaxOpenShiftVersion) + s.normalizedMaxOpenShiftVersion = &version } -func (s skew) String() string { +func (s *skew) String() string { + s.process() if s.err != nil { return fmt.Sprintf("- %s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) } - return fmt.Sprintf("- maximum supported OCP version for %s/%s is %s", s.namespace, s.name, s.maxOpenShiftVersion) + return fmt.Sprintf("- maximum supported OCP version for %s/%s is %d.%d", s.namespace, s.name, s.normalizedMaxOpenShiftVersion.Major, s.normalizedMaxOpenShiftVersion.Minor) } type transientError struct { @@ -146,29 +205,18 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error continue } - s := skew{ - name: csv.GetName(), - namespace: csv.GetNamespace(), - } - max, err := maxOpenShiftVersion(&csv) - if err != nil { - s.err = err - incompatible = append(incompatible, s) + s := maxOpenShiftVersion(&csv) + if s == nil { continue - } - - if max == nil { + } else if s.err != nil { + incompatible = append(incompatible, s) continue } - // Normalize the operator's maxOpenShiftVersion: 4.23 and 5.0 are equivalent. - normalizedMax := normalizeOCPVersion(*max) - if normalizedMax.GTE(nextMinor) { + if s.normalizedMaxOpenShiftVersion.GTE(nextMinor) { continue } - s.maxOpenShiftVersion = fmt.Sprintf("%d.%d", max.Major, max.Minor) - incompatible = append(incompatible, s) } @@ -246,16 +294,22 @@ const ( MaxOpenShiftVersionProperty = "olm.maxOpenShiftVersion" ) -func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.Version, error) { +func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) *skew { + s := &skew{ + name: csv.GetName(), + namespace: csv.GetNamespace(), + } + // Extract the property from the CSV's annotations if possible annotation, ok := csv.GetAnnotations()[projection.PropertiesAnnotationKey] if !ok { - return nil, nil + return nil } properties, err := projection.PropertyListFromPropertiesAnnotation(annotation) if err != nil { - return nil, err + s.err = err + return s } var max *string @@ -265,33 +319,23 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver. } if max != nil { - return nil, fmt.Errorf(`defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty) + s.err = fmt.Errorf(`defining more than one %q property is not allowed`, MaxOpenShiftVersionProperty) + return s } max = &property.Value } if max == nil { - return nil, nil + return nil } // Account for any additional quoting value := strings.Trim(*max, "\"") - if value == "" { - // Handle "" separately, so parse doesn't treat it as a zero - return nil, fmt.Errorf(`value cannot be "" (an empty string)`) - } - version, err := semver.ParseTolerant(value) - if err != nil { - return nil, fmt.Errorf(`failed to parse "%s" as semver: %w`, value, err) - } - - truncatedVersion := semver.Version{Major: version.Major, Minor: version.Minor} - if !version.EQ(truncatedVersion) { - return nil, fmt.Errorf("property %s must specify only . version, got invalid value %s", MaxOpenShiftVersionProperty, version) - } - return &truncatedVersion, nil + s.maxOpenShiftVersion = value + s.process() + return s } func notCopiedSelector() (labels.Selector, error) { diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers_test.go b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers_test.go index 43d8c5e63f..a7c6de2cfc 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers_test.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers_test.go @@ -19,6 +19,14 @@ import ( "github.com/operator-framework/operator-registry/pkg/api" ) +func mustParse(s string) *semver.Version { + version, err := semver.ParseTolerant(s) + if err != nil { + panic(fmt.Sprintf("bad version given for test case: %s", err)) + } + return &version +} + func TestConditionsEqual(t *testing.T) { type args struct { a, b *configv1.ClusterOperatorStatusCondition @@ -284,29 +292,36 @@ func TestIncompatibleOperators(t *testing.T) { err: false, incompatible: skews{ { - name: "almond", - namespace: "default", - maxOpenShiftVersion: "1.0", + name: "almond", + namespace: "default", + maxOpenShiftVersion: "1.0.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 1, Minor: 0}, }, { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "1.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "1.0.0+build", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 1, Minor: 0}, }, { - name: "chestnut", - namespace: "default", - err: fmt.Errorf("property olm.maxOpenShiftVersion must specify only . version, got invalid value 1.1.0-pre"), + name: "chestnut", + namespace: "default", + maxOpenShiftVersion: "1.1.0-pre", + err: fmt.Errorf(`olm.maxOpenShiftVersion property must specify only . version, got invalid value "1.1.0-pre"`), + normalizedMaxOpenShiftVersion: mustParse("1.1.0-pre"), }, { - name: "drupe", - namespace: "default", - err: fmt.Errorf("property olm.maxOpenShiftVersion must specify only . version, got invalid value 1.1.0-pre+build"), + name: "drupe", + namespace: "default", + maxOpenShiftVersion: "1.1.0-pre+build", + err: fmt.Errorf(`olm.maxOpenShiftVersion property must specify only . version, got invalid value "1.1.0-pre+build"`), + normalizedMaxOpenShiftVersion: mustParse("1.1.0-pre+build"), }, { - name: "european-hazelnut", - namespace: "default", - maxOpenShiftVersion: "0.1", + name: "european-hazelnut", + namespace: "default", + maxOpenShiftVersion: "0.1.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 0, Minor: 1}, }, }, }, @@ -335,14 +350,16 @@ func TestIncompatibleOperators(t *testing.T) { err: false, incompatible: skews{ { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "1.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "1.0.0", + normalizedMaxOpenShiftVersion: mustParse("1.0.0"), }, { - name: "chestnut", - namespace: "default", - maxOpenShiftVersion: "1.0", + name: "chestnut", + namespace: "default", + maxOpenShiftVersion: "1.0", + normalizedMaxOpenShiftVersion: mustParse("1.0.0"), }, }, }, @@ -371,17 +388,20 @@ func TestIncompatibleOperators(t *testing.T) { err: false, incompatible: skews{ { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "1.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "1.0.0", + normalizedMaxOpenShiftVersion: mustParse("1.0.0"), }, { - name: "chestnut", - namespace: "default", - err: fmt.Errorf(`failed to parse "bad_version" as semver: %w`, func() error { + name: "chestnut", + namespace: "default", + maxOpenShiftVersion: "bad_version", + err: fmt.Errorf(`failed to parse olm.maxOpenShiftVersion="bad_version" as semver: %w`, func() error { _, err := semver.ParseTolerant("bad_version") return err }()), + normalizedMaxOpenShiftVersion: &semver.Version{}, }, }, }, @@ -435,9 +455,10 @@ func TestIncompatibleOperators(t *testing.T) { err: false, incompatible: skews{ { - name: "almond", - namespace: "default", - maxOpenShiftVersion: "1.1", + name: "almond", + namespace: "default", + maxOpenShiftVersion: "1.1.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 1, Minor: 1}, }, }, }, @@ -490,14 +511,16 @@ func TestIncompatibleOperators(t *testing.T) { expect: expect{ incompatible: skews{ { - name: "almond", - namespace: "default", - maxOpenShiftVersion: "4.23", + name: "almond", + namespace: "default", + maxOpenShiftVersion: "4.23", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "5.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "5.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, }, }, @@ -525,14 +548,16 @@ func TestIncompatibleOperators(t *testing.T) { expect: expect{ incompatible: skews{ { - name: "almond", - namespace: "default", - maxOpenShiftVersion: "4.23", + name: "almond", + namespace: "default", + maxOpenShiftVersion: "4.23", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "5.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "5.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, }, }, @@ -560,19 +585,22 @@ func TestIncompatibleOperators(t *testing.T) { expect: expect{ incompatible: skews{ { - name: "almond", - namespace: "default", - maxOpenShiftVersion: "4.23", + name: "almond", + namespace: "default", + maxOpenShiftVersion: "4.23", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, { - name: "beech", - namespace: "default", - maxOpenShiftVersion: "5.0", + name: "beech", + namespace: "default", + maxOpenShiftVersion: "5.0", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 0}, }, { - name: "chestnut", - namespace: "default", - maxOpenShiftVersion: "5.1", + name: "chestnut", + namespace: "default", + maxOpenShiftVersion: "5.1", + normalizedMaxOpenShiftVersion: &semver.Version{Major: 5, Minor: 1}, }, }, }, @@ -619,14 +647,6 @@ func TestIncompatibleOperators(t *testing.T) { } func TestMaxOpenShiftVersion(t *testing.T) { - mustParse := func(s string) *semver.Version { - version, err := semver.ParseTolerant(s) - if err != nil { - panic(fmt.Sprintf("bad version given for test case: %s", err)) - } - return &version - } - type expect struct { err bool max *semver.Version @@ -648,7 +668,7 @@ func TestMaxOpenShiftVersion(t *testing.T) { in: []string{`""`}, expect: expect{ err: true, - max: nil, + max: &semver.Version{}, }, }, { @@ -656,7 +676,7 @@ func TestMaxOpenShiftVersion(t *testing.T) { in: []string{`"bad_version"`}, expect: expect{ err: true, - max: nil, + max: &semver.Version{}, }, }, { @@ -705,14 +725,18 @@ func TestMaxOpenShiftVersion(t *testing.T) { projection.PropertiesAnnotationKey: value, }) - max, err := maxOpenShiftVersion(csv) + skew := maxOpenShiftVersion(csv) if tt.expect.err { - require.Error(t, err) - } else { - require.NoError(t, err) + require.Error(t, skew.err) + } else if skew != nil { + require.NoError(t, skew.err) } - require.Equal(t, tt.expect.max, max) + if tt.in == nil { + require.Nil(t, skew) + } else { + require.Equal(t, tt.expect.max, skew.normalizedMaxOpenShiftVersion) + } }) } } @@ -815,3 +839,20 @@ func TestNormalizeOCPVersion(t *testing.T) { }) } } + +func TestSkewsStringIsDeterministic(t *testing.T) { + // The informer cache lists CSVs in map order. The condition message must not + // depend on that order, or every reorder counts as a condition transition. + a := &skew{namespace: "openshift-logging", name: "cluster-logging.v6.6.1", maxOpenShiftVersion: "4.23"} + b := &skew{namespace: "openshift-cluster-csi-drivers", name: "secrets-store-csi-driver-operator.v4.22.0", maxOpenShiftVersion: "5.0"} + invalid := &skew{namespace: "openshift-logging", name: "broken.v1.0.0", maxOpenShiftVersion: "invalid"} + + expected := "ClusterServiceVersions blocking minor version upgrades to 5.1 or higher:\n" + + invalid.String() + "\n" + // error skews first + b.String() + "\n" + // then by namespace/name + a.String() + + require.Equal(t, expected, skews{a, b, invalid}.String()) + require.Equal(t, expected, skews{invalid, b, a}.String()) + require.Equal(t, expected, skews{b, invalid, a}.String()) +} diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go index 2e85462db2..66e1e14e8c 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "sort" "strings" "sync" @@ -74,39 +75,97 @@ func versionsMatch(a []configv1.OperandVersion, b []configv1.OperandVersion) boo return len(counts) < 1 } -type skews []skew +type skews []*skew func (s skews) String() string { - msg := make([]string, len(s)) - i, j := 0, len(s)-1 - for _, sk := range s { - m := sk.String() - // Partial order: error skews first - if sk.err != nil { - msg[i] = m - i++ - continue + // Sorting depends on a populated .err, to sort invalid skews + // first. incompatibleOperators sets .err through one call path, + // but check again here, to ensure reliability, regardless of call-chain. + var minVersion *semver.Version + for i := range s { + s[i].process() + if s[i].err == nil && (minVersion == nil || s[i].normalizedMaxOpenShiftVersion.LT(*minVersion)) { + minVersion = s[i].normalizedMaxOpenShiftVersion } - msg[j] = m - j-- } - // TODO Revisit this message after 4.23 release once release schedule is determined for future possible minor versions. - return fmt.Sprintf("ClusterServiceVersions blocking minor version upgrades to 4.23 or major version upgrades to 5.0:\n%s", strings.Join(msg, "\n")) + // Sort so the message does not depend on the (unstable) order in which the + // CSVs were listed: error skews first, then by namespace/name. A different + // order would be a different Message, and so a condition transition. + sorted := make(skews, len(s)) + copy(sorted, s) + sort.SliceStable(sorted, func(i, j int) bool { + a, b := sorted[i], sorted[j] + if (a.err != nil) != (b.err != nil) { + return a.err != nil + } + if a.namespace != b.namespace { + return a.namespace < b.namespace + } + return a.name < b.name + }) + + msg := make([]string, len(sorted)) + for i, sk := range sorted { + msg[i] = sk.String() + } + + var next string + if minVersion == nil { + next = "next major or minor version" + } else { + n := nextY(*minVersion) + next = fmt.Sprintf("%d.%d", n.Major, n.Minor) + } + return fmt.Sprintf("ClusterServiceVersions blocking minor version upgrades to %s or higher:\n%s", next, strings.Join(msg, "\n")) } type skew struct { namespace string name string maxOpenShiftVersion string - err error + + // the following properties are populated by skew.process() + err error + normalizedMaxOpenShiftVersion *semver.Version +} + +func (s *skew) process() { + if s == nil || s.normalizedMaxOpenShiftVersion != nil || s.err != nil { + return + } + + if s.maxOpenShiftVersion == "" { + // Handle "" separately, so parse doesn't treat it as a zero + s.err = fmt.Errorf(`%s value cannot be "" (an empty string)`, MaxOpenShiftVersionProperty) + s.normalizedMaxOpenShiftVersion = &semver.Version{} + return + } + + version, err := semver.ParseTolerant(s.maxOpenShiftVersion) + s.normalizedMaxOpenShiftVersion = &version + if err != nil { + s.err = fmt.Errorf(`failed to parse %s=%q as semver: %w`, MaxOpenShiftVersionProperty, s.maxOpenShiftVersion, err) + return + } + + truncatedVersion := semver.Version{Major: version.Major, Minor: version.Minor} + if !s.normalizedMaxOpenShiftVersion.EQ(truncatedVersion) { + s.err = fmt.Errorf("%s property must specify only . version, got invalid value %q", MaxOpenShiftVersionProperty, s.normalizedMaxOpenShiftVersion) + return + } + + // Normalize the operator's maxOpenShiftVersion: 4.23 and 5.0 are equivalent. + version = normalizeOCPVersion(*s.normalizedMaxOpenShiftVersion) + s.normalizedMaxOpenShiftVersion = &version } -func (s skew) String() string { +func (s *skew) String() string { + s.process() if s.err != nil { return fmt.Sprintf("- %s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) } - return fmt.Sprintf("- maximum supported OCP version for %s/%s is %s", s.namespace, s.name, s.maxOpenShiftVersion) + return fmt.Sprintf("- maximum supported OCP version for %s/%s is %d.%d", s.namespace, s.name, s.normalizedMaxOpenShiftVersion.Major, s.normalizedMaxOpenShiftVersion.Minor) } type transientError struct { @@ -146,29 +205,18 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error continue } - s := skew{ - name: csv.GetName(), - namespace: csv.GetNamespace(), - } - max, err := maxOpenShiftVersion(&csv) - if err != nil { - s.err = err - incompatible = append(incompatible, s) + s := maxOpenShiftVersion(&csv) + if s == nil { continue - } - - if max == nil { + } else if s.err != nil { + incompatible = append(incompatible, s) continue } - // Normalize the operator's maxOpenShiftVersion: 4.23 and 5.0 are equivalent. - normalizedMax := normalizeOCPVersion(*max) - if normalizedMax.GTE(nextMinor) { + if s.normalizedMaxOpenShiftVersion.GTE(nextMinor) { continue } - s.maxOpenShiftVersion = fmt.Sprintf("%d.%d", max.Major, max.Minor) - incompatible = append(incompatible, s) } @@ -246,16 +294,22 @@ const ( MaxOpenShiftVersionProperty = "olm.maxOpenShiftVersion" ) -func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.Version, error) { +func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) *skew { + s := &skew{ + name: csv.GetName(), + namespace: csv.GetNamespace(), + } + // Extract the property from the CSV's annotations if possible annotation, ok := csv.GetAnnotations()[projection.PropertiesAnnotationKey] if !ok { - return nil, nil + return nil } properties, err := projection.PropertyListFromPropertiesAnnotation(annotation) if err != nil { - return nil, err + s.err = err + return s } var max *string @@ -265,33 +319,23 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver. } if max != nil { - return nil, fmt.Errorf(`defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty) + s.err = fmt.Errorf(`defining more than one %q property is not allowed`, MaxOpenShiftVersionProperty) + return s } max = &property.Value } if max == nil { - return nil, nil + return nil } // Account for any additional quoting value := strings.Trim(*max, "\"") - if value == "" { - // Handle "" separately, so parse doesn't treat it as a zero - return nil, fmt.Errorf(`value cannot be "" (an empty string)`) - } - version, err := semver.ParseTolerant(value) - if err != nil { - return nil, fmt.Errorf(`failed to parse "%s" as semver: %w`, value, err) - } - - truncatedVersion := semver.Version{Major: version.Major, Minor: version.Minor} - if !version.EQ(truncatedVersion) { - return nil, fmt.Errorf("property %s must specify only . version, got invalid value %s", MaxOpenShiftVersionProperty, version) - } - return &truncatedVersion, nil + s.maxOpenShiftVersion = value + s.process() + return s } func notCopiedSelector() (labels.Selector, error) {