From a55c502bb421a898cfdce4a509331dc9e2fa3c6d Mon Sep 17 00:00:00 2001 From: Aurel Canciu Date: Wed, 16 Dec 2020 21:11:14 +0200 Subject: [PATCH] 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 --- .../helmchart_gitrepository.yaml | 2 +- controllers/helmchart_controller.go | 37 +++- controllers/helmchart_controller_test.go | 50 ++++++ .../testdata/charts/helmchart/override.yaml | 66 +++++++ internal/helm/chart.go | 30 +--- internal/helm/chart_test.go | 161 +++++++----------- 6 files changed, 214 insertions(+), 132 deletions(-) create mode 100644 controllers/testdata/charts/helmchart/override.yaml diff --git a/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml b/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml index eab8c5b7..911132d8 100644 --- a/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml +++ b/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 diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 6c56e384..9f53c4d1 100644 --- a/controllers/helmchart_controller.go +++ b/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 { - // Overwrite default values if configured - isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile) + if chart.Spec.ValuesFile != "" { + srcPath, err := securejoin.SecureJoin(tmpDir, 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 } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 7b99063f..140e0b7e 100644 --- a/controllers/helmchart_controller_test.go +++ b/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()) + }) }) }) }) diff --git a/controllers/testdata/charts/helmchart/override.yaml b/controllers/testdata/charts/helmchart/override.yaml new file mode 100644 index 00000000..e08cec5b --- /dev/null +++ b/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: {} diff --git a/internal/helm/chart.go b/internal/helm/chart.go index a7d60cd1..6630f4f7 100644 --- a/internal/helm/chart.go +++ b/internal/helm/chart.go @@ -25,46 +25,30 @@ import ( ) // OverwriteChartDefaultValues overwrites the chart default values file with the -// contents of the given valuesFile. -func OverwriteChartDefaultValues(chart *helmchart.Chart, valuesFile string) (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) - } - +// given data. +func OverwriteChartDefaultValues(chart *helmchart.Chart, data []byte) (bool, error) { // 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 } diff --git a/internal/helm/chart_test.go b/internal/helm/chart_test.go index 6e8a9d85..c0b3e8c5 100644 --- a/internal/helm/chart_test.go +++ b/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"} { - t.Run(tt, func(t *testing.T) { - fixture := chartFixture - ok, err := OverwriteChartDefaultValues(&fixture, tt) - if ok { - t.Error("OverwriteChartDefaultValues() should return false") - return + invalidChartFixture := chartFixture + invalidChartFixture.Raw = []*helmchart.File{} + invalidChartFixture.Files = []*helmchart.File{} + + testCases := []struct { + desc string + 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 { - t.Errorf("OverwriteChartDefaultValues() error = %v", err) - return + 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) { - t.Error("OverwriteChartDefaultValues() should not override values.yaml in Raw field") - return + if f.Name == chartutil.ValuesfileName && reflect.DeepEqual(f.Data, originalValuesFixture) && tt.ok { + t.Error("should override values.yaml in Raw field") } } for _, f := range fixture.Files { - if f.Name == chartutil.ValuesfileName && !reflect.DeepEqual(f.Data, originalValuesFixture) { - t.Error("OverwriteChartDefaultValues() should not override values.yaml in Files field") - return + if f.Name == chartutil.ValuesfileName && reflect.DeepEqual(f.Data, originalValuesFixture) && tt.ok { + t.Error("should override values.yaml in Files field") } } }) } - - 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 - } - }) }