From aeac55dba9cb27069a9960eadf335a8acc105ca3 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 2 Apr 2024 23:38:05 +0200 Subject: [PATCH] Adding 12 first character of digest to chart version This is needed for an OCIRepository source in order to detect change for mutable tags. Signed-off-by: Soule BA --- api/v2beta2/helmrelease_types.go | 7 +- .../helm.toolkit.fluxcd.io_helmreleases.yaml | 3 +- docs/api/v2beta2/helm.md | 3 +- go.mod | 1 + go.sum | 2 + internal/action/reset.go | 2 +- internal/controller/helmrelease_controller.go | 52 +++++- .../controller/helmrelease_controller_test.go | 153 +++++++++++++----- 8 files changed, 178 insertions(+), 45 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index 0e71af0..01a1746 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -1002,7 +1002,8 @@ type HelmReleaseStatus struct { LastAppliedRevision string `json:"lastAppliedRevision,omitempty"` // LastAttemptedRevision is the Source revision of the last reconciliation - // attempt. + // attempt. For OCIRegistry sources, the 12 first characters of the digest are + // appended to the chart version e.g. "1.2.3+1234567890ab". // +optional LastAttemptedRevision string `json:"lastAttemptedRevision,omitempty"` @@ -1058,6 +1059,10 @@ func (in HelmReleaseStatus) GetHelmChart() (string, string) { return "", "" } +func (in *HelmReleaseStatus) GetLastAttemptedRevision() string { + return in.LastAttemptedRevision +} + const ( // SourceIndexKey is the key used for indexing HelmReleases based on // their sources. diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index f9ff5a7..429c43e 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -2452,7 +2452,8 @@ spec: lastAttemptedRevision: description: |- LastAttemptedRevision is the Source revision of the last reconciliation - attempt. + attempt. For OCIRegistry sources, the 12 first characters of the digest are + appended to the chart version e.g. "1.2.3+1234567890ab". type: string lastAttemptedValuesChecksum: description: |- diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index 546ca63..5decee5 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -1584,7 +1584,8 @@ string (Optional)

LastAttemptedRevision is the Source revision of the last reconciliation -attempt.

+attempt. For OCIRegistry sources, the 12 first characters of the digest are +appended to the chart version e.g. “1.2.3+1234567890ab”.

diff --git a/go.mod b/go.mod index 29df92c..2860dfc 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ replace ( ) require ( + github.com/Masterminds/semver v1.5.0 github.com/fluxcd/cli-utils v0.36.0-flux.5 github.com/fluxcd/helm-controller/api v0.37.4 github.com/fluxcd/pkg/apis/acl v0.2.0 diff --git a/go.sum b/go.sum index 7eee71c..77a580d 100644 --- a/go.sum +++ b/go.sum @@ -10,6 +10,8 @@ github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ github.com/MakeNowJust/heredoc v1.0.0/go.mod h1:mG5amYoWBHf8vpLOuehzbGGw0EHxpZZ6lCpQ4fNJ8LE= github.com/Masterminds/goutils v1.1.1 h1:5nUrii3FMTL5diU80unEVvNevw1nH4+ZV4DSLVJLSYI= github.com/Masterminds/goutils v1.1.1/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU= +github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww= +github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= github.com/Masterminds/semver/v3 v3.2.0/go.mod h1:qvl/7zhW3nngYb5+80sSMF+FG2BjYrf8m9wsX0PNOMQ= github.com/Masterminds/semver/v3 v3.2.1 h1:RN9w6+7QoMeJVGyfmbcgs28Br8cvmnucEXnY0rYXWg0= github.com/Masterminds/semver/v3 v3.2.1/go.mod h1:qvl/7zhW3nngYb5+80sSMF+FG2BjYrf8m9wsX0PNOMQ= diff --git a/internal/action/reset.go b/internal/action/reset.go index a293b73..0b579ca 100644 --- a/internal/action/reset.go +++ b/internal/action/reset.go @@ -48,7 +48,7 @@ func MustResetFailures(obj *v2.HelmRelease, chart *chart.Metadata, values chartu switch { case obj.Status.LastAttemptedGeneration != obj.Generation: return differentGenerationReason, true - case obj.Status.LastAttemptedRevision != chart.Version: + case obj.Status.GetLastAttemptedRevision() != chart.Version: return differentRevisionReason, true case obj.Status.LastAttemptedConfigDigest != "" || obj.Status.LastAttemptedValuesChecksum != "": d := obj.Status.LastAttemptedConfigDigest diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index c16f795..0615bb9 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "helm.sh/helm/v3/pkg/chart" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -42,6 +43,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/ratelimiter" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/Masterminds/semver" aclv1 "github.com/fluxcd/pkg/apis/acl" "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/acl" @@ -333,6 +335,12 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") } + err = mutateChartWithSourceRevision(loadedChart, source) + if err != nil { + conditions.MarkFalse(obj, meta.ReadyCondition, "ChartMutateError", err.Error()) + return ctrl.Result{}, err + } + // Build the REST client getter. getter, err := r.buildRESTClientGetter(ctx, obj) if err != nil { @@ -761,7 +769,7 @@ func (r *HelmReleaseReconciler) requestsForHelmChartChange(ctx context.Context, for i, hr := range list.Items { // If the HelmRelease is ready and the revision of the artifact equals to the // last attempted revision, we should not make a request for this HelmRelease - if conditions.IsReady(&list.Items[i]) && hc.GetArtifact().HasRevision(hr.Status.LastAttemptedRevision) { + if conditions.IsReady(&list.Items[i]) && hc.GetArtifact().HasRevision(hr.Status.GetLastAttemptedRevision()) { continue } reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) @@ -793,7 +801,7 @@ func (r *HelmReleaseReconciler) requestsForOCIRrepositoryChange(ctx context.Cont for i, hr := range list.Items { // If the HelmRelease is ready and the revision of the artifact equals to the // last attempted revision, we should not make a request for this HelmRelease - if conditions.IsReady(&list.Items[i]) && or.GetArtifact().HasRevision(hr.Status.LastAttemptedRevision) { + if conditions.IsReady(&list.Items[i]) && or.GetArtifact().HasRevision(hr.Status.GetLastAttemptedRevision()) { continue } reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) @@ -856,3 +864,43 @@ func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { return namespacedName, nil } + +func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) 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 { + return nil + } + ver, err := semver.NewVersion(chart.Metadata.Version) + if err != nil { + return err + } + revision := obj.GetArtifact().Revision + switch { + case strings.Contains(revision, "@"): + tagD := strings.Split(revision, "@") + if len(tagD) != 2 || tagD[0] != chart.Metadata.Version { + 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 := strings.Split(tagD[1], ":")[1] + // add the digest to the chart version to make sure mutable tags are detected + *ver, err = ver.SetMetadata(sha[0:12]) + if err != nil { + return err + } + default: + // default to the digest + sha := strings.Split(revision, ":")[1] + *ver, err = ver.SetMetadata(sha[0:12]) + if err != nil { + return err + } + } + + chart.Metadata.Version = ver.String() + return nil +} diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 89e69d1..f6e4c39 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -24,8 +24,17 @@ import ( "testing" "time" + "github.com/fluxcd/pkg/apis/acl" + aclv1 "github.com/fluxcd/pkg/apis/acl" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" + feathelper "github.com/fluxcd/pkg/runtime/features" + "github.com/fluxcd/pkg/runtime/patch" + sourcev1 "github.com/fluxcd/source-controller/api/v1" + sourcev1b2 "github.com/fluxcd/source-controller/api/v1beta2" . "github.com/onsi/gomega" "github.com/opencontainers/go-digest" + "helm.sh/helm/v3/pkg/chart" helmrelease "helm.sh/helm/v3/pkg/release" helmstorage "helm.sh/helm/v3/pkg/storage" helmdriver "helm.sh/helm/v3/pkg/storage/driver" @@ -44,15 +53,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" - "github.com/fluxcd/pkg/apis/acl" - aclv1 "github.com/fluxcd/pkg/apis/acl" - "github.com/fluxcd/pkg/apis/meta" - "github.com/fluxcd/pkg/runtime/conditions" - feathelper "github.com/fluxcd/pkg/runtime/features" - "github.com/fluxcd/pkg/runtime/patch" - sourcev1 "github.com/fluxcd/source-controller/api/v1" - sourcev1b2 "github.com/fluxcd/source-controller/api/v1beta2" - v2 "github.com/fluxcd/helm-controller/api/v2beta2" intacl "github.com/fluxcd/helm-controller/internal/acl" "github.com/fluxcd/helm-controller/internal/action" @@ -1211,19 +1211,24 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin })) }) - t.Run("handles reconciliation of new artifact", func(t *testing.T) { + t.Run("handle chartRef mutable tag", func(t *testing.T) { g := NewWithT(t) + // Create HelmChart mock. chartMock := testutil.BuildChart() chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root()) g.Expect(err).ToNot(HaveOccurred()) + chartArtifact.Revision += "@" + chartArtifact.Digest - ociRepo := &sourcev1b2.OCIRepository{ + ocirepo := &sourcev1b2.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "ocirepo", Namespace: "mock", Generation: 1, }, + Spec: sourcev1b2.OCIRepositorySpec{ + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, Status: sourcev1b2.OCIRepositoryStatus{ ObservedGeneration: 1, Artifact: chartArtifact, @@ -1247,44 +1252,46 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin Name: "ocirepo", Namespace: "mock", }, - StorageNamespace: "other", - }, - Status: v2.HelmReleaseStatus{ - History: v2.Snapshots{ - { - Name: "mock", - Namespace: "mock", - }, - }, - StorageNamespace: "mock", }, } - c := fake.NewClientBuilder(). - WithScheme(NewTestScheme()). - WithStatusSubresource(&v2.HelmRelease{}). - WithObjects(ociRepo, obj). - Build() - r := &HelmReleaseReconciler{ - Client: c, + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(ocirepo, obj). + Build(), GetClusterConfig: GetTestClusterConfig, EventRecorder: record.NewFakeRecorder(32), } - res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, c), obj) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(res.Requeue).To(BeTrue()) + _, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("namespaces \"mock\" not found")) - g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, v2.UninstallSucceededReason, "Release mock/mock.v0 was not found, assuming it is uninstalled"), - *conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallSucceededReason, "Release mock/mock.v0 was not found, assuming it is uninstalled"), - })) + // Verify attempted values are set. + g.Expect(obj.Status.LastAttemptedGeneration).To(Equal(obj.Generation)) + dig := strings.Split(chartArtifact.Revision, ":")[1][0:12] + g.Expect(obj.Status.LastAttemptedRevision).To(Equal(chartMock.Metadata.Version + "+" + dig)) + g.Expect(obj.Status.LastAttemptedConfigDigest).To(Equal("sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + g.Expect(obj.Status.LastAttemptedValuesChecksum).To(BeEmpty()) + + // change the chart revision to simulate a new digest + chartArtifact.Revision = chartMock.Metadata.Version + "@" + "sha256:adebc5e3cbcd6a0918bd470f3a6c9855bfe95d506c74726bc0f2edb0aecb1f4e" + ocirepo.Status.Artifact = chartArtifact + r.Client.Update(context.Background(), ocirepo) + + _, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("namespaces \"mock\" not found")) + + // Verify attempted values are set. + g.Expect(obj.Status.LastAttemptedGeneration).To(Equal(obj.Generation)) + dig2 := strings.Split(chartArtifact.Revision, ":")[1][0:12] + g.Expect(obj.Status.LastAttemptedRevision).To(Equal(chartMock.Metadata.Version + "+" + dig2)) + g.Expect(obj.Status.LastAttemptedConfigDigest).To(Equal("sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + g.Expect(obj.Status.LastAttemptedValuesChecksum).To(BeEmpty()) - // Verify history and storage namespace are cleared. - g.Expect(obj.Status.History).To(BeNil()) - g.Expect(obj.Status.StorageNamespace).To(BeEmpty()) }) } @@ -2908,3 +2915,71 @@ func Test_isOCIRepositoryReady(t *testing.T) { }) } } + +func Test_TryMutateChartWithSourceRevision(t *testing.T) { + tests := []struct { + name string + version string + revision string + wantVersion string + wantErr bool + }{ + { + name: "valid version and revision", + version: "1.2.3", + revision: "1.2.3@sha256:9933f58f8bf459eb199d59ebc8a05683f3944e1242d9f5467d99aa2cf08a5370", + wantVersion: "1.2.3+9933f58f8bf4", + wantErr: false, + }, + { + name: "valid version and invalid revision", + version: "1.2.3", + revision: "1.2.4@sha256:9933f58f8bf459eb199d59ebc8a05683f3944e1242d9f5467d99aa2cf08a5370", + wantVersion: "", + wantErr: true, + }, + { + name: "valid version and revision without version", + version: "1.2.3", + revision: "sha256:9933f58f8bf459eb199d59ebc8a05683f3944e1242d9f5467d99aa2cf08a5370", + wantVersion: "1.2.3+9933f58f8bf4", + wantErr: false, + }, + { + name: "invalid version", + version: "sha:123456", + revision: "1.2.3@sha:123456", + wantVersion: "", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Version: tt.version, + }, + } + + s := &sourcev1b2.OCIRepository{ + Status: sourcev1b2.OCIRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: tt.revision, + }, + }, + } + + err := mutateChartWithSourceRevision(c, s) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(c.Metadata.Version).To(Equal(tt.wantVersion)) + } + }) + } + +}