From d023277d6bb83d8819c6d4f1ca42baa93c0c63bb Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 28 Jul 2023 17:04:04 +0200 Subject: [PATCH] controller: start w/ adding tests for HelmRelease This adds base coverage for some of the simpler methods which do not require extensive mocking. Signed-off-by: Hidde Beydals --- internal/controller/helmrelease_controller.go | 102 ++--- .../controller/helmrelease_controller_test.go | 421 +++++++++++++++++- main.go | 19 +- 3 files changed, 469 insertions(+), 73 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index cf4250fcc..ad095539e 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -20,12 +20,10 @@ import ( "context" "errors" "fmt" - intacl "github.com/fluxcd/helm-controller/internal/acl" "time" "github.com/hashicorp/go-retryablehttp" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" apierrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -53,6 +51,7 @@ import ( sourcev1 "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" "github.com/fluxcd/helm-controller/internal/chartutil" "github.com/fluxcd/helm-controller/internal/digest" @@ -67,11 +66,9 @@ type HelmReleaseReconciler struct { kuberecorder.EventRecorder helper.Metrics - Config *rest.Config - Scheme *runtime.Scheme - - ClientOpts runtimeClient.Options - KubeConfigOpts runtimeClient.KubeConfigOptions + GetClusterConfig func() (*rest.Config, error) + ClientOpts runtimeClient.Options + KubeConfigOpts runtimeClient.KubeConfigOptions PollingOpts polling.Options StatusPoller *polling.StatusPoller @@ -209,7 +206,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe if c := len(obj.Spec.DependsOn); c > 0 { log.Info(fmt.Sprintf("checking %d dependencies", c)) - if err := r.checkDependencies(obj); err != nil { + if err := r.checkDependencies(ctx, obj); err != nil { msg := fmt.Sprintf("dependencies do not meet ready condition (%s): retrying in %s", err.Error(), r.requeueDependency.String()) r.Eventf(obj, obj.Status.Current.ChartVersion, corev1.EventTypeWarning, err.Error()) @@ -317,46 +314,40 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // reconcileDelete deletes the v1beta2.HelmChart of the v2beta2.HelmRelease, // and uninstalls the Helm release if the resource has not been suspended. func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, obj *v2.HelmRelease) (ctrl.Result, error) { - if obj.Status.Current != nil { - // Only uninstall the Helm Release if the resource is not suspended. - if !obj.Spec.Suspend { - // Build client getter. - getter, err := r.buildRESTClientGetter(ctx, obj) - if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "GetterError", err.Error()) - return ctrl.Result{}, err - } - - // Attempt to uninstall the release. - err = r.reconcileUninstall(ctx, getter, obj) - if err != nil && !errors.Is(err, intreconcile.ErrNoCurrent) { - return ctrl.Result{}, err - } - if err == nil { - ctrl.LoggerFrom(ctx).Info("uninstalled Helm release for deleted resource") - } + // Only uninstall the Helm Release if the resource is not suspended. + if !obj.Spec.Suspend { + // Build client getter. + getter, err := r.buildRESTClientGetter(ctx, obj) + if err != nil { + conditions.MarkFalse(obj, meta.ReadyCondition, "GetterError", err.Error()) + return ctrl.Result{}, err + } - // Truncate the current release details in the status. - obj.Status.Current = nil - obj.Status.StorageNamespace = "" - } else { - ctrl.LoggerFrom(ctx).Info("skipping Helm uninstall for suspended resource") + // Attempt to uninstall the release. + if err = r.reconcileUninstall(ctx, getter, obj); err != nil && !errors.Is(err, intreconcile.ErrNoCurrent) { + return ctrl.Result{}, err + } + if err == nil { + ctrl.LoggerFrom(ctx).Info("uninstalled Helm release for deleted resource") } - } - // Delete the HelmChart resource. - if err := r.reconcileChartTemplate(ctx, obj); err != nil { - return ctrl.Result{}, err + // Truncate the current release details in the status. + obj.Status.Current = nil + obj.Status.StorageNamespace = "" + + // Delete the HelmChart resource. + if err := r.reconcileChartTemplate(ctx, obj); err != nil { + return ctrl.Result{}, err + } + } else { + ctrl.LoggerFrom(ctx).Info("skipping Helm uninstall and chart removal for suspended resource") } - if !obj.DeletionTimestamp.IsZero() { - // Remove our finalizer from the list. - controllerutil.RemoveFinalizer(obj, v2.HelmReleaseFinalizer) + // Remove our finalizer from the list. + controllerutil.RemoveFinalizer(obj, v2.HelmReleaseFinalizer) - // Stop reconciliation as the object is being deleted. - return ctrl.Result{}, nil - } - return ctrl.Result{Requeue: true}, nil + // Stop reconciliation as the object is being deleted. + return ctrl.Result{}, nil } // reconcileChartTemplate reconciles the HelmChart template from the HelmRelease. @@ -384,23 +375,23 @@ func (r *HelmReleaseReconciler) reconcileUninstall(ctx context.Context, getter g // are Ready. // It returns an error if a dependency can not be retrieved or is not Ready, // otherwise nil. -func (r *HelmReleaseReconciler) checkDependencies(obj *v2.HelmRelease) error { +func (r *HelmReleaseReconciler) checkDependencies(ctx context.Context, obj *v2.HelmRelease) error { for _, d := range obj.Spec.DependsOn { - if d.Namespace == "" { - d.Namespace = obj.GetNamespace() - } - dName := types.NamespacedName{ + ref := types.NamespacedName{ Namespace: d.Namespace, Name: d.Name, } + if ref.Namespace == "" { + ref.Namespace = obj.GetNamespace() + } + dHr := &v2.HelmRelease{} - err := r.Get(context.Background(), dName, dHr) - if err != nil { - return fmt.Errorf("unable to get '%s' dependency: %w", dName, err) + if err := r.Get(ctx, ref, dHr); err != nil { + return fmt.Errorf("unable to get '%s' dependency: %w", ref, err) } - if dHr.Generation != dHr.Status.ObservedGeneration || len(dHr.Status.Conditions) == 0 || !conditions.IsTrue(dHr, meta.ReadyCondition) { - return fmt.Errorf("dependency '%s' is not ready", dName) + if dHr.Generation != dHr.Status.ObservedGeneration || !conditions.IsTrue(dHr, meta.ReadyCondition) { + return fmt.Errorf("dependency '%s' is not ready", ref) } } return nil @@ -431,7 +422,12 @@ func (r *HelmReleaseReconciler) buildRESTClientGetter(ctx context.Context, obj * } return kube.NewMemoryRESTClientGetter(kubeConfig, opts...), nil } - return kube.NewInClusterMemoryRESTClientGetter(opts...) + + cfg, err := r.GetClusterConfig() + if err != nil { + return nil, fmt.Errorf("could not get in-cluster REST config: %w", err) + } + return kube.NewMemoryRESTClientGetter(cfg, opts...), nil } // getHelmChart retrieves the v1beta2.HelmChart for the given v2beta2.HelmRelease diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 05178e9cd..6689467b6 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -18,32 +18,433 @@ package controller import ( "context" - "github.com/fluxcd/helm-controller/internal/acl" + "errors" + "k8s.io/client-go/rest" "strings" "testing" "time" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" 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/runtime" + "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/tools/clientcmd" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/yaml" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" v2 "github.com/fluxcd/helm-controller/api/v2beta2" + "github.com/fluxcd/helm-controller/internal/acl" + "github.com/fluxcd/helm-controller/internal/kube" + "github.com/fluxcd/helm-controller/internal/reconcile" ) +func TestHelmReleaseReconciler_reconcileChartTemplate(t *testing.T) { + t.Run("attempts to reconcile chart template", func(t *testing.T) { + g := NewWithT(t) + + r := &HelmReleaseReconciler{ + Client: fake.NewClientBuilder().WithScheme(NewTestScheme()).Build(), + EventRecorder: record.NewFakeRecorder(32), + } + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + StorageNamespace: "default", + }, + } + + // We do not care about the result of the reconcile, only that it was attempted. + err := r.reconcileChartTemplate(context.TODO(), obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to run server-side apply")) + }) +} + +func TestHelmReleaseReconciler_reconcileUninstall(t *testing.T) { + t.Run("attempts to uninstall release", func(t *testing.T) { + g := NewWithT(t) + + getter := kube.NewMemoryRESTClientGetter(testEnv.GetConfig()) + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + StorageNamespace: "default", + }, + } + + // We do not care about the result of the uninstall, only that it was attempted. + err := (&HelmReleaseReconciler{}).reconcileUninstall(context.TODO(), getter, obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, reconcile.ErrNoCurrent)).To(BeTrue()) + }) + + t.Run("error on empty storage namespace", func(t *testing.T) { + g := NewWithT(t) + + obj := &v2.HelmRelease{ + Status: v2.HelmReleaseStatus{ + StorageNamespace: "", + }, + } + + err := (&HelmReleaseReconciler{}).reconcileUninstall(context.TODO(), nil, obj) + g.Expect(err).To(HaveOccurred()) + + g.Expect(conditions.IsFalse(obj, meta.ReadyCondition)).To(BeTrue()) + g.Expect(conditions.GetReason(obj, meta.ReadyCondition)).To(Equal("ConfigFactoryErr")) + g.Expect(conditions.GetMessage(obj, meta.ReadyCondition)).To(ContainSubstring("no namespace provided")) + g.Expect(obj.GetConditions()).To(HaveLen(1)) + }) +} + +func TestHelmReleaseReconciler_checkDependencies(t *testing.T) { + tests := []struct { + name string + obj *v2.HelmRelease + objects []client.Object + expect func(g *WithT, err error) + }{ + { + name: "all dependencies ready", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dependant", + Namespace: "some-namespace", + }, + Spec: v2.HelmReleaseSpec{ + DependsOn: []meta.NamespacedObjectReference{ + { + Name: "dependency-1", + }, + { + Name: "dependency-2", + Namespace: "some-other-namespace", + }, + }, + }, + }, + objects: []client.Object{ + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Name: "dependency-1", + Namespace: "some-namespace", + }, + Status: v2.HelmReleaseStatus{ + ObservedGeneration: 1, + Conditions: []metav1.Condition{ + {Type: meta.ReadyCondition, Status: metav1.ConditionTrue}, + }, + }, + }, + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + Name: "dependency-2", + Namespace: "some-other-namespace", + }, + Status: v2.HelmReleaseStatus{ + ObservedGeneration: 2, + Conditions: []metav1.Condition{ + {Type: meta.ReadyCondition, Status: metav1.ConditionTrue}, + }, + }, + }, + }, + expect: func(g *WithT, err error) { + g.Expect(err).ToNot(HaveOccurred()) + }, + }, + { + name: "error on dependency with ObservedGeneration < Generation", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dependant", + Namespace: "some-namespace", + }, + Spec: v2.HelmReleaseSpec{ + DependsOn: []meta.NamespacedObjectReference{ + { + Name: "dependency-1", + }, + }, + }, + }, + objects: []client.Object{ + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + Name: "dependency-1", + Namespace: "some-namespace", + }, + Status: v2.HelmReleaseStatus{ + ObservedGeneration: 1, + Conditions: []metav1.Condition{ + {Type: meta.ReadyCondition, Status: metav1.ConditionTrue}, + }, + }, + }, + }, + expect: func(g *WithT, err error) { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("is not ready")) + }, + }, + { + name: "error on dependency with ObservedGeneration = Generation and ReadyCondition = False", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dependant", + Namespace: "some-namespace", + }, + Spec: v2.HelmReleaseSpec{ + DependsOn: []meta.NamespacedObjectReference{ + { + Name: "dependency-1", + }, + }, + }, + }, + objects: []client.Object{ + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Name: "dependency-1", + Namespace: "some-namespace", + }, + Status: v2.HelmReleaseStatus{ + ObservedGeneration: 1, + Conditions: []metav1.Condition{ + {Type: meta.ReadyCondition, Status: metav1.ConditionFalse}, + }, + }, + }, + }, + expect: func(g *WithT, err error) { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("is not ready")) + }, + }, + { + name: "error on dependency without conditions", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dependant", + Namespace: "some-namespace", + }, + Spec: v2.HelmReleaseSpec{ + DependsOn: []meta.NamespacedObjectReference{ + { + Name: "dependency-1", + }, + }, + }, + }, + objects: []client.Object{ + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Name: "dependency-1", + Namespace: "some-namespace", + }, + Status: v2.HelmReleaseStatus{ + ObservedGeneration: 1, + }, + }, + }, + expect: func(g *WithT, err error) { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("is not ready")) + }, + }, + { + name: "error on missing dependency", + obj: &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dependant", + Namespace: "some-namespace", + }, + Spec: v2.HelmReleaseSpec{ + DependsOn: []meta.NamespacedObjectReference{ + { + Name: "dependency-1", + }, + }, + }, + }, + expect: func(g *WithT, err error) { + g.Expect(err).To(HaveOccurred()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().WithScheme(NewTestScheme()) + if len(tt.objects) > 0 { + c.WithObjects(tt.objects...) + } + + r := &HelmReleaseReconciler{ + Client: c.Build(), + } + + err := r.checkDependencies(context.TODO(), tt.obj) + tt.expect(g, err) + }) + } +} + +func TestHelmReleaseReconciler_buildRESTClientGetter(t *testing.T) { + const ( + namespace = "some-namespace" + kubeCfg = `apiVersion: v1 +kind: Config +clusters: +- cluster: + insecure-skip-tls-verify: true + server: https://1.2.3.4 + name: development +contexts: +- context: + cluster: development + namespace: frontend + user: developer + name: dev-frontend +current-context: dev-frontend +preferences: {} +users: +- name: developer + user: + password: some-password + username: exp` + ) + + tests := []struct { + name string + env map[string]string + getConfig func() (*rest.Config, error) + spec v2.HelmReleaseSpec + secret *corev1.Secret + want genericclioptions.RESTClientGetter + wantErr string + }{ + { + name: "builds in-cluster RESTClientGetter for HelmRelease", + getConfig: func() (*rest.Config, error) { + return clientcmd.RESTConfigFromKubeConfig([]byte(kubeCfg)) + }, + spec: v2.HelmReleaseSpec{}, + want: &kube.MemoryRESTClientGetter{}, + }, + { + name: "returns error when in-cluster GetClusterConfig fails", + getConfig: func() (*rest.Config, error) { + return nil, errors.New("some-error") + }, + wantErr: "some-error", + }, + { + name: "builds RESTClientGetter from HelmRelease with KubeConfig", + spec: v2.HelmReleaseSpec{ + KubeConfig: &meta.KubeConfigReference{ + SecretRef: meta.SecretKeyReference{ + Name: "kubeconfig", + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kubeconfig", + Namespace: namespace, + }, + Data: map[string][]byte{ + kube.DefaultKubeConfigSecretKey: []byte(kubeCfg), + }, + }, + want: &kube.MemoryRESTClientGetter{}, + }, + { + name: "error on missing KubeConfig secret", + spec: v2.HelmReleaseSpec{ + KubeConfig: &meta.KubeConfigReference{ + SecretRef: meta.SecretKeyReference{ + Name: "kubeconfig", + }, + }, + }, + wantErr: "could not get KubeConfig secret", + }, + { + name: "error on invalid KubeConfig secret", + spec: v2.HelmReleaseSpec{ + KubeConfig: &meta.KubeConfigReference{ + SecretRef: meta.SecretKeyReference{ + Name: "kubeconfig", + Key: "invalid-key", + }, + }, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kubeconfig", + Namespace: namespace, + }, + }, + wantErr: "does not contain a 'invalid-key' key", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + for k, v := range tt.env { + t.Setenv(k, v) + } + + c := fake.NewClientBuilder() + if tt.secret != nil { + c.WithObjects(tt.secret) + } + + r := &HelmReleaseReconciler{ + Client: c.Build(), + GetClusterConfig: tt.getConfig, + } + + getter, err := r.buildRESTClientGetter(context.Background(), &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-name", + Namespace: namespace, + }, + Spec: tt.spec, + }) + if len(tt.wantErr) > 0 { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(getter).To(BeAssignableToTypeOf(tt.want)) + } + }) + } +} + func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { g := NewWithT(t) - scheme := runtime.NewScheme() - g.Expect(v2.AddToScheme(scheme)).To(Succeed()) - g.Expect(sourcev1.AddToScheme(scheme)).To(Succeed()) - chart := &sourcev1.HelmChart{ ObjectMeta: metav1.ObjectMeta{ Namespace: "some-namespace", @@ -109,14 +510,14 @@ func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - builder := fake.NewClientBuilder() - builder.WithScheme(scheme) + c := fake.NewClientBuilder() + c.WithScheme(NewTestScheme()) if tt.chart != nil { - builder.WithObjects(tt.chart) + c.WithObjects(tt.chart) } r := &HelmReleaseReconciler{ - Client: builder.Build(), + Client: c.Build(), EventRecorder: record.NewFakeRecorder(32), } diff --git a/main.go b/main.go index 8d468242d..b16171934 100644 --- a/main.go +++ b/main.go @@ -237,16 +237,15 @@ func main() { statusPoller := polling.NewStatusPoller(mgr.GetClient(), mgr.GetRESTMapper(), pollingOpts) if err = (&controller.HelmReleaseReconciler{ - Client: mgr.GetClient(), - Config: mgr.GetConfig(), - Scheme: mgr.GetScheme(), - EventRecorder: eventRecorder, - Metrics: metricsH, - ClientOpts: clientOptions, - KubeConfigOpts: kubeConfigOpts, - PollingOpts: pollingOpts, - StatusPoller: statusPoller, - FieldManager: controllerName, + Client: mgr.GetClient(), + EventRecorder: eventRecorder, + Metrics: metricsH, + GetClusterConfig: ctrl.GetConfig, + ClientOpts: clientOptions, + KubeConfigOpts: kubeConfigOpts, + PollingOpts: pollingOpts, + StatusPoller: statusPoller, + FieldManager: controllerName, }).SetupWithManager(ctx, mgr, controller.HelmReleaseReconcilerOptions{ DependencyRequeueInterval: requeueDependency, HTTPRetry: httpRetry,