Skip to content

Commit c53d553

Browse files
stefanprodanstefanprodan
authored
Merge pull request #1296 from fluxcd/ea-digest-watcher
[RFC-0012] Add support for ExternalArtifact revision with digest
2 parents b7ef8b4 + ff87fbb commit c53d553

File tree

3 files changed

+146
-67
lines changed

3 files changed

+146
-67
lines changed

‎internal/controller/helmrelease_controller.go‎

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -913,39 +913,31 @@ func isValidChartRef(obj *v2.HelmRelease) bool {
913913
(!obj.HasChartRef() && obj.HasChartTemplate())
914914
}
915915

916+
// mutateChartWithSourceRevision mutates the chart version by appending the
917+
// digest part of the source revision to the chart version metadata.
918+
// It returns the digest that was appended, or an error if the mutation failed.
916919
func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart, source sourcev1.Source) (string, error) {
917-
// If the source is an OCIRepository, we can try to mutate the chart version
918-
// with the artifact revision. The revision is either a <tag>@<digest> or
919-
// just a digest.
920-
obj, ok := source.(*sourcev1.OCIRepository)
921-
if !ok {
922-
// if not make sure to return an empty string to delete the digest of the
923-
// last attempted revision
920+
if !isSourceWithRevisionDigest(source) {
921+
// Clear any previously set digest in status
922+
// if the source revision does not contain a digest.
924923
return "", nil
925924
}
926925
ver, err := semver.NewVersion(chart.Metadata.Version)
927926
if err != nil {
928-
return "", err
927+
return "", fmt.Errorf("invalid chart version %s", chart.Metadata.Version)
929928
}
930929

931-
var ociDigest string
932-
revision := obj.GetArtifact().Revision
930+
var revDigest string
931+
revision := source.GetArtifact().Revision
932+
tagVer, isSemverTag := parseSemverFromTag(revision)
933933
switch {
934-
case strings.Contains(revision, "@"):
935-
tagD := strings.Split(revision, "@")
936-
// replace '+' with '_' for OCI tag semver compatibility
937-
// per https://github.com/helm/helm/blob/v3.14.4/pkg/registry/client.go#L45-L50
938-
tagConverted := strings.ReplaceAll(tagD[0], "_", "+")
939-
tagVer, err := semver.NewVersion(tagConverted)
940-
if err != nil {
941-
return "", fmt.Errorf("failed parsing artifact revision %s", tagConverted)
934+
case isSemverTag:
935+
tagDigestPair := strings.Split(revision, "@")
936+
if len(tagDigestPair) != 2 || !tagVer.Equal(ver) {
937+
return "", fmt.Errorf("artifact revision %s does not match chart version %s", tagDigestPair[0], chart.Metadata.Version)
942938
}
943-
if len(tagD) != 2 || !tagVer.Equal(ver) {
944-
return "", fmt.Errorf("artifact revision %s does not match chart version %s", tagD[0], chart.Metadata.Version)
945-
}
946-
// algotithm are sha256, sha384, sha512 with the canonical being sha256
947-
// So every digest starts with a sha algorithm and a colon
948-
sha, err := extractDigestSubString(tagD[1])
939+
// The digest should be in format <algorithm>:<digest>
940+
sha, err := extractDigestSubString(tagDigestPair[1])
949941
if err != nil {
950942
return "", err
951943
}
@@ -954,7 +946,7 @@ func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart
954946
if err != nil {
955947
return "", err
956948
}
957-
ociDigest = tagD[1]
949+
revDigest = tagDigestPair[1]
958950
default:
959951
// default to the digest
960952
sha, err := extractDigestSubString(revision)
@@ -965,14 +957,52 @@ func (r *HelmReleaseReconciler) mutateChartWithSourceRevision(chart *chart.Chart
965957
if err != nil {
966958
return "", err
967959
}
968-
ociDigest = revision
960+
revDigest = revision
969961
}
970962

971963
if !r.DisableChartDigestTracking {
972964
chart.Metadata.Version = ver.String()
973965
}
974966

975-
return ociDigest, nil
967+
return revDigest, nil
968+
}
969+
970+
// isSourceWithRevisionDigest returns true if the source
971+
// is of a type that supports immutable revisions,
972+
// such as OCIRepository or ExternalArtifact.
973+
func isSourceWithRevisionDigest(source sourcev1.Source) bool {
974+
if _, ok := source.(*sourcev1.OCIRepository); ok {
975+
return true
976+
}
977+
if _, ok := source.(*sourcev1.ExternalArtifact); ok {
978+
// ExternalArtifact revision can be a semantic version or a digest.
979+
// We only want to mutate the chart version if the revision contains a digest.
980+
if source.GetArtifact() != nil &&
981+
strings.Contains(source.GetArtifact().Revision, ":") {
982+
return true
983+
}
984+
}
985+
return false
986+
}
987+
988+
// parseSemverFromTag attempts to parse a semver version from a tag
989+
// in the <version>@<algorithm>:<digest> or <version> format.
990+
// It returns the parsed semver version, or nil if the tag does not
991+
// contain a valid semver version. The boolean return value indicates
992+
// whether the parsing was successful.
993+
func parseSemverFromTag(tag string) (*semver.Version, bool) {
994+
semverCandidate := tag
995+
if strings.Contains(tag, "@") {
996+
tagDigestPair := strings.Split(tag, "@")
997+
// Replace '+' with '_' for OCI tag semver compatibility
998+
// per https://github.com/helm/helm/blob/v3.14.4/pkg/registry/client.go#L45-L50
999+
semverCandidate = strings.ReplaceAll(tagDigestPair[0], "_", "+")
1000+
}
1001+
ver, err := semver.NewVersion(semverCandidate)
1002+
if err != nil {
1003+
return nil, false
1004+
}
1005+
return ver, true
9761006
}
9771007

9781008
func extractDigestSubString(revision string) (string, error) {
@@ -992,11 +1022,11 @@ func extractDigestSubString(revision string) (string, error) {
9921022
func extractDigest(revision string) string {
9931023
if strings.Contains(revision, "@") {
9941024
// expects a revision in the <version>@<algorithm>:<digest> format
995-
tagD := strings.Split(revision, "@")
996-
if len(tagD) != 2 {
1025+
tagDigestPair := strings.Split(revision, "@")
1026+
if len(tagDigestPair) != 2 {
9971027
return ""
9981028
}
999-
return tagD[1]
1029+
return tagDigestPair[1]
10001030
} else {
10011031
// revision in the <algorithm>:<digest> format
10021032
return revision

‎internal/controller/helmrelease_indexers.go‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package controller
1919
import (
2020
"context"
2121
"fmt"
22+
"strings"
2223

2324
"github.com/fluxcd/pkg/runtime/conditions"
2425
sourcev1 "github.com/fluxcd/source-controller/api/v1"
@@ -96,7 +97,8 @@ func (r *HelmReleaseReconciler) requestsForOCIRepositoryChange(ctx context.Conte
9697
continue
9798
}
9899

99-
if digest == hr.Status.LastAttemptedRevisionDigest {
100+
// Skip if the HelmRelease is ready and the digest matches the last attempted revision digest.
101+
if conditions.IsReady(&list.Items[i]) && digest == hr.Status.LastAttemptedRevisionDigest {
100102
continue
101103
}
102104

@@ -108,38 +110,40 @@ func (r *HelmReleaseReconciler) requestsForOCIRepositoryChange(ctx context.Conte
108110
// requestsForExternalArtifactChange enqueues requests for watched ExternalArtifacts
109111
// according to the specified index.
110112
func (r *HelmReleaseReconciler) requestsForExternalArtifactChange(ctx context.Context, o client.Object) []reconcile.Request {
111-
or, ok := o.(*sourcev1.ExternalArtifact)
113+
log := ctrl.LoggerFrom(ctx)
114+
ea, ok := o.(*sourcev1.ExternalArtifact)
112115
if !ok {
113116
err := fmt.Errorf("expected an ExternalArtifact, got %T", o)
114-
ctrl.LoggerFrom(ctx).Error(err, "failed to get requests for ExternalArtifact change")
117+
log.Error(err, "failed to get requests for ExternalArtifact change")
115118
return nil
116119
}
117120
// If we do not have an artifact, we have no requests to make
118-
if or.GetArtifact() == nil {
121+
if ea.GetArtifact() == nil {
119122
return nil
120123
}
121124

122125
var list v2.HelmReleaseList
123126
if err := r.List(ctx, &list, client.MatchingFields{
124-
v2.SourceIndexKey: sourcev1.ExternalArtifactKind + "/" + client.ObjectKeyFromObject(or).String(),
127+
v2.SourceIndexKey: sourcev1.ExternalArtifactKind + "/" + client.ObjectKeyFromObject(ea).String(),
125128
}); err != nil {
126-
ctrl.LoggerFrom(ctx).Error(err, "failed to list HelmReleases for ExternalArtifact change")
129+
log.Error(err, "failed to list HelmReleases for ExternalArtifact change")
127130
return nil
128131
}
129-
130132
var reqs []reconcile.Request
131133
for i, hr := range list.Items {
132-
// If the HelmRelease is ready and the digest of the artifact equals to the
133-
// last attempted revision digest, we should not make a request for this HelmRelease,
134-
// likewise if we cannot retrieve the artifact digest.
135-
digest := extractDigest(or.GetArtifact().Revision)
136-
if digest == "" {
137-
ctrl.LoggerFrom(ctx).Error(fmt.Errorf("wrong digest for %T", or), "failed to get requests for ExternalArtifact change")
138-
continue
139-
}
140-
141-
if digest == hr.Status.LastAttemptedRevisionDigest {
142-
continue
134+
revision := ea.GetArtifact().Revision
135+
136+
// Handle both revision formats: digest or semantic version.
137+
if strings.Contains(revision, ":") {
138+
// Skip if the HelmRelease is ready and the digest matches the last attempted revision digest.
139+
if conditions.IsReady(&list.Items[i]) && extractDigest(revision) == hr.Status.LastAttemptedRevisionDigest {
140+
continue
141+
}
142+
} else {
143+
// Skip if the HelmRelease is ready and the revision matches the last attempted revision.
144+
if conditions.IsReady(&list.Items[i]) && revision == hr.Status.LastAttemptedRevision {
145+
continue
146+
}
143147
}
144148

145149
reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])})

‎internal/controller_test/external_artifact_test.go‎

Lines changed: 64 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package controller_test
1818

1919
import (
2020
"context"
21+
"fmt"
2122
"testing"
2223
"time"
2324

@@ -54,7 +55,7 @@ func TestExternalArtifact_LifeCycle(t *testing.T) {
5455
Namespace: ns.Name,
5556
Name: "test-ea",
5657
}
57-
ea, err := applyExternalArtifact(eaKey, revision)
58+
ea, err := applyExternalArtifact(eaKey, revision, "")
5859
g.Expect(err).ToNot(HaveOccurred(), "failed to create ExternalArtifact")
5960

6061
// Create a HelmRelease that references the ExternalArtifact
@@ -80,24 +81,26 @@ func TestExternalArtifact_LifeCycle(t *testing.T) {
8081
g.Expect(err).ToNot(HaveOccurred())
8182

8283
t.Run("installs from external artifact", func(t *testing.T) {
83-
g.Eventually(func() bool {
84+
gt := NewWithT(t)
85+
gt.Eventually(func() bool {
8486
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
8587
if err != nil {
8688
return false
8789
}
8890
return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition)
8991
}, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not become ready")
9092

91-
g.Expect(hr.Status.LastAttemptedRevision).To(Equal(revision))
92-
g.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionInstall))
93+
gt.Expect(hr.Status.LastAttemptedRevision).To(Equal(revision))
94+
gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionInstall))
9395
})
9496

9597
t.Run("upgrades at external artifact revision change", func(t *testing.T) {
98+
gt := NewWithT(t)
9699
newRevision := "2.0.0"
97-
ea, err = applyExternalArtifact(eaKey, newRevision)
98-
g.Expect(err).ToNot(HaveOccurred())
100+
ea, err = applyExternalArtifact(eaKey, newRevision, "")
101+
gt.Expect(err).ToNot(HaveOccurred())
99102

100-
g.Eventually(func() bool {
103+
gt.Eventually(func() bool {
101104
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
102105
if err != nil {
103106
return false
@@ -106,42 +109,81 @@ func TestExternalArtifact_LifeCycle(t *testing.T) {
106109
hr.Status.LastAttemptedRevision == newRevision
107110
}, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade")
108111

109-
g.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade))
112+
gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade))
113+
})
114+
115+
t.Run("upgrades at external artifact revision switch to digest", func(t *testing.T) {
116+
gt := NewWithT(t)
117+
fixedRevision := "2.0.0"
118+
newDigest := fmt.Sprintf("latest@%s", digest.FromString("1"))
119+
ea, err = applyExternalArtifact(eaKey, fixedRevision, newDigest)
120+
gt.Expect(err).ToNot(HaveOccurred())
121+
122+
gt.Eventually(func() bool {
123+
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
124+
if err != nil {
125+
return false
126+
}
127+
return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition) &&
128+
hr.Status.LastAttemptedRevisionDigest == newDigest
129+
}, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade")
130+
131+
gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade))
132+
})
133+
134+
t.Run("upgrades at external artifact digest change", func(t *testing.T) {
135+
gt := NewWithT(t)
136+
fixedRevision := "2.0.0"
137+
newDigest := digest.FromString("2").String()
138+
ea, err = applyExternalArtifact(eaKey, fixedRevision, newDigest)
139+
gt.Expect(err).ToNot(HaveOccurred())
140+
141+
gt.Eventually(func() bool {
142+
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
143+
if err != nil {
144+
return false
145+
}
146+
return apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.ReadyCondition) &&
147+
hr.Status.LastAttemptedRevisionDigest == newDigest
148+
}, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease did not upgrade")
149+
150+
gt.Expect(hr.Status.LastAttemptedReleaseAction).To(Equal(v2.ReleaseActionUpgrade))
110151
})
111152

112153
t.Run("fails when external artifact feature gate is disable", func(t *testing.T) {
113-
newRevision := "3.0.0"
154+
gt := NewWithT(t)
114155
reconciler.AllowExternalArtifact = false
156+
newRevision := "3.0.0"
157+
ea, err = applyExternalArtifact(eaKey, newRevision, "")
158+
gt.Expect(err).ToNot(HaveOccurred())
115159

116-
ea, err = applyExternalArtifact(eaKey, newRevision)
117-
g.Expect(err).ToNot(HaveOccurred())
118-
119-
g.Eventually(func() bool {
160+
gt.Eventually(func() bool {
120161
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
121162
if err != nil {
122163
return false
123164
}
124165
return apimeta.IsStatusConditionFalse(hr.Status.Conditions, meta.ReadyCondition)
125166
}, 5*time.Second, time.Second).Should(BeTrue())
126167

127-
g.Expect(apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.StalledCondition)).Should(BeTrue())
168+
gt.Expect(apimeta.IsStatusConditionTrue(hr.Status.Conditions, meta.StalledCondition)).Should(BeTrue())
128169
readyCondition := apimeta.FindStatusCondition(hr.Status.Conditions, meta.ReadyCondition)
129-
g.Expect(readyCondition.Reason).To(Equal(aclv1.AccessDeniedReason))
170+
gt.Expect(readyCondition.Reason).To(Equal(aclv1.AccessDeniedReason))
130171
})
131172

132173
t.Run("uninstalls successfully", func(t *testing.T) {
174+
gt := NewWithT(t)
133175
err = k8sClient.Delete(context.Background(), hr)
134-
g.Expect(err).ToNot(HaveOccurred())
176+
gt.Expect(err).ToNot(HaveOccurred())
135177

136-
g.Eventually(func() bool {
178+
gt.Eventually(func() bool {
137179
err = testEnv.Get(context.Background(), client.ObjectKeyFromObject(hr), hr)
138180
return err != nil && client.IgnoreNotFound(err) == nil
139181
}, 5*time.Second, time.Second).Should(BeTrue(), "HelmRelease was not deleted")
140182
})
141183
}
142184

143-
func applyExternalArtifact(objKey client.ObjectKey, revision string) (*sourcev1.ExternalArtifact, error) {
144-
chart := testutil.BuildChart(testutil.ChartWithVersion(revision))
185+
func applyExternalArtifact(objKey client.ObjectKey, aVersion, aDigest string) (*sourcev1.ExternalArtifact, error) {
186+
chart := testutil.BuildChart(testutil.ChartWithVersion(aVersion))
145187
artifact, err := testutil.SaveChartAsArtifact(chart, digest.SHA256, testServer.URL(), testServer.Root())
146188
if err != nil {
147189
return nil, err
@@ -169,6 +211,9 @@ func applyExternalArtifact(objKey client.ObjectKey, revision string) (*sourcev1.
169211
}
170212

171213
ea.ManagedFields = nil
214+
if aDigest != "" {
215+
artifact.Revision = aDigest
216+
}
172217
ea.Status = sourcev1.ExternalArtifactStatus{
173218
Artifact: artifact,
174219
Conditions: []metav1.Condition{

0 commit comments

Comments
 (0)