Merge pull request #490 from wking/current-channels-by-digest

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

Author: openshift-merge-robot

pkg/cvo/availableupdates.go

@@ -215,7 +215,7 @@ func calculateAvailableUpdatesStatus(ctx context.Context, clusterID string, prox
 		}
 	}
 
-	cvoCurrent, err = convertRetreivedUpdateToRelease(current)
+	cvoCurrent, err = convertRetrievedUpdateToRelease(current)
 	if err != nil {
 		return cvoCurrent, nil, configv1.ClusterOperatorStatusCondition{
 			Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "ResponseInvalid",
@@ -225,7 +225,7 @@ func calculateAvailableUpdatesStatus(ctx context.Context, clusterID string, prox
 
 	var cvoUpdates []configv1.Release
 	for _, update := range updates {
-		cvoUpdate, err := convertRetreivedUpdateToRelease(update)
+		cvoUpdate, err := convertRetrievedUpdateToRelease(update)
 		if err != nil {
 			return cvoCurrent, nil, configv1.ClusterOperatorStatusCondition{
 				Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "ResponseInvalid",
@@ -243,7 +243,7 @@ func calculateAvailableUpdatesStatus(ctx context.Context, clusterID string, prox
 	}
 }
 
-func convertRetreivedUpdateToRelease(update cincinnati.Update) (configv1.Release, error) {
+func convertRetrievedUpdateToRelease(update cincinnati.Update) (configv1.Release, error) {
 	cvoUpdate := configv1.Release{
 		Version: update.Version.String(),
 		Image:   update.Image,

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