From 87fd31736e2bbd6c92711f9e6bc2169b70db2c20 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Sat, 9 Mar 2024 22:14:56 +0100 Subject: [PATCH] Adding HelmChart as well as OCIRepository as a ref source This commit add 2 possible sources references: OCIRepository and existing HelmCharts. It takes into account switching from a chart template to a referenced source (garbage collection). Signed-off-by: Soule BA --- api/v2beta2/helmrelease_types.go | 5 +- .../helm.toolkit.fluxcd.io_helmreleases.yaml | 4 +- docs/api/v2beta2/helm.md | 116 ++++++++++-- internal/controller/helmrelease_controller.go | 166 +++++++++++++++--- .../controller/helmrelease_controller_test.go | 16 +- internal/reconcile/helmchart_template.go | 32 ++++ 6 files changed, 293 insertions(+), 46 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index 71f4243e5..a81b1ddb8 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -74,14 +74,15 @@ 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" +// +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. // +optional Chart HelmChartTemplate `json:"chart"` - // ChartRef holds a reference to a v1beta2.HelmChart resource + // ChartRef holds a reference to a source controller resource containing the + // Helm chart artifact. // +optional ChartRef *CrosssNameSpaceSourceReference `json:"chartRef,omitempty"` diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index 551f0cd3b..f62730f0b 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -2032,8 +2032,8 @@ spec: 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)) + rule: (has(self.chart) && !has(self.chartRef)) || (!has(self.chart) + && has(self.chartRef)) status: default: observedGeneration: -1 diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index 840a1ca2d..3cdf75032 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -78,12 +78,27 @@ HelmChartTemplate +(Optional)

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

+chartRef
+ + +CrosssNameSpaceSourceReference + + + + +(Optional) +

ChartRef holds a reference to a v1beta2.HelmChart resource

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

CrosssNameSpaceSourceReference +

+

+(Appears on: +HelmReleaseSpec) +

+

CrosssNameSpaceSourceReference 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

@@ -534,10 +617,6 @@ handle differences between the manifest in the Helm storage and the resources currently existing in the cluster.

Filter

-

-(Appears on: -Test) -

Filter holds the configuration for individual Helm test filters.

@@ -1009,12 +1088,27 @@ HelmChartTemplate +(Optional)

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

+chartRef
+ + +CrosssNameSpaceSourceReference + + + + +(Optional) +

ChartRef holds a reference to a v1beta2.HelmChart resource

+ + + + interval
@@ -2267,9 +2361,7 @@ Kubernetes meta/v1.Time testHooks
- -TestHookStatus - +map[string]*github.com/fluxcd/helm-controller/api/v2beta2.TestHookStatus @@ -2283,7 +2375,7 @@ run by the controller.

Snapshots -([]*./api/v2beta2.Snapshot alias)

+([]*github.com/fluxcd/helm-controller/api/v2beta2.Snapshot alias)

(Appears on: HelmReleaseStatus) @@ -2352,8 +2444,8 @@ actions in ‘Install.IgnoreTestFailures’ and ‘Upgrade.IgnoreTes filters
- -Filter + +[]github.com/fluxcd/helm-controller/api/v2beta2.Filter @@ -2367,10 +2459,6 @@ Filter

TestHookStatus

-

-(Appears on: -Snapshot) -

TestHookStatus holds the status information for a test hook as observed to be run by the controller.

diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 1068ae84d..85f47f9c3 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" @@ -108,15 +109,10 @@ 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 := types.NamespacedName{} - if obj.Spec.ChartRef != nil { - namespacedName.Namespace = obj.Spec.ChartRef.Namespace - namespacedName.Name = obj.Spec.ChartRef.Name - } else { - namespacedName.Namespace = obj.Spec.Chart.GetNamespace(obj.GetNamespace()) - namespacedName.Name = obj.GetHelmChartName() + namespacedName, err := getNamespacedName(obj) + if err != nil { + return nil } - return []string{ namespacedName.String(), } @@ -158,6 +154,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 HelmChart reference")) + } + // Initialize the patch helper with the current version of the object. patchHelper := patch.NewSerialPatcher(obj, r.Client) @@ -262,8 +262,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()) @@ -286,9 +286,8 @@ 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) // Do not requeue immediately, when the artifact is created @@ -313,7 +312,7 @@ 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()) @@ -668,11 +667,21 @@ 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 isChartRefPresent(obj) { + if obj.Spec.ChartRef.Kind == sourcev1.OCIRepositoryKind { + return r.getHelmChartFromOCIRef(ctx, obj) + } + name, namespace = obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace + } else { + namespace, name = obj.Status.GetHelmChart() + } + chartRef := types.NamespacedName{Namespace: namespace, Name: name} if err := intacl.AllowsAccessTo(obj, sourcev1.HelmChartKind, chartRef); err != nil { @@ -686,6 +695,21 @@ 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) { + namespace, name := obj.Spec.ChartRef.Name, obj.Spec.ChartRef.Namespace + 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. @@ -736,6 +760,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) { @@ -752,12 +821,65 @@ 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 isChartRefPresent(obj *v2.HelmRelease) bool { + return obj.Spec.ChartRef != nil +} + +func isChartTemplatePresent(obj *v2.HelmRelease) bool { + return obj.Spec.Chart.Spec.Chart != "" +} + +func isValidChartRef(obj *v2.HelmRelease) bool { + return (isChartRefPresent(obj) && !isChartTemplatePresent(obj)) || + (!isChartRefPresent(obj) && isChartTemplatePresent(obj)) +} + +func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { + namespacedName := types.NamespacedName{} + switch { + case isChartRefPresent(obj) && !isChartTemplatePresent(obj): + namespacedName.Namespace = obj.Spec.ChartRef.Namespace + namespacedName.Name = obj.Spec.ChartRef.Name + case !isChartRefPresent(obj) && isChartTemplatePresent(obj): + 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..0c8138d79 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -2034,14 +2034,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 +2332,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 +2367,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 +2378,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 +2389,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 +2399,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 { diff --git a/internal/reconcile/helmchart_template.go b/internal/reconcile/helmchart_template.go index 5bf5a9630..c7d9d77b6 100644 --- a/internal/reconcile/helmchart_template.go +++ b/internal/reconcile/helmchart_template.go @@ -76,6 +76,24 @@ func NewHelmChartTemplate(client client.Client, recorder record.EventRecorder, f } } +func isChartRefPresent(obj *v2.HelmRelease) bool { + return obj.Spec.ChartRef != nil +} + +func isChartTemplatePresent(obj *v2.HelmRelease) bool { + return obj.Spec.Chart.Spec.Chart != "" +} + +func mustCleanDeployedChart(obj *v2.HelmRelease) bool { + if isChartRefPresent(obj) && !isChartTemplatePresent(obj) { + if obj.Status.HelmChart != "" { + return true + } + } + + return false +} + func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error { var ( obj = req.Object @@ -95,6 +113,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 isChartRefPresent(obj) { + // 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