pkg/cvo: Compare Cincinnati data by digest when merging metadata

Fixing [1], where clusters installed with pullspecs like:

  registry.svc.ci.openshift.org/ocp/release@sha256:c7e8f18e8116356701bd23ae3a23fb9892dd5ea66c8300662ef30563d7104f39

would not pick up channel metadata that Cincinnati was associating
with canonical pullspecs like:

  quay.io/openshift-release-dev/ocp-release@sha256:c7e8f18e8116356701bd23ae3a23fb9892dd5ea66c8300662ef30563d7104f39

I really wish source-repo "hints" were not part of pullspecs, and
these images were purely content-addressable.  But wishing doesn't
make it true ;).

The "Identical by tag" case is because even though we have no
guarantee that a particular tag is stable, in any given moment there
should only be one manifest backing a given pullspec, regardless of
whether that pullspec is by-tag or by-digest.  This is unlikely to
matter in the wild, where Cincinnati will serve by-digest pullspecs.
But it currently matters for CVO unit tests, where we rely on
image/image:v1.0.0 matching for metadata population.  And it matches
the:

  merged.Image == availableUpdates.Current.Image

and similar conditions we're replacing with equalDigest (so
equalDigest will match what we used to match, and also the
same-digest-different-repo cases we didn't match before).

I'm not using the new equalDigest for some of our "is this change an
update?" logic, because we want to go through "updates" on repository
changes [2] to ensure that the release content is still available when
accessed via the new pullspec (although the presence of
ImageContentSourcePolicies may mean that even with a change to the
repository portion of the pullspec, we still end up actually pulling
the release from the same location we were initially using).

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1879976
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=1879963#c4

Author: wking

pkg/cvo/cvo.go

@@ -614,11 +614,11 @@ func (optr *Operator) mergeReleaseMetadata(release configv1.Release) configv1.Re
 		availableUpdates := optr.getAvailableUpdates()
 		if availableUpdates != nil {
 			var update *configv1.Release
-			if merged.Image == availableUpdates.Current.Image {
+			if equalDigest(merged.Image, availableUpdates.Current.Image) {
 				update = &availableUpdates.Current
 			} else {
 				for _, u := range availableUpdates.Updates {
-					if u.Image == merged.Image {
+					if equalDigest(u.Image, merged.Image) {
 						update = &u
 						break
 					}

pkg/cvo/sync_worker.go

@@ -430,11 +430,36 @@ func (w *SyncWorker) calculateNext(work *SyncWork) bool {
 }
 
 // equalUpdate returns true if two updates are semantically equivalent.
-// It checks if the updates have have the same force and image values
+// It checks if the updates have have the same force and image values.
+//
+// We require complete pullspec equality, not just digest equality,
+// because we want to go through the usual update process even if it's
+// only the registry portion of the pullspec which has changed.
 func equalUpdate(a, b configv1.Update) bool {
 	return a.Force == b.Force && a.Image == b.Image
 }
 
+// equalDigest returns true if and only if the two pullspecs are
+// by-digest pullspecs and the digests are equal or the two pullspecs
+// are identical.
+func equalDigest(pullspecA string, pullspecB string) bool {
+	if pullspecA == pullspecB {
+		return true
+	}
+	digestA := splitDigest(pullspecA)
+	return digestA != "" && digestA == splitDigest(pullspecB)
+}
+
+// splitDigest returns the pullspec's digest, or an empty string if
+// the pullspec is not by-digest (e.g. it is by-tag, or implicit).
+func splitDigest(pullspec string) string {
+	parts := strings.SplitN(pullspec, "@", 3)
+	if len(parts) != 2 {
+		return ""
+	}
+	return parts[1]
+}
+
 // equalSyncWork returns true if a and b are equal.
 func equalSyncWork(a, b *SyncWork) bool {
 	if a == b {

pkg/cvo/sync_worker_test.go

@@ -189,3 +189,98 @@ func Test_runThrottledStatusNotifier(t *testing.T) {
 	case <-time.After(100 * time.Millisecond):
 	}
 }
+
+func Test_equalDigest(t *testing.T) {
+	for _, testCase := range []struct {
+		name      string
+		pullspecA string
+		pullspecB string
+		expected  bool
+	}{
+		{
+			name:      "both empty",
+			pullspecA: "",
+			pullspecB: "",
+			expected:  true,
+		},
+		{
+			name:      "A empty",
+			pullspecA: "",
+			pullspecB: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  false,
+		},
+		{
+			name:      "B empty",
+			pullspecA: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "",
+			expected:  false,
+		},
+		{
+			name:      "A implicit tag",
+			pullspecA: "example.com",
+			pullspecB: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  false,
+		},
+		{
+			name:      "B implicit tag",
+			pullspecA: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "example.com",
+			expected:  false,
+		},
+		{
+			name:      "A by tag",
+			pullspecA: "example.com:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  false,
+		},
+		{
+			name:      "B by tag",
+			pullspecA: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "example.com:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  false,
+		},
+		{
+			name:      "identical by tag",
+			pullspecA: "example.com:latest",
+			pullspecB: "example.com:latest",
+			expected:  true,
+		},
+		{
+			name:      "different repositories, same tag",
+			pullspecA: "a.example.com:latest",
+			pullspecB: "b.example.com:latest",
+			expected:  false,
+		},
+		{
+			name:      "different repositories, same digest",
+			pullspecA: "a.example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "b.example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  true,
+		},
+		{
+			name:      "same repository, different digests",
+			pullspecA: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "example.com@sha256:01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b",
+			expected:  false,
+		},
+		{
+			name:      "A empty repository, same digest",
+			pullspecA: "@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  true,
+		},
+		{
+			name:      "B empty repository, same digest",
+			pullspecA: "example.com@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			pullspecB: "@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expected:  true,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			actual := equalDigest(testCase.pullspecA, testCase.pullspecB)
+			if actual != testCase.expected {
+				t.Fatalf("got %t, not the expected %t", actual, testCase.expected)
+			}
+		})
+	}
+}