Repository: kinvolk/lokomotive
Pull request #1416 (2021-03-04 12:22:15)

pkg/components/util: retain all hooks from a single file while rendering
With existing implementation, if there is more than one hook defined in
a single file, last hook will overwrite the previous one when rendering,
as we index hooks using Path and 2 hooks will have the same path.

CRDs are not affected, as we index them by name and there should be no
duplicate CRDs in chart defined.

Closes #1345


Patch:
diff --git a/pkg/components/util/helm.go b/pkg/components/util/helm.go
index 00c50dc4..f162abdb 100644
--- a/pkg/components/util/helm.go
+++ b/pkg/components/util/helm.go
@@ -59,7 +59,11 @@ func RenderChart(helmChart *chart.Chart, name, namespace, values string) (map[st
 
 	// Include hooks
 	for _, m := range template.Hooks {
-		ret[m.Path] = m.Manifest
+		if _, ok := ret[m.Path]; !ok {
+			ret[m.Path] = ""
+		}
+
+		ret[m.Path] += "---\n" + m.Manifest + "\n"
 	}
 
 	// CRD are not rendered, so do this manually


Test patch:
diff --git a/pkg/components/util/helm_internal_test.go b/pkg/components/util/helm_internal_test.go
new file mode 100644
index 00000000..637aadd1
--- /dev/null
+++ b/pkg/components/util/helm_internal_test.go
@@ -0,0 +1,202 @@
+// Copyright 2020 The Lokomotive Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package util
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/kinvolk/lokomotive/pkg/components"
+	"github.com/kinvolk/lokomotive/pkg/k8sutil"
+)
+
+func TestChartFromManifests(t *testing.T) {
+	tc := []struct {
+		metadata  components.Metadata
+		manifests map[string]string
+		err       bool
+	}{
+		{
+			components.Metadata{
+				Name: "foo",
+			},
+			map[string]string{
+				"foo.yaml": "bar",
+			},
+			true,
+		},
+		{
+			components.Metadata{
+				Name: "foo",
+			},
+			map[string]string{
+				"foo.yaml": "---\nfoo: bar",
+			},
+			false,
+		},
+	}
+
+	for _, c := range tc {
+		c := c
+
+		t.Run("", func(t *testing.T) {
+			chart, err := chartFromManifests(c.metadata, c.manifests)
+			if c.err && err == nil {
+				t.Fatalf("Expected error, got nil")
+			}
+
+			if !c.err && err != nil {
+				t.Fatalf("Didn't expect error, got: %v", err)
+			}
+
+			if c.err && err != nil {
+				return
+			}
+
+			if err := chart.Validate(); err != nil {
+				t.Fatalf("Generated chart should be valid, got: %v", err)
+			}
+		})
+	}
+}
+
+func TestChartFromManifestsRemoveNamespace(t *testing.T) {
+	manifests := map[string]string{
+		"namespace.yaml": `
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: foo
+`,
+	}
+
+	m := components.Metadata{
+		Name: "foo",
+		Namespace: k8sutil.Namespace{
+			Name: "foo",
+		},
+	}
+
+	chart, err := chartFromManifests(m, manifests)
+	if err != nil {
+		t.Fatalf("Chart should be created, got: %v", err)
+	}
+
+	if len(chart.Manifests) != 1 {
+		t.Fatalf("Manifest file with the namespace should still be added, as it may contain other objects")
+	}
+
+	if len(chart.Manifests[0].Data) != 0 {
+		t.Fatalf("Namespace object should be removed from chart")
+	}
+}
+
+func TestChartFromManifestsRemoveNamespaceRetainObject(t *testing.T) {
+	manifests := map[string]string{
+		"objects.yaml": `
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: foo
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: bar
+`,
+	}
+
+	m := components.Metadata{
+		Name: "foo",
+		Namespace: k8sutil.Namespace{
+			Name: "foo",
+		},
+	}
+
+	chart, err := chartFromManifests(m, manifests)
+	if err != nil {
+		t.Fatalf("Chart should be created, got: %v", err)
+	}
+
+	if len(chart.Manifests[0].Data) == 0 {
+		t.Fatalf("Other objects should be retained in the file containing Namespace object")
+	}
+}
+
+func TestChartFromManifestsRemoveOnlyReleaseNamespace(t *testing.T) {
+	manifests := map[string]string{
+		"objects.yaml": `
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: foo
+`,
+	}
+
+	m := components.Metadata{
+		Name: "foo",
+		Namespace: k8sutil.Namespace{
+			Name: "bar",
+		},
+	}
+
+	chart, err := chartFromManifests(m, manifests)
+	if err != nil {
+		t.Fatalf("Chart should be created, got: %v", err)
+	}
+
+	if len(chart.Manifests[0].Data) == 0 {
+		t.Fatalf("Only Namespace object with matching namespace name should be filtered")
+	}
+}
+
+func TestChartFromManifestsMoveCRDs(t *testing.T) {
+	manifests := map[string]string{
+		"crd.yaml": `
+kind: CustomResourceDefinition
+metadata:
+  name: foo
+`,
+	}
+
+	m := components.Metadata{
+		Name: "foo",
+	}
+
+	chart, err := chartFromManifests(m, manifests)
+	if err != nil {
+		t.Fatalf("Chart should be created, got: %v", err)
+	}
+
+	if len(chart.Manifests) != 1 {
+		t.Fatalf("Manifest file with the CRDs should still be added, as it may contain other objects")
+	}
+
+	if len(chart.Manifests[0].Data) != 0 {
+		t.Fatalf("CRD object should be removed from the manifests file")
+	}
+
+	if len(chart.Files) != 1 {
+		t.Fatalf("CRD object should be added to Files field")
+	}
+
+	if len(chart.Files[0].Data) == 0 {
+		t.Fatalf("CRD object in unmanaged files shouldn't be empty")
+	}
+
+	if strings.Split(chart.Files[0].Name, "/")[0] != "crds" {
+		t.Fatalf("CRD object should be added to file in 'crds' directory")
+	}
+}
diff --git a/pkg/components/util/helm_test.go b/pkg/components/util/helm_test.go
index a36a0ae4..be2ff4aa 100644
--- a/pkg/components/util/helm_test.go
+++ b/pkg/components/util/helm_test.go
@@ -12,14 +12,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package util
+package util_test
 
 import (
-	"strings"
+	"path/filepath"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"helm.sh/helm/v3/pkg/chart"
+
 	"github.com/kinvolk/lokomotive/pkg/components"
-	"github.com/kinvolk/lokomotive/pkg/k8sutil"
+	"github.com/kinvolk/lokomotive/pkg/components/util"
 )
 
 func TestRenderChartBadValues(t *testing.T) {
@@ -31,186 +34,74 @@ func TestRenderChartBadValues(t *testing.T) {
 		t.Fatalf("Loading chart from assets should succeed, got: %v", err)
 	}
 
-	if _, err := RenderChart(helmChart, c, c, values); err == nil {
+	if _, err := util.RenderChart(helmChart, c, c, values); err == nil {
 		t.Fatalf("Rendering chart with malformed values should fail")
 	}
 }
 
-func TestChartFromManifests(t *testing.T) {
-	tc := []struct {
-		metadata  components.Metadata
-		manifests map[string]string
-		err       bool
-	}{
-		{
-			components.Metadata{
-				Name: "foo",
-			},
-			map[string]string{
-				"foo.yaml": "bar",
-			},
-			true,
-		},
-		{
-			components.Metadata{
-				Name: "foo",
-			},
-			map[string]string{
-				"foo.yaml": "---\nfoo: bar",
-			},
-			false,
-		},
-	}
-
-	for _, c := range tc {
-		c := c
-
-		t.Run("", func(t *testing.T) {
-			chart, err := chartFromManifests(c.metadata, c.manifests)
-			if c.err && err == nil {
-				t.Fatalf("Expected error, got nil")
-			}
-
-			if !c.err && err != nil {
-				t.Fatalf("Didn't expect error, got: %v", err)
-			}
-
-			if c.err && err != nil {
-				return
-			}
+//nolint:funlen
+func Test_RenderChart_include_multiple_hooks_from_single_file(t *testing.T) {
+	values := ""
+	chartName := "foo"
+	fileName := "secrets.yaml"
 
-			if err := chart.Validate(); err != nil {
-				t.Fatalf("Generated chart should be valid, got: %v", err)
-			}
-		})
-	}
-}
-
-func TestChartFromManifestsRemoveNamespace(t *testing.T) {
-	manifests := map[string]string{
-		"namespace.yaml": `
+	expectedManifest := `---
 apiVersion: v1
-kind: Namespace
+kind: Secret
 metadata:
-  name: foo
-`,
-	}
-
-	m := components.Metadata{
-		Name: "foo",
-		Namespace: k8sutil.Namespace{
-			Name: "foo",
-		},
-	}
-
-	chart, err := chartFromManifests(m, manifests)
-	if err != nil {
-		t.Fatalf("Chart should be created, got: %v", err)
-	}
-
-	if len(chart.Manifests) != 1 {
-		t.Fatalf("Manifest file with the namespace should still be added, as it may contain other objects")
-	}
-
-	if len(chart.Manifests[0].Data) != 0 {
-		t.Fatalf("Namespace object should be removed from chart")
-	}
-}
-
-func TestChartFromManifestsRemoveNamespaceRetainObject(t *testing.T) {
-	manifests := map[string]string{
-		"objects.yaml": `
-apiVersion: v1
-kind: Namespace
-metadata:
-  name: foo
+  name: foo-controller-tls
+  annotations:
+    "helm.sh/resource-policy": "keep"
+    "helm.sh/hook": "pre-install"
+    "helm.sh/hook-delete-policy": "before-hook-creation"
+type: kubernetes.io/tls
+data:
+  tls.crt: "[...]"
+  tls.key: "[...]"
+  ca.crt: "[...]"
 ---
 apiVersion: v1
-kind: Pod
+kind: Secret
 metadata:
-  name: bar
-`,
-	}
-
-	m := components.Metadata{
-		Name: "foo",
-		Namespace: k8sutil.Namespace{
-			Name: "foo",
+  name: foo-client-tls
+  annotations:
+    "helm.sh/resource-policy": "keep"
+    "helm.sh/hook": "pre-install"
+    "helm.sh/hook-delete-policy": "before-hook-creation"
+type: kubernetes.io/tls
+data:
+  tls.crt: [...]
+  tls.key: [...]
+  ca.crt: [...]
+`
+
+	chart := &chart.Chart{
+		Metadata: &chart.Metadata{
+			APIVersion: "2.0.0",
+			Name:       chartName,
+			Version:    "0.1.0",
 		},
-	}
-
-	chart, err := chartFromManifests(m, manifests)
-	if err != nil {
-		t.Fatalf("Chart should be created, got: %v", err)
-	}
-
-	if len(chart.Manifests[0].Data) == 0 {
-		t.Fatalf("Other objects should be retained in the file containing Namespace object")
-	}
-}
-
-func TestChartFromManifestsRemoveOnlyReleaseNamespace(t *testing.T) {
-	manifests := map[string]string{
-		"objects.yaml": `
-apiVersion: v1
-kind: Namespace
-metadata:
-  name: foo
-`,
-	}
-
-	m := components.Metadata{
-		Name: "foo",
-		Namespace: k8sutil.Namespace{
-			Name: "bar",
+		Templates: []*chart.File{
+			{
+				Name: fileName,
+				Data: []byte(expectedManifest),
+			},
 		},
 	}
 
-	chart, err := chartFromManifests(m, manifests)
-	if err != nil {
-		t.Fatalf("Chart should be created, got: %v", err)
-	}
-
-	if len(chart.Manifests[0].Data) == 0 {
-		t.Fatalf("Only Namespace object with matching namespace name should be filtered")
-	}
-}
-
-func TestChartFromManifestsMoveCRDs(t *testing.T) {
-	manifests := map[string]string{
-		"crd.yaml": `
-kind: CustomResourceDefinition
-metadata:
-  name: foo
-`,
-	}
-
-	m := components.Metadata{
-		Name: "foo",
-	}
-
-	chart, err := chartFromManifests(m, manifests)
+	manifests, err := util.RenderChart(chart, "chartName", "chartNamespace", values)
 	if err != nil {
-		t.Fatalf("Chart should be created, got: %v", err)
-	}
-
-	if len(chart.Manifests) != 1 {
-		t.Fatalf("Manifest file with the CRDs should still be added, as it may contain other objects")
+		t.Fatalf("Rendering chart: %v", err)
 	}
 
-	if len(chart.Manifests[0].Data) != 0 {
-		t.Fatalf("CRD object should be removed from the manifests file")
-	}
-
-	if len(chart.Files) != 1 {
-		t.Fatalf("CRD object should be added to Files field")
-	}
+	expectedKey := filepath.Join(chartName, fileName)
 
-	if len(chart.Files[0].Data) == 0 {
-		t.Fatalf("CRD object in unmanaged files shouldn't be empty")
+	manifest, ok := manifests[expectedKey]
+	if !ok {
+		t.Fatalf("Expected manifest not found, got %+v", manifests)
 	}
 
-	if strings.Split(chart.Files[0].Name, "/")[0] != "crds" {
-		t.Fatalf("CRD object should be added to file in 'crds' directory")
+	if diff := cmp.Diff(expectedManifest, manifest); diff != "" {
+		t.Fatalf("Unexpected manifest diff: %v", diff)
 	}
 }