From 90140470a239cde3024ac2ef5805f07570dbd15d Mon Sep 17 00:00:00 2001 From: Nick Hale Date: Mon, 2 Aug 2021 19:51:53 -0400 Subject: [PATCH] fix(openshift): block upgrades on invalid max properties (#2302) Block OpenShift upgrades while: - olm.maxOpenShiftVersion properties have invalid values - cluster information is unavailable; e.g. the desired version of the cluster is undefined - an installed operator has declared more than one olm.MaxOpenShiftVersion property See https://bugzilla.redhat.com/show_bug.cgi?id=1986753 for motivation. Signed-off-by: Nick Hale Upstream-repository: operator-lifecycle-manager Upstream-commit: 734c6d031cbb2eb00acdf6cac91bba3d33311a0d --- .../openshift/clusteroperator_controller.go | 31 +++-- .../clusteroperator_controller_test.go | 91 ++++++++++++- .../controller/operators/openshift/helpers.go | 120 +++++++++--------- .../operators/openshift/helpers_test.go | 70 ++-------- .../openshift/clusteroperator_controller.go | 31 +++-- .../controller/operators/openshift/helpers.go | 120 +++++++++--------- 6 files changed, 262 insertions(+), 201 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go index 78c3bd2b6b..744efe76c1 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go @@ -209,7 +209,8 @@ func (r *ClusterOperatorReconciler) setDegraded(_ context.Context, co *ClusterOp } const ( - IncompatibleOperatorsInstalled = "IncompatibleOperatorsInstalled" + IncompatibleOperatorsInstalled = "IncompatibleOperatorsInstalled" + ErrorCheckingOperatorCompatibility = "ErrorCheckingOperatorCompatibility" ) func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *ClusterOperator) error { @@ -221,31 +222,39 @@ func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *Clus // Set upgradeable=false if (either/or): // 1. OLM currently upgrading (takes priorty in the status message) - // 2. Operators currently installed that are incompatible with the next OCP minor version + // 2. Operators currently installed that are incompatible with the next minor version of OpenShift + // 3. An error occurs while determining 2 + var err error if r.syncTracker.SuccessfulSyncs() < 1 || !versionsMatch(co.Status.Versions, r.TargetVersions) { // OLM is still upgrading desired.Status = configv1.ConditionFalse desired.Message = "Waiting for updates to take effect" } else { - incompatible, err := incompatibleOperators(ctx, r.Client) + var incompatible skews + incompatible, err = incompatibleOperators(ctx, r.Client) if err != nil { - return err - } - - if len(incompatible) > 0 { - // Some incompatible operator is installed + // "Fail closed" when we can't determine compatibility + // Note: Unspecified compatibility = universal compatibility; i.e. operators that don't specify a "maxOpenShiftVersion" property are compatible with everything. + desired.Status = configv1.ConditionFalse + desired.Reason = ErrorCheckingOperatorCompatibility + desired.Message = fmt.Sprintf("Encountered errors while checking compatibility with the next minor version of OpenShift: %s", err) + } else if len(incompatible) > 0 { + // Operators are installed that have incompatible and/or invalid max versions desired.Status = configv1.ConditionFalse desired.Reason = IncompatibleOperatorsInstalled - desired.Message = incompatible.String() // TODO: Truncate message to field length + desired.Message = incompatible.String() } } + // Only return transient errors likely resolved by retrying immediately + err = transientErrors(err) + current := co.GetCondition(configv1.OperatorUpgradeable) if conditionsEqual(current, desired) { // Comparison ignores lastUpdated - return nil + return err } co.SetCondition(desired) - return nil + return err } 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 6f00839d1b..8d86c580b3 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 @@ -142,8 +142,35 @@ var _ = Describe("ClusterOperator controller", func() { LastTransitionTime: fixedNow(), })) - By("setting upgradeable=false when incompatible operators exist") + By("setting upgradeable=false when there's an error determining compatibility") + cv.Status = configv1.ClusterVersionStatus{} + + Eventually(func() error { + return k8sClient.Status().Update(ctx, cv) + }).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: ErrorCheckingOperatorCompatibility, + Message: "Encountered errors while checking compatibility with the next minor version of OpenShift: Desired release version missing from ClusterVersion", + LastTransitionTime: fixedNow(), + })) + + cv.Status = configv1.ClusterVersionStatus{ + Desired: configv1.Update{ + Version: clusterVersion, + }, + } + + Eventually(func() error { + return k8sClient.Status().Update(ctx, cv) + }).Should(Succeed()) + By("setting upgradeable=false when incompatible operators exist") ns := &corev1.Namespace{} ns.SetName("nostromo") @@ -160,12 +187,15 @@ var _ = Describe("ClusterOperator controller", func() { incompatible.SetName("xenomorph") incompatible.SetNamespace(ns.GetName()) - withMax := func(version string) map[string]string { - maxProperty := &api.Property{ - Type: MaxOpenShiftVersionProperty, - Value: version, + withMax := func(versions ...string) map[string]string { + var properties []*api.Property + for _, v := range versions { + properties = append(properties, &api.Property{ + Type: MaxOpenShiftVersionProperty, + Value: v, + }) } - value, err := projection.PropertiesAnnotationFromPropertyList([]*api.Property{maxProperty}) + value, err := projection.PropertiesAnnotationFromPropertyList(properties) Expect(err).ToNot(HaveOccurred()) return map[string]string{ @@ -245,5 +275,54 @@ var _ = Describe("ClusterOperator controller", func() { }.String(), LastTransitionTime: fixedNow(), })) + + By("setting upgradeable=false when invalid max versions are found") + incompatible.SetAnnotations(withMax(`"garbage"`)) + + Eventually(func() error { + 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 + }, timeout).Should(ContainElement(configv1.ClusterOperatorStatusCondition{ + Type: configv1.OperatorUpgradeable, + Status: configv1.ConditionFalse, + Reason: IncompatibleOperatorsInstalled, + Message: skews{ + { + namespace: ns.GetName(), + name: incompatible.GetName(), + err: fmt.Errorf(`Failed to parse "garbage" as semver: %w`, parseErr), + }, + }.String(), + LastTransitionTime: fixedNow(), + })) + + By("setting upgradeable=false when more than one max version property is defined") + incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, clusterVersion), fmt.Sprintf(`"%s"`, next.String()))) + + 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(), + err: fmt.Errorf(`Defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty), + }, + }.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 c7b0a00f2f..3c6159518d 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go +++ b/staging/operator-lifecycle-manager/pkg/controller/operators/openshift/helpers.go @@ -2,6 +2,7 @@ package openshift import ( "context" + "errors" "fmt" "strings" @@ -80,22 +81,47 @@ func versionsMatch(a []configv1.OperandVersion, b []configv1.OperandVersion) boo type skews []skew func (s skews) String() string { - var msg []string + msg := make([]string, len(s)) + i, j := 0, len(s)-1 for _, sk := range s { - msg = append(msg, sk.String()) + m := sk.String() + // Partial order: error skews first + if sk.err != nil { + msg[i] = m + i++ + continue + } + msg[j] = m + j-- } - return "The following operators block OpenShift upgrades: " + strings.Join(msg, ",") + return "ClusterServiceVersions blocking cluster upgrade: " + strings.Join(msg, ",") } type skew struct { namespace string name string maxOpenShiftVersion string + err error } func (s skew) String() string { - return fmt.Sprintf("Operator %s in namespace %s is not compatible with OpenShift versions greater than %s", s.name, s.namespace, s.maxOpenShiftVersion) + if s.err != nil { + return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) + } + + return fmt.Sprintf("%s/%s is incompatible with OpenShift versions greater than %s", s.namespace, s.name, s.maxOpenShiftVersion) +} + +type transientError struct { + error +} + +// transientErrors returns the result of stripping all wrapped errors not of type transientError from the given error. +func transientErrors(err error) error { + return utilerrors.FilterOut(err, func(e error) bool { + return !errors.As(e, new(transientError)) + }) } func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) { @@ -105,58 +131,59 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error } if next == nil { - return nil, nil + // Note: This shouldn't happen + return nil, fmt.Errorf("Failed to determine next OpenShift Y-stream release") } next.Minor++ csvList := &operatorsv1alpha1.ClusterServiceVersionList{} if err := cli.List(ctx, csvList); err != nil { - return nil, err + return nil, &transientError{fmt.Errorf("Failed to list ClusterServiceVersions: %w", err)} } - var ( - s skews - errs []error - ) + var incompatible skews for _, csv := range csvList.Items { if csv.IsCopied() { continue } + s := skew{ + name: csv.GetName(), + namespace: csv.GetNamespace(), + } max, err := maxOpenShiftVersion(&csv) if err != nil { - errs = append(errs, err) + s.err = err + incompatible = append(incompatible, s) continue } + if max == nil || max.GTE(*next) { continue } + s.maxOpenShiftVersion = max.String() - s = append(s, skew{ - name: csv.GetName(), - namespace: csv.GetNamespace(), - maxOpenShiftVersion: max.String(), - }) + incompatible = append(incompatible, s) } - return s, utilerrors.NewAggregate(errs) + return incompatible, nil } func desiredRelease(ctx context.Context, cli client.Client) (*semver.Version, error) { cv := configv1.ClusterVersion{} if err := cli.Get(ctx, client.ObjectKey{Name: "version"}, &cv); err != nil { // "version" is the name of OpenShift's ClusterVersion singleton - return nil, err + return nil, &transientError{fmt.Errorf("Failed to get ClusterVersion: %w", err)} } v := cv.Status.Desired.Version if v == "" { // The release version hasn't been set yet - return nil, nil + return nil, fmt.Errorf("Desired release version missing from ClusterVersion") } desired, err := semver.ParseTolerant(v) if err != nil { - return nil, err + return nil, fmt.Errorf("ClusterVersion has invalid desired release version: %w", err) } return &desired, nil @@ -178,57 +205,36 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver. return nil, err } - // Take the highest semver if there's more than one max version specified - var ( - max *semver.Version - dups []semver.Version - errs []error - ) + var max *string for _, property := range properties { if property.Type != MaxOpenShiftVersionProperty { 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 { + return nil, fmt.Errorf(`Defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty) } - if max == nil { - max = &version - continue - } - if version.LT(*max) { - continue - } - if version.EQ(*max) { - // Found a duplicate, mark it - dups = append(dups, *max) - } + max = &property.Value + } - max = &version + if max == nil { + return nil, nil } - // Return an error if THE max version has a duplicate (i.e. equivalent version) - // Note: This may not be a problem since there should be no difference as far as blocking upgrades is concerned. - // This is more for clear status messages. - for _, dup := range dups { - if max.EQ(dup) && max.String() != dup.String() { // "1.0.0" vs "1.0.0" is fine, but not "1.0.0" vs "1.0.0+1" - errs = append(errs, fmt.Errorf("max openshift version ambiguous, equivalent versions %s and %s have been specified concurrently", max, dup)) - } + // 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)`) } - if len(errs) > 0 { - return nil, utilerrors.NewAggregate(errs) + version, err := semver.ParseTolerant(value) + if err != nil { + return nil, fmt.Errorf(`Failed to parse "%s" as semver: %w`, value, err) } - return max, nil + return &version, nil } 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 3e6a12dd6f..1f4b57120b 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 @@ -394,13 +394,21 @@ func TestIncompatibleOperators(t *testing.T) { }, }, expect: expect{ - err: true, + err: false, incompatible: skews{ { name: "beech", namespace: "default", maxOpenShiftVersion: "1.0.0", }, + { + name: "chestnut", + namespace: "default", + err: fmt.Errorf(`Failed to parse "bad_version" as semver: %w`, func() error { + _, err := semver.ParseTolerant("bad_version") + return err + }()), + }, }, }, }, @@ -429,7 +437,7 @@ func TestIncompatibleOperators(t *testing.T) { }, }, expect: expect{ - err: false, + err: true, incompatible: nil, }, }, @@ -501,21 +509,10 @@ func TestMaxOpenShiftVersion(t *testing.T) { description: "Nothing", in: []string{`""`}, expect: expect{ - err: false, + err: true, max: nil, }, }, - { - description: "Nothing/Mixed", - in: []string{ - `""`, - `"1.0.0"`, - }, - expect: expect{ - err: false, - max: mustParse("1.0.0"), - }, - }, { description: "Garbage", in: []string{`"bad_version"`}, @@ -524,17 +521,6 @@ func TestMaxOpenShiftVersion(t *testing.T) { max: nil, }, }, - { - description: "Garbage/Mixed", - in: []string{ - `"bad_version"`, - `"1.0.0"`, - }, - expect: expect{ - err: true, - max: nil, - }, - }, { description: "Single", in: []string{`"1.0.0"`}, @@ -549,40 +535,6 @@ func TestMaxOpenShiftVersion(t *testing.T) { `"1.0.0"`, `"2.0.0"`, }, - expect: expect{ - err: false, - max: mustParse("2.0.0"), - }, - }, - { - description: "Duplicates", - in: []string{ - `"1.0.0"`, - `"1.0.0"`, - }, - expect: expect{ - err: false, - max: mustParse("1.0.0"), - }, - }, - { - description: "Duplicates/NonMax", - in: []string{ - `"1.0.0"`, - `"1.0.0"`, - `"2.0.0"`, - }, - expect: expect{ - err: false, - max: mustParse("2.0.0"), - }, - }, - { - description: "Ambiguous", - in: []string{ - `"1.0.0"`, - `"1.0.0+1"`, - }, expect: expect{ err: true, max: nil, diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go index 78c3bd2b6b..744efe76c1 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller.go @@ -209,7 +209,8 @@ func (r *ClusterOperatorReconciler) setDegraded(_ context.Context, co *ClusterOp } const ( - IncompatibleOperatorsInstalled = "IncompatibleOperatorsInstalled" + IncompatibleOperatorsInstalled = "IncompatibleOperatorsInstalled" + ErrorCheckingOperatorCompatibility = "ErrorCheckingOperatorCompatibility" ) func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *ClusterOperator) error { @@ -221,31 +222,39 @@ func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *Clus // Set upgradeable=false if (either/or): // 1. OLM currently upgrading (takes priorty in the status message) - // 2. Operators currently installed that are incompatible with the next OCP minor version + // 2. Operators currently installed that are incompatible with the next minor version of OpenShift + // 3. An error occurs while determining 2 + var err error if r.syncTracker.SuccessfulSyncs() < 1 || !versionsMatch(co.Status.Versions, r.TargetVersions) { // OLM is still upgrading desired.Status = configv1.ConditionFalse desired.Message = "Waiting for updates to take effect" } else { - incompatible, err := incompatibleOperators(ctx, r.Client) + var incompatible skews + incompatible, err = incompatibleOperators(ctx, r.Client) if err != nil { - return err - } - - if len(incompatible) > 0 { - // Some incompatible operator is installed + // "Fail closed" when we can't determine compatibility + // Note: Unspecified compatibility = universal compatibility; i.e. operators that don't specify a "maxOpenShiftVersion" property are compatible with everything. + desired.Status = configv1.ConditionFalse + desired.Reason = ErrorCheckingOperatorCompatibility + desired.Message = fmt.Sprintf("Encountered errors while checking compatibility with the next minor version of OpenShift: %s", err) + } else if len(incompatible) > 0 { + // Operators are installed that have incompatible and/or invalid max versions desired.Status = configv1.ConditionFalse desired.Reason = IncompatibleOperatorsInstalled - desired.Message = incompatible.String() // TODO: Truncate message to field length + desired.Message = incompatible.String() } } + // Only return transient errors likely resolved by retrying immediately + err = transientErrors(err) + current := co.GetCondition(configv1.OperatorUpgradeable) if conditionsEqual(current, desired) { // Comparison ignores lastUpdated - return nil + return err } co.SetCondition(desired) - return nil + return err } 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 c7b0a00f2f..3c6159518d 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 @@ -2,6 +2,7 @@ package openshift import ( "context" + "errors" "fmt" "strings" @@ -80,22 +81,47 @@ func versionsMatch(a []configv1.OperandVersion, b []configv1.OperandVersion) boo type skews []skew func (s skews) String() string { - var msg []string + msg := make([]string, len(s)) + i, j := 0, len(s)-1 for _, sk := range s { - msg = append(msg, sk.String()) + m := sk.String() + // Partial order: error skews first + if sk.err != nil { + msg[i] = m + i++ + continue + } + msg[j] = m + j-- } - return "The following operators block OpenShift upgrades: " + strings.Join(msg, ",") + return "ClusterServiceVersions blocking cluster upgrade: " + strings.Join(msg, ",") } type skew struct { namespace string name string maxOpenShiftVersion string + err error } func (s skew) String() string { - return fmt.Sprintf("Operator %s in namespace %s is not compatible with OpenShift versions greater than %s", s.name, s.namespace, s.maxOpenShiftVersion) + if s.err != nil { + return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err) + } + + return fmt.Sprintf("%s/%s is incompatible with OpenShift versions greater than %s", s.namespace, s.name, s.maxOpenShiftVersion) +} + +type transientError struct { + error +} + +// transientErrors returns the result of stripping all wrapped errors not of type transientError from the given error. +func transientErrors(err error) error { + return utilerrors.FilterOut(err, func(e error) bool { + return !errors.As(e, new(transientError)) + }) } func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) { @@ -105,58 +131,59 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error } if next == nil { - return nil, nil + // Note: This shouldn't happen + return nil, fmt.Errorf("Failed to determine next OpenShift Y-stream release") } next.Minor++ csvList := &operatorsv1alpha1.ClusterServiceVersionList{} if err := cli.List(ctx, csvList); err != nil { - return nil, err + return nil, &transientError{fmt.Errorf("Failed to list ClusterServiceVersions: %w", err)} } - var ( - s skews - errs []error - ) + var incompatible skews for _, csv := range csvList.Items { if csv.IsCopied() { continue } + s := skew{ + name: csv.GetName(), + namespace: csv.GetNamespace(), + } max, err := maxOpenShiftVersion(&csv) if err != nil { - errs = append(errs, err) + s.err = err + incompatible = append(incompatible, s) continue } + if max == nil || max.GTE(*next) { continue } + s.maxOpenShiftVersion = max.String() - s = append(s, skew{ - name: csv.GetName(), - namespace: csv.GetNamespace(), - maxOpenShiftVersion: max.String(), - }) + incompatible = append(incompatible, s) } - return s, utilerrors.NewAggregate(errs) + return incompatible, nil } func desiredRelease(ctx context.Context, cli client.Client) (*semver.Version, error) { cv := configv1.ClusterVersion{} if err := cli.Get(ctx, client.ObjectKey{Name: "version"}, &cv); err != nil { // "version" is the name of OpenShift's ClusterVersion singleton - return nil, err + return nil, &transientError{fmt.Errorf("Failed to get ClusterVersion: %w", err)} } v := cv.Status.Desired.Version if v == "" { // The release version hasn't been set yet - return nil, nil + return nil, fmt.Errorf("Desired release version missing from ClusterVersion") } desired, err := semver.ParseTolerant(v) if err != nil { - return nil, err + return nil, fmt.Errorf("ClusterVersion has invalid desired release version: %w", err) } return &desired, nil @@ -178,57 +205,36 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver. return nil, err } - // Take the highest semver if there's more than one max version specified - var ( - max *semver.Version - dups []semver.Version - errs []error - ) + var max *string for _, property := range properties { if property.Type != MaxOpenShiftVersionProperty { 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 { + return nil, fmt.Errorf(`Defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty) } - if max == nil { - max = &version - continue - } - if version.LT(*max) { - continue - } - if version.EQ(*max) { - // Found a duplicate, mark it - dups = append(dups, *max) - } + max = &property.Value + } - max = &version + if max == nil { + return nil, nil } - // Return an error if THE max version has a duplicate (i.e. equivalent version) - // Note: This may not be a problem since there should be no difference as far as blocking upgrades is concerned. - // This is more for clear status messages. - for _, dup := range dups { - if max.EQ(dup) && max.String() != dup.String() { // "1.0.0" vs "1.0.0" is fine, but not "1.0.0" vs "1.0.0+1" - errs = append(errs, fmt.Errorf("max openshift version ambiguous, equivalent versions %s and %s have been specified concurrently", max, dup)) - } + // 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)`) } - if len(errs) > 0 { - return nil, utilerrors.NewAggregate(errs) + version, err := semver.ParseTolerant(value) + if err != nil { + return nil, fmt.Errorf(`Failed to parse "%s" as semver: %w`, value, err) } - return max, nil + return &version, nil } func notCopiedSelector() (labels.Selector, error) {