Merge d51d3eecc7 into 2b128dcd27

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

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

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

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

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

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()
 
@@ -146,3 +255,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")
+}