From d51d3eecc781074440ed41bd23acb91102cad8c0 Mon Sep 17 00:00:00 2001 From: Jojin Date: Sat, 25 Jul 2026 13:31:25 +0530 Subject: [PATCH] feat(lint): warn when dependency conditions do not resolve to a value A dependency condition whose paths do not exist in the chart's coalesced values is silently ignored when dependencies are processed, leaving the dependency enabled. That regularly surprises chart users when the condition key is missing from the chart's default values and the dependency gets installed although its condition was never evaluated. Changing the runtime behavior would be breaking, so instead teach 'helm lint' to emit a warning when none of the comma-separated paths of a dependency condition resolves to any value. Conditions are resolved the same way dependency processing does, against the chart's values coalesced with the values passed to lint, including the default values of vendored dependencies (with alias re-keying taken into account). The check is skipped while dependencies are missing from charts/, since their default values cannot participate yet. rules.Dependencies is public API in pkg/, so the values are threaded through a new rules.DependenciesWithValues function, mirroring the existing ValuesWithOverrides pattern. The chart v3 lint rules receive the identical change. Closes #12264 Signed-off-by: Jojin --- internal/chart/v3/lint/lint.go | 2 +- internal/chart/v3/lint/rules/dependencies.go | 80 ++++++++- .../chart/v3/lint/rules/dependencies_test.go | 153 ++++++++++++++++++ pkg/chart/v2/lint/lint.go | 2 +- pkg/chart/v2/lint/rules/dependencies.go | 80 ++++++++- pkg/chart/v2/lint/rules/dependencies_test.go | 153 ++++++++++++++++++ 6 files changed, 466 insertions(+), 4 deletions(-) diff --git a/internal/chart/v3/lint/lint.go b/internal/chart/v3/lint/lint.go index 193f0f796..fd230393a 100644 --- a/internal/chart/v3/lint/lint.go +++ b/internal/chart/v3/lint/lint.go @@ -58,7 +58,7 @@ func RunAll(baseDir string, values map[string]any, namespace string, options ... rules.Chartfile(&result) rules.ValuesWithOverrides(&result, values, lo.SkipSchemaValidation) rules.TemplatesWithSkipSchemaValidation(&result, values, namespace, lo.KubeVersion, lo.SkipSchemaValidation) - rules.Dependencies(&result) + rules.DependenciesWithValues(&result, values) rules.Crds(&result) return result diff --git a/internal/chart/v3/lint/rules/dependencies.go b/internal/chart/v3/lint/rules/dependencies.go index 2f558aaf0..c64e6766c 100644 --- a/internal/chart/v3/lint/rules/dependencies.go +++ b/internal/chart/v3/lint/rules/dependencies.go @@ -23,12 +23,22 @@ import ( chart "helm.sh/helm/v4/internal/chart/v3" "helm.sh/helm/v4/internal/chart/v3/lint/support" "helm.sh/helm/v4/internal/chart/v3/loader" + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/chart/common/util" ) // Dependencies runs lints against a chart's dependencies // // See https://github.com/helm/helm/issues/7910 func Dependencies(linter *support.Linter) { + DependenciesWithValues(linter, map[string]any{}) +} + +// DependenciesWithValues runs lints against a chart's dependencies, resolving +// dependency conditions against the chart's values coalesced with valueOverrides. +// +// See https://github.com/helm/helm/issues/7910 +func DependenciesWithValues(linter *support.Linter, valueOverrides map[string]any) { c, err := loader.LoadDir(linter.ChartDir) if !linter.RunLinterRule(support.ErrorSev, "", validateChartFormat(err)) { return @@ -36,7 +46,12 @@ func Dependencies(linter *support.Linter) { linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependencyInMetadata(c)) linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependenciesUnique(c)) - linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c)) + dependenciesPresent := linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c)) + // Conditions are resolved against the default values of the dependencies + // too, so only check them when all dependencies are present. + if dependenciesPresent { + linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyConditions(c, valueOverrides)) + } } func validateChartFormat(chartError error) error { @@ -80,6 +95,69 @@ func validateDependencyInMetadata(c *chart.Chart) (err error) { return err } +// validateDependencyConditions checks that the condition of each dependency +// resolves to a value. When none of the values a condition references exists, +// the condition is silently ignored when dependencies are processed, leaving +// the dependency enabled. That is usually an oversight in the chart's default +// values rather than intentional. +// +// See https://github.com/helm/helm/issues/12264 +func validateDependencyConditions(c *chart.Chart, valueOverrides map[string]any) error { + if len(c.Metadata.Dependencies) == 0 { + return nil + } + cvals, err := util.CoalesceValues(c, valueOverrides) + if err != nil { + return fmt.Errorf("unable to coalesce chart values: %w", err) + } + + // The values of an aliased dependency are still keyed by the chart name + // at this point; they are re-keyed by the alias when dependencies are + // processed. Map aliases back to chart names so conditions referencing + // the default values of an aliased dependency resolve correctly. + aliases := map[string]string{} + for _, dep := range c.Metadata.Dependencies { + if dep.Alias != "" { + aliases[dep.Alias] = dep.Name + } + } + + unresolved := []string{} + for _, dep := range c.Metadata.Dependencies { + if strings.TrimSpace(dep.Condition) == "" { + continue + } + if !conditionResolves(cvals, aliases, dep.Condition) { + unresolved = append(unresolved, fmt.Sprintf("%s (condition %q)", dep.Name, dep.Condition)) + } + } + if len(unresolved) > 0 { + return fmt.Errorf("conditions of these dependencies do not resolve to a value and have no effect: %s", strings.Join(unresolved, ", ")) + } + return nil +} + +// conditionResolves reports whether at least one of the comma-separated paths +// of a dependency condition resolves to a value. +func conditionResolves(cvals common.Values, aliases map[string]string, condition string) bool { + for path := range strings.SplitSeq(strings.TrimSpace(condition), ",") { + if path == "" { + continue + } + if _, err := cvals.PathValue(path); err == nil { + return true + } + if head, rest, found := strings.Cut(path, "."); found { + if name, ok := aliases[head]; ok { + if _, err := cvals.PathValue(name + "." + rest); err == nil { + return true + } + } + } + } + return false +} + func validateDependenciesUnique(c *chart.Chart) (err error) { dependencies := map[string]*chart.Dependency{} shadowing := []string{} diff --git a/internal/chart/v3/lint/rules/dependencies_test.go b/internal/chart/v3/lint/rules/dependencies_test.go index ae5882110..f924ebbf3 100644 --- a/internal/chart/v3/lint/rules/dependencies_test.go +++ b/internal/chart/v3/lint/rules/dependencies_test.go @@ -16,6 +16,7 @@ limitations under the License. package rules import ( + "os" "path/filepath" "testing" @@ -134,6 +135,114 @@ func TestValidateDependenciesUnique(t *testing.T) { } } +func TestValidateDependencyConditions(t *testing.T) { + newChart := func(deps []*chart.Dependency, values map[string]any, subcharts ...*chart.Chart) *chart.Chart { + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parentchart", + Version: "0.1.0", + APIVersion: "v3", + Dependencies: deps, + }, + Values: values, + } + c.SetDependencies(subcharts...) + return c + } + subchart := func(values map[string]any) *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "sub", + Version: "0.1.0", + APIVersion: "v3", + }, + Values: values, + } + } + + tests := []struct { + name string + chart *chart.Chart + valueOverrides map[string]any + wantErr string + }{ + { + name: "dependency without condition", + chart: newChart([]*chart.Dependency{{Name: "sub"}}, nil, subchart(nil)), + }, + { + name: "condition resolving to a parent chart value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + map[string]any{"sub": map[string]any{"enabled": false}}, + subchart(nil), + ), + }, + { + name: "condition resolving to a dependency default value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + nil, + subchart(map[string]any{"enabled": true}), + ), + }, + { + name: "condition resolving to a value override", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + nil, + subchart(nil), + ), + valueOverrides: map[string]any{"sub": map[string]any{"enabled": false}}, + }, + { + name: "second condition path resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled,global.sub.enabled"}}, + map[string]any{"global": map[string]any{"sub": map[string]any{"enabled": false}}}, + subchart(nil), + ), + }, + { + name: "condition not resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + map[string]any{"sub": map[string]any{"nested": false}}, + subchart(nil), + ), + wantErr: `sub (condition "sub.enabled")`, + }, + { + name: "condition of aliased dependency resolving to a dependency default value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Alias: "other", Condition: "other.enabled"}}, + nil, + subchart(map[string]any{"enabled": false}), + ), + }, + { + name: "condition of aliased dependency not resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Alias: "other", Condition: "other.enabled"}}, + nil, + subchart(nil), + ), + wantErr: `sub (condition "other.enabled")`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateDependencyConditions(tt.chart, tt.valueOverrides) + if tt.wantErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, tt.wantErr) + } + }) + } +} + func TestDependencies(t *testing.T) { tmp := t.TempDir() @@ -148,3 +257,47 @@ func TestDependencies(t *testing.T) { } } } + +func TestDependenciesWithValues(t *testing.T) { + tmp := t.TempDir() + + c := chart.Chart{ + Metadata: &chart.Metadata{ + Name: "condchart", + Version: "0.1.0", + APIVersion: "v3", + Dependencies: []*chart.Dependency{ + {Name: "sub", Version: "0.1.0", Condition: "sub.enabled"}, + }, + }, + } + c.SetDependencies(&chart.Chart{ + Metadata: &chart.Metadata{ + Name: "sub", + Version: "0.1.0", + APIVersion: "v3", + }, + }) + require.NoError(t, chartutil.SaveDir(&c, tmp)) + chartDir := filepath.Join(tmp, c.Metadata.Name) + + // The condition does not resolve to any value, warn about it + linter := support.Linter{ChartDir: chartDir} + Dependencies(&linter) + require.Len(t, linter.Messages, 1) + assert.Equal(t, support.WarningSev, linter.Messages[0].Severity) + require.ErrorContains(t, linter.Messages[0].Err, `sub (condition "sub.enabled")`) + + // The condition resolves to a value override, no warning + linter = support.Linter{ChartDir: chartDir} + DependenciesWithValues(&linter, map[string]any{"sub": map[string]any{"enabled": true}}) + assert.Empty(t, linter.Messages) + + // Conditions are not checked when dependencies are missing from the + // charts directory, as their default values cannot be resolved + require.NoError(t, os.RemoveAll(filepath.Join(chartDir, "charts"))) + linter = support.Linter{ChartDir: chartDir} + Dependencies(&linter) + require.Len(t, linter.Messages, 1) + assert.ErrorContains(t, linter.Messages[0].Err, "chart directory is missing these dependencies") +} diff --git a/pkg/chart/v2/lint/lint.go b/pkg/chart/v2/lint/lint.go index 204c15861..e0099d07a 100644 --- a/pkg/chart/v2/lint/lint.go +++ b/pkg/chart/v2/lint/lint.go @@ -63,7 +63,7 @@ func RunAll(baseDir string, values map[string]any, namespace string, options ... values, rules.TemplateLinterKubeVersion(lo.KubeVersion), rules.TemplateLinterSkipSchemaValidation(lo.SkipSchemaValidation)) - rules.Dependencies(&result) + rules.DependenciesWithValues(&result, values) rules.Crds(&result) return result diff --git a/pkg/chart/v2/lint/rules/dependencies.go b/pkg/chart/v2/lint/rules/dependencies.go index 616984c08..ae2957909 100644 --- a/pkg/chart/v2/lint/rules/dependencies.go +++ b/pkg/chart/v2/lint/rules/dependencies.go @@ -20,6 +20,8 @@ import ( "fmt" "strings" + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/chart/common/util" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/lint/support" "helm.sh/helm/v4/pkg/chart/v2/loader" @@ -29,6 +31,14 @@ import ( // // See https://github.com/helm/helm/issues/7910 func Dependencies(linter *support.Linter) { + DependenciesWithValues(linter, map[string]any{}) +} + +// DependenciesWithValues runs lints against a chart's dependencies, resolving +// dependency conditions against the chart's values coalesced with valueOverrides. +// +// See https://github.com/helm/helm/issues/7910 +func DependenciesWithValues(linter *support.Linter, valueOverrides map[string]any) { c, err := loader.LoadDir(linter.ChartDir) if !linter.RunLinterRule(support.ErrorSev, "", validateChartFormat(err)) { return @@ -36,7 +46,12 @@ func Dependencies(linter *support.Linter) { linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependencyInMetadata(c)) linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependenciesUnique(c)) - linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c)) + dependenciesPresent := linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c)) + // Conditions are resolved against the default values of the dependencies + // too, so only check them when all dependencies are present. + if dependenciesPresent { + linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyConditions(c, valueOverrides)) + } } func validateChartFormat(chartError error) error { @@ -80,6 +95,69 @@ func validateDependencyInMetadata(c *chart.Chart) (err error) { return err } +// validateDependencyConditions checks that the condition of each dependency +// resolves to a value. When none of the values a condition references exists, +// the condition is silently ignored when dependencies are processed, leaving +// the dependency enabled. That is usually an oversight in the chart's default +// values rather than intentional. +// +// See https://github.com/helm/helm/issues/12264 +func validateDependencyConditions(c *chart.Chart, valueOverrides map[string]any) error { + if len(c.Metadata.Dependencies) == 0 { + return nil + } + cvals, err := util.CoalesceValues(c, valueOverrides) + if err != nil { + return fmt.Errorf("unable to coalesce chart values: %w", err) + } + + // The values of an aliased dependency are still keyed by the chart name + // at this point; they are re-keyed by the alias when dependencies are + // processed. Map aliases back to chart names so conditions referencing + // the default values of an aliased dependency resolve correctly. + aliases := map[string]string{} + for _, dep := range c.Metadata.Dependencies { + if dep.Alias != "" { + aliases[dep.Alias] = dep.Name + } + } + + unresolved := []string{} + for _, dep := range c.Metadata.Dependencies { + if strings.TrimSpace(dep.Condition) == "" { + continue + } + if !conditionResolves(cvals, aliases, dep.Condition) { + unresolved = append(unresolved, fmt.Sprintf("%s (condition %q)", dep.Name, dep.Condition)) + } + } + if len(unresolved) > 0 { + return fmt.Errorf("conditions of these dependencies do not resolve to a value and have no effect: %s", strings.Join(unresolved, ", ")) + } + return nil +} + +// conditionResolves reports whether at least one of the comma-separated paths +// of a dependency condition resolves to a value. +func conditionResolves(cvals common.Values, aliases map[string]string, condition string) bool { + for path := range strings.SplitSeq(strings.TrimSpace(condition), ",") { + if path == "" { + continue + } + if _, err := cvals.PathValue(path); err == nil { + return true + } + if head, rest, found := strings.Cut(path, "."); found { + if name, ok := aliases[head]; ok { + if _, err := cvals.PathValue(name + "." + rest); err == nil { + return true + } + } + } + } + return false +} + func validateDependenciesUnique(c *chart.Chart) (err error) { dependencies := map[string]*chart.Dependency{} shadowing := []string{} diff --git a/pkg/chart/v2/lint/rules/dependencies_test.go b/pkg/chart/v2/lint/rules/dependencies_test.go index c800887bd..7fad86b32 100644 --- a/pkg/chart/v2/lint/rules/dependencies_test.go +++ b/pkg/chart/v2/lint/rules/dependencies_test.go @@ -16,6 +16,7 @@ limitations under the License. package rules import ( + "os" "path/filepath" "testing" @@ -132,6 +133,114 @@ func TestValidateDependenciesUnique(t *testing.T) { } } +func TestValidateDependencyConditions(t *testing.T) { + newChart := func(deps []*chart.Dependency, values map[string]any, subcharts ...*chart.Chart) *chart.Chart { + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parentchart", + Version: "0.1.0", + APIVersion: "v2", + Dependencies: deps, + }, + Values: values, + } + c.SetDependencies(subcharts...) + return c + } + subchart := func(values map[string]any) *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "sub", + Version: "0.1.0", + APIVersion: "v2", + }, + Values: values, + } + } + + tests := []struct { + name string + chart *chart.Chart + valueOverrides map[string]any + wantErr string + }{ + { + name: "dependency without condition", + chart: newChart([]*chart.Dependency{{Name: "sub"}}, nil, subchart(nil)), + }, + { + name: "condition resolving to a parent chart value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + map[string]any{"sub": map[string]any{"enabled": false}}, + subchart(nil), + ), + }, + { + name: "condition resolving to a dependency default value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + nil, + subchart(map[string]any{"enabled": true}), + ), + }, + { + name: "condition resolving to a value override", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + nil, + subchart(nil), + ), + valueOverrides: map[string]any{"sub": map[string]any{"enabled": false}}, + }, + { + name: "second condition path resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled,global.sub.enabled"}}, + map[string]any{"global": map[string]any{"sub": map[string]any{"enabled": false}}}, + subchart(nil), + ), + }, + { + name: "condition not resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Condition: "sub.enabled"}}, + map[string]any{"sub": map[string]any{"nested": false}}, + subchart(nil), + ), + wantErr: `sub (condition "sub.enabled")`, + }, + { + name: "condition of aliased dependency resolving to a dependency default value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Alias: "other", Condition: "other.enabled"}}, + nil, + subchart(map[string]any{"enabled": false}), + ), + }, + { + name: "condition of aliased dependency not resolving to a value", + chart: newChart( + []*chart.Dependency{{Name: "sub", Alias: "other", Condition: "other.enabled"}}, + nil, + subchart(nil), + ), + wantErr: `sub (condition "other.enabled")`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateDependencyConditions(tt.chart, tt.valueOverrides) + if tt.wantErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, tt.wantErr) + } + }) + } +} + func TestDependencies(t *testing.T) { tmp := t.TempDir() @@ -147,3 +256,47 @@ func TestDependencies(t *testing.T) { } } } + +func TestDependenciesWithValues(t *testing.T) { + tmp := t.TempDir() + + c := chart.Chart{ + Metadata: &chart.Metadata{ + Name: "condchart", + Version: "0.1.0", + APIVersion: "v2", + Dependencies: []*chart.Dependency{ + {Name: "sub", Version: "0.1.0", Condition: "sub.enabled"}, + }, + }, + } + c.SetDependencies(&chart.Chart{ + Metadata: &chart.Metadata{ + Name: "sub", + Version: "0.1.0", + APIVersion: "v2", + }, + }) + require.NoError(t, chartutil.SaveDir(&c, tmp)) + chartDir := filepath.Join(tmp, c.Metadata.Name) + + // The condition does not resolve to any value, warn about it + linter := support.Linter{ChartDir: chartDir} + Dependencies(&linter) + require.Len(t, linter.Messages, 1) + assert.Equal(t, support.WarningSev, linter.Messages[0].Severity) + require.ErrorContains(t, linter.Messages[0].Err, `sub (condition "sub.enabled")`) + + // The condition resolves to a value override, no warning + linter = support.Linter{ChartDir: chartDir} + DependenciesWithValues(&linter, map[string]any{"sub": map[string]any{"enabled": true}}) + assert.Empty(t, linter.Messages) + + // Conditions are not checked when dependencies are missing from the + // charts directory, as their default values cannot be resolved + require.NoError(t, os.RemoveAll(filepath.Join(chartDir, "charts"))) + linter = support.Linter{ChartDir: chartDir} + Dependencies(&linter) + require.Len(t, linter.Messages, 1) + assert.ErrorContains(t, linter.Messages[0].Err, "chart directory is missing these dependencies") +}