diff --git a/docs/index.md b/docs/index.md index 218d9d10..3783a2bb 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1522,7 +1522,8 @@ helmfiles: selectorsInherited: true ``` -* When a selector is specified, only this selector applies and the parents or CLI selectors are ignored. +* When a subhelmfile has explicit `selectors`, those selectors determine which releases from that subhelmfile are considered; parent and CLI selectors are not combined with them for release filtering. +* When CLI selectors are provided (e.g. `helmfile -l name=b sync`) and a subhelmfile has explicit selectors that are provably incompatible with them (same key, different value), that subhelmfile may be **skipped entirely** without loading or rendering it. For example, with `-l name=b`, a subhelmfile with `selectors: [name=a]` will be skipped since no release could match both. This optimization does not apply when `selectorsInherited: true` is set or when no CLI selectors are provided. Use `--debug` to see log messages about skipped subhelmfiles. * When not selector is specified there are 2 modes for the selector inheritance because we would like to change the current inheritance behavior (see [issue #344](https://github.com/roboll/helmfile/issues/344) ). * Legacy mode, sub-helmfiles without selectors inherit selectors from their parent helmfile. The initial helmfiles inherit from the command line selectors. * explicit mode, sub-helmfile without selectors do not inherit from their parent or the CLI selector. If you want them to inherit from their parent selector then use `selectorsInherited: true`. To enable this explicit mode you need to set the following environment variable `HELMFILE_EXPERIMENTAL=explicit-selector-inheritance` (see [experimental](#experimental-features)). diff --git a/pkg/app/app.go b/pkg/app/app.go index 7cbee422..1ee1cb48 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1007,6 +1007,11 @@ func (a *App) processStateFileParallel(relPath string, defOpts LoadOpts, converg func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, defOpts, opts LoadOpts, converge func(*state.HelmState) (bool, []error), sharedCtx *Context) (bool, error) { anyMatched := false for i, m := range st.Helmfiles { + if subhelmfileSelectorsConflict(a.Selectors, m, a.Logger) { + a.Logger.Debugf("skipping subhelmfile %q: CLI selectors %v conflict with subhelmfile selectors %v", m.Path, a.Selectors, m.Selectors) + continue + } + optsForNestedState := LoadOpts{ CalleePath: filepath.Join(absd, file), Environment: m.Environment, @@ -1032,6 +1037,24 @@ func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, def return anyMatched, nil } +// subhelmfileSelectorsConflict returns true when the subhelmfile has explicit +// selectors that are provably incompatible with the CLI selectors, +// meaning no release could satisfy both. In that case the subhelmfile can be +// safely skipped without loading or evaluating it. +// Only CLI selectors (not inherited parent selectors) are used for comparison, +// so this optimization is restricted to cases where the user explicitly +// provided selectors via the command line (e.g. -l name=b). +func subhelmfileSelectorsConflict(cliSelectors []string, m state.SubHelmfileSpec, logger *zap.SugaredLogger) bool { + if len(cliSelectors) == 0 || len(m.Selectors) == 0 || m.SelectorsInherited { + return false + } + compatible, err := state.SelectorsAreCompatible(cliSelectors, m.Selectors) + if err != nil { + logger.Debugf("selector compatibility check failed for subhelmfile %q: %v", m.Path, err) + } + return !compatible +} + func (a *App) visitStatesWithContext(fileOrDir string, defOpts LoadOpts, converge func(*state.HelmState) (bool, []error), sharedCtx *Context) error { noMatchInHelmfiles := true diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 40de4b3b..aa792b33 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -722,8 +722,8 @@ releases: }{ {label: "duplicatedOK=yes", expectedReleases: []string{"zipkin", "prometheus", "bar", "bar", "grafana", "postgresql"}, expectErr: false}, {label: "name=zipkin", expectedReleases: []string{"zipkin", "prometheus", "grafana", "postgresql"}, expectErr: false}, - {label: "name=grafana", expectedReleases: []string{"zipkin", "prometheus", "grafana", "grafana", "postgresql"}, expectErr: false}, - {label: "name=doesnotexists", expectedReleases: []string{"zipkin", "prometheus", "grafana", "postgresql"}, expectErr: false}, + {label: "name=grafana", expectedReleases: []string{"grafana", "grafana", "postgresql"}, expectErr: false}, + {label: "name=doesnotexists", expectedReleases: []string{"grafana", "postgresql"}, expectErr: false}, } runFilterSubHelmFilesTests(legacyTestcases, files, t, "1st EmbeddedSelectors") @@ -735,7 +735,7 @@ releases: errMsg string }{ {label: "duplicatedOK=yes", expectedReleases: []string{"zipkin", "prometheus", "grafana", "bar", "bar", "grafana", "postgresql"}, expectErr: false}, - {label: "name=doesnotexists", expectedReleases: []string{"zipkin", "prometheus", "grafana", "bar", "bar", "grafana", "postgresql"}, expectErr: false}, + {label: "name=doesnotexists", expectedReleases: []string{"grafana", "bar", "bar", "grafana", "postgresql"}, expectErr: false}, } t.Setenv(envvar.Experimental, ExperimentalSelectorExplicit) @@ -851,6 +851,89 @@ releases: runFilterSubHelmFilesTests(desiredTestcases, files, t, "2nd inherits") } +func TestSubhelmfileSelectorsConflict(t *testing.T) { + tests := []struct { + name string + cliSelectors []string + spec state.SubHelmfileSpec + conflict bool + }{ + { + name: "no CLI selectors", + cliSelectors: nil, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: false, + }, + { + name: "no subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: nil}, + conflict: false, + }, + { + name: "selectorsInherited true", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}, SelectorsInherited: true}, + conflict: false, + }, + { + name: "conflicting selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: true, + }, + { + name: "matching selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "b.yaml", Selectors: []string{"name=b"}}, + conflict: false, + }, + { + name: "different keys no conflict", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"env=prod"}}, + conflict: false, + }, + { + name: "empty CLI selectors slice", + cliSelectors: []string{}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}}, + conflict: false, + }, + { + name: "empty subhelmfile selectors slice", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{}}, + conflict: false, + }, + { + name: "all pairs conflict with multiple subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a", "name=c"}}, + conflict: true, + }, + { + name: "one matching pair among multiple subhelmfile selectors", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a", "name=b"}}, + conflict: false, + }, + { + name: "selectorsInherited false with conflicting selectors still conflicts", + cliSelectors: []string{"name=b"}, + spec: state.SubHelmfileSpec{Path: "a.yaml", Selectors: []string{"name=a"}, SelectorsInherited: false}, + conflict: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := subhelmfileSelectorsConflict(tt.cliSelectors, tt.spec, newAppTestLogger()) + assert.Equal(t, tt.conflict, got) + }) + } +} + func runFilterSubHelmFilesTests(testcases []struct { label string expectedReleases []string diff --git a/pkg/state/release_filters.go b/pkg/state/release_filters.go index dcdd68dd..4371ac6e 100644 --- a/pkg/state/release_filters.go +++ b/pkg/state/release_filters.go @@ -47,6 +47,69 @@ func (l LabelFilter) Match(r ReleaseSpec) bool { return true } +// SelectorsAreCompatible checks whether any pair of selectors from two sets could +// potentially match the same release. It compares only positive labels (key=value): +// two selectors conflict if they require the same key to have different values. +// Returns true if at least one pair is compatible, false only if all pairs conflict. +// On parse error, returns (true, err): the conservative true ensures subhelmfiles +// are never incorrectly skipped due to malformed input, while the non-nil error +// allows callers to log or handle the parse failure if desired. +func SelectorsAreCompatible(selectorsA, selectorsB []string) (bool, error) { + if len(selectorsA) == 0 || len(selectorsB) == 0 { + return true, nil + } + + filtersA, err := parseLabelFilters(selectorsA) + if err != nil { + return true, err + } + + filtersB, err := parseLabelFilters(selectorsB) + if err != nil { + return true, err + } + + for _, a := range filtersA { + for _, b := range filtersB { + if a.positiveLabelsCompatibleWith(b) { + return true, nil + } + } + } + + return false, nil +} + +func parseLabelFilters(selectors []string) ([]LabelFilter, error) { + filters := make([]LabelFilter, 0, len(selectors)) + for _, s := range selectors { + f, err := ParseLabels(s) + if err != nil { + return nil, err + } + filters = append(filters, f) + } + return filters, nil +} + +// positiveLabelsCompatibleWith returns true if the positive labels of two filters +// do not conflict (i.e., no shared key with a different value). +func (l LabelFilter) positiveLabelsCompatibleWith(other LabelFilter) bool { + for _, a := range l.positiveLabels { + for _, b := range other.positiveLabels { + if a[0] == b[0] && a[1] != b[1] { + return false + } + } + } + return true +} + +var ( + reLabelMismatch = regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+!=[a-zA-Z0-9_\.\/\+-]+$`) + reLabelMatch = regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+=[a-zA-Z0-9_\.\/\+-]+$`) +) + // ParseLabels takes a label in the form foo=bar,baz!=bat and returns a LabelFilter that will match the labels func ParseLabels(l string) (LabelFilter, error) { lf := LabelFilter{} @@ -54,16 +117,14 @@ func ParseLabels(l string) (LabelFilter, error) { lf.negativeLabels = [][]string{} var err error labels := strings.Split(l, ",") - reMissmatch := regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+!=[a-zA-Z0-9_\.\/\+-]+$`) - reMatch := regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+=[a-zA-Z0-9_\.\/\+-]+$`) for _, label := range labels { - if match := reMissmatch.MatchString(label); match { // k!=v case + if match := reLabelMismatch.MatchString(label); match { kv := strings.Split(label, "!=") lf.negativeLabels = append(lf.negativeLabels, kv) - } else if match := reMatch.MatchString(label); match { // k=v case + } else if match := reLabelMatch.MatchString(label); match { kv := strings.Split(label, "=") lf.positiveLabels = append(lf.positiveLabels, kv) - } else { // malformed case + } else { return lf, fmt.Errorf("malformed label: %s. Expected label in form k=v or k!=v", label) } } diff --git a/pkg/state/release_filters_test.go b/pkg/state/release_filters_test.go index 817d4769..6d969387 100644 --- a/pkg/state/release_filters_test.go +++ b/pkg/state/release_filters_test.go @@ -67,3 +67,105 @@ func TestParseLabelsInvalidFormat(t *testing.T) { expectedErrorMsg := "malformed label: invalid_label. Expected label in form k=v or k!=v" assert.EqualError(t, err, expectedErrorMsg, "unexpected error message") } + +func TestSelectorsAreCompatible(t *testing.T) { + tests := []struct { + name string + selectorsA []string + selectorsB []string + compatible bool + wantErr bool + }{ + { + name: "same key different value", + selectorsA: []string{"name=b"}, + selectorsB: []string{"name=a"}, + compatible: false, + }, + { + name: "same key same value", + selectorsA: []string{"name=b"}, + selectorsB: []string{"name=b"}, + compatible: true, + }, + { + name: "different keys no conflict", + selectorsA: []string{"name=b"}, + selectorsB: []string{"env=prod"}, + compatible: true, + }, + { + name: "one compatible pair among multiple selectors", + selectorsA: []string{"name=b"}, + selectorsB: []string{"name=a", "name=b"}, + compatible: true, + }, + { + name: "all pairs conflict", + selectorsA: []string{"name=b"}, + selectorsB: []string{"name=a", "name=c"}, + compatible: false, + }, + { + name: "one compatible pair with same key same value", + selectorsA: []string{"name=b", "env=prod"}, + selectorsB: []string{"name=b"}, + compatible: true, + }, + { + name: "empty selectorsA always compatible", + selectorsA: []string{}, + selectorsB: []string{"name=a"}, + compatible: true, + }, + { + name: "empty selectorsB always compatible", + selectorsA: []string{"name=a"}, + selectorsB: []string{}, + compatible: true, + }, + { + name: "negative labels not compared treated as compatible", + selectorsA: []string{"name!=a"}, + selectorsB: []string{"name=a"}, + compatible: true, + }, + { + name: "compound selector with conflicting key", + selectorsA: []string{"name=a,env=prod"}, + selectorsB: []string{"name=a,env=staging"}, + compatible: false, + }, + { + name: "compound selector with matching keys", + selectorsA: []string{"name=a,env=prod"}, + selectorsB: []string{"name=a,env=prod"}, + compatible: true, + }, + { + name: "compound selector partial overlap different key", + selectorsA: []string{"name=a,env=prod"}, + selectorsB: []string{"name=a"}, + compatible: true, + }, + { + name: "malformed selector returns conservative true with error", + selectorsA: []string{"name=b"}, + selectorsB: []string{"invalid_label"}, + compatible: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := SelectorsAreCompatible(tt.selectorsA, tt.selectorsB) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.compatible, got) + }) + } +} diff --git a/test/integration/run.sh b/test/integration/run.sh index 6b3cd92a..9fb6ecac 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -139,6 +139,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-2418.sh . ${dir}/test-cases/issue-2424-sequential-values-paths.sh . ${dir}/test-cases/issue-2431.sh +. ${dir}/test-cases/issue-2544.sh . ${dir}/test-cases/kubedog-tracking.sh # ALL DONE ----------------------------------------------------------------------------------------------------------- diff --git a/test/integration/test-cases/issue-2544.sh b/test/integration/test-cases/issue-2544.sh new file mode 100644 index 00000000..978808e9 --- /dev/null +++ b/test/integration/test-cases/issue-2544.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash + +# Test for issue #2544: subhelmfiles should only be evaluated if their selectors match +# When CLI selectors are provided, subhelmfiles with incompatible selectors are skipped. + +issue_2544_input_dir="${cases_dir}/issue-2544/input" +issue_2544_tmp_dir=$(mktemp -d) + +cleanup_issue_2544() { + rm -rf "${issue_2544_tmp_dir}" +} + +test_start "issue-2544: subhelmfiles skipped when selectors conflict with CLI" + +# --- Test 1: -l app=b should only produce output from subhelmfile-b --------------------------- + +info "Running helmfile template -l app=b" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=b \ + > "${issue_2544_tmp_dir}/output-b.txt" 2>&1 || { + cat "${issue_2544_tmp_dir}/output-b.txt" + cleanup_issue_2544 + fail "helmfile template -l app=b failed" +} + +# Should contain release-b +grep -q "release-b" "${issue_2544_tmp_dir}/output-b.txt" \ + || { cleanup_issue_2544; fail "output should contain release-b (matching selector app=b)"; } + +# Should NOT contain release-a or release-c (their subhelmfiles have incompatible selectors) +grep -q "release-a" "${issue_2544_tmp_dir}/output-b.txt" \ + && { cleanup_issue_2544; fail "output should NOT contain release-a (subhelmfile selectors app=a conflict with app=b)"; } + +grep -q "release-c" "${issue_2544_tmp_dir}/output-b.txt" \ + && { cleanup_issue_2544; fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=b)"; } + +info "PASS: -l app=b produces only release-b" + +# --- Test 2: no selector should produce all releases ----------------------------------------- + +info "Running helmfile template without selector" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template \ + > "${issue_2544_tmp_dir}/output-all.txt" 2>&1 || { + cat "${issue_2544_tmp_dir}/output-all.txt" + cleanup_issue_2544 + fail "helmfile template without selector failed" +} + +grep -q "release-a" "${issue_2544_tmp_dir}/output-all.txt" \ + || { cleanup_issue_2544; fail "output should contain release-a"; } +grep -q "release-b" "${issue_2544_tmp_dir}/output-all.txt" \ + || { cleanup_issue_2544; fail "output should contain release-b"; } +grep -q "release-c" "${issue_2544_tmp_dir}/output-all.txt" \ + || { cleanup_issue_2544; fail "output should contain release-c"; } + +info "PASS: no selector produces all releases" + +# --- Test 3: -l app=a should skip subhelmfiles with selectors app=b and app=c --------------- + +info "Running helmfile template -l app=a" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=a \ + > "${issue_2544_tmp_dir}/output-a.txt" 2>&1 || { + cat "${issue_2544_tmp_dir}/output-a.txt" + cleanup_issue_2544 + fail "helmfile template -l app=a failed" +} + +grep -q "release-a" "${issue_2544_tmp_dir}/output-a.txt" \ + || { cleanup_issue_2544; fail "output should contain release-a (matching selector app=a)"; } + +grep -q "release-b" "${issue_2544_tmp_dir}/output-a.txt" \ + && { cleanup_issue_2544; fail "output should NOT contain release-b (subhelmfile selectors app=b conflict with app=a)"; } + +grep -q "release-c" "${issue_2544_tmp_dir}/output-a.txt" \ + && { cleanup_issue_2544; fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=a)"; } + +info "PASS: -l app=a produces only release-a" + +cleanup_issue_2544 +test_pass "issue-2544: subhelmfiles skipped when selectors conflict with CLI" diff --git a/test/integration/test-cases/issue-2544/input/helmfile.yaml b/test/integration/test-cases/issue-2544/input/helmfile.yaml new file mode 100644 index 00000000..ee171717 --- /dev/null +++ b/test/integration/test-cases/issue-2544/input/helmfile.yaml @@ -0,0 +1,10 @@ +helmfiles: +- path: subhelmfile-a.yaml + selectors: + - app=a +- path: subhelmfile-b.yaml + selectors: + - app=b +- path: subhelmfile-c.yaml + selectors: + - app=c diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml new file mode 100644 index 00000000..e2e0acbb --- /dev/null +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml @@ -0,0 +1,15 @@ +releases: +- name: release-a + chart: ../../../charts/raw + labels: + app: a + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: release-a + namespace: {{ .Release.Namespace }} + data: + group: a diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml new file mode 100644 index 00000000..882f4b40 --- /dev/null +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml @@ -0,0 +1,15 @@ +releases: +- name: release-b + chart: ../../../charts/raw + labels: + app: b + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: release-b + namespace: {{ .Release.Namespace }} + data: + group: b diff --git a/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml b/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml new file mode 100644 index 00000000..933d0f8b --- /dev/null +++ b/test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml @@ -0,0 +1,15 @@ +releases: +- name: release-c + chart: ../../../charts/raw + labels: + app: c + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: release-c + namespace: {{ .Release.Namespace }} + data: + group: c