From 20e14fe304b17f098ae62888f20563fd3169e1f7 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 5 Mar 2024 15:42:40 +0100 Subject: [PATCH 01/12] This commit enable reusing an existing OCIRepo as chartRef. It takes into account switching from a chart template to a referenced source (garbage collection). Signed-off-by: Soule BA --- .github/workflows/e2e.yaml | 5 + api/v2beta2/helmrelease_types.go | 18 +- api/v2beta2/reference_types.go | 26 + api/v2beta2/zz_generated.deepcopy.go | 20 + .../helm.toolkit.fluxcd.io_helmreleases.yaml | 32 +- config/rbac/role.yaml | 14 + .../install-from-ocirepo-source/test.yaml | 25 + docs/api/v2beta2/helm.md | 100 ++++ internal/controller/helmrelease_controller.go | 177 +++++- .../controller/helmrelease_controller_test.go | 566 +++++++++++++++++- internal/reconcile/helmchart_template.go | 24 + internal/reconcile/helmchart_template_test.go | 54 ++ 12 files changed, 1024 insertions(+), 37 deletions(-) create mode 100644 config/testdata/install-from-ocirepo-source/test.yaml diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index eb12098d5..2545b030f 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -138,6 +138,11 @@ jobs: kubectl -n install-create-target-ns get deployment install-create-target-ns-install-create-target-ns-podinfo kubectl -n helm-system delete -f config/testdata/install-create-target-ns + - name: Run install from ocirepo test + run: | + kubectl -n helm-system apply -f config/testdata/install-from-ocirepo-source + kubectl -n helm-system wait helmreleases/podinfo-from-ocirepo --for=condition=ready --timeout=4m + kubectl -n helm-system delete -f config/testdata/install-from-ocirepo-source - name: Run install fail test run: | test_name=install-fail diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index 2c280849e..a4d28d7ab 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -74,12 +74,18 @@ type PostRenderer struct { } // HelmReleaseSpec defines the desired state of a Helm release. +// +kubebuilder:validation:XValidation:rule="(has(self.chart) && !has(self.chartRef)) || (!has(self.chart) && has(self.chartRef))", message="either chart or chartRef must be set" type HelmReleaseSpec struct { // Chart defines the template of the v1beta2.HelmChart that should be created // for this HelmRelease. - // +required + // +optional Chart HelmChartTemplate `json:"chart"` + // ChartRef holds a reference to a source controller resource containing the + // Helm chart artifact. + // +optional + ChartRef *CrossNamespaceSourceReference `json:"chartRef,omitempty"` + // Interval at which to reconcile the Helm release. // +kubebuilder:validation:Type=string // +kubebuilder:validation:Pattern="^([0-9]+(\\.[0-9]+)?(ms|s|m|h))+$" @@ -1241,6 +1247,16 @@ func (in *HelmRelease) GetStatusConditions() *[]metav1.Condition { return &in.Status.Conditions } +// IsChartRefPresent returns true if the HelmRelease has a ChartRef. +func (in *HelmRelease) IsChartRefPresent() bool { + return in.Spec.ChartRef != nil +} + +// IsChartTemplatePresent returns true if the HelmRelease has a ChartTemplate. +func (in *HelmRelease) IsChartTemplatePresent() bool { + return in.Spec.Chart.Spec.Chart != "" +} + // +kubebuilder:object:root=true // HelmReleaseList contains a list of HelmRelease objects. diff --git a/api/v2beta2/reference_types.go b/api/v2beta2/reference_types.go index 4c899fe5d..6208b2207 100644 --- a/api/v2beta2/reference_types.go +++ b/api/v2beta2/reference_types.go @@ -42,6 +42,32 @@ type CrossNamespaceObjectReference struct { Namespace string `json:"namespace,omitempty"` } +// CrossNamespaceSourceReference contains enough information to let you locate +// the typed referenced object at cluster level. +type CrossNamespaceSourceReference struct { + // APIVersion of the referent. + // +optional + APIVersion string `json:"apiVersion,omitempty"` + + // Kind of the referent. + // +kubebuilder:validation:Enum=OCIRepository + // +required + Kind string `json:"kind"` + + // Name of the referent. + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + // +required + Name string `json:"name"` + + // Namespace of the referent. + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Optional + // +optional + Namespace string `json:"namespace,omitempty"` +} + // ValuesReference contains a reference to a resource containing Helm values, // and optionally the key they can be found at. type ValuesReference struct { diff --git a/api/v2beta2/zz_generated.deepcopy.go b/api/v2beta2/zz_generated.deepcopy.go index c9db4f094..7d7f3200a 100644 --- a/api/v2beta2/zz_generated.deepcopy.go +++ b/api/v2beta2/zz_generated.deepcopy.go @@ -43,6 +43,21 @@ func (in *CrossNamespaceObjectReference) DeepCopy() *CrossNamespaceObjectReferen return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CrossNamespaceSourceReference) DeepCopyInto(out *CrossNamespaceSourceReference) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CrossNamespaceSourceReference. +func (in *CrossNamespaceSourceReference) DeepCopy() *CrossNamespaceSourceReference { + if in == nil { + return nil + } + out := new(CrossNamespaceSourceReference) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DriftDetection) DeepCopyInto(out *DriftDetection) { *out = *in @@ -244,6 +259,11 @@ func (in *HelmReleaseList) DeepCopyObject() runtime.Object { func (in *HelmReleaseSpec) DeepCopyInto(out *HelmReleaseSpec) { *out = *in in.Chart.DeepCopyInto(&out.Chart) + if in.ChartRef != nil { + in, out := &in.ChartRef, &out.ChartRef + *out = new(CrossNamespaceSourceReference) + **out = **in + } out.Interval = in.Interval if in.KubeConfig != nil { in, out := &in.KubeConfig, &out.KubeConfig diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index 55ab127b9..e6279b081 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -1420,6 +1420,33 @@ spec: required: - spec type: object + chartRef: + description: |- + ChartRef holds a reference to a source controller resource containing the + Helm chart artifact. + properties: + apiVersion: + description: APIVersion of the referent. + type: string + kind: + description: Kind of the referent. + enum: + - OCIRepository + type: string + name: + description: Name of the referent. + maxLength: 253 + minLength: 1 + type: string + namespace: + description: Namespace of the referent. + maxLength: 63 + minLength: 1 + type: string + required: + - kind + - name + type: object dependsOn: description: |- DependsOn may contain a meta.NamespacedObjectReference slice with @@ -2199,9 +2226,12 @@ spec: type: object type: array required: - - chart - interval type: object + x-kubernetes-validations: + - message: either chart or chartRef must be set + rule: (has(self.chart) && !has(self.chartRef)) || (!has(self.chart) + && has(self.chartRef)) status: default: observedGeneration: -1 diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 954df8e75..52b74ee1f 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -55,3 +55,17 @@ rules: - helmcharts/status verbs: - get +- apiGroups: + - source.toolkit.fluxcd.io + resources: + - ocirepositories + verbs: + - get + - list + - watch +- apiGroups: + - source.toolkit.fluxcd.io + resources: + - ocirepositories/status + verbs: + - get diff --git a/config/testdata/install-from-ocirepo-source/test.yaml b/config/testdata/install-from-ocirepo-source/test.yaml new file mode 100644 index 000000000..f7f75dd80 --- /dev/null +++ b/config/testdata/install-from-ocirepo-source/test.yaml @@ -0,0 +1,25 @@ +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: OCIRepository +metadata: + name: podinfo-ocirepo +spec: + interval: 30s + url: oci://ghcr.io/stefanprodan/charts/podinfo + ref: + tag: 6.6.0 +--- +apiVersion: helm.toolkit.fluxcd.io/v2beta2 +kind: HelmRelease +metadata: + name: podinfo-from-ocirepo +spec: + chartRef: + kind: OCIRepository + name: podinfo-ocirepo + interval: 30s + values: + resources: + requests: + cpu: 100m + memory: 64Mi diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index 840a1ca2d..ba76811c6 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -78,12 +78,28 @@ HelmChartTemplate +(Optional)

Chart defines the template of the v1beta2.HelmChart that should be created for this HelmRelease.

+chartRef
+ + +CrossNamespaceSourceReference + + + + +(Optional) +

ChartRef holds a reference to a source controller resource containing the +Helm chart artifact.

+ + + + interval
@@ -469,6 +485,74 @@ string +

CrossNamespaceSourceReference +

+

+(Appears on: +HelmReleaseSpec) +

+

CrossNamespaceSourceReference contains enough information to let you locate +the typed referenced object at cluster level.

+
+
+ + + + + + + + + + + + + + + + + + + + + + + + + +
FieldDescription
+apiVersion
+ +string + +		 +(Optional) +

APIVersion of the referent.

+
+kind
+ +string + +		 +

Kind of the referent.

+
+name
+ +string + +		 +

Name of the referent.

+
+namespace
+ +string + +		 +(Optional) +

Namespace of the referent.

+
+
+

DriftDetection

@@ -1009,12 +1093,28 @@ HelmChartTemplate +(Optional)

Chart defines the template of the v1beta2.HelmChart that should be created for this HelmRelease.

+chartRef
+ + +CrossNamespaceSourceReference + + + + +(Optional) +

ChartRef holds a reference to a source controller resource containing the +Helm chart artifact.

+ + + + interval
diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 8e5d84e09..8c1109139 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -52,6 +52,7 @@ import ( "github.com/fluxcd/pkg/runtime/logger" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + source "github.com/fluxcd/source-controller/api/v1" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" v2 "github.com/fluxcd/helm-controller/api/v2beta2" @@ -73,6 +74,8 @@ import ( // +kubebuilder:rbac:groups=helm.toolkit.fluxcd.io,resources=helmreleases/finalizers,verbs=get;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts,verbs=get;list;watch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts/status,verbs=get +// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=ocirepositories,verbs=get;list;watch +// +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=ocirepositories/status,verbs=get // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // HelmReleaseReconciler reconciles a HelmRelease object. @@ -108,11 +111,12 @@ func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M if err := mgr.GetFieldIndexer().IndexField(ctx, &v2.HelmRelease{}, v2.SourceIndexKey, func(o client.Object) []string { obj := o.(*v2.HelmRelease) + namespacedName, err := getNamespacedName(obj) + if err != nil { + return nil + } return []string{ - types.NamespacedName{ - Namespace: obj.Spec.Chart.GetNamespace(obj.GetNamespace()), - Name: obj.GetHelmChartName(), - }.String(), + namespacedName.String(), } }, ); err != nil { @@ -131,6 +135,11 @@ func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M handler.EnqueueRequestsFromMapFunc(r.requestsForHelmChartChange), builder.WithPredicates(intpredicates.SourceRevisionChangePredicate{}), ). + Watches( + &sourcev1.OCIRepository{}, + handler.EnqueueRequestsFromMapFunc(r.requestsForOCIRrepositoryChange), + builder.WithPredicates(intpredicates.SourceRevisionChangePredicate{}), + ). WithOptions(controller.Options{ RateLimiter: opts.RateLimiter, }). @@ -147,6 +156,10 @@ func (r *HelmReleaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, client.IgnoreNotFound(err) } + if !isValidChartRef(obj) { + return ctrl.Result{}, reconcile.TerminalError(fmt.Errorf("invalid Chart reference")) + } + // Initialize the patch helper with the current version of the object. patchHelper := patch.NewSerialPatcher(obj, r.Client) @@ -251,8 +264,8 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") } - // Get the HelmChart object for the release. - hc, err := r.getHelmChart(ctx, obj) + // Get the source object containing the HelmChart. + source, err := r.getSource(ctx, obj) if err != nil { if acl.IsAccessDenied(err) { conditions.MarkStalled(obj, aclv1.AccessDeniedReason, err.Error()) @@ -266,7 +279,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe return ctrl.Result{}, reconcile.TerminalError(err) } - msg := fmt.Sprintf("could not get HelmChart object: %s", err.Error()) + msg := fmt.Sprintf("could not get Source object: %s", err.Error()) conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) return ctrl.Result{}, err } @@ -275,17 +288,16 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") } - // Check if the HelmChart is ready. - if ready, reason := isHelmChartReady(hc); !ready { - msg := fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", hc.GetNamespace(), hc.GetName(), reason) + // Check if the source is ready. + if ready, msg := isSourceReady(source); !ready { log.Info(msg) - conditions.MarkFalse(obj, meta.ReadyCondition, "HelmChartNotReady", msg) + conditions.MarkFalse(obj, meta.ReadyCondition, "SourceNotReady", msg) // Do not requeue immediately, when the artifact is created // the watcher should trigger a reconciliation. return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), errWaitForChart } // Remove any stale corresponding Ready=False condition with Unknown. - if conditions.HasAnyReason(obj, meta.ReadyCondition, "HelmChartNotReady") { + if conditions.HasAnyReason(obj, meta.ReadyCondition, "SourceNotReady") { conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") } @@ -302,10 +314,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe } // Load chart from artifact. - loadedChart, err := loader.SecureLoadChartFromURL(loader.NewRetryableHTTPClient(ctx, r.artifactFetchRetries), hc.GetArtifact().URL, hc.GetArtifact().Digest) + loadedChart, err := loader.SecureLoadChartFromURL(loader.NewRetryableHTTPClient(ctx, r.artifactFetchRetries), source.GetArtifact().URL, source.GetArtifact().Digest) if err != nil { if errors.Is(err, loader.ErrFileNotFound) { - msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency.String()) + msg := fmt.Sprintf("Source not ready: artifact not found. Retrying in %s", r.requeueDependency.String()) conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) log.Info(msg) return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency @@ -657,11 +669,24 @@ func (r *HelmReleaseReconciler) buildRESTClientGetter(ctx context.Context, obj * return kube.NewMemoryRESTClientGetter(cfg, opts...), nil } -// getHelmChart retrieves the v1beta2.HelmChart for the given v2beta2.HelmRelease -// using the name that is advertised in the status object. -// It returns the v1beta2.HelmChart, or an error. -func (r *HelmReleaseReconciler) getHelmChart(ctx context.Context, obj *v2.HelmRelease) (*sourcev1.HelmChart, error) { - namespace, name := obj.Status.GetHelmChart() +// getSource returns the source object containing the HelmChart, either by +// using the chartRef in the spec, or by looking up the HelmChart +// referenced in the status object. +// It returns the source object or an error. +func (r *HelmReleaseReconciler) getSource(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { + var name, namespace string + if obj.IsChartRefPresent() { + if obj.Spec.ChartRef.Kind == sourcev1.OCIRepositoryKind { + return r.getHelmChartFromOCIRef(ctx, obj) + } + name, namespace = obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace + if namespace == "" { + namespace = obj.GetNamespace() + } + } else { + namespace, name = obj.Status.GetHelmChart() + } + chartRef := types.NamespacedName{Namespace: namespace, Name: name} if err := intacl.AllowsAccessTo(obj, sourcev1.HelmChartKind, chartRef); err != nil { @@ -675,6 +700,24 @@ func (r *HelmReleaseReconciler) getHelmChart(ctx context.Context, obj *v2.HelmRe return &hc, nil } +func (r *HelmReleaseReconciler) getHelmChartFromOCIRef(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { + name, namespace := obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace + if namespace == "" { + namespace = obj.GetNamespace() + } + ociRepoRef := types.NamespacedName{Namespace: namespace, Name: name} + + if err := intacl.AllowsAccessTo(obj, sourcev1.OCIRepositoryKind, ociRepoRef); err != nil { + return nil, err + } + + or := sourcev1.OCIRepository{} + if err := r.Client.Get(ctx, ociRepoRef, &or); err != nil { + return nil, err + } + return &or, nil +} + // waitForHistoryCacheSync returns a function that can be used to wait for the // cache backing the Kubernetes client to be in sync with the current state of // the v2beta2.HelmRelease. @@ -725,6 +768,51 @@ func (r *HelmReleaseReconciler) requestsForHelmChartChange(ctx context.Context, return reqs } +func (r *HelmReleaseReconciler) requestsForOCIRrepositoryChange(ctx context.Context, o client.Object) []reconcile.Request { + or, ok := o.(*sourcev1.OCIRepository) + if !ok { + err := fmt.Errorf("expected an OCIRepository, got %T", o) + ctrl.LoggerFrom(ctx).Error(err, "failed to get requests for OCIRepository change") + return nil + } + // If we do not have an artifact, we have no requests to make + if or.GetArtifact() == nil { + return nil + } + + var list v2.HelmReleaseList + if err := r.List(ctx, &list, client.MatchingFields{ + v2.SourceIndexKey: client.ObjectKeyFromObject(or).String(), + }); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to list HelmReleases for OCIRepository change") + return nil + } + + var reqs []reconcile.Request + 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) { + continue + } + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) + } + return reqs +} + +func isSourceReady(obj source.Source) (bool, string) { + if hc, ok := obj.(*sourcev1.HelmChart); ok { + return isHelmChartReady(hc) + } + + or, ok := obj.(*sourcev1.OCIRepository) + if ok { + return isOCIRepositoryReady(or) + } + + return false, "unknown source type" +} + // isHelmChartReady returns true if the given HelmChart is ready, and a reason // why it is not ready otherwise. func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) { @@ -741,12 +829,57 @@ func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) { if conditions.IsFalse(obj, meta.ReadyCondition) { msg = conditions.GetMessage(obj, meta.ReadyCondition) } - return false, msg + return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), msg) case conditions.IsStalled(obj): - return false, conditions.GetMessage(obj, meta.StalledCondition) + return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), conditions.GetMessage(obj, meta.StalledCondition)) case obj.Status.Artifact == nil: - return false, "does not have an artifact" + return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), "does not have an artifact") default: return true, "" } } + +func isOCIRepositoryReady(obj *sourcev1.OCIRepository) (bool, string) { + switch { + case obj.Generation != obj.Status.ObservedGeneration: + msg := "latest generation of object has not been reconciled" + + if conditions.IsFalse(obj, meta.ReadyCondition) { + msg = conditions.GetMessage(obj, meta.ReadyCondition) + } + return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), msg) + case conditions.IsStalled(obj): + return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), conditions.GetMessage(obj, meta.StalledCondition)) + case obj.Status.Artifact == nil: + return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", + obj.GetNamespace(), obj.GetName(), "does not have an artifact") + default: + return true, "" + } +} + +func isValidChartRef(obj *v2.HelmRelease) bool { + return (obj.IsChartRefPresent() && !obj.IsChartTemplatePresent()) || + (!obj.IsChartRefPresent() && obj.IsChartTemplatePresent()) +} + +func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { + namespacedName := types.NamespacedName{} + switch { + case obj.IsChartRefPresent() && !obj.IsChartTemplatePresent(): + namespacedName.Namespace = obj.Spec.ChartRef.Namespace + namespacedName.Name = obj.Spec.ChartRef.Name + case !obj.IsChartRefPresent() && obj.IsChartTemplatePresent(): + namespacedName.Namespace = obj.Spec.Chart.GetNamespace(obj.GetNamespace()) + namespacedName.Name = obj.GetHelmChartName() + default: + return namespacedName, fmt.Errorf("one of chartRef or chart must be present") + } + + return namespacedName, nil +} diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 54166b445..ea7d82269 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "errors" + "fmt" "strings" "testing" "time" @@ -32,6 +33,7 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -143,7 +145,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "Fulfilling prerequisites"), - *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "could not get HelmChart object"), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "could not get Source object"), })) }) @@ -228,7 +230,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, "HelmChartNotReady", "HelmChart 'mock/chart' is not ready"), + *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "HelmChart 'mock/chart' is not ready"), })) }) @@ -280,7 +282,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, "HelmChartNotReady", "HelmChart 'mock/chart' is not ready"), + *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "HelmChart 'mock/chart' is not ready"), })) }) @@ -332,7 +334,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, "HelmChartNotReady", "HelmChart 'mock/chart' is not ready"), + *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "HelmChart 'mock/chart' is not ready"), })) }) @@ -444,7 +446,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Chart not ready"), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Source not ready"), })) }) @@ -862,7 +864,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { v2.DependencyNotReadyReason, aclv1.AccessDeniedReason, v2.ArtifactFailedReason, - "HelmChartNotReady", + "SourceNotReady", "ValuesError", "RESTClientError", "FactoryError", @@ -888,6 +890,435 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { }) } +func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testing.T) { + + t.Run("handles chartRef and Chart definition failure", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + Chart: v2.HelmChartTemplate{ + Spec: v2.HelmChartTemplateSpec{ + Chart: "mychart", + SourceRef: v2.CrossNamespaceObjectReference{ + Name: "something", + }, + }, + }, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(obj). + Build(), + } + + res, err := r.Reconcile(context.TODO(), reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.GetName(), + }, + }) + + // only chartRef or Chart must be set + g.Expect(errors.Is(err, reconcile.TerminalError(fmt.Errorf("invalid Chart reference")))).To(BeTrue()) + g.Expect(res.IsZero()).To(BeTrue()) + }) + t.Run("handles ChartRef get failure", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(obj). + Build(), + EventRecorder: record.NewFakeRecorder(32), + } + + _, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(HaveOccurred()) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "Fulfilling prerequisites"), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "could not get Source object"), + })) + }) + + t.Run("handles ACL error for ChartRef", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock-other", + }, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(obj). + Build(), + EventRecorder: record.NewFakeRecorder(32), + } + + res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, reconcile.TerminalError(nil))).To(BeTrue()) + g.Expect(res.IsZero()).To(BeTrue()) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.StalledCondition, acl.AccessDeniedReason, "cross-namespace references are not allowed"), + *conditions.FalseCondition(meta.ReadyCondition, acl.AccessDeniedReason, "cross-namespace references are not allowed"), + })) + }) + + t.Run("waits for ChartRef to have an Artifact", func(t *testing.T) { + g := NewWithT(t) + + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 2, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Artifact: nil, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(ocirepo, obj). + Build(), + } + + res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(Equal(errWaitForChart)) + g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), + *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "OCIRepository 'mock/ocirepo' is not ready"), + })) + }) + + t.Run("confirms ChartRef has an Artifact", func(t *testing.T) { + g := NewWithT(t) + + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 2, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Artifact: nil, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(ocirepo, obj). + Build(), + } + + res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(Equal(errWaitForChart)) + g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), + *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "OCIRepository 'mock/ocirepo' is not ready"), + })) + }) + + t.Run("reports values composition failure", func(t *testing.T) { + g := NewWithT(t) + + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 2, + }, + Spec: sourcev1b2.OCIRepositorySpec{ + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Artifact: &sourcev1.Artifact{}, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + ValuesFrom: []v2.ValuesReference{ + { + Kind: "Secret", + Name: "missing", + }, + }, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(ocirepo, obj). + Build(), + EventRecorder: record.NewFakeRecorder(32), + } + + _, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(HaveOccurred()) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "Fulfilling prerequisites"), + *conditions.FalseCondition(meta.ReadyCondition, "ValuesError", "could not resolve Secret chart values reference 'mock/missing' with key 'values.yaml'"), + })) + }) + + t.Run("reports Helm chart load failure", func(t *testing.T) { + g := NewWithT(t) + + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 2, + }, + Spec: sourcev1b2.OCIRepositorySpec{ + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Artifact: &sourcev1.Artifact{ + URL: testServer.URL() + "/does-not-exist", + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(ocirepo, obj). + Build(), + requeueDependency: 10 * time.Second, + EventRecorder: record.NewFakeRecorder(32), + } + + res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(Equal(errWaitForDependency)) + g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Source not ready"), + })) + }) + t.Run("report helmChart load failure when switching from existing HelmChat to chartRef", func(t *testing.T) { + g := NewWithT(t) + + chart := &sourcev1b2.HelmChart{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chart", + Namespace: "mock", + Generation: 1, + }, + Status: sourcev1b2.HelmChartStatus{ + ObservedGeneration: 1, + Artifact: &sourcev1.Artifact{}, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 2, + }, + Spec: sourcev1b2.OCIRepositorySpec{ + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Artifact: &sourcev1.Artifact{ + URL: testServer.URL() + "/does-not-exist", + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + Namespace: "mock", + }, + }, + Status: v2.HelmReleaseStatus{ + HelmChart: "mock/chart", + }, + } + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(chart, ocirepo, obj). + Build(), + requeueDependency: 10 * time.Second, + EventRecorder: record.NewFakeRecorder(32), + } + + res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).To(Equal(errWaitForDependency)) + g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency)) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Source not ready"), + })) + }) +} + func TestHelmReleaseReconciler_reconcileDelete(t *testing.T) { t.Run("uninstalls Helm release and removes chart", func(t *testing.T) { g := NewWithT(t) @@ -1020,7 +1451,7 @@ func TestHelmReleaseReconciler_reconcileDelete(t *testing.T) { }) } -func TestHelmReleaseReconciler_reconileReleaseDeletion(t *testing.T) { +func TestHelmReleaseReconciler_reconcileReleaseDeletion(t *testing.T) { t.Run("uninstalls Helm release", func(t *testing.T) { g := NewWithT(t) @@ -2016,6 +2447,25 @@ func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { wantErr: true, disallowCrossNS: true, }, + { + "get chart from reference", + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "HelmChart", + Name: "some-chart-name", + Namespace: "some-namespace", + }, + }, + }, + chart, + true, + false, + false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2034,14 +2484,16 @@ func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { intacl.AllowCrossNamespaceRef = !tt.disallowCrossNS t.Cleanup(func() { intacl.AllowCrossNamespaceRef = !curAllow }) - got, err := r.getHelmChart(context.TODO(), tt.rel) + got, err := r.getSource(context.TODO(), tt.rel) if tt.wantErr { g.Expect(err).To(HaveOccurred()) g.Expect(got).To(BeNil()) return } g.Expect(err).ToNot(HaveOccurred()) - expect := g.Expect(got.ObjectMeta) + hc, ok := got.(*sourcev1b2.HelmChart) + g.Expect(ok).To(BeTrue()) + expect := g.Expect(hc.ObjectMeta) if tt.expectChart { expect.To(BeEquivalentTo(tt.chart.ObjectMeta)) } else { @@ -2330,6 +2782,8 @@ func TestValuesReferenceValidation(t *testing.T) { func Test_isHelmChartReady(t *testing.T) { mock := &sourcev1b2.HelmChart{ ObjectMeta: metav1.ObjectMeta{ + Name: "mock", + Namespace: "default", Generation: 2, }, Status: sourcev1b2.HelmChartStatus{ @@ -2363,7 +2817,7 @@ func Test_isHelmChartReady(t *testing.T) { return m }(), want: false, - wantReason: "latest generation of object has not been reconciled", + wantReason: "HelmChart 'default/mock' is not ready: latest generation of object has not been reconciled", }, { name: "chart generation differs from observed generation while Ready=False", @@ -2374,7 +2828,7 @@ func Test_isHelmChartReady(t *testing.T) { return m }(), want: false, - wantReason: "some reason", + wantReason: "HelmChart 'default/mock' is not ready: some reason", }, { name: "chart has Stalled=True", @@ -2385,7 +2839,7 @@ func Test_isHelmChartReady(t *testing.T) { return m }(), want: false, - wantReason: "some stalled reason", + wantReason: "HelmChart 'default/mock' is not ready: some stalled reason", }, { name: "chart does not have an Artifact", @@ -2395,7 +2849,7 @@ func Test_isHelmChartReady(t *testing.T) { return m }(), want: false, - wantReason: "does not have an artifact", + wantReason: "HelmChart 'default/mock' is not ready: does not have an artifact", }, } for _, tt := range tests { @@ -2410,3 +2864,89 @@ func Test_isHelmChartReady(t *testing.T) { }) } } + +func Test_isOCIRepositoryReady(t *testing.T) { + mock := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mock", + Namespace: "default", + Generation: 2, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + Artifact: &sourcev1.Artifact{}, + }, + } + + tests := []struct { + name string + obj *sourcev1b2.OCIRepository + want bool + wantReason string + }{ + { + name: "OCIRepository is ready", + obj: mock.DeepCopy(), + want: true, + }, + { + name: "OCIRepository generation differs from observed generation while Ready=True", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Generation = 3 + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: latest generation of object has not been reconciled", + }, + { + name: "OCIRepository generation differs from observed generation while Ready=False", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Generation = 3 + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: some reason", + }, + { + name: "OCIRepository has Stalled=True", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + conditions.MarkStalled(m, "Reason", "some stalled reason") + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: some stalled reason", + }, + { + name: "OCIRepository does not have an Artifact", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Status.Artifact = nil + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: does not have an artifact", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotReason := isOCIRepositoryReady(tt.obj) + if got != tt.want { + t.Errorf("isOCIRepositoryReady() got = %v, want %v", got, tt.want) + } + if gotReason != tt.wantReason { + t.Errorf("isOCIRepositoryReady() reason = %v, want %v", gotReason, tt.wantReason) + } + }) + } +} diff --git a/internal/reconcile/helmchart_template.go b/internal/reconcile/helmchart_template.go index 5bf5a9630..57fd257fd 100644 --- a/internal/reconcile/helmchart_template.go +++ b/internal/reconcile/helmchart_template.go @@ -95,6 +95,20 @@ func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error { } } + if mustCleanDeployedChart(obj) { + // If the HelmRelease has a ChartRef and no Chart template, and the + // HelmChart is present, we need to clean it up. + if err := r.reconcileDelete(ctx, req.Object); err != nil { + return err + } + return nil + } + + if obj.IsChartRefPresent() { + // if a chartRef is present, we do not need to reconcile the HelmChart from the template. + return nil + } + // Confirm we are allowed to fetch the HelmChart. if err := acl.AllowsAccessTo(req.Object, sourcev1.HelmChartKind, chartRef); err != nil { return err @@ -230,3 +244,13 @@ func buildHelmChartFromTemplate(obj *v2.HelmRelease) *sourcev1.HelmChart { } return result } + +func mustCleanDeployedChart(obj *v2.HelmRelease) bool { + if obj.IsChartRefPresent() && !obj.IsChartTemplatePresent() { + if obj.Status.HelmChart != "" { + return true + } + } + + return false +} diff --git a/internal/reconcile/helmchart_template_test.go b/internal/reconcile/helmchart_template_test.go index 0385997ff..5265d5778 100644 --- a/internal/reconcile/helmchart_template_test.go +++ b/internal/reconcile/helmchart_template_test.go @@ -442,6 +442,60 @@ func TestHelmChartTemplate_Reconcile(t *testing.T) { g.Expect(err).To(HaveOccurred()) g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) + + t.Run("Spec ChartRef and existing chart trigger delete", func(t *testing.T) { + g := NewWithT(t) + + releaseName := "garbage-collection" + existingChart := sourcev1.HelmChart{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace.GetName(), + Name: fmt.Sprintf("%s-%s", namespace.GetName(), releaseName), + Labels: map[string]string{ + v2.GroupVersion.Group + "/name": releaseName, + v2.GroupVersion.Group + "/namespace": namespace.GetName(), + }, + }, + Spec: sourcev1.HelmChartSpec{ + Chart: "./bar", + SourceRef: sourcev1.LocalHelmChartSourceReference{ + Kind: sourcev1.HelmRepositoryKind, + Name: "bar-repository", + }, + }, + } + g.Expect(testEnv.CreateAndWait(context.TODO(), &existingChart)).To(Succeed()) + t.Cleanup(func() { + g.Expect(testEnv.Cleanup(context.Background(), &existingChart)).To(Succeed()) + }) + + recorder := record.NewFakeRecorder(32) + r := &HelmChartTemplate{ + client: testEnv, + eventRecorder: recorder, + fieldManager: testFieldManager, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace.GetName(), + Name: releaseName, + }, + Spec: v2.HelmReleaseSpec{ + Interval: metav1.Duration{Duration: 1 * time.Hour}, + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: sourcev1.OCIRepositoryKind, + Name: "oci-repository", + }, + }, + Status: v2.HelmReleaseStatus{ + HelmChart: fmt.Sprintf("%s/%s", existingChart.GetNamespace(), existingChart.GetName()), + }, + } + err := r.Reconcile(context.TODO(), &Request{Object: obj}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(obj.Status.HelmChart).To(BeEmpty()) + }) } func TestHelmChartTemplate_reconcileDelete(t *testing.T) { From 350accfba9a875529c9ce5dd254d137e279bdd62 Mon Sep 17 00:00:00 2001 From: souleb Date: Tue, 26 Mar 2024 15:39:26 +0100 Subject: [PATCH 02/12] Update api/v2beta2/helmrelease_types.go Co-authored-by: Hidde Beydals Signed-off-by: souleb --- api/v2beta2/helmrelease_types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index a4d28d7ab..73e4750e3 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -1248,12 +1248,12 @@ func (in *HelmRelease) GetStatusConditions() *[]metav1.Condition { } // IsChartRefPresent returns true if the HelmRelease has a ChartRef. -func (in *HelmRelease) IsChartRefPresent() bool { +func (in *HelmRelease) HasChartRef() bool { return in.Spec.ChartRef != nil } // IsChartTemplatePresent returns true if the HelmRelease has a ChartTemplate. -func (in *HelmRelease) IsChartTemplatePresent() bool { +func (in *HelmRelease) HasChartTemplate() bool { return in.Spec.Chart.Spec.Chart != "" } From 157f806598ddc673d8ee7d93a49bcef913485463 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Thu, 28 Mar 2024 14:47:56 +0100 Subject: [PATCH 03/12] fix methods names Signed-off-by: Soule BA --- internal/controller/helmrelease_controller.go | 10 +++++----- internal/reconcile/helmchart_template.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 8c1109139..5774a447b 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -675,7 +675,7 @@ func (r *HelmReleaseReconciler) buildRESTClientGetter(ctx context.Context, obj * // It returns the source object or an error. func (r *HelmReleaseReconciler) getSource(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { var name, namespace string - if obj.IsChartRefPresent() { + if obj.HasChartRef() { if obj.Spec.ChartRef.Kind == sourcev1.OCIRepositoryKind { return r.getHelmChartFromOCIRef(ctx, obj) } @@ -864,17 +864,17 @@ func isOCIRepositoryReady(obj *sourcev1.OCIRepository) (bool, string) { } func isValidChartRef(obj *v2.HelmRelease) bool { - return (obj.IsChartRefPresent() && !obj.IsChartTemplatePresent()) || - (!obj.IsChartRefPresent() && obj.IsChartTemplatePresent()) + return (obj.HasChartRef() && !obj.HasChartTemplate()) || + (!obj.HasChartRef() && obj.HasChartTemplate()) } func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { namespacedName := types.NamespacedName{} switch { - case obj.IsChartRefPresent() && !obj.IsChartTemplatePresent(): + case obj.HasChartRef() && !obj.HasChartTemplate(): namespacedName.Namespace = obj.Spec.ChartRef.Namespace namespacedName.Name = obj.Spec.ChartRef.Name - case !obj.IsChartRefPresent() && obj.IsChartTemplatePresent(): + case !obj.HasChartRef() && obj.HasChartTemplate(): namespacedName.Namespace = obj.Spec.Chart.GetNamespace(obj.GetNamespace()) namespacedName.Name = obj.GetHelmChartName() default: diff --git a/internal/reconcile/helmchart_template.go b/internal/reconcile/helmchart_template.go index 57fd257fd..4769c21dd 100644 --- a/internal/reconcile/helmchart_template.go +++ b/internal/reconcile/helmchart_template.go @@ -104,7 +104,7 @@ func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error { return nil } - if obj.IsChartRefPresent() { + if obj.HasChartRef() { // if a chartRef is present, we do not need to reconcile the HelmChart from the template. return nil } @@ -246,7 +246,7 @@ func buildHelmChartFromTemplate(obj *v2.HelmRelease) *sourcev1.HelmChart { } func mustCleanDeployedChart(obj *v2.HelmRelease) bool { - if obj.IsChartRefPresent() && !obj.IsChartTemplatePresent() { + if obj.HasChartRef() && !obj.HasChartTemplate() { if obj.Status.HelmChart != "" { return true } From 686fe58f6e4eb51c1532c3dc42383189de594060 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 2 Apr 2024 09:57:10 +0200 Subject: [PATCH 04/12] address review comments Signed-off-by: Soule BA --- api/v2beta2/helmrelease_types.go | 2 +- api/v2beta2/reference_types.go | 3 +- internal/controller/helmrelease_controller.go | 69 ++---- .../controller/helmrelease_controller_test.go | 214 +++++++----------- 4 files changed, 110 insertions(+), 178 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index 73e4750e3..0e71af042 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -79,7 +79,7 @@ type HelmReleaseSpec struct { // Chart defines the template of the v1beta2.HelmChart that should be created // for this HelmRelease. // +optional - Chart HelmChartTemplate `json:"chart"` + Chart HelmChartTemplate `json:"chart,omitempty"` // ChartRef holds a reference to a source controller resource containing the // Helm chart artifact. diff --git a/api/v2beta2/reference_types.go b/api/v2beta2/reference_types.go index 6208b2207..344e5038f 100644 --- a/api/v2beta2/reference_types.go +++ b/api/v2beta2/reference_types.go @@ -60,7 +60,8 @@ type CrossNamespaceSourceReference struct { // +required Name string `json:"name"` - // Namespace of the referent. + // Namespace of the referent, defaults to the namespace of the Kubernetes + // resource object that contains the reference. // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=63 // +kubebuilder:validation:Optional diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 5774a447b..c16f795bf 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -50,6 +50,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/jitter" "github.com/fluxcd/pkg/runtime/logger" + "github.com/fluxcd/pkg/runtime/object" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" source "github.com/fluxcd/source-controller/api/v1" @@ -107,7 +108,7 @@ var ( ) func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opts HelmReleaseReconcilerOptions) error { - // Index the HelmRelease by the HelmChart references they point at + // Index the HelmRelease by the Source reference they point to. if err := mgr.GetFieldIndexer().IndexField(ctx, &v2.HelmRelease{}, v2.SourceIndexKey, func(o client.Object) []string { obj := o.(*v2.HelmRelease) @@ -677,7 +678,7 @@ func (r *HelmReleaseReconciler) getSource(ctx context.Context, obj *v2.HelmRelea var name, namespace string if obj.HasChartRef() { if obj.Spec.ChartRef.Kind == sourcev1.OCIRepositoryKind { - return r.getHelmChartFromOCIRef(ctx, obj) + return r.getSourceFromOCIRef(ctx, obj) } name, namespace = obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace if namespace == "" { @@ -700,7 +701,7 @@ func (r *HelmReleaseReconciler) getSource(ctx context.Context, obj *v2.HelmRelea return &hc, nil } -func (r *HelmReleaseReconciler) getHelmChartFromOCIRef(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { +func (r *HelmReleaseReconciler) getSourceFromOCIRef(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { name, namespace := obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace if namespace == "" { namespace = obj.GetNamespace() @@ -801,63 +802,35 @@ func (r *HelmReleaseReconciler) requestsForOCIRrepositoryChange(ctx context.Cont } func isSourceReady(obj source.Source) (bool, string) { - if hc, ok := obj.(*sourcev1.HelmChart); ok { - return isHelmChartReady(hc) + if o, ok := obj.(conditions.Getter); ok { + return isReady(o, obj.GetArtifact()) } - - or, ok := obj.(*sourcev1.OCIRepository) - if ok { - return isOCIRepositoryReady(or) - } - - return false, "unknown source type" + return false, fmt.Sprintf("unknown source type: %T", obj) } -// isHelmChartReady returns true if the given HelmChart is ready, and a reason -// why it is not ready otherwise. -func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) { - switch { - case obj.Generation != obj.Status.ObservedGeneration: - msg := "latest generation of object has not been reconciled" - - // If the chart is not ready, we can likely provide a more - // concise reason why. - // We do not do this while the Generation matches the Observed - // Generation, as we could then potentially stall on e.g. - // temporary errors which do not have an impact as long as - // there is an Artifact for the current Generation. - if conditions.IsFalse(obj, meta.ReadyCondition) { - msg = conditions.GetMessage(obj, meta.ReadyCondition) - } - return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), msg) - case conditions.IsStalled(obj): - return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), conditions.GetMessage(obj, meta.StalledCondition)) - case obj.Status.Artifact == nil: - return false, fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), "does not have an artifact") - default: - return true, "" +func isReady(obj conditions.Getter, artifact *source.Artifact) (bool, string) { + observedGen, err := object.GetStatusObservedGeneration(obj) + if err != nil { + return false, err.Error() } -} -func isOCIRepositoryReady(obj *sourcev1.OCIRepository) (bool, string) { + kind := obj.GetObjectKind().GroupVersionKind().Kind + switch { - case obj.Generation != obj.Status.ObservedGeneration: + case obj.GetGeneration() != observedGen: msg := "latest generation of object has not been reconciled" if conditions.IsFalse(obj, meta.ReadyCondition) { msg = conditions.GetMessage(obj, meta.ReadyCondition) } - return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), msg) + return false, fmt.Sprintf("%s '%s/%s' is not ready: %s", + kind, obj.GetNamespace(), obj.GetName(), msg) case conditions.IsStalled(obj): - return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), conditions.GetMessage(obj, meta.StalledCondition)) - case obj.Status.Artifact == nil: - return false, fmt.Sprintf("OCIRepository '%s/%s' is not ready: %s", - obj.GetNamespace(), obj.GetName(), "does not have an artifact") + return false, fmt.Sprintf("%s '%s/%s' is not ready: %s", + kind, obj.GetNamespace(), obj.GetName(), conditions.GetMessage(obj, meta.StalledCondition)) + case artifact == nil: + return false, fmt.Sprintf("%s '%s/%s' is not ready: %s", + kind, obj.GetNamespace(), obj.GetName(), "does not have an artifact") default: return true, "" } diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index ea7d82269..89e69d173 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -286,58 +286,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { })) }) - t.Run("confirms HelmChart has an Artifact", func(t *testing.T) { - g := NewWithT(t) - - chart := &sourcev1b2.HelmChart{ - ObjectMeta: metav1.ObjectMeta{ - Name: "chart", - Namespace: "mock", - Generation: 2, - }, - Status: sourcev1b2.HelmChartStatus{ - ObservedGeneration: 2, - Artifact: nil, - Conditions: []metav1.Condition{ - { - Type: meta.ReadyCondition, - Status: metav1.ConditionTrue, - }, - }, - }, - } - - obj := &v2.HelmRelease{ - ObjectMeta: metav1.ObjectMeta{ - Name: "release", - Namespace: "mock", - }, - Spec: v2.HelmReleaseSpec{ - Interval: metav1.Duration{Duration: 1 * time.Second}, - }, - Status: v2.HelmReleaseStatus{ - HelmChart: "mock/chart", - }, - } - - r := &HelmReleaseReconciler{ - Client: fake.NewClientBuilder(). - WithScheme(NewTestScheme()). - WithStatusSubresource(&v2.HelmRelease{}). - WithObjects(chart, obj). - Build(), - } - - res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).To(Equal(errWaitForChart)) - g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) - - g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "HelmChart 'mock/chart' is not ready"), - })) - }) - t.Run("reports values composition failure", func(t *testing.T) { g := NewWithT(t) @@ -1019,61 +967,6 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin }, Status: sourcev1b2.OCIRepositoryStatus{ ObservedGeneration: 2, - Artifact: nil, - Conditions: []metav1.Condition{ - { - Type: meta.ReadyCondition, - Status: metav1.ConditionTrue, - }, - }, - }, - } - - obj := &v2.HelmRelease{ - ObjectMeta: metav1.ObjectMeta{ - Name: "release", - Namespace: "mock", - }, - Spec: v2.HelmReleaseSpec{ - ChartRef: &v2.CrossNamespaceSourceReference{ - Kind: "OCIRepository", - Name: "ocirepo", - Namespace: "mock", - }, - Interval: metav1.Duration{Duration: 1 * time.Second}, - }, - } - - r := &HelmReleaseReconciler{ - Client: fake.NewClientBuilder(). - WithScheme(NewTestScheme()). - WithStatusSubresource(&v2.HelmRelease{}). - WithObjects(ocirepo, obj). - Build(), - } - - res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).To(Equal(errWaitForChart)) - g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) - - g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ - *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, "SourceNotReady", "OCIRepository 'mock/ocirepo' is not ready"), - })) - }) - - t.Run("confirms ChartRef has an Artifact", func(t *testing.T) { - g := NewWithT(t) - - ocirepo := &sourcev1b2.OCIRepository{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ocirepo", - Namespace: "mock", - Generation: 2, - }, - Status: sourcev1b2.OCIRepositoryStatus{ - ObservedGeneration: 2, - Artifact: nil, Conditions: []metav1.Condition{ { Type: meta.ReadyCondition, @@ -1317,6 +1210,82 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Source not ready"), })) }) + + t.Run("handles reconciliation of new artifact", func(t *testing.T) { + g := NewWithT(t) + + chartMock := testutil.BuildChart() + chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root()) + g.Expect(err).ToNot(HaveOccurred()) + + ociRepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: "mock", + Generation: 1, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 1, + Artifact: chartArtifact, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: "mock", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + 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, + 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()) + + 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 history and storage namespace are cleared. + g.Expect(obj.Status.History).To(BeNil()) + g.Expect(obj.Status.StorageNamespace).To(BeEmpty()) + }) } func TestHelmReleaseReconciler_reconcileDelete(t *testing.T) { @@ -2447,25 +2416,6 @@ func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { wantErr: true, disallowCrossNS: true, }, - { - "get chart from reference", - &v2.HelmRelease{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - }, - Spec: v2.HelmReleaseSpec{ - ChartRef: &v2.CrossNamespaceSourceReference{ - Kind: "HelmChart", - Name: "some-chart-name", - Namespace: "some-namespace", - }, - }, - }, - chart, - true, - false, - false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2781,6 +2731,10 @@ func TestValuesReferenceValidation(t *testing.T) { func Test_isHelmChartReady(t *testing.T) { mock := &sourcev1b2.HelmChart{ + TypeMeta: metav1.TypeMeta{ + Kind: "HelmChart", + APIVersion: sourcev1b2.GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "mock", Namespace: "default", @@ -2854,7 +2808,7 @@ func Test_isHelmChartReady(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, gotReason := isHelmChartReady(tt.obj) + got, gotReason := isReady(tt.obj, tt.obj.GetArtifact()) if got != tt.want { t.Errorf("isHelmChartReady() got = %v, want %v", got, tt.want) } @@ -2867,6 +2821,10 @@ func Test_isHelmChartReady(t *testing.T) { func Test_isOCIRepositoryReady(t *testing.T) { mock := &sourcev1b2.OCIRepository{ + TypeMeta: metav1.TypeMeta{ + Kind: "OCIRepository", + APIVersion: sourcev1b2.GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "mock", Namespace: "default", @@ -2940,7 +2898,7 @@ func Test_isOCIRepositoryReady(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, gotReason := isOCIRepositoryReady(tt.obj) + got, gotReason := isReady(tt.obj, tt.obj.GetArtifact()) if got != tt.want { t.Errorf("isOCIRepositoryReady() got = %v, want %v", got, tt.want) } From d1d2d0002e915c520ebc8301dd34557557ad6902 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 2 Apr 2024 11:13:16 +0200 Subject: [PATCH 05/12] adding an upgrade from ocirepo test Signed-off-by: Soule BA --- .github/workflows/e2e.yaml | 39 +++++++++++++++++++ .../helm.toolkit.fluxcd.io_helmreleases.yaml | 4 +- .../upgrade-from-ocirepo-source/install.yaml | 25 ++++++++++++ .../upgrade-from-ocirepo-source/upgrade.yaml | 10 +++++ docs/api/v2beta2/helm.md | 3 +- 5 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 config/testdata/upgrade-from-ocirepo-source/install.yaml create mode 100644 config/testdata/upgrade-from-ocirepo-source/upgrade.yaml diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 2545b030f..d44ac70fb 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -463,6 +463,45 @@ jobs: fi kubectl delete -n helm-system -f config/testdata/$test_name/install.yaml + - name: Run upgrade from ocirepo source + run: | + test_name=upgrade-from-ocirepo-source + kubectl -n helm-system apply -f config/testdata/$test_name/install.yaml + echo -n ">>> Waiting for expected conditions" + count=0 + until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="True" and .Ready=="True"' )" ]; do + echo -n '.' + sleep 5 + count=$((count + 1)) + if [[ ${count} -eq 24 ]]; then + echo ' No more retries left!' + exit 1 + fi + done + echo ' done' + + # Validate release was installed. + REVISION_COUNT=$(helm -n helm-system history -o json $test_name | jq 'length') + if [ "$REVISION_COUNT" != 1 ]; then + echo -e "Unexpected revision count: $REVISION_COUNT" + exit 1 + fi + + kubectl -n helm-system apply -f config/testdata/$test_name/upgrade.yaml + echo -n ">>> Waiting for expected conditions" + count=0 + until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="True" and .Ready=="True"' )" ]; do + echo -n '.' + sleep 5 + count=$((count + 1)) + if [[ ${count} -eq 24 ]]; then + echo ' No more retries left!' + exit 1 + fi + done + echo ' done' + + kubectl delete -n helm-system -f config/testdata/$test_name/install.yaml - name: Run upgrade fail with uninstall remediation strategy test run: | test_name=upgrade-fail-remediate-uninstall diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index e6279b081..f9ff5a713 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -1439,7 +1439,9 @@ spec: minLength: 1 type: string namespace: - description: Namespace of the referent. + description: |- + Namespace of the referent, defaults to the namespace of the Kubernetes + resource object that contains the reference. maxLength: 63 minLength: 1 type: string diff --git a/config/testdata/upgrade-from-ocirepo-source/install.yaml b/config/testdata/upgrade-from-ocirepo-source/install.yaml new file mode 100644 index 000000000..d56176df9 --- /dev/null +++ b/config/testdata/upgrade-from-ocirepo-source/install.yaml @@ -0,0 +1,25 @@ +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: OCIRepository +metadata: + name: upgrade-from-ocirepo-source +spec: + interval: 30s + url: oci://ghcr.io/stefanprodan/charts/podinfo + ref: + tag: 6.6.0 +--- +apiVersion: helm.toolkit.fluxcd.io/v2beta2 +kind: HelmRelease +metadata: + name: upgrade-from-ocirepo-source +spec: + chartRef: + kind: OCIRepository + name: upgrade-from-ocirepo-source + interval: 30s + values: + resources: + requests: + cpu: 100m + memory: 64Mi diff --git a/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml b/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml new file mode 100644 index 000000000..8118ecb9c --- /dev/null +++ b/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml @@ -0,0 +1,10 @@ +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: OCIRepository +metadata: + name: upgrade-from-ocirepo-source +spec: + interval: 30s + url: oci://ghcr.io/stefanprodan/charts/podinfo + ref: + tag: 6.6.1 diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index ba76811c6..546ca6346 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -546,7 +546,8 @@ string (Optional) -

Namespace of the referent.

+

Namespace of the referent, defaults to the namespace of the Kubernetes +resource object that contains the reference.

From 7864e3a9a2c59d6138c00b0690df146e27883c57 Mon Sep 17 00:00:00 2001 From: souleb Date: Tue, 2 Apr 2024 23:35:23 +0200 Subject: [PATCH 06/12] Apply suggestions from code review Co-authored-by: Stefan Prodan Signed-off-by: souleb --- config/testdata/upgrade-from-ocirepo-source/install.yaml | 2 +- config/testdata/upgrade-from-ocirepo-source/upgrade.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/testdata/upgrade-from-ocirepo-source/install.yaml b/config/testdata/upgrade-from-ocirepo-source/install.yaml index d56176df9..18430c96f 100644 --- a/config/testdata/upgrade-from-ocirepo-source/install.yaml +++ b/config/testdata/upgrade-from-ocirepo-source/install.yaml @@ -7,7 +7,7 @@ spec: interval: 30s url: oci://ghcr.io/stefanprodan/charts/podinfo ref: - tag: 6.6.0 + digest: "sha256:cdd538a0167e4b51152b71a477e51eb6737553510ce8797dbcc537e1342311bb" --- apiVersion: helm.toolkit.fluxcd.io/v2beta2 kind: HelmRelease diff --git a/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml b/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml index 8118ecb9c..8478426a0 100644 --- a/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml +++ b/config/testdata/upgrade-from-ocirepo-source/upgrade.yaml @@ -7,4 +7,4 @@ spec: interval: 30s url: oci://ghcr.io/stefanprodan/charts/podinfo ref: - tag: 6.6.1 + digest: "sha256:0cc9a8446c95009ef382f5eade883a67c257f77d50f84e78ecef2aac9428d1e5" From aeac55dba9cb27069a9960eadf335a8acc105ca3 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 2 Apr 2024 23:38:05 +0200 Subject: [PATCH 07/12] 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 0e71af042..01a174653 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 f9ff5a713..429c43e3e 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 546ca6346..5decee575 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 29df92c59..2860dfc60 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 7eee71cea..77a580de8 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 a293b7386..0b579ca6d 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 c16f795bf..0615bb9d4 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 89e69d173..f6e4c398a 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)) + } + }) + } + +} From 49b47d4c44b5a2edd5495d92ca5d96fcaad86667 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 2 Apr 2024 23:51:52 +0200 Subject: [PATCH 08/12] adding a section for chartRef in the doc Signed-off-by: Soule BA --- docs/spec/v2beta2/helmreleases.md | 65 ++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/docs/spec/v2beta2/helmreleases.md b/docs/spec/v2beta2/helmreleases.md index b37bbb25a..159d37054 100644 --- a/docs/spec/v2beta2/helmreleases.md +++ b/docs/spec/v2beta2/helmreleases.md @@ -176,7 +176,7 @@ A HelmRelease also needs a ### Chart template -`.spec.chart` is a required field used by the helm-controller as a template to +`.spec.chart` is an optional field used by the helm-controller as a template to create a new [HelmChart resource](https://fluxcd.io/flux/components/source/helmcharts/). The spec for the HelmChart is provided via `.spec.chart.spec`, refer to @@ -203,6 +203,69 @@ references with the `--no-cross-namespace-refs=true` flag. When this flag is set, the HelmRelease can only refer to Sources in the same namespace as the HelmRelease object. +### Chart reference + +`.spec.chartRef` is an optional field used to refer to an [OCIRepository resource](https://fluxcd.io/flux/components/source/ocirepositories/) +from which to fetch the Helm chart. The chart is fetched by the controller with the +information provided by `.status.artifact` of the OCIRepository. + +The chart version of the last release attempt is reported in `.status.lastAttemptedRevision`. +The version is in the format +. The digest of the artifact is +appended to the version to ensure that a change in the artifact content triggers a new release. +The controller will automatically perform a Helm release when the OCIRepository produces a new artifact. + +**Warning:** One of `.spec.chart` or `.spec.chartRef` must be set, but not both. +If switching from `.spec.chart` to `.spec.chartRef`, the HelmRelease will uninstall +the previous release before installing the new one. + +**Note:** On multi-tenant clusters, platform admins can disable cross-namespace +references with the `--no-cross-namespace-refs=true` flag. When this flag is +set, the HelmRelease can only refer to Sources in the same namespace as the +HelmRelease object. + +```yaml +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: OCIRepository +metadata: + name: podinfo-ocirepo +spec: + interval: 30s + url: oci://ghcr.io/stefanprodan/charts/podinfo + ref: + tag: 6.6.0 +--- +apiVersion: helm.toolkit.fluxcd.io/v2beta2 +kind: HelmRelease +metadata: + name: podinfo + namespace: default +spec: + interval: 10m + timeout: 5m + chartRef: + kind: OCIRepository + name: podinfo + namespace: default + releaseName: podinfo + install: + remediation: + retries: 3 + upgrade: + remediation: + retries: 3 + test: + enable: true + driftDetection: + mode: enabled + ignore: + - paths: ["/spec/replicas"] + target: + kind: Deployment + values: + replicaCount: 2 +``` + ### Release name `.spec.releaseName` is an optional field used to specify the name of the Helm From edec322a3d065d6ae345de0adbb3588377d3659a Mon Sep 17 00:00:00 2001 From: Soule BA Date: Thu, 4 Apr 2024 11:30:59 +0200 Subject: [PATCH 09/12] Take into account the oci-digest This commit add the oci artifact digest into the release observed snapshot. This is used to later to add that value as an annotation. Signed-off-by: Soule BA --- api/v2beta2/helmrelease_types.go | 7 +- api/v2beta2/snapshot_types.go | 3 + .../helm.toolkit.fluxcd.io_helmreleases.yaml | 15 +- docs/api/v2beta2/helm.md | 27 ++- docs/spec/v2beta2/helmreleases.md | 16 -- internal/action/verify.go | 5 + internal/controller/helmrelease_controller.go | 23 ++- .../controller/helmrelease_controller_test.go | 14 +- internal/reconcile/correct_cluster_drift.go | 4 +- internal/reconcile/install.go | 7 +- internal/reconcile/install_test.go | 4 + internal/reconcile/release.go | 49 ++++- internal/reconcile/release_test.go | 178 ++++++++++++++++++ internal/reconcile/rollback_remediation.go | 4 +- internal/reconcile/test.go | 4 +- internal/reconcile/uninstall.go | 4 +- internal/reconcile/uninstall_remediation.go | 4 +- internal/reconcile/unlock.go | 4 +- internal/reconcile/upgrade.go | 7 +- internal/reconcile/upgrade_test.go | 4 + internal/release/digest_test.go | 2 +- internal/release/observation.go | 3 + 22 files changed, 336 insertions(+), 52 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index 01a174653..9db38102f 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -1002,11 +1002,16 @@ type HelmReleaseStatus struct { LastAppliedRevision string `json:"lastAppliedRevision,omitempty"` // LastAttemptedRevision is the Source revision of the last reconciliation - // attempt. For OCIRegistry sources, the 12 first characters of the digest are + // attempt. For OCIRepository 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"` + // LastAttemptedRevisionDigest is the digest of the last reconciliation attempt. + // This is only set for OCIRepository sources. + // +optional + LastAttemptedRevisionDigest string `json:"lastAttemptedRevisionDigest,omitempty"` + // LastAttemptedValuesChecksum is the SHA1 checksum for the values of the last // reconciliation attempt. // Deprecated: Use LastAttemptedConfigDigest instead. diff --git a/api/v2beta2/snapshot_types.go b/api/v2beta2/snapshot_types.go index 587667665..ca7589a32 100644 --- a/api/v2beta2/snapshot_types.go +++ b/api/v2beta2/snapshot_types.go @@ -156,6 +156,9 @@ type Snapshot struct { // run by the controller. // +optional TestHooks *map[string]*TestHookStatus `json:"testHooks,omitempty"` + // OCIDigest is the digest of the OCI artifact associated with the release. + // +optional + OCIDigest string `json:"ociDigest,omitempty"` } // FullReleaseName returns the full name of the release in the format diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index 429c43e3e..e38f8922e 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -1100,6 +1100,10 @@ spec: description: Namespace is the namespace the release is deployed to. type: string + ociDigest: + description: OCIDigest is the digest of the OCI artifact associated + with the release. + type: string status: description: Status is the current state of the release. type: string @@ -2374,6 +2378,10 @@ spec: description: Namespace is the namespace the release is deployed to. type: string + ociDigest: + description: OCIDigest is the digest of the OCI artifact associated + with the release. + type: string status: description: Status is the current state of the release. type: string @@ -2452,9 +2460,14 @@ spec: lastAttemptedRevision: description: |- LastAttemptedRevision is the Source revision of the last reconciliation - attempt. For OCIRegistry sources, the 12 first characters of the digest are + attempt. For OCIRepository sources, the 12 first characters of the digest are appended to the chart version e.g. "1.2.3+1234567890ab". type: string + lastAttemptedRevisionDigest: + description: |- + LastAttemptedRevisionDigest is the digest of the last reconciliation attempt. + This is only set for OCIRepository sources. + type: string lastAttemptedValuesChecksum: description: |- LastAttemptedValuesChecksum is the SHA1 checksum for the values of the last diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index 5decee575..020813053 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -1584,12 +1584,25 @@ string (Optional)

LastAttemptedRevision is the Source revision of the last reconciliation -attempt. For OCIRegistry sources, the 12 first characters of the digest are +attempt. For OCIRepository sources, the 12 first characters of the digest are appended to the chart version e.g. “1.2.3+1234567890ab”.

+lastAttemptedRevisionDigest
+ +string + + + +(Optional) +

LastAttemptedRevisionDigest is the digest of the last reconciliation attempt. +This is only set for OCIRepository sources.

+ + + + lastAttemptedValuesChecksum
string @@ -2380,6 +2393,18 @@ TestHookStatus run by the controller.

+ + +ociDigest
+ +string + + + +(Optional) +

OCIDigest is the digest of the OCI artifact associated with the release.

+ + diff --git a/docs/spec/v2beta2/helmreleases.md b/docs/spec/v2beta2/helmreleases.md index 159d37054..a8efba435 100644 --- a/docs/spec/v2beta2/helmreleases.md +++ b/docs/spec/v2beta2/helmreleases.md @@ -242,26 +242,10 @@ metadata: namespace: default spec: interval: 10m - timeout: 5m chartRef: kind: OCIRepository name: podinfo namespace: default - releaseName: podinfo - install: - remediation: - retries: 3 - upgrade: - remediation: - retries: 3 - test: - enable: true - driftDetection: - mode: enabled - ignore: - - paths: ["/spec/replicas"] - target: - kind: Deployment values: replicaCount: 2 ``` diff --git a/internal/action/verify.go b/internal/action/verify.go index 3c6852260..aaa143e1b 100644 --- a/internal/action/verify.go +++ b/internal/action/verify.go @@ -126,6 +126,11 @@ func VerifyReleaseObject(snapshot *v2.Snapshot, rls *helmrelease.Release) error verifier := relDig.Verifier() obs := release.ObserveRelease(rls) + + // unfortunately we have to pass in the OciDigest as is, because helmrelease.Release + // does not have a field for it. + obs.OCIDigest = snapshot.OCIDigest + if err = obs.Encode(verifier); err != nil { // We are expected to be able to encode valid JSON, error out without a // typed error assuming malfunction to signal to e.g. retry. diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 0615bb9d4..a997a1cab 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -335,7 +335,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress") } - err = mutateChartWithSourceRevision(loadedChart, source) + ociDigest, err := mutateChartWithSourceRevision(loadedChart, source) if err != nil { conditions.MarkFalse(obj, meta.ReadyCondition, "ChartMutateError", err.Error()) return ctrl.Result{}, err @@ -388,6 +388,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // Set last attempt values. obj.Status.LastAttemptedGeneration = obj.Generation obj.Status.LastAttemptedRevision = loadedChart.Metadata.Version + obj.Status.LastAttemptedRevisionDigest = ociDigest obj.Status.LastAttemptedConfigDigest = chartutil.DigestValues(digest.Canonical, values).String() obj.Status.LastAttemptedValuesChecksum = "" obj.Status.LastReleaseRevision = 0 @@ -865,24 +866,28 @@ func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { return namespacedName, nil } -func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) error { +func mutateChartWithSourceRevision(chart *chart.Chart, source source.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 { - return nil + // if not make sure to return an empty string to delete the digest of the + // last attempted revision + return "", nil } ver, err := semver.NewVersion(chart.Metadata.Version) if err != nil { - return err + return "", err } + + var ociDigest string 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) + 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 @@ -890,17 +895,19 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) err // 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 + return "", err } + ociDigest = tagD[1] default: // default to the digest sha := strings.Split(revision, ":")[1] *ver, err = ver.SetMetadata(sha[0:12]) if err != nil { - return err + return "", err } + ociDigest = revision } chart.Metadata.Version = ver.String() - return nil + return ociDigest, nil } diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index f6e4c398a..e54b495fc 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -186,6 +186,10 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g := NewWithT(t) chart := &sourcev1b2.HelmChart{ + TypeMeta: metav1.TypeMeta{ + APIVersion: sourcev1b2.GroupVersion.String(), + Kind: sourcev1b2.HelmChartKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: "chart", Namespace: "mock", @@ -238,6 +242,10 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { g := NewWithT(t) chart := &sourcev1b2.HelmChart{ + TypeMeta: metav1.TypeMeta{ + APIVersion: sourcev1b2.GroupVersion.String(), + Kind: sourcev1b2.HelmChartKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: "chart", Namespace: "mock", @@ -960,6 +968,10 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin g := NewWithT(t) ocirepo := &sourcev1b2.OCIRepository{ + TypeMeta: metav1.TypeMeta{ + APIVersion: sourcev1b2.GroupVersion.String(), + Kind: sourcev1b2.OCIRepositoryKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: "ocirepo", Namespace: "mock", @@ -2972,7 +2984,7 @@ func Test_TryMutateChartWithSourceRevision(t *testing.T) { }, } - err := mutateChartWithSourceRevision(c, s) + _, err := mutateChartWithSourceRevision(c, s) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { diff --git a/internal/reconcile/correct_cluster_drift.go b/internal/reconcile/correct_cluster_drift.go index bbd6f7a31..a6b4c8891 100644 --- a/internal/reconcile/correct_cluster_drift.go +++ b/internal/reconcile/correct_cluster_drift.go @@ -99,10 +99,10 @@ func (r *CorrectClusterDrift) report(obj *v2.HelmRelease, changeSet *ssa.ChangeS sb.WriteString(changeSet.String()) } - r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest), corev1.EventTypeWarning, + r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeWarning, "DriftCorrectionFailed", sb.String()) case changeSet != nil && len(changeSet.Entries) > 0: - r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest), corev1.EventTypeNormal, + r.eventRecorder.AnnotatedEventf(obj, eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, "DriftCorrected", "Cluster state of release %s has been corrected:\n%s", obj.Status.History.Latest().FullReleaseName(), changeSet.String()) } diff --git a/internal/reconcile/install.go b/internal/reconcile/install.go index 0567b549d..0d51a5556 100644 --- a/internal/reconcile/install.go +++ b/internal/reconcile/install.go @@ -92,7 +92,7 @@ func (r *Install) Reconcile(ctx context.Context, req *Request) error { _, err := action.Install(ctx, cfg, req.Object, req.Chart, req.Values) // Record the history of releases observed during the install. - obsReleases.recordOnObject(req.Object) + obsReleases.recordOnObject(req.Object, mutateOCIDigest) if err != nil { r.failure(req, logBuf, err) @@ -154,7 +154,8 @@ func (r *Install) failure(req *Request, buffer *action.LogBuffer, err error) { // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(req.Chart.Metadata.Version, chartutil.DigestValues(digest.Canonical, req.Values).String()), + eventMeta(req.Chart.Metadata.Version, chartutil.DigestValues(digest.Canonical, req.Values).String(), + addOCIDigest(req.Object.Status.LastAttemptedRevisionDigest)), corev1.EventTypeWarning, v2.InstallFailedReason, eventMessageWithLog(msg, buffer), @@ -181,7 +182,7 @@ func (r *Install) success(req *Request) { // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, v2.InstallSucceededReason, msg, diff --git a/internal/reconcile/install_test.go b/internal/reconcile/install_test.go index d156e45ca..4b4fe3efe 100644 --- a/internal/reconcile/install_test.go +++ b/internal/reconcile/install_test.go @@ -307,6 +307,9 @@ func TestInstall_failure(t *testing.T) { ReleaseName: mockReleaseName, TargetNamespace: mockReleaseNamespace, }, + Status: v2.HelmReleaseStatus{ + LastAttemptedRevisionDigest: "sha256:1234567890", + }, } chrt = testutil.BuildChart() err = errors.New("installation error") @@ -337,6 +340,7 @@ func TestInstall_failure(t *testing.T) { Message: expectMsg, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ + eventMetaGroupKey(metaOCIDigestKey): obj.Status.LastAttemptedRevisionDigest, eventMetaGroupKey(eventv1.MetaRevisionKey): chrt.Metadata.Version, eventMetaGroupKey(eventv1.MetaTokenKey): chartutil.DigestValues(digest.Canonical, req.Values).String(), }, diff --git a/internal/reconcile/release.go b/internal/reconcile/release.go index ce0a74d50..a83ddb2e1 100644 --- a/internal/reconcile/release.go +++ b/internal/reconcile/release.go @@ -43,6 +43,10 @@ var ( ErrReleaseMismatch = errors.New("release mismatch") ) +// mutateObservedRelease is a function that mutates the Observation with the +// given HelmRelease object. +type mutateObservedRelease func(*v2.HelmRelease, release.Observation) release.Observation + // observedReleases is a map of Helm releases as observed to be written to the // Helm storage. The key is the version of the release. type observedReleases map[int]release.Observation @@ -58,7 +62,7 @@ func (r observedReleases) sortedVersions() (versions []int) { } // recordOnObject records the observed releases on the HelmRelease object. -func (r observedReleases) recordOnObject(obj *v2.HelmRelease) { +func (r observedReleases) recordOnObject(obj *v2.HelmRelease, mutators ...mutateObservedRelease) { switch len(r) { case 0: return @@ -67,17 +71,25 @@ func (r observedReleases) recordOnObject(obj *v2.HelmRelease) { for _, o := range r { obs = o } + for _, mut := range mutators { + obs = mut(obj, obs) + } obj.Status.History = append(v2.Snapshots{release.ObservedToSnapshot(obs)}, obj.Status.History...) default: versions := r.sortedVersions() - - obj.Status.History = append(v2.Snapshots{release.ObservedToSnapshot(r[versions[0]])}, obj.Status.History...) + obs := r[versions[0]] + for _, mut := range mutators { + obs = mut(obj, obs) + } + obj.Status.History = append(v2.Snapshots{release.ObservedToSnapshot(obs)}, obj.Status.History...) for _, ver := range versions[1:] { for i := range obj.Status.History { snap := obj.Status.History[i] if snap.Targets(r[ver].Name, r[ver].Namespace, r[ver].Version) { - newSnap := release.ObservedToSnapshot(r[ver]) + obs := r[ver] + obs.OCIDigest = snap.OCIDigest + newSnap := release.ObservedToSnapshot(obs) newSnap.SetTestHooks(snap.GetTestHooks()) obj.Status.History[i] = newSnap return @@ -87,6 +99,11 @@ func (r observedReleases) recordOnObject(obj *v2.HelmRelease) { } } +func mutateOCIDigest(obj *v2.HelmRelease, obs release.Observation) release.Observation { + obs.OCIDigest = obj.Status.LastAttemptedRevisionDigest + return obs +} + // observeRelease returns a storage.ObserveFunc that stores the observed // releases in the given observedReleases map. // It can be used for Helm actions that modify multiple releases in the @@ -174,9 +191,15 @@ func eventMessageWithLog(msg string, log *action.LogBuffer) string { return msg } +// addMeta is a function that adds metadata to an event map. +type addMeta func(map[string]string) + +// metaOCIDigestKey is the key for the OCI digest metadata. +const metaOCIDigestKey = "oci-digest" + // eventMeta returns the event (annotation) metadata based on the given // parameters. -func eventMeta(revision, token string) map[string]string { +func eventMeta(revision, token string, metas ...addMeta) map[string]string { var metadata map[string]string if revision != "" || token != "" { metadata = make(map[string]string) @@ -187,9 +210,25 @@ func eventMeta(revision, token string) map[string]string { metadata[eventMetaGroupKey(eventv1.MetaTokenKey)] = token } } + + for _, add := range metas { + add(metadata) + } + return metadata } +func addOCIDigest(digest string) addMeta { + return func(m map[string]string) { + if digest != "" { + if m == nil { + m = make(map[string]string) + } + m[eventMetaGroupKey(metaOCIDigestKey)] = digest + } + } +} + // eventMetaGroupKey returns the event (annotation) metadata key prefixed with // the group. func eventMetaGroupKey(key string) string { diff --git a/internal/reconcile/release_test.go b/internal/reconcile/release_test.go index 167b8df72..66ca6c9b2 100644 --- a/internal/reconcile/release_test.go +++ b/internal/reconcile/release_test.go @@ -17,10 +17,12 @@ limitations under the License. package reconcile import ( + "fmt" "testing" "github.com/go-logr/logr" . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/fluxcd/pkg/apis/meta" @@ -454,3 +456,179 @@ func mockLogBuffer(size int, lines int) *action.LogBuffer { } return log } + +func Test_RecordOnObject(t *testing.T) { + tests := []struct { + name string + obj *v2.HelmRelease + r observedReleases + mutate bool + testFunc func(*v2.HelmRelease) error + }{ + { + name: "record observed releases", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + }, + r: observedReleases{ + 1: { + Name: mockReleaseName, + Version: 1, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "1.0.0", + }, + }, + }, + testFunc: func(obj *v2.HelmRelease) error { + if len(obj.Status.History) != 1 { + return fmt.Errorf("history length is not 1") + } + if obj.Status.History[0].Name != mockReleaseName { + return fmt.Errorf("release name is not %s", mockReleaseName) + } + return nil + }, + }, + { + name: "record observed releases with multiple versions", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + }, + r: observedReleases{ + 1: { + Name: mockReleaseName, + Version: 1, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "1.0.0", + }, + }, + 2: { + Name: mockReleaseName, + Version: 2, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "2.0.0", + }, + }, + }, + testFunc: func(obj *v2.HelmRelease) error { + if len(obj.Status.History) != 1 { + return fmt.Errorf("want history length 1, got %d", len(obj.Status.History)) + } + if obj.Status.History[0].Name != mockReleaseName { + return fmt.Errorf("release name is not %s", mockReleaseName) + } + if obj.Status.History[0].ChartVersion != "2.0.0" { + return fmt.Errorf("want chart version %s, got %s", "2.0.0", obj.Status.History[0].ChartVersion) + } + return nil + }, + }, + { + name: "record observed releases with status digest", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + Status: v2.HelmReleaseStatus{ + LastAttemptedRevisionDigest: "sha256:123456", + }, + }, + r: observedReleases{ + 1: { + Name: mockReleaseName, + Version: 1, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "1.0.0", + }, + }, + }, + mutate: true, + testFunc: func(obj *v2.HelmRelease) error { + h := obj.Status.History.Latest() + if h.Name != mockReleaseName { + return fmt.Errorf("release name is not %s", mockReleaseName) + } + if h.ChartVersion != "1.0.0" { + return fmt.Errorf("want chart version %s, got %s", "1.0.0", h.ChartVersion) + } + if h.OCIDigest != obj.Status.LastAttemptedRevisionDigest { + return fmt.Errorf("want digest %s, got %s", obj.Status.LastAttemptedRevisionDigest, h.OCIDigest) + } + return nil + }, + }, + { + name: "record observed releases with multiple versions and status digest", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + }, + Status: v2.HelmReleaseStatus{ + LastAttemptedRevisionDigest: "sha256:123456", + }, + }, + r: observedReleases{ + 1: { + Name: mockReleaseName, + Version: 1, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "1.0.0", + }, + }, + 2: { + Name: mockReleaseName, + Version: 2, + ChartMetadata: chart.Metadata{ + Name: mockReleaseName, + Version: "2.0.0", + }, + }, + }, + mutate: true, + testFunc: func(obj *v2.HelmRelease) error { + if len(obj.Status.History) != 1 { + return fmt.Errorf("want history length 1, got %d", len(obj.Status.History)) + } + h := obj.Status.History.Latest() + if h.Name != mockReleaseName { + return fmt.Errorf("release name is not %s", mockReleaseName) + } + if h.ChartVersion != "2.0.0" { + return fmt.Errorf("want chart version %s, got %s", "2.0.0", h.ChartVersion) + } + if h.OCIDigest != obj.Status.LastAttemptedRevisionDigest { + return fmt.Errorf("want digest %s, got %s", obj.Status.LastAttemptedRevisionDigest, h.OCIDigest) + } + return nil + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + if tt.mutate { + tt.r.recordOnObject(tt.obj, mutateOCIDigest) + } else { + tt.r.recordOnObject(tt.obj) + } + err := tt.testFunc(tt.obj) + g.Expect(err).ToNot(HaveOccurred()) + }) + } + +} diff --git a/internal/reconcile/rollback_remediation.go b/internal/reconcile/rollback_remediation.go index e614f5a30..2bb91e9bf 100644 --- a/internal/reconcile/rollback_remediation.go +++ b/internal/reconcile/rollback_remediation.go @@ -143,7 +143,7 @@ func (r *RollbackRemediation) failure(req *Request, prev *v2.Snapshot, buffer *a // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(prev.ChartVersion, chartutil.DigestValues(digest.Canonical, req.Values).String()), + eventMeta(prev.ChartVersion, chartutil.DigestValues(digest.Canonical, req.Values).String(), addOCIDigest(prev.OCIDigest)), corev1.EventTypeWarning, v2.RollbackFailedReason, eventMessageWithLog(msg, buffer), @@ -162,7 +162,7 @@ func (r *RollbackRemediation) success(req *Request, prev *v2.Snapshot) { // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(prev.ChartVersion, chartutil.DigestValues(digest.Canonical, req.Values).String()), + eventMeta(prev.ChartVersion, chartutil.DigestValues(digest.Canonical, req.Values).String(), addOCIDigest(prev.OCIDigest)), corev1.EventTypeNormal, v2.RollbackSucceededReason, msg, diff --git a/internal/reconcile/test.go b/internal/reconcile/test.go index f40a3d4cf..e71f1ddcf 100644 --- a/internal/reconcile/test.go +++ b/internal/reconcile/test.go @@ -145,7 +145,7 @@ func (r *Test) failure(req *Request, err error) { // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeWarning, v2.TestFailedReason, msg, @@ -181,7 +181,7 @@ func (r *Test) success(req *Request) { // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, v2.TestSucceededReason, msg, diff --git a/internal/reconcile/uninstall.go b/internal/reconcile/uninstall.go index 8c8158745..3c5f8a769 100644 --- a/internal/reconcile/uninstall.go +++ b/internal/reconcile/uninstall.go @@ -180,7 +180,7 @@ func (r *Uninstall) failure(req *Request, buffer *action.LogBuffer, err error) { // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeWarning, v2.UninstallFailedReason, eventMessageWithLog(msg, buffer), ) @@ -201,7 +201,7 @@ func (r *Uninstall) success(req *Request) { // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, v2.UninstallSucceededReason, msg, diff --git a/internal/reconcile/uninstall_remediation.go b/internal/reconcile/uninstall_remediation.go index 4e244cdc0..eeea2d0fc 100644 --- a/internal/reconcile/uninstall_remediation.go +++ b/internal/reconcile/uninstall_remediation.go @@ -154,7 +154,7 @@ func (r *UninstallRemediation) failure(req *Request, buffer *action.LogBuffer, e // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeWarning, v2.UninstallFailedReason, eventMessageWithLog(msg, buffer), @@ -175,7 +175,7 @@ func (r *UninstallRemediation) success(req *Request) { // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, v2.UninstallSucceededReason, msg, diff --git a/internal/reconcile/unlock.go b/internal/reconcile/unlock.go index 7d045856c..e08bd558c 100644 --- a/internal/reconcile/unlock.go +++ b/internal/reconcile/unlock.go @@ -119,7 +119,7 @@ func (r *Unlock) failure(req *Request, cur *v2.Snapshot, status helmrelease.Stat // Record warning event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeWarning, "PendingRelease", msg, @@ -138,7 +138,7 @@ func (r *Unlock) success(req *Request, cur *v2.Snapshot, status helmrelease.Stat // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, "PendingRelease", msg, diff --git a/internal/reconcile/upgrade.go b/internal/reconcile/upgrade.go index 8cdbb0828..6a43b79d1 100644 --- a/internal/reconcile/upgrade.go +++ b/internal/reconcile/upgrade.go @@ -83,7 +83,7 @@ func (r *Upgrade) Reconcile(ctx context.Context, req *Request) error { _, err := action.Upgrade(ctx, cfg, req.Object, req.Chart, req.Values) // Record the history of releases observed during the upgrade. - obsReleases.recordOnObject(req.Object) + obsReleases.recordOnObject(req.Object, mutateOCIDigest) if err != nil { r.failure(req, logBuf, err) @@ -144,7 +144,8 @@ func (r *Upgrade) failure(req *Request, buffer *action.LogBuffer, err error) { // Condition summary. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(req.Chart.Metadata.Version, chartutil.DigestValues(digest.Canonical, req.Values).String()), + eventMeta(req.Chart.Metadata.Version, chartutil.DigestValues(digest.Canonical, req.Values).String(), + addOCIDigest(req.Object.Status.LastAttemptedRevisionDigest)), corev1.EventTypeWarning, v2.UpgradeFailedReason, eventMessageWithLog(msg, buffer), @@ -171,7 +172,7 @@ func (r *Upgrade) success(req *Request) { // Record event. r.eventRecorder.AnnotatedEventf( req.Object, - eventMeta(cur.ChartVersion, cur.ConfigDigest), + eventMeta(cur.ChartVersion, cur.ConfigDigest, addOCIDigest(cur.OCIDigest)), corev1.EventTypeNormal, v2.UpgradeSucceededReason, msg, diff --git a/internal/reconcile/upgrade_test.go b/internal/reconcile/upgrade_test.go index dbfb85b7c..08bc98632 100644 --- a/internal/reconcile/upgrade_test.go +++ b/internal/reconcile/upgrade_test.go @@ -438,6 +438,9 @@ func TestUpgrade_failure(t *testing.T) { ReleaseName: mockReleaseName, TargetNamespace: mockReleaseNamespace, }, + Status: v2.HelmReleaseStatus{ + LastAttemptedRevisionDigest: "sha256:1234567890", + }, } chrt = testutil.BuildChart() err = errors.New("upgrade error") @@ -468,6 +471,7 @@ func TestUpgrade_failure(t *testing.T) { Message: expectMsg, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ + eventMetaGroupKey(metaOCIDigestKey): obj.Status.LastAttemptedRevisionDigest, eventMetaGroupKey(eventv1.MetaRevisionKey): chrt.Metadata.Version, eventMetaGroupKey(eventv1.MetaTokenKey): chartutil.DigestValues(digest.Canonical, req.Values).String(), }, diff --git a/internal/release/digest_test.go b/internal/release/digest_test.go index c340ca965..3aca5b8eb 100644 --- a/internal/release/digest_test.go +++ b/internal/release/digest_test.go @@ -37,7 +37,7 @@ func TestDigest(t *testing.T) { rel: Observation{ Name: "foo", }, - exp: "sha256:91b6773f7696d3eb405708a07e2daedc6e69664dabac8e10af7d570d09f947d5", + exp: "sha256:ca8901e499a79368647134cc4f1c2118f8e7ec64c8a4703b281d17fb01acfbed", }, } for _, tt := range tests { diff --git a/internal/release/observation.go b/internal/release/observation.go index 35c0a4340..0afcc0be9 100644 --- a/internal/release/observation.go +++ b/internal/release/observation.go @@ -79,6 +79,8 @@ type Observation struct { Hooks []helmrelease.Hook `json:"hooks"` // Namespace is the Kubernetes namespace of the release. Namespace string `json:"namespace"` + // OCIDigest is the digest of the OCI artifact that was used to + OCIDigest string `json:"ociDigest"` } // Targets returns if the release matches the given name, namespace and @@ -166,6 +168,7 @@ func ObservedToSnapshot(rls Observation) *v2.Snapshot { LastDeployed: metav1.NewTime(rls.Info.LastDeployed.Time), Deleted: metav1.NewTime(rls.Info.Deleted.Time), Status: rls.Info.Status.String(), + OCIDigest: rls.OCIDigest, } } From 65a02c8c6c6eb18cfe5749582ed03e5a03aa8e69 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Fri, 12 Apr 2024 14:01:15 +0200 Subject: [PATCH 10/12] Add a test when switching from chart template to chartRef The test case successfully upgrade with the same chart because version is not computed the same way (12 digits of digest appended for OCIRepository source). Signed-off-by: Soule BA --- docs/spec/v2beta2/helmreleases.md | 20 +-- internal/controller/helmrelease_controller.go | 27 +++- .../controller/helmrelease_controller_test.go | 142 +++++++++++++++++- 3 files changed, 174 insertions(+), 15 deletions(-) diff --git a/docs/spec/v2beta2/helmreleases.md b/docs/spec/v2beta2/helmreleases.md index a8efba435..02f475145 100644 --- a/docs/spec/v2beta2/helmreleases.md +++ b/docs/spec/v2beta2/helmreleases.md @@ -210,25 +210,27 @@ from which to fetch the Helm chart. The chart is fetched by the controller with information provided by `.status.artifact` of the OCIRepository. The chart version of the last release attempt is reported in `.status.lastAttemptedRevision`. -The version is in the format +. The digest of the artifact is -appended to the version to ensure that a change in the artifact content triggers a new release. -The controller will automatically perform a Helm release when the OCIRepository produces a new artifact. +The version is in the format `+`. The digest of the OCI artifact +is appended to the version to ensure that a change in the artifact content triggers +a new release. The controller will automatically perform a Helm upgrade when the +OCIRepository detects a new digest in the OCI artifact stored in registry, even if +the version inside `Chart.yaml` is unchanged. **Warning:** One of `.spec.chart` or `.spec.chartRef` must be set, but not both. -If switching from `.spec.chart` to `.spec.chartRef`, the HelmRelease will uninstall -the previous release before installing the new one. +When switching from `.spec.chart` to `.spec.chartRef`, the controller will perform +an Helm upgrade and will garbage collect the old HelmChart object. **Note:** On multi-tenant clusters, platform admins can disable cross-namespace -references with the `--no-cross-namespace-refs=true` flag. When this flag is -set, the HelmRelease can only refer to Sources in the same namespace as the +references with the `--no-cross-namespace-refs=true` controller flag. When this flag is +set, the HelmRelease can only refer to OCIRepositories in the same namespace as the HelmRelease object. ```yaml ---- apiVersion: source.toolkit.fluxcd.io/v1beta2 kind: OCIRepository metadata: - name: podinfo-ocirepo + name: podinfo + namespace: default spec: interval: 30s url: oci://ghcr.io/stefanprodan/charts/podinfo diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index a997a1cab..634557725 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -891,17 +891,23 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st } // 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] + sha, err := extractDigetString(tagD[1]) + if err != nil { + return "", err + } // add the digest to the chart version to make sure mutable tags are detected - *ver, err = ver.SetMetadata(sha[0:12]) + *ver, err = ver.SetMetadata(sha) if err != nil { return "", err } ociDigest = tagD[1] default: // default to the digest - sha := strings.Split(revision, ":")[1] - *ver, err = ver.SetMetadata(sha[0:12]) + sha, err := extractDigetString(revision) + if err != nil { + return "", err + } + *ver, err = ver.SetMetadata(sha) if err != nil { return "", err } @@ -911,3 +917,16 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st chart.Metadata.Version = ver.String() return ociDigest, nil } + +func extractDigetString(revision string) (string, error) { + var sha string + if pair := strings.Split(revision, ":"); len(pair) != 2 { + return "", fmt.Errorf("invalid artifact revision %s", revision) + } else { + sha = pair[1] + } + if len(sha) < 12 { + return "", fmt.Errorf("invalid artifact revision %s", revision) + } + return sha[0:12], nil +} diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index e54b495fc..6cd56bc72 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -1299,11 +1299,149 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin // 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)) + 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()) + }) + + t.Run("upgrade by switching from existing HelmChat to chartRef", 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()) + // copy the artifact to mutate the revision + ociArtifact := chartArtifact.DeepCopy() + ociArtifact.Revision += "@" + chartArtifact.Digest + + ns, err := testEnv.CreateNamespace(context.TODO(), "mock") + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { + _ = testEnv.Delete(context.TODO(), ns) + }) + + // hc is the HelmChart object created by the HelmRelease object. + hc := &sourcev1b2.HelmChart{ + ObjectMeta: metav1.ObjectMeta{ + Name: "chart", + Namespace: ns.Name, + Generation: 1, + }, + Spec: sourcev1b2.HelmChartSpec{ + Chart: "testdata/test-helmrepo", + Version: "0.1.0", + SourceRef: sourcev1b2.LocalHelmChartSourceReference{ + Kind: sourcev1b2.HelmRepositoryKind, + Name: "test-helmrepo", + }, + }, + Status: sourcev1b2.HelmChartStatus{ + ObservedGeneration: 1, + Artifact: chartArtifact, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + + // ocirepo is the chartRef object to switch to. + ocirepo := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ocirepo", + Namespace: ns.Name, + Generation: 1, + }, + Spec: sourcev1b2.OCIRepositorySpec{ + URL: "oci://test-example.com", + Interval: metav1.Duration{Duration: 1 * time.Second}, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 1, + Artifact: ociArtifact, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + }, + } + // Create a test Helm release storage mock. + rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: "release", + Namespace: ns.Name, + Version: 1, + Chart: chartMock, + Status: helmrelease.StatusDeployed, + }) + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "release", + Namespace: ns.Name, + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "OCIRepository", + Name: "ocirepo", + }, + }, + Status: v2.HelmReleaseStatus{ + StorageNamespace: ns.Name, + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(rls)), + }, + HelmChart: hc.Namespace + "/" + hc.Name, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(NewTestScheme()). + WithStatusSubresource(&v2.HelmRelease{}). + WithObjects(hc, ocirepo, obj). + Build() + + r := &HelmReleaseReconciler{ + Client: c, + GetClusterConfig: GetTestClusterConfig, + EventRecorder: record.NewFakeRecorder(32), + } + + // Store the Helm release mock in the test namespace. + getter, err := r.buildRESTClientGetter(context.TODO(), obj) + g.Expect(err).ToNot(HaveOccurred()) + + cfg, err := action.NewConfigFactory(getter, action.WithStorage(helmdriver.SecretsDriverName, obj.Status.StorageNamespace)) + g.Expect(err).ToNot(HaveOccurred()) + + store := helmstorage.Init(cfg.Driver) + g.Expect(store.Create(rls)).To(Succeed()) + + _, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) + g.Expect(err).ToNot(HaveOccurred()) + + // Verify attempted values are set. + g.Expect(obj.Status.LastAttemptedGeneration).To(Equal(obj.Generation)) + dig := strings.Split(ociArtifact.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()) + + // verify upgrade succeeded + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s", + fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(), + fmt.Sprintf("%s+%s", chartMock.Metadata.Version, dig))), + *conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s", + fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(), + fmt.Sprintf("%s+%s", chartMock.Metadata.Version, dig))), + })) }) } From f5447b4430b127705888ffd0973c181e3af19d47 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Sun, 14 Apr 2024 23:02:45 +0200 Subject: [PATCH 11/12] All observeFuncs make take into account existing OCI Digest in snapshots Signed-off-by: Soule BA --- internal/reconcile/release.go | 6 + internal/reconcile/rollback_remediation.go | 2 +- .../reconcile/rollback_remediation_test.go | 43 +++++++ internal/reconcile/test.go | 3 +- internal/reconcile/test_test.go | 32 +++++ internal/reconcile/uninstall.go | 2 +- internal/reconcile/uninstall_test.go | 32 +++++ internal/reconcile/unlock.go | 18 ++- internal/reconcile/unlock_test.go | 121 ++++++++++++++++++ 9 files changed, 254 insertions(+), 5 deletions(-) diff --git a/internal/reconcile/release.go b/internal/reconcile/release.go index a83ddb2e1..825d81291 100644 --- a/internal/reconcile/release.go +++ b/internal/reconcile/release.go @@ -104,6 +104,12 @@ func mutateOCIDigest(obj *v2.HelmRelease, obs release.Observation) release.Obser return obs } +func releaseToObservation(rls *helmrelease.Release, snapshot *v2.Snapshot) release.Observation { + obs := release.ObserveRelease(rls) + obs.OCIDigest = snapshot.OCIDigest + return obs +} + // observeRelease returns a storage.ObserveFunc that stores the observed // releases in the given observedReleases map. // It can be used for Helm actions that modify multiple releases in the diff --git a/internal/reconcile/rollback_remediation.go b/internal/reconcile/rollback_remediation.go index 2bb91e9bf..3fbad7099 100644 --- a/internal/reconcile/rollback_remediation.go +++ b/internal/reconcile/rollback_remediation.go @@ -182,7 +182,7 @@ func observeRollback(obj *v2.HelmRelease) storage.ObserveFunc { for i := range obj.Status.History { snap := obj.Status.History[i] if snap.Targets(rls.Name, rls.Namespace, rls.Version) { - newSnap := release.ObservedToSnapshot(release.ObserveRelease(rls)) + newSnap := release.ObservedToSnapshot(releaseToObservation(rls, snap)) newSnap.SetTestHooks(snap.GetTestHooks()) obj.Status.History[i] = newSnap return diff --git a/internal/reconcile/rollback_remediation_test.go b/internal/reconcile/rollback_remediation_test.go index 2300aefc3..71d0bb134 100644 --- a/internal/reconcile/rollback_remediation_test.go +++ b/internal/reconcile/rollback_remediation_test.go @@ -613,4 +613,47 @@ func Test_observeRollback(t *testing.T) { expect, })) }) + + t.Run("rollback with update to previous deployed with OCI Digest", func(t *testing.T) { + g := NewWithT(t) + + previous := &v2.Snapshot{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 2, + Status: helmrelease.StatusFailed.String(), + OCIDigest: "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6", + } + latest := &v2.Snapshot{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 3, + Status: helmrelease.StatusDeployed.String(), + OCIDigest: "sha256:aedc2b0de1576a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6", + } + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + latest, + previous, + }, + }, + } + rls := helmrelease.Mock(&helmrelease.MockReleaseOptions{ + Name: previous.Name, + Namespace: previous.Namespace, + Version: previous.Version, + Status: helmrelease.StatusSuperseded, + }) + obs := release.ObserveRelease(rls) + obs.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + expect := release.ObservedToSnapshot(obs) + + observeRollback(obj)(rls) + g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{ + latest, + expect, + })) + }) } diff --git a/internal/reconcile/test.go b/internal/reconcile/test.go index e71f1ddcf..56e9882ef 100644 --- a/internal/reconcile/test.go +++ b/internal/reconcile/test.go @@ -200,7 +200,8 @@ func observeTest(obj *v2.HelmRelease) storage.ObserveFunc { } // Update the latest snapshot with the test result. - tested := release.ObservedToSnapshot(release.ObserveRelease(rls)) + latest := obj.Status.History.Latest() + tested := release.ObservedToSnapshot(releaseToObservation(rls, latest)) tested.SetTestHooks(release.TestHooksFromRelease(rls)) obj.Status.History[0] = tested } diff --git a/internal/reconcile/test_test.go b/internal/reconcile/test_test.go index d97dbe0c9..8ed0bdd8c 100644 --- a/internal/reconcile/test_test.go +++ b/internal/reconcile/test_test.go @@ -376,6 +376,38 @@ func Test_observeTest(t *testing.T) { })) }) + t.Run("test with current OCI Digest", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + &v2.Snapshot{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + OCIDigest: "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6", + }, + }, + }, + } + rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + }, testutil.ReleaseWithHooks(testHookFixtures)) + + obs := release.ObserveRelease(rls) + obs.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + expect := release.ObservedToSnapshot(obs) + expect.SetTestHooks(release.TestHooksFromRelease(rls)) + + observeTest(obj)(rls) + g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{ + expect, + })) + }) + t.Run("test targeting different version than latest", func(t *testing.T) { g := NewWithT(t) diff --git a/internal/reconcile/uninstall.go b/internal/reconcile/uninstall.go index 3c5f8a769..f92843499 100644 --- a/internal/reconcile/uninstall.go +++ b/internal/reconcile/uninstall.go @@ -224,7 +224,7 @@ func observeUninstall(obj *v2.HelmRelease) storage.ObserveFunc { for i := range obj.Status.History { snap := obj.Status.History[i] if snap.Targets(rls.Name, rls.Namespace, rls.Version) { - newSnap := release.ObservedToSnapshot(release.ObserveRelease(rls)) + newSnap := release.ObservedToSnapshot(releaseToObservation(rls, snap)) newSnap.SetTestHooks(snap.GetTestHooks()) obj.Status.History[i] = newSnap return diff --git a/internal/reconcile/uninstall_test.go b/internal/reconcile/uninstall_test.go index ec0a9e23a..65b118ccc 100644 --- a/internal/reconcile/uninstall_test.go +++ b/internal/reconcile/uninstall_test.go @@ -702,4 +702,36 @@ func Test_observeUninstall(t *testing.T) { current, })) }) + t.Run("uninstall of current with OCI Digest", func(t *testing.T) { + g := NewWithT(t) + + current := &v2.Snapshot{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + Status: helmrelease.StatusDeployed.String(), + OCIDigest: "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6", + } + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + current, + }, + }, + } + rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: current.Name, + Namespace: current.Namespace, + Version: current.Version, + Status: helmrelease.StatusUninstalled, + }) + obs := release.ObserveRelease(rls) + obs.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + expect := release.ObservedToSnapshot(obs) + + observeUninstall(obj)(rls) + g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{ + expect, + })) + }) } diff --git a/internal/reconcile/unlock.go b/internal/reconcile/unlock.go index e08bd558c..e6e4da5fa 100644 --- a/internal/reconcile/unlock.go +++ b/internal/reconcile/unlock.go @@ -77,7 +77,7 @@ func (r *Unlock) Reconcile(_ context.Context, req *Request) error { } // Ensure the release is in a pending state. - cur := release.ObservedToSnapshot(release.ObserveRelease(rls)) + cur := processCurrentSnaphot(req.Object, rls) if status := rls.Info.Status; status.IsPending() { // Update pending status to failed and persist. rls.SetStatus(helmrelease.StatusFailed, fmt.Sprintf("Release unlocked from stale '%s' state", status.String())) @@ -154,9 +154,23 @@ func observeUnlock(obj *v2.HelmRelease) storage.ObserveFunc { for i := range obj.Status.History { snap := obj.Status.History[i] if snap.Targets(rls.Name, rls.Namespace, rls.Version) { - obj.Status.History[i] = release.ObservedToSnapshot(release.ObserveRelease(rls)) + obj.Status.History[i] = release.ObservedToSnapshot(releaseToObservation(rls, snap)) return } } } } + +// processCurrentSnaphot processes the current snapshot based on a Helm release. +// It also looks for the OCIDigest in the corresponding v2.HelmRelease history and +// updates the current snapshot with the OCIDigest if found. +func processCurrentSnaphot(obj *v2.HelmRelease, rls *helmrelease.Release) *v2.Snapshot { + cur := release.ObservedToSnapshot(release.ObserveRelease(rls)) + for i := range obj.Status.History { + snap := obj.Status.History[i] + if snap.Targets(rls.Name, rls.Namespace, rls.Version) { + cur.OCIDigest = snap.OCIDigest + } + } + return cur +} diff --git a/internal/reconcile/unlock_test.go b/internal/reconcile/unlock_test.go index 6799fe198..7b13c8300 100644 --- a/internal/reconcile/unlock_test.go +++ b/internal/reconcile/unlock_test.go @@ -457,6 +457,95 @@ func TestUnlock_success(t *testing.T) { })) } +func TestUnlock_withOCIDigest(t *testing.T) { + g := NewWithT(t) + + namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace) + g.Expect(err).NotTo(HaveOccurred()) + t.Cleanup(func() { + _ = testEnv.Delete(context.TODO(), namedNS) + }) + releaseNamespace := namedNS.Name + + rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: releaseNamespace, + Chart: testutil.BuildChart(), + Version: 4, + Status: helmrelease.StatusPendingInstall, + }) + + obs := release.ObserveRelease(rls) + obs.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + snap := release.ObservedToSnapshot(obs) + + obj := &v2.HelmRelease{ + Spec: v2.HelmReleaseSpec{ + ReleaseName: mockReleaseName, + TargetNamespace: releaseNamespace, + StorageNamespace: releaseNamespace, + Timeout: &metav1.Duration{Duration: 100 * time.Millisecond}, + }, + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + snap, + }, + }, + } + + getter, err := RESTClientGetterFromManager(testEnv.Manager, obj.GetReleaseNamespace()) + g.Expect(err).ToNot(HaveOccurred()) + + cfg, err := action.NewConfigFactory(getter, + action.WithStorage(action.DefaultStorageDriver, obj.GetStorageNamespace()), + ) + g.Expect(err).ToNot(HaveOccurred()) + + store := helmstorage.Init(cfg.Driver) + g.Expect(store.Create(rls)).To(Succeed()) + + recorder := testutil.NewFakeRecorder(10, false) + got := NewUnlock(cfg, recorder).Reconcile(context.TODO(), &Request{ + Object: obj, + }) + + g.Expect(got).ToNot(HaveOccurred()) + + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions( + []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, "PendingRelease", "Unlocked Helm release"), + *conditions.FalseCondition(v2.ReleasedCondition, "PendingRelease", "Unlocked Helm release"), + })) + + releases, _ := store.History(mockReleaseName) + helmreleaseutil.SortByRevision(releases) + expected := release.ObserveRelease(releases[0]) + expected.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{ + release.ObservedToSnapshot(expected), + })) + + expectMsg := fmt.Sprintf(fmtUnlockSuccess, + fmt.Sprintf("%s/%s.v%d", rls.Namespace, snap.Name, snap.Version), + fmt.Sprintf("%s@%s", rls.Chart.Name(), rls.Chart.Metadata.Version), + rls.Info.Status.String()) + + g.Expect(recorder.GetEvents()).To(ConsistOf([]corev1.Event{ + { + Type: corev1.EventTypeNormal, + Reason: "PendingRelease", + Message: expectMsg, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + eventMetaGroupKey(metaOCIDigestKey): expected.OCIDigest, + eventMetaGroupKey(eventv1.MetaRevisionKey): rls.Chart.Metadata.Version, + eventMetaGroupKey(eventv1.MetaTokenKey): chartutil.DigestValues(digest.Canonical, rls.Config).String(), + }, + }, + }, + })) +} + func Test_observeUnlock(t *testing.T) { t.Run("unlock", func(t *testing.T) { g := NewWithT(t) @@ -487,6 +576,38 @@ func Test_observeUnlock(t *testing.T) { })) }) + t.Run("unlock with OCI Digest", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + History: v2.Snapshots{ + { + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + Status: helmrelease.StatusPendingRollback.String(), + OCIDigest: "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6", + }, + }, + }, + } + rls := helmrelease.Mock(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + Status: helmrelease.StatusFailed, + }) + obs := release.ObserveRelease(rls) + obs.OCIDigest = "sha256:fcdc2b0de1581a3633ada4afee3f918f6eaa5b5ab38c3fef03d5b48d3f85d9f6" + expect := release.ObservedToSnapshot(obs) + observeUnlock(obj)(rls) + + g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{ + expect, + })) + }) + t.Run("unlock without current", func(t *testing.T) { g := NewWithT(t) From a98d9574d62fdd3c0ceb56934934eeb37cbaf7c6 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 15 Apr 2024 17:15:42 +0200 Subject: [PATCH 12/12] fix requestForOCIRepository change Use artifact digest instead of revision to validate whether to trigger a new reconciliation Signed-off-by: Soule BA --- internal/controller/helmrelease_controller.go | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 634557725..ba7c0fe85 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -800,11 +800,19 @@ func (r *HelmReleaseReconciler) requestsForOCIRrepositoryChange(ctx context.Cont var reqs []reconcile.Request 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.GetLastAttemptedRevision()) { + // 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 OCIRepository change") continue } + + if digest == hr.Status.LastAttemptedRevisionDigest { + continue + } + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])}) } return reqs @@ -855,6 +863,9 @@ func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { switch { case obj.HasChartRef() && !obj.HasChartTemplate(): namespacedName.Namespace = obj.Spec.ChartRef.Namespace + if namespacedName.Namespace == "" { + namespacedName.Namespace = obj.GetNamespace() + } namespacedName.Name = obj.Spec.ChartRef.Name case !obj.HasChartRef() && obj.HasChartTemplate(): namespacedName.Namespace = obj.Spec.Chart.GetNamespace(obj.GetNamespace()) @@ -891,7 +902,7 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st } // algotithm are sha256, sha384, sha512 with the canonical being sha256 // So every digest starts with a sha algorithm and a colon - sha, err := extractDigetString(tagD[1]) + sha, err := extractDigestSubString(tagD[1]) if err != nil { return "", err } @@ -903,7 +914,7 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st ociDigest = tagD[1] default: // default to the digest - sha, err := extractDigetString(revision) + sha, err := extractDigestSubString(revision) if err != nil { return "", err } @@ -918,8 +929,9 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st return ociDigest, nil } -func extractDigetString(revision string) (string, error) { +func extractDigestSubString(revision string) (string, error) { var sha string + // expects a revision in the : format if pair := strings.Split(revision, ":"); len(pair) != 2 { return "", fmt.Errorf("invalid artifact revision %s", revision) } else { @@ -930,3 +942,17 @@ func extractDigetString(revision string) (string, error) { } return sha[0:12], nil } + +func extractDigest(revision string) string { + if strings.Contains(revision, "@") { + // expects a revision in the @: format + tagD := strings.Split(revision, "@") + if len(tagD) != 2 { + return "" + } + return tagD[1] + } else { + // revision in the : format + return revision + } +}