From fdb0eac88e4fb51173927ad56dbe285f20eb00d0 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 19 Apr 2026 09:22:26 +0800 Subject: [PATCH] fix: skip subhelmfiles when selectors conflict with CLI selectors (#2544) When CLI selectors are provided (e.g. -l name=b), subhelmfiles whose explicit selectors are provably incompatible are now skipped entirely, avoiding unnecessary YAML loading and template rendering. Two selector sets are incompatible when every pair has a positive label conflict: same key with different values (e.g. name=b vs name=a). Negative labels are not compared. Fixes #2544 Signed-off-by: yxxhero --- pkg/app/app.go | 16 ++++ pkg/app/app_test.go | 6 +- pkg/state/release_filters.go | 58 ++++++++++++ pkg/state/release_filters_test.go | 90 +++++++++++++++++++ test/integration/run.sh | 1 + test/integration/test-cases/issue-2544.sh | 77 ++++++++++++++++ .../test-cases/issue-2544/input/helmfile.yaml | 10 +++ .../issue-2544/input/subhelmfile-a.yaml | 15 ++++ .../issue-2544/input/subhelmfile-b.yaml | 15 ++++ .../issue-2544/input/subhelmfile-c.yaml | 15 ++++ 10 files changed, 300 insertions(+), 3 deletions(-) create mode 100644 test/integration/test-cases/issue-2544.sh create mode 100644 test/integration/test-cases/issue-2544/input/helmfile.yaml create mode 100644 test/integration/test-cases/issue-2544/input/subhelmfile-a.yaml create mode 100644 test/integration/test-cases/issue-2544/input/subhelmfile-b.yaml create mode 100644 test/integration/test-cases/issue-2544/input/subhelmfile-c.yaml diff --git a/pkg/app/app.go b/pkg/app/app.go index 7cbee422..b1ba64c0 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1007,6 +1007,10 @@ 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(opts.Selectors, m) { + continue + } + optsForNestedState := LoadOpts{ CalleePath: filepath.Join(absd, file), Environment: m.Environment, @@ -1032,6 +1036,18 @@ 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 parent's selectors, +// meaning no release could satisfy both. In that case the subhelmfile can be +// safely skipped without loading or evaluating it. +func subhelmfileSelectorsConflict(parentSelectors []string, m state.SubHelmfileSpec) bool { + if len(parentSelectors) == 0 || len(m.Selectors) == 0 || m.SelectorsInherited { + return false + } + compatible, _ := state.SelectorsAreCompatible(parentSelectors, m.Selectors) + 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..804660c9 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) diff --git a/pkg/state/release_filters.go b/pkg/state/release_filters.go index dcdd68dd..b3bf27d4 100644 --- a/pkg/state/release_filters.go +++ b/pkg/state/release_filters.go @@ -47,6 +47,64 @@ 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 (conservative: never skip on malformed input). +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 { + otherPos := make(map[string]string, len(other.positiveLabels)) + for _, kv := range other.positiveLabels { + otherPos[kv[0]] = kv[1] + } + for _, kv := range l.positiveLabels { + if v, ok := otherPos[kv[0]]; ok && v != kv[1] { + return false + } + } + return true +} + // 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{} diff --git a/pkg/state/release_filters_test.go b/pkg/state/release_filters_test.go index 817d4769..098c79b9 100644 --- a/pkg/state/release_filters_test.go +++ b/pkg/state/release_filters_test.go @@ -67,3 +67,93 @@ 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 + }{ + { + 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, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := SelectorsAreCompatible(tt.selectorsA, tt.selectorsB) + 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..1851c73f --- /dev/null +++ b/test/integration/test-cases/issue-2544.sh @@ -0,0 +1,77 @@ +#!/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}" +} +trap cleanup_issue_2544 EXIT + +test_start "issue-2544: subhelmfiles skipped when selectors conflict with CLI" + +# --- Test 1: -l name=b should only produce output from subhelmfile-b --------------------------- + +info "Running helmfile template -l name=b" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l name=b \ + > "${issue_2544_tmp_dir}/output-b.txt" 2>&1 || { + cat "${issue_2544_tmp_dir}/output-b.txt" + fail "helmfile template -l name=b failed" +} + +# Should contain release-b +grep -q "release-b" "${issue_2544_tmp_dir}/output-b.txt" \ + || fail "output should contain release-b (matching selector name=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" \ + && fail "output should NOT contain release-a (subhelmfile selectors name=a conflict with name=b)" + +grep -q "release-c" "${issue_2544_tmp_dir}/output-b.txt" \ + && fail "output should NOT contain release-c (subhelmfile selectors name=c conflict with name=b)" + +info "PASS: -l name=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" + fail "helmfile template without selector failed" +} + +grep -q "release-a" "${issue_2544_tmp_dir}/output-all.txt" \ + || fail "output should contain release-a" +grep -q "release-b" "${issue_2544_tmp_dir}/output-all.txt" \ + || fail "output should contain release-b" +grep -q "release-c" "${issue_2544_tmp_dir}/output-all.txt" \ + || fail "output should contain release-c" + +info "PASS: no selector produces all releases" + +# --- Test 3: -l name=a should skip subhelmfiles with selectors name=b and name=c --------------- + +info "Running helmfile template -l name=a" +${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l name=a \ + > "${issue_2544_tmp_dir}/output-a.txt" 2>&1 || { + cat "${issue_2544_tmp_dir}/output-a.txt" + fail "helmfile template -l name=a failed" +} + +grep -q "release-a" "${issue_2544_tmp_dir}/output-a.txt" \ + || fail "output should contain release-a (matching selector name=a)" + +grep -q "release-b" "${issue_2544_tmp_dir}/output-a.txt" \ + && fail "output should NOT contain release-b (subhelmfile selectors name=b conflict with name=a)" + +grep -q "release-c" "${issue_2544_tmp_dir}/output-a.txt" \ + && fail "output should NOT contain release-c (subhelmfile selectors name=c conflict with name=a)" + +info "PASS: -l name=a produces only release-a" + +trap - EXIT +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..35659b60 --- /dev/null +++ b/test/integration/test-cases/issue-2544/input/helmfile.yaml @@ -0,0 +1,10 @@ +helmfiles: +- path: subhelmfile-a.yaml + selectors: + - name=a +- path: subhelmfile-b.yaml + selectors: + - name=b +- path: subhelmfile-c.yaml + selectors: + - name=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..91822849 --- /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: + name: 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..fc016f8b --- /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: + name: 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..9e93dc0d --- /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: + name: c + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: release-c + namespace: {{ .Release.Namespace }} + data: + group: c