controllers: use TransformLegacyRevision helper

Signed-off-by: Hidde Beydals <hello@hidde.co>

Author: hiddeco

controllers/artifact.go

@@ -21,31 +21,19 @@ import sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 type artifactSet []*sourcev1.Artifact
 
 // Diff returns true if any of the revisions in the artifactSet does not match any of the given artifacts.
-func (s artifactSet) Diff(set artifactSet, comp func(x, y *sourcev1.Artifact) bool) bool {
+func (s artifactSet) Diff(set artifactSet) bool {
 	if len(s) != len(set) {
 		return true
 	}
 
-	if comp == nil {
-		comp = defaultCompare
-	}
-
 outer:
 	for _, j := range s {
 		for _, k := range set {
-			if comp(j, k) {
+			if k.HasRevision(j.Revision) {
 				continue outer
 			}
 		}
 		return true
 	}
 	return false
 }
-
-func defaultCompare(x, y *sourcev1.Artifact) bool {
-	if y == nil {
-		return false
-	}
-	return x.HasRevision(y.Revision)
-}
-

controllers/artifact_test.go

@@ -115,7 +115,7 @@ func Test_artifactSet_Diff(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := tt.current.Diff(tt.updated, nil)
+			result := tt.current.Diff(tt.updated)
 			if result != tt.expected {
 				t.Errorf("Archive() result = %v, wantResult %v", result, tt.expected)
 			}

controllers/bucket_controller.go

@@ -465,8 +465,8 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
 	// Check if index has changed compared to current Artifact revision.
 	var changed bool
 	if artifact := obj.Status.Artifact; artifact != nil && artifact.Revision != "" {
-		curRev := backwardsCompatibleDigest(artifact.Revision)
-		changed = curRev != index.Digest(curRev.Algorithm())
+		curRev := digest.Digest(sourcev1.TransformLegacyRevision(artifact.Revision))
+		changed = curRev.Validate() != nil || curRev != index.Digest(curRev.Algorithm())
 	}
 
 	// Fetch the bucket objects if required to.
@@ -518,8 +518,8 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri
 	// Set the ArtifactInStorageCondition if there's no drift.
 	defer func() {
 		if curArtifact := obj.GetArtifact(); curArtifact != nil && curArtifact.Revision != "" {
-			curRev := backwardsCompatibleDigest(curArtifact.Revision)
-			if index.Digest(curRev.Algorithm()) == curRev {
+			curRev := digest.Digest(sourcev1.TransformLegacyRevision(curArtifact.Revision))
+			if curRev.Validate() == nil && index.Digest(curRev.Algorithm()) == curRev {
 				conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
 				conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason,
 					"stored artifact: revision '%s'", artifact.Revision)
@@ -529,8 +529,8 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri
 
 	// The artifact is up-to-date
 	if curArtifact := obj.GetArtifact(); curArtifact != nil && curArtifact.Revision != "" {
-		curRev := backwardsCompatibleDigest(curArtifact.Revision)
-		if index.Digest(curRev.Algorithm()) == curRev {
+		curRev := digest.Digest(sourcev1.TransformLegacyRevision(curArtifact.Revision))
+		if curRev.Validate() == nil && index.Digest(curRev.Algorithm()) == curRev {
 			r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
 			return sreconcile.ResultSuccess, nil
 		}
@@ -796,10 +796,3 @@ func fetchIndexFiles(ctx context.Context, provider BucketProvider, obj *sourcev1
 
 	return nil
 }
-
-func backwardsCompatibleDigest(d string) digest.Digest {
-	if !strings.Contains(d, ":") {
-		d = digest.SHA256.String() + ":" + d
-	}
-	return digest.Digest(d)
-}

controllers/gitrepository_controller.go

@@ -515,7 +515,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 	}
 
 	// Observe if the artifacts still match the previous included ones
-	if artifacts.Diff(obj.Status.IncludedArtifacts, gitArtifactRevisionEqual) {
+	if artifacts.Diff(obj.Status.IncludedArtifacts) {
 		message := fmt.Sprintf("included artifacts differ from last observed includes")
 		if obj.Status.IncludedArtifacts != nil {
 			conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message)
@@ -584,8 +584,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 	}
 
 	// Mark observations about the revision on the object
-	if curArtifact := obj.Status.Artifact; curArtifact == nil ||
-		git.TransformRevision(curArtifact.Revision) != commit.String() {
+	if !obj.GetArtifact().HasRevision(commit.String()) {
 		message := fmt.Sprintf("new upstream revision '%s'", commit.String())
 		if obj.GetArtifact() != nil {
 			conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
@@ -618,9 +617,8 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
 
 	// Set the ArtifactInStorageCondition if there's no drift.
 	defer func() {
-		if curArtifact := obj.GetArtifact(); curArtifact != nil &&
-			git.TransformRevision(curArtifact.Revision) == artifact.Revision &&
-			!includes.Diff(obj.Status.IncludedArtifacts, gitArtifactRevisionEqual) &&
+		if curArtifact := obj.GetArtifact(); curArtifact.HasRevision(artifact.Revision) &&
+			!includes.Diff(obj.Status.IncludedArtifacts) &&
 			!gitContentConfigChanged(obj, includes) {
 			conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
 			conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason,
@@ -629,9 +627,8 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
 	}()
 
 	// The artifact is up-to-date
-	if curArtifact := obj.GetArtifact(); curArtifact != nil &&
-		git.TransformRevision(curArtifact.Revision) == artifact.Revision &&
-		!includes.Diff(obj.Status.IncludedArtifacts, gitArtifactRevisionEqual) &&
+	if curArtifact := obj.GetArtifact(); curArtifact.HasRevision(artifact.Revision) &&
+		!includes.Diff(obj.Status.IncludedArtifacts) &&
 		!gitContentConfigChanged(obj, includes) {
 		r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", curArtifact.Revision)
 		return sreconcile.ResultSuccess, nil
@@ -1018,7 +1015,7 @@ func gitContentConfigChanged(obj *sourcev1.GitRepository, includes *artifactSet)
 		}
 
 		// Check if the included repositories are still the same.
-		if git.TransformRevision(observedInclArtifact.Revision) != git.TransformRevision(currentIncl.Revision) {
+		if !observedInclArtifact.HasRevision(currentIncl.Revision) {
 			return true
 		}
 		if observedInclArtifact.Checksum != currentIncl.Checksum {
@@ -1041,10 +1038,3 @@ func gitRepositoryIncludeEqual(a, b sourcev1.GitRepositoryInclude) bool {
 	}
 	return true
 }
-
-func gitArtifactRevisionEqual(x, y *sourcev1.Artifact) bool {
-	if x == nil || y == nil {
-		return false
-	}
-	return git.TransformRevision(x.Revision) == git.TransformRevision(y.Revision)
-}

controllers/gitrepository_controller_test.go

@@ -2215,7 +2215,7 @@ func TestGitRepositoryReconciler_fetchIncludes(t *testing.T) {
 			g.Expect(err != nil).To(Equal(tt.wantErr))
 			g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions))
 			if !tt.wantErr && gotArtifactSet != nil {
-				g.Expect(gotArtifactSet.Diff(tt.wantArtifactSet, gitArtifactRevisionEqual)).To(BeFalse())
+				g.Expect(gotArtifactSet.Diff(tt.wantArtifactSet)).To(BeFalse())
 			}
 		})
 	}

controllers/helmchart_controller.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"github.com/fluxcd/pkg/git"
+	"github.com/opencontainers/go-digest"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -792,7 +793,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj
 			rev = git.ExtractHashFromRevision(rev).String()
 		}
 		if obj.Spec.SourceRef.Kind == sourcev1.BucketKind {
-			rev = backwardsCompatibleDigest(rev).Hex()
+			if dig := digest.Digest(sourcev1.TransformLegacyRevision(rev)); dig.Validate() == nil {
+				rev = dig.Hex()
+			}
 		}
 		if kind := obj.Spec.SourceRef.Kind; kind == sourcev1.GitRepositoryKind || kind == sourcev1.BucketKind {
 			// The SemVer from the metadata is at times used in e.g. the label metadata for a resource
@@ -1243,10 +1246,9 @@ func (r *HelmChartReconciler) requestsForGitRepositoryChange(o client.Object) []
 		return nil
 	}
 
-	revision := git.TransformRevision(repo.GetArtifact().Revision)
 	var reqs []reconcile.Request
 	for _, i := range list.Items {
-		if git.TransformRevision(i.Status.ObservedSourceArtifactRevision) != revision {
+		if !repo.GetArtifact().HasRevision(i.Status.ObservedSourceArtifactRevision) {
 			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)})
 		}
 	}
@@ -1271,10 +1273,9 @@ func (r *HelmChartReconciler) requestsForBucketChange(o client.Object) []reconci
 		return nil
 	}
 
-	revision := backwardsCompatibleDigest(bucket.GetArtifact().Revision)
 	var reqs []reconcile.Request
 	for _, i := range list.Items {
-		if backwardsCompatibleDigest(i.Status.ObservedSourceArtifactRevision) != revision {
+		if !bucket.GetArtifact().HasRevision(i.Status.ObservedSourceArtifactRevision) {
 			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)})
 		}
 	}

controllers/ocirepository_controller.go

@@ -22,7 +22,6 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
-	"github.com/fluxcd/pkg/git"
 	"io"
 	"net/http"
 	"os"
@@ -390,7 +389,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 		return sreconcile.ResultEmpty, e
 	}
 
-	// Get the upstream revision from the artifact revision
+	// Get the upstream revision from the artifact digest
 	revision, err := r.getRevision(url, opts.craneOpts)
 	if err != nil {
 		e := serror.NewGeneric(
@@ -405,7 +404,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 
 	// Mark observations about the revision on the object
 	defer func() {
-		if obj.GetArtifact() == nil || git.TransformRevision(obj.GetArtifact().Revision) != git.TransformRevision(revision) {
+		if !obj.GetArtifact().HasRevision(revision) {
 			message := fmt.Sprintf("new revision '%s' for '%s'", revision, url)
 			if obj.GetArtifact() != nil {
 				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
@@ -425,7 +424,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 	if obj.Spec.Verify == nil {
 		// Remove old observations if verification was disabled
 		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
-	} else if (obj.GetArtifact() == nil || git.TransformRevision(obj.GetArtifact().Revision) != git.TransformRevision(revision)) ||
+	} else if !obj.GetArtifact().HasRevision(revision) ||
 		conditions.GetObservedGeneration(obj, sourcev1.SourceVerifiedCondition) != obj.Generation ||
 		conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) {
 
@@ -458,9 +457,7 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 
 	// Skip pulling if the artifact revision and the source configuration has
 	// not changed.
-	if (obj.GetArtifact() != nil &&
-		git.TransformRevision(obj.GetArtifact().Revision) == git.TransformRevision(revision)) &&
-		!ociContentConfigChanged(obj) {
+	if obj.GetArtifact().HasRevision(revision) && !ociContentConfigChanged(obj) {
 		conditions.Delete(obj, sourcev1.FetchFailedCondition)
 		return sreconcile.ResultSuccess, nil
 	}
@@ -584,7 +581,8 @@ func (r *OCIRepositoryReconciler) selectLayer(obj *sourcev1.OCIRepository, image
 	return blob, nil
 }
 
-// getRevision fetches the upstream revision and returns the revision in the format `<tag>/<revision>`
+// getRevision fetches the upstream digest, returning the revision in the
+// format '<tag>@<digest>'.
 func (r *OCIRepositoryReconciler) getRevision(url string, options []crane.Option) (string, error) {
 	ref, err := name.ParseReference(url)
 	if err != nil {
@@ -618,14 +616,15 @@ func (r *OCIRepositoryReconciler) getRevision(url string, options []crane.Option
 	return revision, nil
 }
 
-// digestFromRevision extract the revision from the revision string
+// digestFromRevision extracts the digest from the revision string.
 func (r *OCIRepositoryReconciler) digestFromRevision(revision string) string {
 	parts := strings.Split(revision, "@")
 	return parts[len(parts)-1]
 }
 
-// verifySignature verifies the authenticity of the given image reference url. First, it tries using a key
-// if a secret with a valid public key is provided. If not, it falls back to a keyless approach for verification.
+// verifySignature verifies the authenticity of the given image reference URL.
+// First, it tries to use a key if a Secret with a valid public key is provided.
+// If not, it falls back to a keyless approach for verification.
 func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sourcev1.OCIRepository, url string, opt ...remote.Option) error {
 	ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
@@ -953,11 +952,9 @@ func (r *OCIRepositoryReconciler) reconcileStorage(ctx context.Context, sp *patc
 // and the symlink in the Storage is updated to its path.
 func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *patch.SerialPatcher,
 	obj *sourcev1.OCIRepository, metadata *sourcev1.Artifact, dir string) (sreconcile.Result, error) {
-	revision := metadata.Revision
-
 	// Create artifact
-	artifact := r.Storage.NewArtifactFor(obj.Kind, obj, revision,
-		fmt.Sprintf("%s.tar.gz", r.digestFromRevision(revision)))
+	artifact := r.Storage.NewArtifactFor(obj.Kind, obj, metadata.Revision,
+		fmt.Sprintf("%s.tar.gz", r.digestFromRevision(metadata.Revision)))
 
 	// Set the ArtifactInStorageCondition if there's no drift.
 	defer func() {
@@ -969,9 +966,7 @@ func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
 	}()
 
 	// The artifact is up-to-date
-	if (obj.GetArtifact() != nil &&
-		git.TransformRevision(obj.GetArtifact().Revision) == git.TransformRevision(revision)) &&
-		!ociContentConfigChanged(obj) {
+	if obj.GetArtifact().HasRevision(artifact.Revision) && !ociContentConfigChanged(obj) {
 		r.eventLogf(ctx, obj, eventv1.EventTypeTrace, sourcev1.ArtifactUpToDateReason,
 			"artifact up-to-date with remote revision: '%s'", artifact.Revision)
 		return sreconcile.ResultSuccess, nil

controllers/ocirepository_controller_test.go

@@ -1281,7 +1281,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 func TestOCIRepository_reconcileSource_noop(t *testing.T) {
 	g := NewWithT(t)
 
-	testRevision := "6.1.5@sha256:8a0eed109e056ab1f7e70e8fb47e00cf6f560ca5cd910c83451882e07edb77fa"
+	testRevision := "6.1.5@sha256:8e4057c22d531d40e12b065443cb0d80394b7257c4dc557cb1fbd4dce892b86d"
 
 	tmpDir := t.TempDir()
 	server, err := setupRegistryServer(ctx, tmpDir, registryOptions{})
@@ -1319,18 +1319,7 @@ func TestOCIRepository_reconcileSource_noop(t *testing.T) {
 			name: "noop - artifact revisions match (legacy)",
 			beforeFunc: func(obj *sourcev1.OCIRepository) {
 				obj.Status.Artifact = &sourcev1.Artifact{
-					Revision: "6.1.5/8a0eed109e056ab1f7e70e8fb47e00cf6f560ca5cd910c83451882e07edb77fa",
-				}
-			},
-			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
-				g.Expect(artifact.Metadata).To(BeEmpty())
-			},
-		},
-		{
-			name: "noop - artifact revisions match (legacy: digest)",
-			beforeFunc: func(obj *sourcev1.OCIRepository) {
-				obj.Status.Artifact = &sourcev1.Artifact{
-					Revision: "8a0eed109e056ab1f7e70e8fb47e00cf6f560ca5cd910c83451882e07edb77fa",
+					Revision: "6.1.5/8e4057c22d531d40e12b065443cb0d80394b7257c4dc557cb1fbd4dce892b86d",
 				}
 			},
 			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
@@ -2257,7 +2246,7 @@ func pushMultiplePodinfoImages(serverURL string, versions ...string) (map[string
 func setPodinfoImageAnnotations(img gcrv1.Image, tag string) gcrv1.Image {
 	metadata := map[string]string{
 		oci.SourceAnnotation:   "https://github.com/stefanprodan/podinfo",
-		oci.RevisionAnnotation: fmt.Sprintf("%s@sha256:8a0eed109e056ab1f7e70e8fb47e00cf6f560ca5cd910c83451882e07edb77fa", tag),
+		oci.RevisionAnnotation: fmt.Sprintf("%s@sha1:b3b00fe35424a45d373bf4c7214178bc36fd7872", tag),
 	}
 	return mutate.Annotations(img, metadata).(gcrv1.Image)
 }