fix: skip subhelmfiles when selectors conflict with CLI selectors (#2545)

* 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 <aiopsclub@163.com> * fix: address PR review comments - use CLI selectors, fix doc comment, add malformed selector test Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/1f1c33ce-e50d-4781-85b8-d606b5d4ca54 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * fix: add debug logging, unit tests, docs, and fix integration test for subhelmfile selector skip - Add debug log when skipping subhelmfile due to selector conflict - Add TestSubhelmfileSelectorsConflict with 11 cases for direct unit coverage - Document the selector-based subhelmfile skip optimization in docs/index.md - Fix integration test: use 'app' label key instead of reserved 'name' key (GetReleasesWithLabels overwrites labels["name"] with the release name) Signed-off-by: yxxhero <aiopsclub@163.com> * refactor: avoid map allocation in positiveLabelsCompatibleWith Compare positive label slices directly instead of allocating a map per comparison, as label counts are typically small (1-3 entries). Addresses Copilot review comment on PR #2545. Signed-off-by: yxxhero <aiopsclub@163.com> * docs: clarify subhelmfile selector docs per Copilot review feedback Reword the first two bullets to avoid the contradiction between 'CLI selectors are ignored' and the new skip optimization. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: address Copilot review comments round 3 - Log parse errors from SelectorsAreCompatible at debug level instead of silently discarding them - Hoist regex compilation to package-level vars in ParseLabels to avoid repeated compilation per selector - Replace EXIT traps with explicit cleanup calls in integration test to avoid interfering with the parent runner's trap Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
2026-09-30 07:08:38 +02:00 · 2026-04-25 17:18:39 +08:00
parent c57134cda7
commit 5368ab8d95
11 changed files with 414 additions and 9 deletions
@@ -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

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