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