fix(openshift): block upgrades on invalid max properties #2302

njhale · 2021-07-29T04:59:04Z

Fail open when olm.maxOpenShiftVersion properties have invalid
values. This means that all invalid values are logged but are ultimately
ignored when determining cluster upgradeability.

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.

joelanford · 2021-07-29T14:27:55Z

Is ignoring invalid max versions the right thing to do here? It seems like it would be fairly easy for someone to accidentally fat-finger a version number and end up getting an upgrade when one isn't actually supported, which could then lead to an even worse situation.

Is it possible to error out and notify the user in some way that they have an invalid property value?

exdx · 2021-07-29T15:05:52Z

Is ignoring invalid max versions the right thing to do here? It seems like it would be fairly easy for someone to accidentally fat-finger a version number and end up getting an upgrade when one isn't actually supported, which could then lead to an even worse situation.

Is it possible to error out and notify the user in some way that they have an invalid property value?

I agree, maybe it safer to fail closed versus fail open when its a (delicate) update process?

njhale · 2021-07-29T15:28:53Z

@joelanford @exdx

Thanks for the quick review.

Is it possible to error out and notify the user in some way that they have an invalid property value?

I had the same thought as I was implementing this. I'll update the logic to fail closed and bubble an error to the ClusterVersion when an unparsable max property is found.

(part of the reason I did it like this in the first place was the expectation of QE to fail open in the original BZ)

njhale · 2021-07-29T20:21:45Z

/hold

until I update this

openshift-ci · 2021-07-30T06:22:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhale

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [njhale]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

njhale · 2021-07-30T06:31:17Z

/hold cancel

updated behavior to address feedback.

pkg/controller/operators/openshift/helpers.go

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 <[email protected]>

njhale · 2021-07-30T19:16:33Z

/hold cancel

added behavior

exdx

Just some thoughts, we can let @joelanford review as well.
/lgtm
/hold

exdx · 2021-08-02T14:45:47Z

pkg/controller/operators/openshift/helpers.go

@@ -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{}


nit: is it possible to build this list excluding copied CSVs via the annotation, so we don't potentially make a huge cache call? Not sure how much that matters though.

Maybe we could do something with the newly introduced metadata client?

My preference would be to optimize in a follow up, since the topic of the PR is potentially release blocking.

exdx · 2021-08-02T14:49:18Z

pkg/controller/operators/openshift/helpers.go

-		s    skews
-		errs []error
-	)
+	var incompatible skews
 	for _, csv := range csvList.Items {


One case I'm thinking about is where Operator A has a defined maxOpenshiftVersion property, but depends on Operator B which does not have one defined. In that case, on upgrade Operator A will report ready, so will Operator B, but B may break, breaking A.

Catching this case would require doing introspection on the CSV, and checking any dependencies for their properties, which may be overly restrictive. But it's a scenario you could imagine happening on the cluster somewhat frequently if a popular dependency doesn't specify its maxOpenshiftVersion. Not sure how important this is to address up-front though.

In that case, on upgrade Operator A will report ready, so will Operator B, but B may break, breaking A.

could you expand on how this relates to blocking cluster upgrades wrt maxOpenShiftVersion?

I guess it would not block cluster upgrades per se but this scenario would break the Operator that was using the maxOpenShiftVersion property, since its dependencies don't specify max version

joelanford · 2021-08-02T18:34:06Z

pkg/controller/operators/openshift/clusteroperator_controller_test.go

+				{
+					namespace: ns.GetName(),
+					name:      incompatible.GetName(),
+					err:       fmt.Errorf(`Defining more than one "%s" property is not allowed`, MaxOpenShiftVersionProperty),


For future reference, you can also use %q instead of "%s" to get a quoted string, which helps when you'd rather use double quotes to define the string literal.

joelanford · 2021-08-02T18:36:18Z

pkg/controller/operators/openshift/helpers.go

 }

 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)}


One thing that just crossed my mind: Is this section of code disabled in non-OpenShift clusters? I assume maxOpenshiftVersion is just ignored unless we're in Openshift?

It looks like the only call site for this function is in incompatibleOperators, and that function is only used in the context of the ClusterOperator controller that handles the maxOpenshiftVersion logic.

I guess we gate the creation of this controller on the --writeStatusName CLI flag that we set downstream, which seems like a relatively loose check (couldn't we discovery this API dynamically instead?), so tl;dr should be ignored on non-OCP clusters.

@joelanford, @timflannagan hit the nail on the head: the entire controller is disabled unless --writeStatusName is given when running olm-operator.

timflannagan · 2021-08-02T21:10:06Z

pkg/controller/operators/openshift/clusteroperator_controller.go

+			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


Just trying to wrap my head around whether it makes sense to specify configv1.ConditionTrue if we're setting the reason to IncompatibleOperatorsInstalled when encountering incompatibility?

Disregard - I realize I was missing the full context and needed to look at the full function control flow.

timflannagan · 2021-08-02T21:13:04Z

/lgtm

joelanford · 2021-08-02T23:51:35Z

/lgtm
/hold cancel

openshift-ci bot requested review from anik120 and dinhxuanvu July 29, 2021 04:59

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2021

njhale force-pushed the fix/openshift/compat-best-effort branch from 53b048b to 9780e9a Compare July 29, 2021 04:59

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2021

njhale force-pushed the fix/openshift/compat-best-effort branch 4 times, most recently from 45c1dd9 to 1ec1e4b Compare July 30, 2021 06:21

njhale force-pushed the fix/openshift/compat-best-effort branch from 1ec1e4b to 10bf3c2 Compare July 30, 2021 06:27

njhale changed the title ~~fix(openshift): ignore invalid max properties~~ fix(openshift): block upgrades on invalid max properties Jul 30, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

njhale force-pushed the fix/openshift/compat-best-effort branch from 10bf3c2 to 649f635 Compare July 30, 2021 13:19

joelanford reviewed Jul 30, 2021

View reviewed changes

pkg/controller/operators/openshift/helpers.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

njhale force-pushed the fix/openshift/compat-best-effort branch from 649f635 to 1314632 Compare July 30, 2021 19:16

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

exdx reviewed Aug 2, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

openshift-ci bot assigned exdx Aug 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021

joelanford reviewed Aug 2, 2021

View reviewed changes

timflannagan reviewed Aug 2, 2021

View reviewed changes

openshift-ci bot assigned timflannagan Aug 2, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2021

openshift-ci bot assigned joelanford Aug 2, 2021

openshift-ci bot merged commit 734c6d0 into operator-framework:master Aug 2, 2021

fix(openshift): block upgrades on invalid max properties #2302

fix(openshift): block upgrades on invalid max properties #2302

Uh oh!

Conversation

njhale commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelanford commented Jul 29, 2021

Uh oh!

exdx commented Jul 29, 2021

Uh oh!

njhale commented Jul 29, 2021

Uh oh!

njhale commented Jul 29, 2021

Uh oh!

openshift-ci bot commented Jul 30, 2021

Uh oh!

njhale commented Jul 30, 2021

Uh oh!

Uh oh!

njhale commented Jul 30, 2021

Uh oh!

exdx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhale Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timflannagan commented Aug 2, 2021

Uh oh!

joelanford commented Aug 2, 2021

Uh oh!

Uh oh!

njhale commented Jul 29, 2021 •

edited

Loading

njhale Aug 2, 2021 •

edited

Loading

joelanford Aug 2, 2021 •

edited

Loading