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 <jojin.kb@gmail.com>
pull/32472/head
Jojin 2 months ago
parent 39231d01e1
commit d51d3eecc7
No known key found for this signature in database

@ -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

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

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

@ -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

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

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

Loading…
Cancel
Save