Merge pull request #244 from fluxcd/fix-values-file-path-support

Fix HelmChart valuesFile chart path restriction

config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml

@@ -8,4 +8,4 @@ spec:
     kind: GitRepository
     name: podinfo
   chart: charts/podinfo
-  valuesFile: values-prod.yaml
+  valuesFile: charts/podinfo/values-prod.yaml

controllers/helmchart_controller.go

@@ -22,6 +22,7 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strings"
 	"time"
@@ -378,8 +379,18 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 		}
 
+		// Find override file and retrieve contents
+		var valuesData []byte
+		cfn := filepath.Clean(chart.Spec.ValuesFile)
+		for _, f := range helmChart.Files {
+			if f.Name == cfn {
+				valuesData = f.Data
+				break
+			}
+		}
+
 		// Overwrite values file
-		if changed, err := helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile); err != nil {
+		if changed, err := helm.OverwriteChartDefaultValues(helmChart, valuesData); err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		} else if !changed {
 			// No changes, skip to write original package to storage
@@ -483,9 +494,27 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	// or write the chart directly to storage.
 	pkgPath := chartPath
 	isValuesFileOverriden := false
-	if chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName {
+	if chart.Spec.ValuesFile != "" {
-		// Overwrite default values if configured
+		srcPath, err := securejoin.SecureJoin(tmpDir, chart.Spec.ValuesFile)
-		isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile)
+		if err != nil {
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
+			err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
+			return chart, err
+		}
+		src, err := os.Open(srcPath)
+		if err != nil {
+			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
+			return chart, err
+		}
+		defer src.Close()
+
+		var valuesData []byte
+		if _, err := src.Read(valuesData); err == nil {
+			isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, valuesData)
+		}
+
 		if err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}

controllers/helmchart_controller_test.go

@@ -693,6 +693,31 @@ var _ = Describe("HelmChartReconciler", func() {
 				return got.Status.Artifact != nil &&
 					storage.ArtifactExist(*got.Status.Artifact)
 			}, timeout, interval).Should(BeTrue())
+
+			When("Setting valid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil &&
+						storage.ArtifactExist(*got.Status.Artifact)
+				}, timeout, interval).Should(BeTrue())
+			})
+
+			When("Setting invalid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "invalid.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
+				}, timeout, interval).Should(BeTrue())
+			})
 		})
 	})
 
@@ -936,6 +961,31 @@ var _ = Describe("HelmChartReconciler", func() {
 				return got.Status.Artifact != nil &&
 					storage.ArtifactExist(*got.Status.Artifact)
 			}, timeout, interval).Should(BeTrue())
+
+			When("Setting valid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil &&
+						storage.ArtifactExist(*got.Status.Artifact)
+				}, timeout, interval).Should(BeTrue())
+			})
+
+			When("Setting invalid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
+				}, timeout, interval).Should(BeTrue())
+			})
 		})
 	})
 })

controllers/testdata/charts/helmchart/override.yaml

@@ -0,0 +1,66 @@
+# Override values for helmchart.
+# This is a YAML-formatted file.
+# Declare variables to be passed into your templates.
+
+replicaCount: 3
+
+image:
+  repository: nginx
+  pullPolicy: IfNotPresent
+
+imagePullSecrets: []
+nameOverride: ""
+fullnameOverride: ""
+
+serviceAccount:
+  # Specifies whether a service account should be created
+  create: true
+  # The name of the service account to use.
+  # If not set and create is true, a name is generated using the fullname template
+  name:
+
+podSecurityContext: {}
+  # fsGroup: 2000
+
+securityContext: {}
+  # capabilities:
+  #   drop:
+  #   - ALL
+  # readOnlyRootFilesystem: true
+  # runAsNonRoot: true
+  # runAsUser: 1000
+
+service:
+  type: ClusterIP
+  port: 80
+
+ingress:
+  enabled: false
+  annotations: {}
+    # kubernetes.io/ingress.class: nginx
+    # kubernetes.io/tls-acme: "true"
+  hosts:
+    - host: chart-example.local
+      paths: []
+  tls: []
+  #  - secretName: chart-example-tls
+  #    hosts:
+  #      - chart-example.local
+
+resources: {}
+  # We usually recommend not to specify default resources and to leave this as a conscious
+  # choice for the user. This also increases chances charts run on environments with little
+  # resources, such as Minikube. If you do want to specify resources, uncomment the following
+  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.
+  # limits:
+  #   cpu: 100m
+  #   memory: 128Mi
+  # requests:
+  #   cpu: 100m
+  #   memory: 128Mi
+
+nodeSelector: {}
+
+tolerations: []
+
+affinity: {}

internal/helm/chart.go

@@ -25,46 +25,30 @@ import (
 )
 
 // OverwriteChartDefaultValues overwrites the chart default values file with the
-// contents of the given valuesFile.
+// given data.
-func OverwriteChartDefaultValues(chart *helmchart.Chart, valuesFile string) (bool, error) {
+func OverwriteChartDefaultValues(chart *helmchart.Chart, data []byte) (bool, error) {
-	if valuesFile == "" || valuesFile == chartutil.ValuesfileName {
-		return false, nil
-	}
-
-	// Find override file and retrieve contents
-	var valuesData []byte
-	for _, f := range chart.Files {
-		if f.Name == valuesFile {
-			valuesData = f.Data
-			break
-		}
-	}
-	if valuesData == nil {
-		return false, fmt.Errorf("failed to locate override values file: %s", valuesFile)
-	}
-
 	// Read override values file data
-	values, err := chartutil.ReadValues(valuesData)
+	values, err := chartutil.ReadValues(data)
 	if err != nil {
-		return false, fmt.Errorf("failed to parse override values file: %s", valuesFile)
+		return false, fmt.Errorf("failed to parse provided override values file data")
 	}
 
 	// Replace current values file in Raw field
 	for _, f := range chart.Raw {
 		if f.Name == chartutil.ValuesfileName {
 			// Do nothing if contents are equal
-			if reflect.DeepEqual(f.Data, valuesData) {
+			if reflect.DeepEqual(f.Data, data) {
 				return false, nil
 			}
 
 			// Replace in Files field
 			for _, f := range chart.Files {
 				if f.Name == chartutil.ValuesfileName {
-					f.Data = valuesData
+					f.Data = data
 				}
 			}
 
-			f.Data = valuesData
+			f.Data = data
 			chart.Values = values
 			return true, nil
 		}

internal/helm/chart_test.go

@@ -18,7 +18,6 @@ package helm
 
 import (
 	"reflect"
-	"strings"
 	"testing"
 
 	helmchart "helm.sh/helm/v3/pkg/chart"
@@ -27,24 +26,11 @@ import (
 
 var (
 	originalValuesFixture []byte            = []byte("override: original")
-	overrideValuesFixture []byte            = []byte("override: test")
 	chartFilesFixture     []*helmchart.File = []*helmchart.File{
 		{
 			Name: "values.yaml",
 			Data: originalValuesFixture,
 		},
-		{
-			Name: "values-identical.yaml",
-			Data: originalValuesFixture,
-		},
-		{
-			Name: "values-override.yaml",
-			Data: overrideValuesFixture,
-		},
-		{
-			Name: "values-invalid.yaml",
-			Data: []byte(":fail!"),
-		},
 	}
 	chartFixture helmchart.Chart = helmchart.Chart{
 		Metadata: &helmchart.Metadata{
@@ -57,104 +43,71 @@ var (
 )
 
 func TestOverwriteChartDefaultValues(t *testing.T) {
-	for _, tt := range []string{"", "values.yaml", "values-identical.yaml"} {
+	invalidChartFixture := chartFixture
-		t.Run(tt, func(t *testing.T) {
+	invalidChartFixture.Raw = []*helmchart.File{}
-			fixture := chartFixture
+	invalidChartFixture.Files = []*helmchart.File{}
-			ok, err := OverwriteChartDefaultValues(&fixture, tt)
+
-			if ok {
+	testCases := []struct {
-				t.Error("OverwriteChartDefaultValues() should return false")
+		desc      string
-				return
+		chart     helmchart.Chart
+		data      []byte
+		ok        bool
+		expectErr bool
+	}{
+		{
+			desc:      "invalid chart",
+			chart:     invalidChartFixture,
+			data:      originalValuesFixture,
+			expectErr: true,
+		},
+		{
+			desc:  "identical override",
+			chart: chartFixture,
+			data:  originalValuesFixture,
+		},
+		{
+			desc:  "valid override",
+			chart: chartFixture,
+			ok:    true,
+			data:  []byte("override: test"),
+		},
+		{
+			desc:  "empty override",
+			chart: chartFixture,
+			ok:    true,
+			data:  []byte(""),
+		},
+		{
+			desc:      "invalid",
+			chart:     chartFixture,
+			data:      []byte("!fail:"),
+			expectErr: true,
+		},
 	}
-			if err != nil {
+	for _, tt := range testCases {
-				t.Errorf("OverwriteChartDefaultValues() error = %v", err)
+		t.Run(tt.desc, func(t *testing.T) {
-				return
+			fixture := tt.chart
+			ok, err := OverwriteChartDefaultValues(&fixture, tt.data)
+			if ok != tt.ok {
+				t.Fatalf("should return %v, returned %v", tt.ok, ok)
 			}
+			if err != nil && !tt.expectErr {
+				t.Fatalf("returned unexpected error: %v", err)
+			}
+			if err == nil && tt.expectErr {
+				t.Fatal("expected error")
+			}
+
 			for _, f := range fixture.Raw {
-				if f.Name == chartutil.ValuesfileName && !reflect.DeepEqual(f.Data, originalValuesFixture) {
+				if f.Name == chartutil.ValuesfileName && reflect.DeepEqual(f.Data, originalValuesFixture) && tt.ok {
-					t.Error("OverwriteChartDefaultValues() should not override values.yaml in Raw field")
+					t.Error("should override values.yaml in Raw field")
-					return
 				}
 			}
 			for _, f := range fixture.Files {
-				if f.Name == chartutil.ValuesfileName && !reflect.DeepEqual(f.Data, originalValuesFixture) {
+				if f.Name == chartutil.ValuesfileName && reflect.DeepEqual(f.Data, originalValuesFixture) && tt.ok {
-					t.Error("OverwriteChartDefaultValues() should not override values.yaml in Files field")
+					t.Error("should override values.yaml in Files field")
-					return
 				}
 			}
 		})
 	}
-
-	t.Run("values-error.yaml", func(t *testing.T) {
-		fixture := chartFixture
-		ok, err := OverwriteChartDefaultValues(&fixture, "values-error.yaml")
-		if ok {
-			t.Error("OverwriteChartDefaultValues() should return false")
-		}
-		if err == nil {
-			t.Error("OverwriteChartDefaultValues() expects an error")
-			return
-		} else if !strings.Contains(err.Error(), "failed to locate override values file") {
-			t.Error("OverwriteChartDefaultValues() returned invalid error")
-			return
-		}
-	})
-
-	t.Run("values-override.yaml", func(t *testing.T) {
-		fixture := chartFixture
-		ok, err := OverwriteChartDefaultValues(&fixture, "values-override.yaml")
-		if err != nil {
-			t.Errorf("OverwriteChartDefaultValues() error = %v", err)
-			return
-		}
-		if !ok {
-			t.Error("OverwriteChartDefaultValues() should return true")
-			return
-		}
-		for _, f := range fixture.Raw {
-			if f.Name == chartutil.ValuesfileName && string(f.Data) != string(overrideValuesFixture) {
-				t.Error("OverwriteChartDefaultValues() should override values.yaml in Raw field")
-				return
-			}
-		}
-		for _, f := range fixture.Files {
-			if f.Name == chartutil.ValuesfileName && string(f.Data) != string(overrideValuesFixture) {
-				t.Error("OverwriteChartDefaultValues() should override values.yaml in Files field")
-				return
-			}
-		}
-
-		// Context: the impossible chart, no values.yaml file defined!
-		fixture.Raw = fixture.Raw[1:]
-		fixture.Files = fixture.Files[1:]
-		ok, err = OverwriteChartDefaultValues(&fixture, "values-override.yaml")
-		if ok {
-			t.Error("OverwriteChartDefaultValues() should return false")
-			return
-		}
-		if err == nil {
-			t.Error("OverwriteChartDefaultValues() expects an error")
-			return
-		} else if !strings.Contains(err.Error(), "failed to locate values file") {
-			t.Error("OverwriteChartDefaultValues() returned invalid error")
-			return
-		}
-	})
-
-	t.Run("values-invalid.yaml", func(t *testing.T) {
-		fixture := chartFixture
-		fixture.Raw[0].Data = fixture.Raw[1].Data
-		fixture.Files[0].Data = fixture.Files[1].Data
-		ok, err := OverwriteChartDefaultValues(&fixture, "values-invalid.yaml")
-		if ok {
-			t.Error("OverwriteChartDefaultValues() should return false")
-			return
-		}
-		if err == nil {
-			t.Error("OverwriteChartDefaultValues() expects an error")
-			return
-		} else if !strings.Contains(err.Error(), "failed to parse override values file") {
-			t.Error("OverwriteChartDefaultValues() returned invalid error")
-			return
-		}
-	})
 }