Fix HelmChart valuesFile chart path restriction

As part of the feature implementation to support helm chart
dependencies, the functionality for allowing values files overwriting
from any location scoped to the same source was altered. This should fix
the problem by allowing users to load files from any arbitrary location
as long as it's in the context of the same source from where the helm
chart itself is loaded.

Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

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,
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.desc, func(t *testing.T) {
+			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 {
+			if err != nil && !tt.expectErr {
-				t.Errorf("OverwriteChartDefaultValues() error = %v", err)
+				t.Fatalf("returned unexpected error: %v", err)
-				return
 			}
+			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
-		}
-	})
 }