Support passing different types of values directly through helm.values (

#178) (#188) Instead of using helm template option --set, using option --values to set values. After this change, users do not need to flatten the nested fields to set values, and will be able to pass special string values like "1234", "true" directly through helm.values Add test cases to test helm.values field with different types of values.
GoogleContainerTools · Oct 5, 2022 · 12c3de4 · 12c3de4
1 parent 67fe073
commit 12c3de4
Show file tree

Hide file tree

Showing 7 changed files with 110 additions and 34 deletions.
diff --git a/cmd/helm-sync/main.go b/cmd/helm-sync/main.go
@@ -37,7 +37,7 @@ var (
 	flVersion = flag.String("version", os.Getenv(reconcilermanager.HelmChartVersion),
 		"the version of the helm chart being synced")
 	flValues = flag.String("values", os.Getenv(reconcilermanager.HelmValues),
-		"set the helm chart values, will be used in helm template --set key=value")
+		"set the helm chart values, will be used to override the default values")
 	flIncludeCRDs = flag.String("include-crds", os.Getenv(reconcilermanager.HelmIncludeCRDs),
 		"include CRDs in the helm rendering output")
 	flAuth = flag.String("auth", util.EnvString(reconcilermanager.HelmAuthType, string(configsync.AuthNone)),

diff --git a/e2e/nomostest/predicates.go b/e2e/nomostest/predicates.go
@@ -169,24 +169,33 @@ func HasExactlyLabelKeys(wantKeys ...string) Predicate {
 	}
 }
 
-// HasExactlyImageName ensures a container has the expected image name
-func HasExactlyImageName(containerName string, expectImageName string) Predicate {
+// HasExactlyImage ensures a container has the expected image.
+func HasExactlyImage(containerName, expectImageName, expectImageTag, expectImageDigest string) Predicate {
 	return func(o client.Object) error {
 		dep, ok := o.(*appsv1.Deployment)
 		if !ok {
 			return WrongTypeErr(dep, &appsv1.Deployment{})
 		}
 		for _, container := range dep.Spec.Template.Spec.Containers {
 			if containerName == container.Name {
-				if !strings.Contains(container.Image, "/"+expectImageName+":") {
-					return errors.Errorf(" Expected %q container image name is %q, however the actual image is %q", container.Name, expectImageName, container.Image)
+				expectImage := ""
+				if expectImageName != "" {
+					expectImage = "/" + expectImageName
+				}
+				if expectImageTag != "" {
+					expectImage += ":" + expectImageTag
+				}
+				if expectImageDigest != "" {
+					expectImage += "@" + expectImageDigest
+				}
+				if !strings.Contains(container.Image, expectImage) {
+					return errors.Errorf("Expected %q container image contains %q, however the actual image is %q", container.Name, expectImage, container.Image)
 				}
 				return nil
 			}
 		}
 		return errors.Errorf("Container %q not found", containerName)
 	}
-
 }
 
 // HasCorrectResourceRequestsLimits verify a root/namespace reconciler container has the correct resource requests and limits.

diff --git a/e2e/testcases/csr_auth_test.go b/e2e/testcases/csr_auth_test.go
@@ -288,7 +288,7 @@ func testWorkloadIdentity(t *testing.T, testSpec workloadIdentityTestSpec) {
 		nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "oci": {"dir": "%s", "image": "%s", "auth": "gcpserviceaccount", "gcpServiceAccountEmail": "%s"}, "git": null}}`,
 			v1beta1.OciSource, tenant, testSpec.sourceRepo, testSpec.gsaEmail))
 	case v1beta1.HelmSource:
-		nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "helm": {"chart": "%s", "repo": "%s", "version": "%s", "auth": "gcpserviceaccount", "gcpServiceAccountEmail": "%s", "releaseName": "my-coredns", "namespace": "coredns", "values": {"image.pullPolicy": "Always"}}, "git": null}}`,
+		nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "helm": {"chart": "%s", "repo": "%s", "version": "%s", "auth": "gcpserviceaccount", "gcpServiceAccountEmail": "%s", "releaseName": "my-coredns", "namespace": "coredns"}, "git": null}}`,
 			v1beta1.HelmSource, testSpec.sourceChart, testSpec.sourceRepo, testSpec.sourceVersion, testSpec.gsaEmail))
 	}
 
@@ -300,8 +300,7 @@ func testWorkloadIdentity(t *testing.T, testSpec workloadIdentityTestSpec) {
 	if testSpec.sourceType == v1beta1.HelmSource {
 		nt.WaitForRepoSyncs(nomostest.WithRootSha1Func(testSpec.rootCommitFn),
 			nomostest.WithSyncDirectoryMap(map[types.NamespacedName]string{nomostest.DefaultRootRepoNamespacedName: testSpec.sourceChart}))
-		if err := nt.Validate("my-coredns-coredns", "coredns", &appsv1.Deployment{},
-			containerImagePullPolicy("Always")); err != nil {
+		if err := nt.Validate("my-coredns-coredns", "coredns", &appsv1.Deployment{}); err != nil {
 			nt.T.Error(err)
 		}
 	} else {

diff --git a/e2e/testcases/helm_sync_test.go b/e2e/testcases/helm_sync_test.go
@@ -20,6 +20,7 @@ import (
 	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/types"
 	"kpt.dev/configsync/e2e/nomostest"
@@ -46,22 +47,32 @@ const (
 // TestPublicHelm can run on both Kind and GKE clusters.
 // It tests Config Sync can pull from public Helm repo without any authentication.
 func TestPublicHelm(t *testing.T) {
-	publicHelmRepo := "https://kubernetes.github.io/ingress-nginx"
 	nt := nomostest.New(t, ntopts.SkipMonoRepo, ntopts.Unstructured)
 	origRepoURL := nt.GitProvider.SyncURL(nt.RootRepos[configsync.RootSyncName].RemoteRepoName)
 
 	rs := fake.RootSyncObjectV1Beta1(configsync.RootSyncName)
-	nt.T.Log("Update RootSync to sync from a public Helm Chart with specified release namespace")
-	nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "helm": {"repo": "%s", "chart": "ingress-nginx", "auth": "none", "version": "4.0.5", "releaseName": "my-ingress-nginx", "namespace": "ingress-nginx"}, "git": null}}`,
-		v1beta1.HelmSource, publicHelmRepo))
+	nt.T.Log("Update RootSync to sync from a public Helm Chart with specified release namespace and multiple inline values")
+	rootSyncFilePath := "../testdata/root-sync-helm-chart-cr.yaml"
+	nt.T.Logf("Apply the RootSync object defined in %s", rootSyncFilePath)
+	nt.MustKubectl("apply", "-f", rootSyncFilePath)
 	nt.T.Cleanup(func() {
 		// Change the rs back so that it works in the shared test environment.
 		nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "helm": null, "git": {"dir": "acme", "branch": "main", "repo": "%s", "auth": "ssh","gcpServiceAccountEmail": "", "secretRef": {"name": "git-creds"}}}}`,
 			v1beta1.GitSource, origRepoURL))
 	})
 	nt.WaitForRepoSyncs(nomostest.WithRootSha1Func(helmChartVersion("ingress-nginx:4.0.5")),
 		nomostest.WithSyncDirectoryMap(map[types.NamespacedName]string{nomostest.DefaultRootRepoNamespacedName: "ingress-nginx"}))
-	if err := nt.Validate("my-ingress-nginx-controller", "ingress-nginx", &appsv1.Deployment{}); err != nil {
+	if err := nt.Validate("my-ingress-nginx-controller", "ingress-nginx", &appsv1.Deployment{}, containerImagePullPolicy("Always"),
+		nomostest.HasCorrectResourceRequestsLimits("controller", resource.MustParse("150m"), resource.MustParse("1"), resource.MustParse("250Mi"), resource.MustParse("300Mi")),
+		nomostest.HasExactlyImage("controller", "ingress-nginx/controller", "v1.4.0", "sha256:54f7fe2c6c5a9db9a0ebf1131797109bb7a4d91f56b9b362bde2abd237dd1974"),
+		nomostest.DeploymentHasEnvVar("controller", "TEST_1", "val1"), nomostest.DeploymentHasEnvVar("controller", "TEST_2", "val2")); err != nil {
+		nt.T.Error(err)
+	}
+	if err := nt.Validate("my-ingress-nginx-defaultbackend", "ingress-nginx", &appsv1.Deployment{},
+		nomostest.HasExactlyImage("ingress-nginx-default-backend", "defaultbackend-amd64", "1.4", "")); err != nil {
+		nt.T.Error(err)
+	}
+	if err := nt.Validate("my-ingress-nginx-controller-metrics", "ingress-nginx", &corev1.Service{}, nomostest.HasAnnotation("prometheus.io/port", "10254"), nomostest.HasAnnotation("prometheus.io/scrape", "true")); err != nil {
 		nt.T.Error(err)
 	}
 	nt.T.Log("Update RootSync to sync from a public Helm Chart without specified release namespace")
@@ -271,9 +282,7 @@ func TestHelmARTokenAuth(t *testing.T) {
 		nt.MustKubectl("delete", "secret", "foo", "-n", v1.NSConfigManagementSystem, "--ignore-not-found")
 	})
 	nt.T.Log("Update RootSync to sync from a private Artifact Registry")
-	nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "git": null, "helm": {"repo": "%s", "chart": "%s", "auth": "token", "version": "%s", "releaseName": "my-coredns", "namespace": "coredns",
-                                                                                                "values": {"image.pullPolicy": "Always", "resources.requests.memory": "250Mi", "resources.requests.cpu": "150m", "resources.limits.memory": "300Mi", "resources.limits.cpu": 1},
-                                                                                                "secretRef": {"name" : "foo"}}}}`,
+	nt.MustMergePatch(rs, fmt.Sprintf(`{"spec": {"sourceType": "%s", "git": null, "helm": {"repo": "%s", "chart": "%s", "auth": "token", "version": "%s", "releaseName": "my-coredns", "namespace": "coredns", "secretRef": {"name" : "foo"}}}}`,
 		v1beta1.HelmSource, privateARHelmRegistry, privateHelmChart, privateHelmChartVersion))
 	nt.T.Cleanup(func() {
 		// Change the rs back so that it works in the shared test environment.
@@ -282,8 +291,7 @@ func TestHelmARTokenAuth(t *testing.T) {
 	})
 	nt.WaitForRepoSyncs(nomostest.WithRootSha1Func(helmChartVersion(privateHelmChart+":"+privateHelmChartVersion)),
 		nomostest.WithSyncDirectoryMap(map[types.NamespacedName]string{nomostest.DefaultRootRepoNamespacedName: privateHelmChart}))
-	if err := nt.Validate("my-coredns-coredns", "coredns", &appsv1.Deployment{},
-		containerImagePullPolicy("Always"), nomostest.HasCorrectResourceRequestsLimits("coredns", resource.MustParse("150m"), resource.MustParse("1"), resource.MustParse("250Mi"), resource.MustParse("300Mi"))); err != nil {
+	if err := nt.Validate("my-coredns-coredns", "coredns", &appsv1.Deployment{}); err != nil {
 		nt.T.Error(err)
 	}
 }

diff --git a/e2e/testcases/hydration_test.go b/e2e/testcases/hydration_test.go
@@ -296,7 +296,7 @@ func TestHydrateRemoteResources(t *testing.T) {
 
 	nt.T.Log("Check hydration controller default image name")
 	err := nt.Validate(nomostest.DefaultRootReconcilerName, v1.NSConfigManagementSystem, &appsv1.Deployment{},
-		nomostest.HasExactlyImageName(reconcilermanager.HydrationController, reconcilermanager.HydrationController))
+		nomostest.HasExactlyImage(reconcilermanager.HydrationController, reconcilermanager.HydrationController, "", ""))
 	if err != nil {
 		nt.T.Fatal(err)
 	}
@@ -313,7 +313,7 @@ func TestHydrateRemoteResources(t *testing.T) {
 	nt.MustMergePatch(rs, `{"spec": {"override": {"enableShellInRendering": true}}}`)
 	nt.WaitForRepoSyncs(nomostest.WithSyncDirectoryMap(map[types.NamespacedName]string{nomostest.DefaultRootRepoNamespacedName: "remote-base"}))
 	err = nt.Validate(nomostest.DefaultRootReconcilerName, v1.NSConfigManagementSystem, &appsv1.Deployment{},
-		nomostest.HasExactlyImageName(reconcilermanager.HydrationController, reconcilermanager.HydrationControllerWithShell))
+		nomostest.HasExactlyImage(reconcilermanager.HydrationController, reconcilermanager.HydrationControllerWithShell, "", ""))
 	if err != nil {
 		nt.T.Fatal(err)
 	}
@@ -352,7 +352,7 @@ func TestHydrateRemoteResources(t *testing.T) {
 	nt.MustMergePatch(rs, `{"spec": {"git": {"dir": "remote-base"}}}`)
 	nt.WaitForRootSyncRenderingError(configsync.RootSyncName, status.ActionableHydrationErrorCode, "")
 	err = nt.Validate(nomostest.DefaultRootReconcilerName, v1.NSConfigManagementSystem, &appsv1.Deployment{},
-		nomostest.HasExactlyImageName(reconcilermanager.HydrationController, reconcilermanager.HydrationController))
+		nomostest.HasExactlyImage(reconcilermanager.HydrationController, reconcilermanager.HydrationController, "", ""))
 	if err != nil {
 		nt.T.Fatal(err)
 	}

diff --git a/e2e/testdata/root-sync-helm-chart-cr.yaml b/e2e/testdata/root-sync-helm-chart-cr.yaml
@@ -0,0 +1,59 @@
+# Copyright 2022 Google LLC
+#
+# 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.
+
+apiVersion: configsync.gke.io/v1beta1
+kind: RootSync
+metadata:
+  name: root-sync
+  namespace: config-management-system
+spec:
+  sourceFormat: unstructured
+  sourceType: helm
+  helm:
+    repo: https://kubernetes.github.io/ingress-nginx
+    chart: ingress-nginx
+    version: 4.0.5
+    values:
+      controller:
+        resources:
+          requests:
+            cpu: 150m
+            memory: 250Mi
+          limits:
+            cpu: 1
+            memory: 300Mi
+        metrics:
+          enabled: true
+          service:
+            annotations:
+              prometheus.io/scrape: "true"
+              prometheus.io/port: "10254"
+        image:
+          pullPolicy: Always
+          image: ingress-nginx/controller
+          tag: "v1.4.0"
+          digest: sha256:54f7fe2c6c5a9db9a0ebf1131797109bb7a4d91f56b9b362bde2abd237dd1974
+        extraEnvs:
+        - name: TEST_1
+          value: "val1"
+        - name: TEST_2
+          value: "val2"
+      defaultBackend:
+        enabled: true
+        image:
+          image: defaultbackend-amd64
+          tag: "1.4"
+    releaseName: my-ingress-nginx
+    namespace: "ingress-nginx"
+    auth: none
diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go
@@ -16,7 +16,6 @@ package helm
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
@@ -31,6 +30,11 @@ import (
 	"kpt.dev/configsync/pkg/util"
 )
 
+const (
+	// valuesFile is the name of the file created to override defualt chart values.
+	valuesFile = "chart-values.yaml"
+)
+
 // Hydrator runs the helm hydration process.
 type Hydrator struct {
 	Chart       string
@@ -61,7 +65,7 @@ func (h *Hydrator) templateArgs(ctx context.Context, destDir string) ([]string,
 		args = append(args, "--repo", h.Repo)
 		args, err = h.appendAuthArgs(ctx, args)
 		if err != nil {
-			return []string{}, err
+			return nil, err
 		}
 	}
 	if h.Namespace != "" {
@@ -75,7 +79,7 @@ func (h *Hydrator) templateArgs(ctx context.Context, destDir string) ([]string,
 	if len(h.Values) > 0 {
 		args, err = h.appendValuesArgs(args)
 		if err != nil {
-			return []string{}, err
+			return nil, err
 		}
 	}
 	includeCRDs, _ := strconv.ParseBool(h.IncludeCRDs)
@@ -87,22 +91,19 @@ func (h *Hydrator) templateArgs(ctx context.Context, destDir string) ([]string,
 }
 
 func (h *Hydrator) appendValuesArgs(args []string) ([]string, error) {
-	var values map[string]interface{}
-	err := json.Unmarshal([]byte(h.Values), &values)
-	if err != nil {
-		return []string{}, fmt.Errorf("failed to unmarshal helm.values, error: %w", err)
-	}
-	for key, val := range values {
-		args = append(args, "--set", fmt.Sprintf("%s=%v", key, val))
+	valuesPath := filepath.Join(os.TempDir(), valuesFile)
+	if err := os.WriteFile(valuesPath, []byte(h.Values), 0644); err != nil {
+		return nil, fmt.Errorf("failed to create values file: %w", err)
 	}
+	args = append(args, "--values", valuesPath)
 	return args, nil
 }
 
 func (h *Hydrator) registryLoginArgs(ctx context.Context) ([]string, error) {
 	args := []string{"registry", "login"}
 	args, err := h.appendAuthArgs(ctx, args)
 	if err != nil {
-		return []string{}, err
+		return nil, err
 	}
 	res := strings.Split(strings.TrimPrefix(h.Repo, "oci://"), "/")
 	args = append(args, "https://"+res[0])
@@ -168,7 +169,7 @@ func (h *Hydrator) appendAuthArgs(ctx context.Context, args []string) ([]string,
 	case configsync.AuthGCPServiceAccount, configsync.AuthGCENode:
 		token, err := fetchNewToken(ctx)
 		if err != nil {
-			return []string{}, fmt.Errorf("failed to fetch new token: %w", err)
+			return nil, fmt.Errorf("failed to fetch new token: %w", err)
 		}
 		args = append(args, "--username", "oauth2accesstoken")
 		args = append(args, "--password", token.AccessToken)