From b84ab9e69862315a03db6b76e0e36b0952e6f2e3 Mon Sep 17 00:00:00 2001 From: Robin Breathe Date: Wed, 17 Apr 2024 14:27:11 +0200 Subject: [PATCH] feat(HelmChartSpec): optionally ignore missing valuesFiles Signed-off-by: Robin Breathe --- api/v1beta2/helmchart_types.go | 5 ++ .../source.toolkit.fluxcd.io_helmcharts.yaml | 10 +++ docs/api/v1beta2/source.md | 26 ++++++++ internal/controller/helmchart_controller.go | 10 +-- internal/helm/chart/builder.go | 2 + internal/helm/chart/builder_local.go | 38 ++++++++---- internal/helm/chart/builder_local_test.go | 42 ++++++++++--- internal/helm/chart/builder_remote.go | 23 ++++--- internal/helm/chart/builder_remote_test.go | 62 ++++++++++++++++--- 9 files changed, 176 insertions(+), 42 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index 43f5984c..0603ee5b 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -79,6 +79,11 @@ type HelmChartSpec struct { // +deprecated ValuesFile string `json:"valuesFile,omitempty"` + // IgnoreMissingValuesFiles controls whether to silently ignore missing values + // files rather than failing. + // +optional + IgnoreMissingValuesFiles bool `json:"ignoreMissingValuesFiles,omitempty"` + // Suspend tells the controller to suspend the reconciliation of this // source. // +optional diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index abf13084..48aa02c1 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -93,6 +93,11 @@ spec: description: The name or path the Helm chart is available at in the SourceRef. type: string + ignoreMissingValuesFiles: + description: |- + IgnoreMissingValuesFiles controls whether to silently ignore missing values + files rather than failing. + type: boolean interval: description: The interval at which to check the Source for updates. type: string @@ -363,6 +368,11 @@ spec: Chart is the name or path the Helm chart is available at in the SourceRef. type: string + ignoreMissingValuesFiles: + description: |- + IgnoreMissingValuesFiles controls whether to silently ignore missing values + files rather than failing. + type: boolean interval: description: |- Interval at which the HelmChart SourceRef is checked for updates. diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index b5d50e9f..28540819 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -660,6 +660,19 @@ is merged before the ValuesFiles items. Ignored when omitted.

+ignoreMissingValuesFiles
+ +bool + + + +(Optional) +

Whether to silently ignore missing values files rather than failing. +

+ + + + suspend
bool @@ -2329,6 +2342,19 @@ is merged before the ValuesFiles items. Ignored when omitted.

+ignoreMissingValuesFiles
+ +bool + + + +(Optional) +

Whether to silently ignore missing values files rather than failing. +

+ + + + suspend
bool diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 647056a4..3e71c1db 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -665,8 +665,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // Construct the chart builder with scoped configuration cb := chart.NewRemoteBuilder(chartRepo) opts := chart.BuildOptions{ - ValuesFiles: obj.GetValuesFiles(), - Force: obj.Generation != obj.Status.ObservedGeneration, + ValuesFiles: obj.GetValuesFiles(), + IgnoreMissingValuesFiles: obj.Spec.IgnoreMissingValuesFiles, + Force: obj.Generation != obj.Status.ObservedGeneration, // The remote builder will not attempt to download the chart if // an artifact exists with the same name and version and `Force` is false. // It will however try to verify the chart if `obj.Spec.Verify` is set, at every reconciliation. @@ -760,8 +761,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj // Configure builder options, including any previously cached chart opts := chart.BuildOptions{ - ValuesFiles: obj.GetValuesFiles(), - Force: obj.Generation != obj.Status.ObservedGeneration, + ValuesFiles: obj.GetValuesFiles(), + IgnoreMissingValuesFiles: obj.Spec.IgnoreMissingValuesFiles, + Force: obj.Generation != obj.Status.ObservedGeneration, } if artifact := obj.Status.Artifact; artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index b116541f..ba4f74e9 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -107,6 +107,8 @@ type BuildOptions struct { // ValuesFiles can be set to a list of relative paths, used to compose // and overwrite an alternative default "values.yaml" for the chart. ValuesFiles []string + // IgnoreMissingValuesFiles controls whether to silently ignore missing values files rather than failing. + IgnoreMissingValuesFiles bool // CachedChart can be set to the absolute path of a chart stored on // the local filesystem, and is used for simple validation by metadata // comparisons. diff --git a/internal/helm/chart/builder_local.go b/internal/helm/chart/builder_local.go index 0e0b20c2..d7fcf516 100644 --- a/internal/helm/chart/builder_local.go +++ b/internal/helm/chart/builder_local.go @@ -140,9 +140,12 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, } // Merge chart values, if instructed - var mergedValues map[string]interface{} + var ( + mergedValues map[string]interface{} + valuesFiles []string + ) if len(opts.GetValuesFiles()) > 0 { - if mergedValues, err = mergeFileValues(localRef.WorkDir, opts.ValuesFiles); err != nil { + if mergedValues, valuesFiles, err = mergeFileValues(localRef.WorkDir, opts.ValuesFiles, opts.IgnoreMissingValuesFiles); err != nil { return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } } @@ -163,7 +166,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, if err != nil { return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } - result.ValuesFiles = opts.GetValuesFiles() + result.ValuesFiles = valuesFiles } // Ensure dependencies are fetched if building from a directory @@ -187,31 +190,42 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, } // mergeFileValues merges the given value file paths into a single "values.yaml" map. -// The provided (relative) paths may not traverse outside baseDir. It returns the merge -// result, or an error. -func mergeFileValues(baseDir string, paths []string) (map[string]interface{}, error) { +// The provided (relative) paths may not traverse outside baseDir. By default, a missing +// file is considered an error. If ignoreMissing is set true, missing files are ignored. +// It returns the merge result and the list of files that contributed to that result, +// or an error. +func mergeFileValues(baseDir string, paths []string, ignoreMissing bool) (map[string]interface{}, []string, error) { mergedValues := make(map[string]interface{}) + valuesFiles := make([]string, 0, len(paths)) for _, p := range paths { secureP, err := securejoin.SecureJoin(baseDir, p) if err != nil { - return nil, err + return nil, nil, err } - if f, err := os.Stat(secureP); err != nil || !f.Mode().IsRegular() { - return nil, fmt.Errorf("no values file found at path '%s' (reference '%s')", + f, err := os.Stat(secureP) + switch { + case err != nil: + if ignoreMissing && os.IsNotExist(err) { + continue + } + fallthrough + case !f.Mode().IsRegular(): + return nil, nil, fmt.Errorf("no values file found at path '%s' (reference '%s')", strings.TrimPrefix(secureP, baseDir), p) } b, err := os.ReadFile(secureP) if err != nil { - return nil, fmt.Errorf("could not read values from file '%s': %w", p, err) + return nil, nil, fmt.Errorf("could not read values from file '%s': %w", p, err) } values := make(map[string]interface{}) err = yaml.Unmarshal(b, &values) if err != nil { - return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) + return nil, nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) } mergedValues = transform.MergeMaps(mergedValues, values) + valuesFiles = append(valuesFiles, p) } - return mergedValues, nil + return mergedValues, valuesFiles, nil } // copyFileToPath attempts to copy in to out. It returns an error if out already exists. diff --git a/internal/helm/chart/builder_local_test.go b/internal/helm/chart/builder_local_test.go index 626dc072..6434b509 100644 --- a/internal/helm/chart/builder_local_test.go +++ b/internal/helm/chart/builder_local_test.go @@ -281,11 +281,13 @@ func TestLocalBuilder_Build_CachedChart(t *testing.T) { func Test_mergeFileValues(t *testing.T) { tests := []struct { - name string - files []*helmchart.File - paths []string - want map[string]interface{} - wantErr string + name string + files []*helmchart.File + paths []string + ignoreMissing bool + wantValues map[string]interface{} + wantFiles []string + wantErr string }{ { name: "merges values from files", @@ -295,10 +297,11 @@ func Test_mergeFileValues(t *testing.T) { {Name: "c.yaml", Data: []byte("b: d")}, }, paths: []string{"a.yaml", "b.yaml", "c.yaml"}, - want: map[string]interface{}{ + wantValues: map[string]interface{}{ "a": "b", "b": "d", }, + wantFiles: []string{"a.yaml", "b.yaml", "c.yaml"}, }, { name: "illegal traverse", @@ -318,6 +321,25 @@ func Test_mergeFileValues(t *testing.T) { paths: []string{"a.yaml"}, wantErr: "no values file found at path '/a.yaml'", }, + { + name: "ignore missing files", + files: []*helmchart.File{ + {Name: "a.yaml", Data: []byte("a: b")}, + }, + paths: []string{"a.yaml", "b.yaml"}, + ignoreMissing: true, + wantValues: map[string]interface{}{ + "a": "b", + }, + wantFiles: []string{"a.yaml"}, + }, + { + name: "all files missing", + paths: []string{"a.yaml"}, + ignoreMissing: true, + wantValues: map[string]interface{}{}, + wantFiles: []string{}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -329,16 +351,18 @@ func Test_mergeFileValues(t *testing.T) { g.Expect(os.WriteFile(filepath.Join(baseDir, f.Name), f.Data, 0o640)).To(Succeed()) } - got, err := mergeFileValues(baseDir, tt.paths) + gotValues, gotFiles, err := mergeFileValues(baseDir, tt.paths, tt.ignoreMissing) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(got).To(BeNil()) + g.Expect(gotValues).To(BeNil()) + g.Expect(gotFiles).To(BeNil()) return } g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) + g.Expect(gotValues).To(Equal(tt.wantValues)) + g.Expect(gotFiles).To(Equal(tt.wantFiles)) }) } } diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 345fedf9..7a87d01d 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -103,7 +103,7 @@ func (b *remoteChartBuilder) Build(ctx context.Context, ref Reference, p string, } chart.Metadata.Version = result.Version - mergedValues, err := mergeChartValues(chart, opts.ValuesFiles) + mergedValues, valuesFiles, err := mergeChartValues(chart, opts.ValuesFiles, opts.IgnoreMissingValuesFiles) if err != nil { err = fmt.Errorf("failed to merge chart values: %w", err) return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} @@ -113,7 +113,7 @@ func (b *remoteChartBuilder) Build(ctx context.Context, ref Reference, p string, if err != nil { return nil, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } - result.ValuesFiles = opts.GetValuesFiles() + result.ValuesFiles = valuesFiles } // Package the chart with the custom values @@ -226,13 +226,18 @@ func setBuildMetaData(version, versionMetadata string) (*semver.Version, error) } // mergeChartValues merges the given chart.Chart Files paths into a single "values.yaml" map. -// It returns the merge result, or an error. -func mergeChartValues(chart *helmchart.Chart, paths []string) (map[string]interface{}, error) { +// By default, a missing file is considered an error. If ignoreMissing is set true, +// missing files are ignored. +// It returns the merge result and the list of files that contributed to that result, +// or an error. +func mergeChartValues(chart *helmchart.Chart, paths []string, ignoreMissing bool) (map[string]interface{}, []string, error) { mergedValues := make(map[string]interface{}) + valuesFiles := make([]string, 0, len(paths)) for _, p := range paths { cfn := filepath.Clean(p) if cfn == chartutil.ValuesfileName { mergedValues = transform.MergeMaps(mergedValues, chart.Values) + valuesFiles = append(valuesFiles, p) continue } var b []byte @@ -243,15 +248,19 @@ func mergeChartValues(chart *helmchart.Chart, paths []string) (map[string]interf } } if b == nil { - return nil, fmt.Errorf("no values file found at path '%s'", p) + if ignoreMissing { + continue + } + return nil, nil, fmt.Errorf("no values file found at path '%s'", p) } values := make(map[string]interface{}) if err := yaml.Unmarshal(b, &values); err != nil { - return nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) + return nil, nil, fmt.Errorf("unmarshaling values from '%s' failed: %w", p, err) } mergedValues = transform.MergeMaps(mergedValues, values) + valuesFiles = append(valuesFiles, p) } - return mergedValues, nil + return mergedValues, valuesFiles, nil } // validatePackageAndWriteToPath atomically writes the packaged chart from reader diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index fa4fcf3e..d43966dc 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/internal/helm/chart/builder_remote_test.go @@ -443,11 +443,13 @@ entries: func Test_mergeChartValues(t *testing.T) { tests := []struct { - name string - chart *helmchart.Chart - paths []string - want map[string]interface{} - wantErr string + name string + chart *helmchart.Chart + paths []string + ignoreMissing bool + wantValues map[string]interface{} + wantFiles []string + wantErr string }{ { name: "merges values", @@ -459,10 +461,11 @@ func Test_mergeChartValues(t *testing.T) { }, }, paths: []string{"a.yaml", "b.yaml", "c.yaml"}, - want: map[string]interface{}{ + wantValues: map[string]interface{}{ "a": "b", "b": "d", }, + wantFiles: []string{"a.yaml", "b.yaml", "c.yaml"}, }, { name: "uses chart values", @@ -475,10 +478,11 @@ func Test_mergeChartValues(t *testing.T) { }, }, paths: []string{chartutil.ValuesfileName, "c.yaml"}, - want: map[string]interface{}{ + wantValues: map[string]interface{}{ "a": "b", "b": "d", }, + wantFiles: []string{chartutil.ValuesfileName, "c.yaml"}, }, { name: "unmarshal error", @@ -496,21 +500,59 @@ func Test_mergeChartValues(t *testing.T) { paths: []string{"a.yaml"}, wantErr: "no values file found at path 'a.yaml'", }, + { + name: "merges values ignoring file missing", + chart: &helmchart.Chart{ + Files: []*helmchart.File{ + {Name: "a.yaml", Data: []byte("a: b")}, + }, + }, + paths: []string{"a.yaml", "b.yaml"}, + ignoreMissing: true, + wantValues: map[string]interface{}{ + "a": "b", + }, + wantFiles: []string{"a.yaml"}, + }, + { + name: "merges values ignoring all missing", + chart: &helmchart.Chart{}, + paths: []string{"a.yaml"}, + ignoreMissing: true, + wantValues: map[string]interface{}{}, + wantFiles: []string{}, + }, + { + name: "uses chart values ignoring missing file", + chart: &helmchart.Chart{ + Values: map[string]interface{}{ + "a": "b", + }, + }, + paths: []string{chartutil.ValuesfileName, "c.yaml"}, + ignoreMissing: true, + wantValues: map[string]interface{}{ + "a": "b", + }, + wantFiles: []string{chartutil.ValuesfileName}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got, err := mergeChartValues(tt.chart, tt.paths) + gotValues, gotFiles, err := mergeChartValues(tt.chart, tt.paths, tt.ignoreMissing) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) - g.Expect(got).To(BeNil()) + g.Expect(gotValues).To(BeNil()) + g.Expect(gotFiles).To(BeNil()) return } g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) + g.Expect(gotValues).To(Equal(tt.wantValues)) + g.Expect(gotFiles).To(Equal(tt.wantFiles)) }) } }