From ff87fbb223d54b34bf0e7bdbdb5fea8cec20036d Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 11 Sep 2025 01:51:50 +0300 Subject: [PATCH] Add support for ExternalArtifact revision with digest Signed-off-by: Stefan Prodan --- internal/controller/helmrelease_controller.go | 90 ++++++++++++------- internal/controller/helmrelease_indexers.go | 40 +++++---- .../controller_test/external_artifact_test.go | 83 +++++++++++++---- 3 files changed, 146 insertions(+), 67 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 16bc3b4e8..af9c6e378 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -913,39 +913,31 @@ func isValidChartRef(obj *v2.HelmRelease) bool { (!obj.HasChartRef() && obj.HasChartTemplate()) } +// mutateChartWithSourceRevision mutates the chart version by appending the +// digest part of the source revision to the chart version metadata. +// It returns the digest that was appended, or an error if the mutation failed. func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart, source sourcev1.Source) (string, error) { - // If the source is an OCIRepository, we can try to mutate the chart version - // with the artifact revision. The revision is either a @ or - // just a digest. - obj, ok := source.(*sourcev1.OCIRepository) - if !ok { - // if not make sure to return an empty string to delete the digest of the - // last attempted revision + if !isSourceWithRevisionDigest(source) { + // Clear any previously set digest in status + // if the source revision does not contain a digest. return "", nil } ver, err := semver.NewVersion(chart.Metadata.Version) if err != nil { - return "", err + return "", fmt.Errorf("invalid chart version %s", chart.Metadata.Version) } - var ociDigest string - revision := obj.GetArtifact().Revision + var revDigest string + revision := source.GetArtifact().Revision + tagVer, isSemverTag := parseSemverFromTag(revision) switch { - case strings.Contains(revision, "@"): - tagD := strings.Split(revision, "@") - // replace '+' with '_' for OCI tag semver compatibility - // per https://github.com/helm/helm/blob/v3.14.4/pkg/registry/client.go#L45-L50 - tagConverted := strings.ReplaceAll(tagD[0], "_", "+") - tagVer, err := semver.NewVersion(tagConverted) - if err != nil { - return "", fmt.Errorf("failed parsing artifact revision %s", tagConverted) + case isSemverTag: + tagDigestPair := strings.Split(revision, "@") + if len(tagDigestPair) != 2 || !tagVer.Equal(ver) { + return "", fmt.Errorf("artifact revision %s does not match chart version %s", tagDigestPair[0], chart.Metadata.Version) } - if len(tagD) != 2 || !tagVer.Equal(ver) { - return "", fmt.Errorf("artifact revision %s does not match chart version %s", tagD[0], chart.Metadata.Version) - } - // algotithm are sha256, sha384, sha512 with the canonical being sha256 - // So every digest starts with a sha algorithm and a colon - sha, err := extractDigestSubString(tagD[1]) + // The digest should be in format : + sha, err := extractDigestSubString(tagDigestPair[1]) if err != nil { return "", err } @@ -954,7 +946,7 @@ func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart if err != nil { return "", err } - ociDigest = tagD[1] + revDigest = tagDigestPair[1] default: // default to the digest sha, err := extractDigestSubString(revision) @@ -965,14 +957,52 @@ func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart if err != nil { return "", err } - ociDigest = revision + revDigest = revision } if !r.DisableChartDigestTracking { chart.Metadata.Version = ver.String() } - return ociDigest, nil + return revDigest, nil +} + +// isSourceWithRevisionDigest returns true if the source +// is of a type that supports immutable revisions, +// such as OCIRepository or ExternalArtifact. +func isSourceWithRevisionDigest(source sourcev1.Source) bool { + if _, ok := source.(*sourcev1.OCIRepository); ok { + return true + } + if _, ok := source.(*sourcev1.ExternalArtifact); ok { + // ExternalArtifact revision can be a semantic version or a digest. + // We only want to mutate the chart version if the revision contains a digest. + if source.GetArtifact() != nil && + strings.Contains(source.GetArtifact().Revision, ":") { + return true + } + } + return false +} + +// parseSemverFromTag attempts to parse a semver version from a tag +// in the @: or format. +// It returns the parsed semver version, or nil if the tag does not +// contain a valid semver version. The boolean return value indicates +// whether the parsing was successful. +func parseSemverFromTag(tag string) (*semver.Version, bool) { + semverCandidate := tag + if strings.Contains(tag, "@") { + tagDigestPair := strings.Split(tag, "@") + // Replace '+' with '_' for OCI tag semver compatibility + // per https://github.com/helm/helm/blob/v3.14.4/pkg/registry/client.go#L45-L50 + semverCandidate = strings.ReplaceAll(tagDigestPair[0], "_", "+") + } + ver, err := semver.NewVersion(semverCandidate) + if err != nil { + return nil, false + } + return ver, true } func extractDigestSubString(revision string) (string, error) { @@ -992,11 +1022,11 @@ func extractDigestSubString(revision string) (string, error) { func extractDigest(revision string) string { if strings.Contains(revision, "@") { // expects a revision in the @: format - tagD := strings.Split(revision, "@") - if len(tagD) != 2 { + tagDigestPair := strings.Split(revision, "@") + if len(tagDigestPair) != 2 { return "" } - return tagD[1] + return tagDigestPair[1] } else { // revision in the : format return revision diff --git a/internal/controller/helmrelease_indexers.go b/internal/controller/helmrelease_indexers.go index d7a42f223..69f6b0657 100644 --- a/internal/controller/helmrelease_indexers.go +++ b/internal/controller/helmrelease_indexers.go @@ -19,6 +19,7 @@ package controller import ( "context" "fmt" + "strings" "github.com/fluxcd/pkg/runtime/conditions" sourcev1 "github.com/fluxcd/source-controller/api/v1" @@ -96,7 +97,8 @@ func (r *HelmReleaseReconciler) requestsForOCIRepositoryChange(ctx context.Conte continue } - if digest == hr.Status.LastAttemptedRevisionDigest { + // Skip if the HelmRelease is ready and the digest matches the last attempted revision digest. + if conditions.IsReady(&list.Items[i]) && digest == hr.Status.LastAttemptedRevisionDigest { continue } @@ -108,38 +110,40 @@ func (r *HelmReleaseReconciler) requestsForOCIRepositoryChange(ctx context.Conte // requestsForExternalArtifactChange enqueues requests for watched ExternalArtifacts // according to the specified index. func (r *HelmReleaseReconciler) requestsForExternalArtifactChange(ctx context.Context, o client.Object) []reconcile.Request { - or, ok := o.(*sourcev1.ExternalArtifact) + log := ctrl.LoggerFrom(ctx) + ea, ok := o.(*sourcev1.ExternalArtifact) if !ok { err := fmt.Errorf("expected an ExternalArtifact, got %T", o) - ctrl.LoggerFrom(ctx).Error(err, "failed to get requests for ExternalArtifact change") + log.Error(err, "failed to get requests for ExternalArtifact change") return nil } // If we do not have an artifact, we have no requests to make - if or.GetArtifact() == nil { + if ea.GetArtifact() == nil { return nil } var list v2.HelmReleaseList if err := r.List(ctx, &list, client.MatchingFields{ - v2.SourceIndexKey: sourcev1.ExternalArtifactKind + "/" + client.ObjectKeyFromObject(or).String(), + v2.SourceIndexKey: sourcev1.ExternalArtifactKind + "/" + client.ObjectKeyFromObject(ea).String(), }); err != nil { - ctrl.LoggerFrom(ctx).Error(err, "failed to list HelmReleases for ExternalArtifact change") + log.Error(err, "failed to list HelmReleases for ExternalArtifact change") return nil } - var reqs []reconcile.Request for i, hr := range list.Items { - // If the HelmRelease is ready and the digest of the artifact equals to the - // last attempted revision digest, we should not make a request for this HelmRelease, - // likewise if we cannot retrieve the artifact digest. - digest := extractDigest(or.GetArtifact().Revision) - if digest == "" { - ctrl.LoggerFrom(ctx).Error(fmt.Errorf("wrong digest for %T", or), "failed to get requests for ExternalArtifact change") - continue - } - - if digest == hr.Status.LastAttemptedRevisionDigest { - continue + revision := ea.GetArtifact().Revision + + // Handle both revision formats: digest or semantic version. + if strings.Contains(revision, ":") { + // Skip if the HelmRelease is ready and the digest matches the last attempted revision digest. + if conditions.IsReady(&list.Items[i]) && extractDigest(revision) == hr.Status.LastAttemptedRevisionDigest { + continue + } + } else { + // Skip if the HelmRelease is ready and the revision matches the last attempted revision. + if conditions.IsReady(&list.Items[i]) && revision == hr.Status.LastAttemptedRevision { + continue + } } reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) diff --git a/internal/controller_test/external_artifact_test.go b/internal/controller_test/external_artifact_test.go index ab67201da..ae5d03327 100644 --- a/internal/controller_test/external_artifact_test.go +++ b/internal/controller_test/external_artifact_test.go @@ -18,6 +18,7 @@ package controller_test import ( "context" + "fmt" "testing" "time" @@ -54,7 +55,7 @@ func TestExternalArtifact_LifeCycle(t *testing.T) { Namespace: ns.Name, Name: "test-ea", } - ea, err := applyExternalArtifact(eaKey, revision) + ea, err := applyExternalArtifact(eaKey, revision, "") g.Expect(err).ToNot(HaveOccurred(), "failed to create ExternalArtifact") // Create a HelmRelease that references the ExternalArtifact @@ -80,7 +81,8 @@ func TestExternalArtifact_LifeCycle(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) t.Run("installs from external artifact", func(t *testing.T) { - g.Eventually(func() bool { + gt := NewWithT(t) + gt.Eventually(func() bool { err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) if err != nil { return false @@ -88,16 +90,17 @@ func TestExternalArtifact_LifeCycle(t *testing.T) { return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition) }, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not become ready") - g.Expect(hr.Status.LastAttemptedRevision).To(Equal(revision)) - g.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionInstall)) + gt.Expect(hr.Status.LastAttemptedRevision).To(Equal(revision)) + gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionInstall)) }) t.Run("upgrades at external artifact revision change", func(t *testing.T) { + gt := NewWithT(t) newRevision := "2.0.0" - ea, err = applyExternalArtifact(eaKey, newRevision) - g.Expect(err).ToNot(HaveOccurred()) + ea, err = applyExternalArtifact(eaKey, newRevision, "") + gt.Expect(err).ToNot(HaveOccurred()) - g.Eventually(func() bool { + gt.Eventually(func() bool { err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) if err != nil { return false @@ -106,17 +109,55 @@ func TestExternalArtifact_LifeCycle(t *testing.T) { hr.Status.LastAttemptedRevision == newRevision }, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade") - g.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade)) + gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade)) + }) + + t.Run("upgrades at external artifact revision switch to digest", func(t *testing.T) { + gt := NewWithT(t) + fixedRevision := "2.0.0" + newDigest := fmt.Sprintf("latest@%s", digest.FromString("1")) + ea, err = applyExternalArtifact(eaKey, fixedRevision, newDigest) + gt.Expect(err).ToNot(HaveOccurred()) + + gt.Eventually(func() bool { + err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) + if err != nil { + return false + } + return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition) && + hr.Status.LastAttemptedRevisionDigest == newDigest + }, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade") + + gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade)) + }) + + t.Run("upgrades at external artifact digest change", func(t *testing.T) { + gt := NewWithT(t) + fixedRevision := "2.0.0" + newDigest := digest.FromString("2").String() + ea, err = applyExternalArtifact(eaKey, fixedRevision, newDigest) + gt.Expect(err).ToNot(HaveOccurred()) + + gt.Eventually(func() bool { + err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) + if err != nil { + return false + } + return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition) && + hr.Status.LastAttemptedRevisionDigest == newDigest + }, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade") + + gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade)) }) t.Run("fails when external artifact feature gate is disable", func(t *testing.T) { - newRevision := "3.0.0" + gt := NewWithT(t) reconciler.AllowExternalArtifact = false + newRevision := "3.0.0" + ea, err = applyExternalArtifact(eaKey, newRevision, "") + gt.Expect(err).ToNot(HaveOccurred()) - ea, err = applyExternalArtifact(eaKey, newRevision) - g.Expect(err).ToNot(HaveOccurred()) - - g.Eventually(func() bool { + gt.Eventually(func() bool { err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) if err != nil { return false @@ -124,24 +165,25 @@ func TestExternalArtifact_LifeCycle(t *testing.T) { return apimeta.IsStatusConditionFalse(hr.Status.Conditions, meta.ReadyCondition) }, 5*time.Second, time.Second).Should(BeTrue()) - g.Expect(apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.StalledCondition)).Should(BeTrue()) + gt.Expect(apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.StalledCondition)).Should(BeTrue()) readyCondition := apimeta.FindStatusCondition(hr.Status.Conditions, meta.ReadyCondition) - g.Expect(readyCondition.Reason).To(Equal(aclv1.AccessDeniedReason)) + gt.Expect(readyCondition.Reason).To(Equal(aclv1.AccessDeniedReason)) }) t.Run("uninstalls successfully", func(t *testing.T) { + gt := NewWithT(t) err = k8sClient.Delete(context.Background(), hr) - g.Expect(err).ToNot(HaveOccurred()) + gt.Expect(err).ToNot(HaveOccurred()) - g.Eventually(func() bool { + gt.Eventually(func() bool { err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr) return err != nil && client.IgnoreNotFound(err) == nil }, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease was not deleted") }) } -func applyExternalArtifact(objKey client.ObjectKey, revision string) (*sourcev1.ExternalArtifact, error) { - chart := testutil.BuildChart(testutil.ChartWithVersion(revision)) +func applyExternalArtifact(objKey client.ObjectKey, aVersion, aDigest string) (*sourcev1.ExternalArtifact, error) { + chart := testutil.BuildChart(testutil.ChartWithVersion(aVersion)) artifact, err := testutil.SaveChartAsArtifact(chart, digest.SHA256, testServer.URL(), testServer.Root()) if err != nil { return nil, err @@ -169,6 +211,9 @@ func applyExternalArtifact(objKey client.ObjectKey, revision string) (*sourcev1. } ea.ManagedFields = nil + if aDigest != "" { + artifact.Revision = aDigest + } ea.Status = sourcev1.ExternalArtifactStatus{ Artifact: artifact, Conditions: []metav1.Condition{