diff --git a/controllers/artifact.go b/controllers/artifact.go index 21023b227..55a545d4e 100644 --- a/controllers/artifact.go +++ b/controllers/artifact.go @@ -21,19 +21,15 @@ 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 } } @@ -41,11 +37,3 @@ outer: } return false } - -func defaultCompare(x, y *sourcev1.Artifact) bool { - if y == nil { - return false - } - return x.HasRevision(y.Revision) -} - diff --git a/controllers/artifact_test.go b/controllers/artifact_test.go index 36a014d3a..935c93bf7 100644 --- a/controllers/artifact_test.go +++ b/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) } diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index f95d536fa..265a85323 100644 --- a/controllers/bucket_controller.go +++ b/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) -} diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 354e00316..eba8cac72 100644 --- a/controllers/gitrepository_controller.go +++ b/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) -} diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c648502b2..00ad71f92 100644 --- a/controllers/gitrepository_controller_test.go +++ b/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()) } }) } diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index e96d67bbc..c4d9f15c0 100644 --- a/controllers/helmchart_controller.go +++ b/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)}) } } diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 40c556182..880d0a5d0 100644 --- a/controllers/ocirepository_controller.go +++ b/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 `/` +// getRevision fetches the upstream digest, returning the revision in the +// format '@'. 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 diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 39afcc008..bf4003b9d 100644 --- a/controllers/ocirepository_controller_test.go +++ b/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) }