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 - } - }) }