*: Port from Update to Release for ClusterVersion status #419

wking · 2020-07-28T22:48:19Z

Bump dependencies to pull in openshift/api#521, and then port our status code to use the new Release type.

This is mostly mechanical, except for the next/last Actual comparison, where I had to avoid:

invalid operation: next.Actual == last.Actual (struct containing []string cannot be compared)

now that Release has a slice of channels. Now I'm comparing only Image, which is sufficient for "is this a different target?". The test suite's expected LastProgress changes are because the shift from:

{Version: "4.0.1", Image: "image/image:1"}

to:

{Version: "1.0.0-abc", Image: "image/image:1"}

no longer counts as a LastProgress-bumping change, because the Image remains unchanged.

WIP because I still have some FIXMEs in there about populating the new Release properties.

pkg/autoupdate/autoupdate.go

pkg/cvo/availableupdates.go

wking · 2020-07-29T19:56:14Z

Pulling the WIP, because 3a84314 -> f666975 addresses the outstanding issues with populating the new Release properties. Review is probably most convenient via git show --word-diff=color ..., because that helps isolate the bits I touched from the bits I didn't (event when an untouched string shifts lines).

pkg/cvo/cvo.go

pkg/cvo/status.go

pkg/cvo/updatepayload.go

pkg/cvo/availableupdates.go

The next/last Actual comparison avoids: invalid operation: next.Actual == last.Actual (struct containing []string cannot be compared) The image pullspec is sufficient for that matching. The test suite's expected LastProgress changes are because the shift from: {Version: "4.0.1", Image: "image/image:1"} to: {Version: "1.0.0-abc", Image: "image/image:1"} no longer counts as a LastProgress-bumping change (because we're now only comparing the Image pullspecs). Also adjusts the upstream/Cincinnati retrieval and payload loading logic to preserve the additional metadata that we're now exposing. Storing a Release on the Operator, instead of extending the previous releaseVersion/releaseImage pattern, makes it easy to keep track of payload-loaded metadata, so we can fall-back to upstream metadata for properties where the payload has no opinion, as described in the enhancement. In findUpdateFromConfigVersion, if some crazy upstream service declared a node with a matching version but no image, we used to say "sorry, no match" without looking through history. Now we skip the useless entry and move on to consider later entries and history. We only say "no match" if we have no image pullspec associated with the requested version available.

LalatenduMohanty · 2020-07-31T17:23:59Z

@wking The commit message in 58356de is too verbose IMO and hard to read/understand. If you can reduce the text and make it more readable, it would be good.

LalatenduMohanty · 2020-07-31T17:34:10Z

pkg/cvo/cvo.go

-	releaseVersion string
+	// release is the release the current operator points to and
+	// metadata read from the release image.  It allows templating of
+	// the CVO deployment manifest.


This comment does not communicate properly about release API. We should reword this or we can remove the comment all together. Also we can fix this later after this PR merges.

This is internal state not directly related to external APIs. Rewording suggestions?

LalatenduMohanty · 2020-07-31T18:00:47Z

pkg/cvo/cvo.go

+// mergeReleaseMetadata returns a deep copy of the input release.
+// Values from any matching availableUpdates release are used as
+// fallbacks for any properties not defined in the input release.
+func (optr *Operator) mergeReleaseMetadata(release configv1.Release) configv1.Release {


I would have named this as updateReleaseMetadata() but we can handle this in another PR.

It doesn't update in place, it returns a new object merging data from two sources

wking · 2020-07-31T18:11:35Z

...too verbose IMO and hard to read/understand...

The commit diff is also verbose and hard to understand. I'd like to explain my motivation behind the fiddly bits. Did you have specific sections which need rewording?

LalatenduMohanty · 2020-07-31T18:13:22Z

The commit diff is also verbose and hard to understand. I'd like to explain my motivation behind the fiddly bits. Did you have specific sections which need rewording?

The next/last Actual comparison avoids:

  invalid operation: next.Actual == last.Actual (struct containing []string cannot be compared)

The image pullspec is sufficient for that matching.  The test suite's
expected LastProgress changes are because the shift from:

  {Version: "4.0.1", Image: "image/image:1"}

to:

  {Version: "1.0.0-abc", Image: "image/image:1"}

no longer counts as a LastProgress-bumping change (because we're now
only comparing the Image pullspecs).

wking · 2020-07-31T18:17:44Z

The next/last Actual comparison avoids:...

Is that pivot obvious without an explanation? Or is the explanation unclear?

LalatenduMohanty · 2020-07-31T18:23:35Z

Is that pivot obvious without an explanation? Or is the explanation unclear?

It is just hard to understand the context and why the text is there.

wking · 2020-07-31T18:45:30Z

Context is changes like this, which you should be able to find in the diff by searching for the mentioned next.Actual == last.Actual, and the remaining text explains (or attempts to explain) why the move to Release forced me to make a change, and why I chose the change I did.

LalatenduMohanty · 2020-07-31T19:13:02Z

Context is changes like this, which you should be able to find in the diff by searching for the mentioned next.Actual == last.Actual, and the remaining text explains (or attempts to explain) why the move to Release forced me to make a change, and why I chose the change I did.

I still cannot understand why this needs to go to the commit message.

LalatenduMohanty

/lgtm

openshift-ci-robot · 2020-07-31T19:25:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, wking

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~~ [LalatenduMohanty,wking]

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

wking · 2020-07-31T19:35:54Z

I still cannot understand why this needs to go to the commit message.

So in a year when we reroll this, we remember the motivation ;)

openshift-bot · 2020-07-31T20:39:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-31T20:52:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

wking · 2020-07-31T23:31:42Z

e2e had som Prom issues.

/retest

openshift-bot · 2020-08-01T00:33:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T02:43:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T02:56:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

If, for example, the configured ClusterVersion spec.upstream advertised a given image as 4.0.1, but the version baked into the release's own metadata was 1.0.0-abc, report VerifyPayloadVersionFailed and continue to apply the previous release image, instead of optimistically moving on to apply the new release image. This protects users from compromised or man-in-the-middled upstreams who attempt downgrade and similar attacks by misrepresenting a recommended update. Addressing a FIXME from 58356de (*: Port from Update to Release for ClusterVersion status, 2020-07-24, openshift#419).

wking · 2020-08-09T20:44:44Z

pkg/cvo/sync_worker.go

+		} else if payloadUpdate.Release.Version != work.Desired.Version {
+			err = fmt.Errorf("release image version %s does not match the expected upstream version %s", payloadUpdate.Release.Version, work.Desired.Version)
+			w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "VerifyPayloadVersionFailed", "verifying payload failed version=%q image=%q failure=%v", work.Desired.Version, work.Desired.Image, err)
+			/* FIXME: Ignore for now.  I will make this fatal in a follow-up pivot


I've opened #431 to address this.

If, for example, the configured ClusterVersion spec.upstream advertised a given image as 4.0.1, but the version baked into the release's own metadata was 1.0.0-abc, report VerifyPayloadVersionFailed and continue to apply the previous release image, instead of optimistically moving on to apply the new release image. This protects users from compromised or man-in-the-middled upstreams who attempt downgrade and similar attacks by misrepresenting a recommended update. Addressing a FIXME from 58356de (*: Port from Update to Release for ClusterVersion status, 2020-07-24, openshift#419).

Typo is from 58356de (*: Port from Update to Release for ClusterVersion status, 2020-07-24, openshift#419).

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2020

openshift-ci-robot requested review from abhinavdahiya and vrutkovs July 28, 2020 22:48

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020

wking force-pushed the release-metadata branch 2 times, most recently from f8c787d to 3a84314 Compare July 29, 2020 03:56

LalatenduMohanty reviewed Jul 29, 2020

View reviewed changes

pkg/autoupdate/autoupdate.go Show resolved Hide resolved

LalatenduMohanty reviewed Jul 29, 2020

View reviewed changes

pkg/cvo/availableupdates.go Show resolved Hide resolved

wking mentioned this pull request Jul 29, 2020

vendor: Bump client-go and library-go to current 4.6 tips (2020-07-24) #420

Merged

wking force-pushed the release-metadata branch from 3a84314 to f666975 Compare July 29, 2020 19:54

wking changed the title ~~WIP: *: Port from Update to Release for ClusterVersion status~~ *: Port from Update to Release for ClusterVersion status Jul 29, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2020