Fix/helm controller does not update the release (#568)

* feat(plugin) Helm controller check difference beetween hooks (#459)

* fix(plugin) Test case with hooks (#459)

* fix(plugin) Extended test case (#459)

* fix(plugin) Change test to set a value by name (#459)

* fix(plugin) Refactor tests (#459)

* Automatic application of license header

* fix(plugin) Move helper from e2e test to test resource (#459)

* feat(plugin) Change comments in resources for test (#459)

---------

Co-authored-by: License Bot <[email protected]>

pkg/helm/diff.go

@@ -6,6 +6,7 @@ package helm
 import (
 	"encoding/json"
 	"fmt"
+	"maps"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -58,17 +59,33 @@ func (d DiffObjectList) String() string {
 }
 
 // diffAgainstRelease returns the diff between the templated manifest and the manifest of the deployed Helm release.
-func diffAgainstRelease(restClientGetter genericclioptions.RESTClientGetter, namespace, manifest string, helmRelease *release.Release) (DiffObjectList, error) {
+func diffAgainstRelease(restClientGetter genericclioptions.RESTClientGetter, namespace string, helmTemplateRelease, helmRelease *release.Release) (DiffObjectList, error) {
 	remoteObjs, err := ObjectMapFromRelease(restClientGetter, helmRelease, nil)
 	if err != nil {
 		return nil, err
 	}
 
-	localObjs, err := ObjectMapFromManifest(restClientGetter, namespace, manifest, nil)
+	for _, hook := range helmRelease.Hooks {
+		hookObjs, err := ObjectMapFromManifest(restClientGetter, helmRelease.Namespace, hook.Manifest, nil)
+		if err != nil {
+			return nil, err
+		}
+		maps.Copy(remoteObjs, hookObjs)
+	}
+
+	localObjs, err := ObjectMapFromManifest(restClientGetter, namespace, helmTemplateRelease.Manifest, nil)
 	if err != nil {
 		return nil, err
 	}
 
+	for _, hook := range helmTemplateRelease.Hooks {
+		hookObjs, err := ObjectMapFromManifest(restClientGetter, namespace, hook.Manifest, nil)
+		if err != nil {
+			return nil, err
+		}
+		maps.Copy(localObjs, hookObjs)
+	}
+
 	// create the set of all keys in local and remote objects
 	keys := make(map[ObjectKey]struct{})
 	for k := range remoteObjs {

pkg/helm/diff_test.go

@@ -79,20 +79,20 @@ var _ = Describe("ensure helm diff against the release manifest works as expecte
 
 	It("should no diff or drift if nothing changes", func() {
 		By("templating the Helm Chart from the Plugin")
-		manifestUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
+		templateUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error templating the helm chart")
 
 		By("retrieving the Release for the Plugin")
 		releaseUT, err := helm.GetReleaseForHelmChartFromPlugin(test.Ctx, test.RestClientGetter, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error getting the release for the helm chart")
 
 		By("diffing the manifest against the helm release")
-		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, manifestUT, releaseUT)
+		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, templateUT, releaseUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(BeEmpty(), "the diff should be empty")
 
 		By("diffing the manifest against the live objects")
-		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, manifestUT)
+		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, templateUT.Manifest)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(BeEmpty(), "the diff should be empty")
 	})
@@ -107,20 +107,20 @@ var _ = Describe("ensure helm diff against the release manifest works as expecte
 		}
 
 		By("templating the Helm Chart from the Plugin")
-		manifestUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
+		templateUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error templating the helm chart")
 
 		By("retrieving the Release for the Plugin")
 		releaseUT, err := helm.GetReleaseForHelmChartFromPlugin(test.Ctx, test.RestClientGetter, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error getting the release for the helm chart")
 
 		By("diffing the manifest against the helm release")
-		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, manifestUT, releaseUT)
+		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, templateUT, releaseUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(ContainSubstring("3.19"), "the diff should not be empty")
 
 		By("diffing the manifest against the live objects")
-		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, manifestUT)
+		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, templateUT.Manifest)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(ContainSubstring("3.19"), "the diff should not be empty")
 	})
@@ -159,20 +159,20 @@ var _ = Describe("ensure helm diff against the release manifest works as expecte
 		Expect(test.K8sClient.Update(test.Ctx, podUT)).To(Succeed(), "the pod should be updated")
 
 		By("templating the Helm Chart from the Plugin")
-		manifestUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
+		templateUT, err := helm.TemplateHelmChartFromPlugin(test.Ctx, test.K8sClient, test.RestClientGetter, pluginDefinitionUT, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error templating the helm chart")
 
 		By("retrieving the Release for the Plugin")
 		releaseUT, err := helm.GetReleaseForHelmChartFromPlugin(test.Ctx, test.RestClientGetter, pluginUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error getting the release for the helm chart")
 
 		By("diffing the manifest against the helm release")
-		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, manifestUT, releaseUT)
+		diff, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, templateUT, releaseUT)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(BeEmpty(), "the diff should be empty")
 
 		By("diffing the manifest against the live objects")
-		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, manifestUT)
+		diff, err = helm.ExportDiffAgainstLiveObjects(test.RestClientGetter, namespace, templateUT.Manifest)
 		Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the manifest against the helm release")
 		Expect(diff).To(ContainSubstring("3.17"), "the diff should not be empty")
 	})
@@ -414,7 +414,7 @@ var _ = Describe("Ensure helm diff does not leak secrets", Ordered, func() {
 
 			helmRelease := &release.Release{Manifest: manifest}
 
-			diffs, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, manifest, helmRelease)
+			diffs, err := helm.ExportDiffAgainstRelease(test.RestClientGetter, namespace, helmRelease, helmRelease)
 			Expect(err).NotTo(HaveOccurred(), "there should be no error diffing the helm release")
 			Expect(diffs).To(BeEmpty(), "the diff should be empty")
 			Expect(diffs).NotTo(ContainSubstring("dGVzdC12YWx1ZQ=="), "the diff should not contain the original value for test")

pkg/helm/helm.go

@@ -164,12 +164,12 @@ func DiffChartToDeployedResources(ctx context.Context, local client.Client, rest
 		return nil, true, nil
 	}
 
-	manifest, err := TemplateHelmChartFromPlugin(ctx, local, restClientGetter, pluginDefinition, plugin)
+	helmTemplateRelease, err := TemplateHelmChartFromPlugin(ctx, local, restClientGetter, pluginDefinition, plugin)
 	if err != nil {
 		return nil, false, err
 	}
 
-	diffObjects, err := diffAgainstRelease(restClientGetter, plugin.Spec.ReleaseNamespace, manifest, helmRelease)
+	diffObjects, err := diffAgainstRelease(restClientGetter, plugin.Spec.ReleaseNamespace, helmTemplateRelease, helmRelease)
 	if err != nil {
 		return nil, false, err
 	}
@@ -194,7 +194,7 @@ func DiffChartToDeployedResources(ctx context.Context, local client.Client, rest
 		return nil, false, nil
 	}
 
-	diffObjects, err = diffAgainstLiveObjects(restClientGetter, plugin.Spec.ReleaseNamespace, manifest)
+	diffObjects, err = diffAgainstLiveObjects(restClientGetter, plugin.Spec.ReleaseNamespace, helmTemplateRelease.Manifest)
 	if err != nil {
 		return nil, false, err
 	}
@@ -277,12 +277,12 @@ func GetReleaseForHelmChartFromPlugin(_ context.Context, restClientGetter generi
 }
 
 // TemplateHelmChartFromPlugin returns the rendered manifest or an error.
-func TemplateHelmChartFromPlugin(ctx context.Context, local client.Client, restClientGetter genericclioptions.RESTClientGetter, pluginDefinition *greenhousev1alpha1.PluginDefinition, plugin *greenhousev1alpha1.Plugin) (string, error) {
+func TemplateHelmChartFromPlugin(ctx context.Context, local client.Client, restClientGetter genericclioptions.RESTClientGetter, pluginDefinition *greenhousev1alpha1.PluginDefinition, plugin *greenhousev1alpha1.Plugin) (*release.Release, error) {
 	helmRelease, err := installRelease(ctx, local, restClientGetter, pluginDefinition, plugin, true)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
-	return helmRelease.Manifest, nil
+	return helmRelease, nil
 }
 
 type ChartLoaderFunc func(name string) (*chart.Chart, error)

pkg/test/fixtures/testHook/Chart.yaml

@@ -0,0 +1,27 @@
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors
+# SPDX-License-Identifier: Apache-2.0
+
+apiVersion: v2
+name: test-hook
+description: Test for hook plugin deployment
+
+# A chart can be either an 'application' or a 'library' chart.
+#
+# Application charts are a collection of templates that can be packaged into versioned archives
+# to be deployed.
+#
+# Library charts provide useful utilities or functions for the chart developer. They're included as
+# a dependency of application charts to inject those utilities and functions into the rendering
+# pipeline. Library charts do not define any templates and therefore cannot be deployed.
+type: application
+
+# This is the chart version. This version number should be incremented each time you make changes
+# to the chart and its templates, including the app version.
+# Versions are expected to follow Semantic Versioning (https://semver.org/)
+version: 0.1.0
+
+# This is the version number of the application being deployed. This version number should be
+# incremented each time you make changes to the application. Versions are not expected to
+# follow Semantic Versioning. They should reflect the version the application is using.
+# It is recommended to use it with quotes.
+appVersion: "0.1.0"

pkg/test/fixtures/testHook/templates/hook.yaml

@@ -0,0 +1,20 @@
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors
+# SPDX-License-Identifier: Apache-2.0
+
+{{ if .Values.hook_enabled }}
+apiVersion: batch/v1
+kind: Job
+metadata:
+  name: hook-job
+  annotations:
+    "helm.sh/hook": post-install, post-upgrade
+    "helm.sh/hook-weight": "10"
+spec:
+  template:
+    spec:
+      containers:
+        - name: pre-update-container
+          image: alpine:{{.Values.imageTag}}
+          command: ['sh', '-c', 'sleep 5']
+      restartPolicy: Never
+{{ end }}

pkg/test/fixtures/testHook/templates/pod.yaml

@@ -0,0 +1,17 @@
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors
+# SPDX-License-Identifier: Apache-2.0
+
+apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    run: alpine
+  name: alpine
+spec:
+  containers:
+  - image: alpine:{{.Values.imageTag}}
+    name: alpine
+    resources: {}
+  dnsPolicy: ClusterFirst
+  restartPolicy: Always
+status: {}

pkg/test/fixtures/testHook/values.yaml

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Greenhouse contributors
+# SPDX-License-Identifier: Apache-2.0
+
+imageTag: 3.18
+
+hook_enabled: false

pkg/test/resources.go

@@ -160,6 +160,13 @@ func WithHelmChart(chart *greenhousev1alpha1.HelmChartReference) func(*greenhous
 	}
 }
 
+// AppendPluginOption sets the plugin option in plugin definition
+func AppendPluginOption(option greenhousev1alpha1.PluginOption) func(*greenhousev1alpha1.PluginDefinition) {
+	return func(pd *greenhousev1alpha1.PluginDefinition) {
+		pd.Spec.Options = append(pd.Spec.Options, option)
+	}
+}
+
 // CreatePluginDefinition creates and returns a PluginDefinition object. Opts can be used to set the desired state of the PluginDefinition.
 func (t *TestSetup) CreatePluginDefinition(ctx context.Context, name string, opts ...func(*greenhousev1alpha1.PluginDefinition)) *greenhousev1alpha1.PluginDefinition {
 	GinkgoHelper()
@@ -234,6 +241,22 @@ func WithPluginOptionValue(name string, value *apiextensionsv1.JSON, valueFrom *
 	}
 }
 
+// SetOptionValueForPlugin sets the value of a PluginOptionValue in plugin
+func SetOptionValueForPlugin(plugin *greenhousev1alpha1.Plugin, key, value string) {
+	for i, keyValue := range plugin.Spec.OptionValues {
+		if keyValue.Name == key {
+			plugin.Spec.OptionValues[i].Value.Raw = []byte(value)
+			return
+		}
+	}
+
+	plugin.Spec.OptionValues = append(plugin.Spec.OptionValues, greenhousev1alpha1.PluginOptionValue{
+		Name:  key,
+		Value: &apiextensionsv1.JSON{Raw: []byte(value)},
+	})
+}
+
+// NewPlugin creates new plugin
 func (t *TestSetup) NewPlugin(ctx context.Context, name string, opts ...func(*greenhousev1alpha1.Plugin)) *greenhousev1alpha1.Plugin {
 	GinkgoHelper()
 	plugin := &greenhousev1alpha1.Plugin{