From afdb2487a6d41f7500c771a0fa274b5542f39052 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:44:16 +0800 Subject: [PATCH] feat: add `inherits:` for sub-helmfile config inheritance (#2680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add `inherits:` for sub-helmfile config inheritance Add an opt-in `inherits:` field to `helmfiles:` entries so a sub-helmfile can inherit specific configuration categories from its parent: helmfiles: - path: myapp.yaml inherits: [repositories, environments] Allowed values: repositories, helmDefaults, commonLabels, apiVersions, kubeVersion, templates, environments. Child values win; parent fills gaps (consistent with `bases:`). This directly fixes #1495, where a repository declared in the parent was unavailable to sub-helmfiles, producing a confusing "repo not found" error. Implementation notes: - The 6 pure fields (repositories, helmDefaults, commonLabels, apiVersions, kubeVersion, templates) are merged post-load via MergeInherited; verified all are consumed post-load (ExecuteTemplates/converge), never at parse. - environments is injected pre-load as ctxEnv, because RenderedValues is baked at load time; the parent's resolved values become the base and the child's own environments: block overrides per key. - helmDefaults uses a *HelmSpec pointer (value type is non-comparable) with a no-override mergo merge, so a child that omits helmDefaults inherits the parent's fully. - A footgun warning (WarnUninheritedRepos) suggests `inherits: [repositories]` when a release references a repo the parent declares but the child lacks. - Unknown inherits keys are rejected at parse time with the allowed set. Inheritance is opt-in and fully backward compatible: empty (the default) preserves the historical independent-sub-helmfile behavior. Fixes #1495 Signed-off-by: yxxhero * fix: address review — deep-copy inherited config and fix bases: doc link - BuildInheritedConfig now deep-copies the pure fields via a YAML round-trip (Env via environment.DeepCopy) so the returned config never aliases the parent state's slices/maps, matching its doc comment. Now returns an error to surface round-trip failures; the call site in processNestedHelmfiles is updated. Added TestBuildInheritedConfig_PureFieldsAreDeepCopied to lock in the no-aliasing guarantee. - Fix the broken `bases:` anchor (#) in shared-configuration-across-teams.md to point to writing-helmfile.md#layering-state-files. Signed-off-by: yxxhero * fix: address review — reject inherits without path and document helmDefaults caveat Signed-off-by: yxxhero * fix: address review — make AllowedInherits immutable and clarify effective-repo wording Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- docs/shared-configuration-across-teams.md | 90 ++++++ pkg/app/app.go | 19 ++ pkg/app/desired_state_file_loader.go | 25 +- pkg/app/inherits_e2e_test.go | 221 +++++++++++++ pkg/app/load_opts.go | 22 ++ pkg/app/load_opts_test.go | 57 ++++ pkg/state/inherited.go | 276 ++++++++++++++++ pkg/state/inherited_test.go | 302 ++++++++++++++++++ pkg/state/inherits_yaml_test.go | 99 ++++++ pkg/state/state.go | 25 ++ test/integration/run.sh | 1 + .../test-cases/inherits-subhelmfile.sh | 66 ++++ .../input/child-env.yaml.gotmpl | 18 ++ .../input/child-transitive.yaml | 6 + .../inherits-subhelmfile/input/child.yaml | 15 + .../input/env-values.yaml | 1 + .../input/grandchild-transitive.yaml.gotmpl | 17 + .../input/helmfile-bad-key.yaml | 12 + .../input/helmfile-env.yaml | 12 + .../input/helmfile-inherits.yaml | 11 + .../input/helmfile-transitive.yaml | 10 + .../test-cases/inherits-subhelmfile/readme | 1 + 22 files changed, 1305 insertions(+), 1 deletion(-) create mode 100644 pkg/app/inherits_e2e_test.go create mode 100644 pkg/state/inherited.go create mode 100644 pkg/state/inherited_test.go create mode 100644 pkg/state/inherits_yaml_test.go create mode 100644 test/integration/test-cases/inherits-subhelmfile.sh create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/child-env.yaml.gotmpl create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/child-transitive.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/child.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/env-values.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/grandchild-transitive.yaml.gotmpl create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/helmfile-bad-key.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/helmfile-env.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/helmfile-inherits.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/input/helmfile-transitive.yaml create mode 100644 test/integration/test-cases/inherits-subhelmfile/readme diff --git a/docs/shared-configuration-across-teams.md b/docs/shared-configuration-across-teams.md index 247860cb..2c2181bb 100644 --- a/docs/shared-configuration-across-teams.md +++ b/docs/shared-configuration-across-teams.md @@ -150,3 +150,93 @@ Now that we use the environment variable `PRODUCT_ID` to as the parameters of re $ PRODUCT_ID=1 helmfile -f product/helmfile.yaml apply $ PRODUCT_ID=2 helmfile -f product/helmfile.yaml apply ``` + +## Inheriting parent configuration with `inherits:` + +By default a sub-helmfile is independent: it does **not** see the `repositories:`, +`helmDefaults:`, `environments:`, etc. declared in the parent that includes it. To +share configuration you either extract it into a separate file referenced via +[`bases:`](writing-helmfile.md#layering-state-files) from each sub-helmfile, or — more concisely — let each sub-helmfile +opt into inheriting specific categories from its parent with `inherits:`. + +```yaml +# parent helmfile.yaml +repositories: +- name: release-charts + url: registry.example.com/release/helm-charts + oci: true + +helmfiles: +- path: myapp.yaml + inherits: + - repositories + - environments +``` + +`myapp.yaml` now sees the `release-charts` repository and the parent's resolved +environment values, without having to re-declare them. + +### Allowed values + +`inherits:` accepts a list. Each entry must be one of: + +| Key | What is inherited | +|-----|-------------------| +| `repositories` | Parent repository definitions (appended; child entry wins on name conflict). | +| `helmDefaults` | Parent helm defaults; parent fills the child's unset sub-fields, child's set sub-fields win (see caveat below). | +| `commonLabels` | Parent common labels; child's keys win on conflict. | +| `apiVersions` | Parent API versions (appended and de-duplicated). | +| `kubeVersion` | Parent kube version, only when the child left it empty. | +| `templates` | Parent templates; child's template wins on name conflict. | +| `environments` | The parent's **resolved** environment values (see note below). | + +### Precedence + +**Child wins, parent fills gaps** — consistent with `bases:`. Slices like +`repositories` accumulate (parent first); maps (`commonLabels`, `templates`) are +unioned with the child's keys winning; `helmDefaults` is deep-merged so the child +overrides individual sub-fields it sets. + +#### Caveat: `helmDefaults` and zero values + +`helmDefaults` is deep-merged field-by-field, and Go value types cannot +distinguish an explicitly-set zero value from "unset". So a child that sets a +sub-field to its zero value (for example `atomic: false` to disable an inherited +`atomic: true`) will still see the parent's non-zero value fill in. To override +such a field, set it to a non-zero value, or omit `helmDefaults` from `inherits:` +and declare the block in full on the child. The common case — a child that omits +`helmDefaults` entirely and inherits the parent's — is unaffected. + +### Note on `environments` + +`environments` inherits the parent's **already-resolved** values (including CLI +overrides and decrypted secrets), not the `environments:` declaration block. This +avoids ambiguity around the directory that relative values files resolve against +(the parent resolves them once). The child's own `environments:` block, if any, +still takes precedence per key. + +### Transitive inheritance + +Inheritance is opt-in at **every** level but the values propagate. If a parent +inherits `repositories` and the child in turn has its own `helmfiles:` entries +declaring `inherits: [repositories]`, the grandchild receives the accumulated set. + +### `inherits:` vs `bases:` + +| | `bases:` | `inherits:` | +|---|---|---| +| Model | Pull — each file lists files to merge into itself | Push — the parent pushes its config to the child | +| Where the shared config lives | Must live in a separate file | Can live inline in the parent | +| Repetition | `bases:` must be repeated in every sub-helmfile | Declared once, per sub-helmfile entry | + +They are complementary: use `bases:` for cross-team reusable building blocks +(environment defaults, shared helm defaults), and `inherits:` to let a sub-helmfile +consume the parent's inline configuration without duplication. + +### Footgun warning + +If a sub-helmfile references a repository that the parent declares but the child +does not (and `repositories` is not inherited), helmfile prints a warning suggesting +`inherits: [repositories]` instead of failing later with a confusing +`repo not found` (see [#1495](https://github.com/helmfile/helmfile/issues/1495)). + diff --git a/pkg/app/app.go b/pkg/app/app.go index cd7fb98b..655023b3 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1111,6 +1111,14 @@ func (a *App) processStateFileParallel(relPath string, defOpts LoadOpts, converg // which is used to update the caller's noMatchInHelmfiles tracking. 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 + // Parent repo names are constant across sub-helmfiles; compute once for the + // "did you mean inherits: [repositories]?" footgun warning. These come from + // st.Repositories — the parent's *effective* set, which includes repositories + // brought in via bases: or inherits:, not just those declared inline here. + parentRepoNames := make([]string, 0, len(st.Repositories)) + for _, r := range st.Repositories { + parentRepoNames = append(parentRepoNames, r.Name) + } 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) @@ -1129,6 +1137,17 @@ func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, def optsForNestedState.Selectors = m.Selectors } + // Carry parent config requested via `inherits:`, and parent repo names + // for the warning, down to the sub-helmfile load path. + optsForNestedState.ParentRepoNames = parentRepoNames + if len(m.Inherits) > 0 { + inherited, err := st.BuildInheritedConfig(m.Inherits) + if err != nil { + return anyMatched, appError(fmt.Sprintf("in .helmfiles[%d]", i), err) + } + optsForNestedState.Inherited = inherited + } + if err := a.visitStatesWithContext(m.Path, optsForNestedState, converge, sharedCtx); err != nil { switch err.(type) { case *NoMatchingHelmfileError: diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index b57a029b..8f7b251d 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -101,11 +101,34 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e file = filepath.Base(f) } - st, err := ld.loadFileWithOverrides(nil, overrodeEnv, dir, file, true) + // environments inheritance must be injected pre-load: environment values are + // baked into RenderedValues during ParseAndLoad (see create.go), so merging + // them post-load would leave stale values. Passing the parent's resolved + // env as ctxEnv makes the parent's values the base, which the child's own + // environments: block then overrides per key (create.go loadEnvValues). + var inheritedEnv *environment.Environment + if opts.Inherited != nil { + inheritedEnv = opts.Inherited.Env + } + + st, err := ld.loadFileWithOverrides(inheritedEnv, overrodeEnv, dir, file, true) if err != nil { return nil, err } + // Apply inherited parent config for the 6 pure fields (environments was + // handled above via inheritedEnv). Child wins; parent fills gaps. + if opts.Inherited != nil { + if err := st.MergeInherited(opts.Inherited); err != nil { + return nil, err + } + } + + // Footgun guard for issue #1495: a release references a repository the + // parent declares but the child lacks (and did not inherit). Suggests + // `inherits: [repositories]`. No-op when ParentRepoNames is empty. + st.WarnUninheritedRepos(opts.ParentRepoNames, ld.logger) + if opts.Reverse { st.Reverse() } diff --git a/pkg/app/inherits_e2e_test.go b/pkg/app/inherits_e2e_test.go new file mode 100644 index 00000000..ff897e33 --- /dev/null +++ b/pkg/app/inherits_e2e_test.go @@ -0,0 +1,221 @@ +package app + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/helmfile/helmfile/pkg/state" + "github.com/helmfile/helmfile/pkg/testhelper" +) + +// runSubhelmfileInheritsTest loads a parent + sub-helmfile via ForEachState and +// returns each visited state keyed by its FilePath. +func runSubhelmfileInheritsTest(t *testing.T, files map[string]string, app *App) map[string]*state.HelmState { + t.Helper() + fs := testhelper.NewTestFs(files) + app = injectFs(app, fs) + expectNoCallsToHelm(app) + + states := map[string]*state.HelmState{} + noop := func(run *Run) (bool, []error) { + states[run.state.FilePath] = run.state + return false, []error{} + } + require.NoError(t, app.ForEachState(noop, false, SetFilter(true))) + return states +} + +// TestSubhelmfileInherits_Repositories is the regression test for issue #1495: +// a repository declared in the parent helmfile is available to the sub-helmfile +// when it opts in via `inherits: [repositories]`. +func TestSubhelmfileInherits_Repositories(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +repositories: +- name: release-charts + url: registry.example.com/release/helm-charts + oci: true +helmfiles: +- path: myapp.yaml + inherits: + - repositories +`, + "/path/to/myapp.yaml": ` +releases: +- name: myapp + chart: release-charts/myapp + namespace: myns +`, + } + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: "helmfile.yaml", + Logger: newAppTestLogger(), + } + states := runSubhelmfileInheritsTest(t, files, app) + + child, ok := states["myapp.yaml"] + require.True(t, ok, "sub-helmfile state should be visited") + require.Len(t, child.Repositories, 1, "child should have inherited the parent repository") + assert.Equal(t, "release-charts", child.Repositories[0].Name) + assert.True(t, child.Repositories[0].OCI) +} + +// TestSubhelmfile_NoInherit_DoesNotGetRepositories verifies the opt-in nature: +// without `inherits:`, the sub-helmfile does NOT receive the parent's repository +// (the historical behavior that #1495 reported), preserving backward compat. +func TestSubhelmfile_NoInherit_DoesNotGetRepositories(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +repositories: +- name: release-charts + url: registry.example.com/release/helm-charts + oci: true +helmfiles: +- path: myapp.yaml +`, + "/path/to/myapp.yaml": ` +releases: +- name: myapp + chart: release-charts/myapp +`, + } + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: "helmfile.yaml", + Logger: newAppTestLogger(), + } + states := runSubhelmfileInheritsTest(t, files, app) + + child, ok := states["myapp.yaml"] + require.True(t, ok) + assert.Empty(t, child.Repositories, "without inherits:, child must NOT receive parent repos (opt-in)") +} + +// TestSubhelmfile_WarnsWhenRepoNotInherited verifies the footgun warning fires +// in the real load flow when a release references a repo the parent has but the +// child lacks (and did not inherit). +func TestSubhelmfile_WarnsWhenRepoNotInherited(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +repositories: +- name: release-charts + url: registry.example.com/release/helm-charts + oci: true +helmfiles: +- path: myapp.yaml +`, + "/path/to/myapp.yaml": ` +releases: +- name: myapp + chart: release-charts/myapp +`, + } + core, recorded := observer.New(zapcore.WarnLevel) + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: "helmfile.yaml", + Logger: zap.New(core).Sugar(), + } + runSubhelmfileInheritsTest(t, files, app) + + var matched int + for _, e := range recorded.All() { + if msg := e.Message; strings.Contains(msg, "release-charts") && strings.Contains(msg, "inherits") { + matched++ + } + } + assert.GreaterOrEqual(t, matched, 1, "expected a footgun warning mentioning release-charts and inherits") +} + +// TestSubhelmfile_NoWarnWhenRepoInherited verifies the warning is naturally +// suppressed when the repo is inherited (it becomes part of the child's repos). +func TestSubhelmfile_NoWarnWhenRepoInherited(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +repositories: +- name: release-charts + url: registry.example.com/release/helm-charts + oci: true +helmfiles: +- path: myapp.yaml + inherits: + - repositories +`, + "/path/to/myapp.yaml": ` +releases: +- name: myapp + chart: release-charts/myapp +`, + } + core, recorded := observer.New(zapcore.WarnLevel) + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: "helmfile.yaml", + Logger: zap.New(core).Sugar(), + } + runSubhelmfileInheritsTest(t, files, app) + + for _, e := range recorded.All() { + assert.False(t, strings.Contains(e.Message, "not inherited"), + "no footgun warning expected when repositories are inherited, got: %s", e.Message) + } +} + +// TestSubhelmfileInherits_Environments verifies that a sub-helmfile opting into +// `inherits: [environments]` receives the parent's resolved environment values +// (FOO from env.yaml flows down). +func TestSubhelmfileInherits_Environments(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +environments: + default: + values: + - env.yaml +helmfiles: +- path: myapp.yaml + inherits: + - environments +`, + "/path/to/env.yaml": `FOO: from-parent +`, + "/path/to/myapp.yaml": ` +releases: +- name: myapp + chart: release-charts/myapp +`, + } + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: "helmfile.yaml", + Logger: newAppTestLogger(), + } + states := runSubhelmfileInheritsTest(t, files, app) + + child, ok := states["myapp.yaml"] + require.True(t, ok, "sub-helmfile state should be visited") + require.NotNil(t, child.RenderedValues, "child should have rendered values") + assert.Equal(t, "from-parent", child.RenderedValues["FOO"], + "inherited environments should make the parent's resolved FOO value visible to the child") +} diff --git a/pkg/app/load_opts.go b/pkg/app/load_opts.go index 4a9772ef..0a0edef4 100644 --- a/pkg/app/load_opts.go +++ b/pkg/app/load_opts.go @@ -17,6 +17,16 @@ type LoadOpts struct { Reverse bool Filter bool + + // Inherited carries parent-helmfile config that this sub-helmfile opts into + // via `inherits:`. See state.InheritedConfig and state.MergeInherited. + Inherited *state.InheritedConfig + + // ParentRepoNames is the parent's effective repository set (derived from + // st.Repositories, so it includes repositories brought in via bases: or + // inherits:). It is used by the "did you mean inherits: [repositories]?" + // footgun warning (state.WarnUninheritedRepos). + ParentRepoNames []string `yaml:"parentRepoNames,omitempty"` } func (o LoadOpts) DeepCopy() LoadOpts { @@ -42,5 +52,17 @@ func (o LoadOpts) DeepCopy() LoadOpts { new.Environment.OverrideCLISetValues = dst } + // InheritedConfig.Env is tagged yaml:"-" so it does not survive the + // marshal/unmarshal round-trip above. Deep-copy it explicitly (mirroring + // the OverrideCLISetValues handling) so sub-helmfile processing never + // shares the parent's environment maps by reference. + if o.Inherited != nil && o.Inherited.Env != nil { + e := o.Inherited.Env.DeepCopy() + if new.Inherited == nil { + new.Inherited = &state.InheritedConfig{} + } + new.Inherited.Env = &e + } + return new } diff --git a/pkg/app/load_opts_test.go b/pkg/app/load_opts_test.go index a4eea50b..f38941b8 100644 --- a/pkg/app/load_opts_test.go +++ b/pkg/app/load_opts_test.go @@ -4,6 +4,9 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/helmfile/helmfile/pkg/environment" + "github.com/helmfile/helmfile/pkg/state" ) // TestLoadOptsDeepCopy tests the DeepCopy function for LoadOpts struct. @@ -51,3 +54,57 @@ func TestLoadOptsDeepCopyOverrideCLISetValuesIsNotShallow(t *testing.T) { require.Equal(t, "original", lOld.Environment.OverrideCLISetValues[0].(map[string]any)["key"], "mutating the copy's OverrideCLISetValues map must not affect the original (aliasing bug)") } + +// TestLoadOptsDeepCopyPreservesInheritedPureFields verifies DeepCopy preserves +// the yaml-tagged fields of Inherited (repositories etc.) via the yaml round-trip. +func TestLoadOptsDeepCopyPreservesInheritedPureFields(t *testing.T) { + lOld := LoadOpts{CalleePath: "test"} + lOld.Inherited = &state.InheritedConfig{ + Repositories: []state.RepositorySpec{{Name: "a", URL: "u"}}, + } + + lNew := lOld.DeepCopy() + + require.NotNil(t, lNew.Inherited) + require.Equal(t, lOld.Inherited.Repositories, lNew.Inherited.Repositories, + "DeepCopy should preserve Inherited.Repositories") +} + +// TestLoadOptsDeepCopyPreservesInheritedEnv verifies DeepCopy preserves +// Inherited.Env, which is tagged yaml:"-" and therefore needs explicit handling. +func TestLoadOptsDeepCopyPreservesInheritedEnv(t *testing.T) { + lOld := LoadOpts{CalleePath: "test"} + env := environment.Environment{Name: "prod", Values: map[string]any{"k": "v"}} + lOld.Inherited = &state.InheritedConfig{Env: &env} + + lNew := lOld.DeepCopy() + + require.NotNil(t, lNew.Inherited, "Inherited must survive round-trip even with only Env set") + require.NotNil(t, lNew.Inherited.Env, "Env must survive DeepCopy (it is yaml:\"-\")") + require.Equal(t, "prod", lNew.Inherited.Env.Name) + require.Equal(t, "v", lNew.Inherited.Env.Values["k"]) +} + +// TestLoadOptsDeepCopyEnvIsNotShallow verifies the deep-copied Env is not +// aliased to the original. +func TestLoadOptsDeepCopyEnvIsNotShallow(t *testing.T) { + lOld := LoadOpts{CalleePath: "test"} + env := environment.Environment{Values: map[string]any{"k": "original"}} + lOld.Inherited = &state.InheritedConfig{Env: &env} + + lNew := lOld.DeepCopy() + lNew.Inherited.Env.Values["k"] = "mutated" + + require.Equal(t, "original", lOld.Inherited.Env.Values["k"], + "mutating the copy's Env must not affect the original (aliasing bug)") +} + +// TestLoadOptsDeepCopyPreservesParentRepoNames verifies the warning field +// survives DeepCopy. +func TestLoadOptsDeepCopyPreservesParentRepoNames(t *testing.T) { + lOld := LoadOpts{CalleePath: "test", ParentRepoNames: []string{"a", "b"}} + + lNew := lOld.DeepCopy() + + require.Equal(t, lOld.ParentRepoNames, lNew.ParentRepoNames) +} diff --git a/pkg/state/inherited.go b/pkg/state/inherited.go new file mode 100644 index 00000000..c77a4f87 --- /dev/null +++ b/pkg/state/inherited.go @@ -0,0 +1,276 @@ +package state + +import ( + "fmt" + "strings" + + "dario.cat/mergo" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/environment" + "github.com/helmfile/helmfile/pkg/yaml" +) + +// allowedInherits is the single source of truth for the keys accepted by the +// sub-helmfile `inherits:` field. It is unexported so external packages cannot +// mutate it at runtime; validation, docs and tests reach it via AllowedInherits +// (which returns a defensive copy) and IsValidInherit. +var allowedInherits = []string{ + "repositories", + "helmDefaults", + "commonLabels", + "apiVersions", + "kubeVersion", + "templates", + "environments", +} + +// AllowedInherits returns a defensive copy of the keys accepted by the +// sub-helmfile `inherits:` field. The returned slice is safe for callers to +// mutate without affecting validation behavior. +func AllowedInherits() []string { + out := make([]string, len(allowedInherits)) + copy(out, allowedInherits) + return out +} + +// IsValidInherit reports whether key is an allowed inherits: entry. +func IsValidInherit(key string) bool { + for _, k := range allowedInherits { + if k == key { + return true + } + } + return false +} + +// InheritedConfig carries parent-helmfile config to a sub-helmfile. Only the +// fields requested via `inherits:` are populated; the rest stay zero/nil so the +// consumer can skip them. Env is resolved pre-load (see desiredStateLoader.Load) +// because environment values are baked into RenderedValues at load time; the +// other fields are merged post-load via MergeInherited. +type InheritedConfig struct { + Repositories []RepositorySpec `yaml:"repositories,omitempty"` + HelmDefaults *HelmSpec `yaml:"helmDefaults,omitempty"` + CommonLabels map[string]string `yaml:"commonLabels,omitempty"` + ApiVersions []string `yaml:"apiVersions,omitempty"` + KubeVersion string `yaml:"kubeVersion,omitempty"` + Templates map[string]TemplateSpec `yaml:"templates,omitempty"` + Env *environment.Environment `yaml:"-"` +} + +// BuildInheritedConfig extracts the requested fields from the parent state and +// returns a fully independent copy. The pure fields are deep-copied via a YAML +// round-trip (these structs ARE the helmfile model, so the round-trip is +// lossless for the user-declared fields and avoids hand-maintained per-field +// copy logic), and Env is deep-copied via environment.DeepCopy. The returned +// config therefore never aliases the parent state's memory. +func (st *HelmState) BuildInheritedConfig(want []string) (*InheritedConfig, error) { + set := make(map[string]bool, len(want)) + for _, w := range want { + set[w] = true + } + + // src temporarily references the parent; the round-trip below decouples it. + src := &InheritedConfig{} + if set["repositories"] { + src.Repositories = st.Repositories + } + if set["helmDefaults"] { + src.HelmDefaults = &st.HelmDefaults + } + if set["commonLabels"] { + src.CommonLabels = st.CommonLabels + } + if set["apiVersions"] { + src.ApiVersions = st.ApiVersions + } + if set["kubeVersion"] { + src.KubeVersion = st.KubeVersion + } + if set["templates"] { + src.Templates = st.Templates + } + + in := &InheritedConfig{} + // Deep-copy the pure fields so the result never shares slices/maps with the + // parent. Skipped only when no pure field was requested. + if set["repositories"] || set["helmDefaults"] || set["commonLabels"] || + set["apiVersions"] || set["kubeVersion"] || set["templates"] { + b, err := yaml.Marshal(src) + if err != nil { + return nil, fmt.Errorf("marshaling inherited config for deep copy: %w", err) + } + if err := yaml.Unmarshal(b, in); err != nil { + return nil, fmt.Errorf("unmarshaling inherited config for deep copy: %w", err) + } + } + + // Env is tagged yaml:"-" so it does not survive the round-trip; deep-copy it + // explicitly via its own DeepCopy (handles nested value maps). + if set["environments"] { + e := st.Env.DeepCopy() + in.Env = &e + } + return in, nil +} + +// MergeInherited merges the 6 pure fields into the child state. Semantics: +// "child wins, parent fills gaps" — matching bases: precedence. +// +// - repositories / apiVersions: parent-first append, de-duplicated by value +// (repositories by Name, child's entry wins on conflict). +// - helmDefaults: deep struct merge via mergo without override, so the +// parent fills the child's zero-valued sub-fields and the child's non-zero +// sub-fields win. Caveat: Go value types cannot distinguish "unset" from +// zero, so a child that explicitly sets a field to its zero value (e.g. +// atomic: false to disable) will see the parent's value fill in. The +// common case — child omits helmDefaults entirely and inherits the parent's +// — works correctly. This intentionally differs from bases:, which uses +// WithOverride and would wipe the parent whenever the child's block is +// absent, making inheritance useless. +// - commonLabels / templates: per-key union, child's key wins. +// - kubeVersion: parent fills only when the child left it empty. +// +// Env (environments) is intentionally NOT handled here; it must be injected +// pre-load because RenderedValues is computed at load time. +func (st *HelmState) MergeInherited(in *InheritedConfig) error { + if in == nil { + return nil + } + + if in.Repositories != nil { + combined := append([]RepositorySpec{}, in.Repositories...) + combined = append(combined, st.Repositories...) + st.Repositories = dedupReposByName(combined) + } + + if in.HelmDefaults != nil { + if err := mergo.Merge(&st.HelmDefaults, *in.HelmDefaults); err != nil { + return fmt.Errorf("merging inherited helmDefaults: %w", err) + } + } + + if in.CommonLabels != nil { + if st.CommonLabels == nil { + st.CommonLabels = map[string]string{} + } + for k, v := range in.CommonLabels { + if _, ok := st.CommonLabels[k]; !ok { + st.CommonLabels[k] = v + } + } + } + + if in.Templates != nil { + if st.Templates == nil { + st.Templates = map[string]TemplateSpec{} + } + for k, v := range in.Templates { + if _, ok := st.Templates[k]; !ok { + st.Templates[k] = v + } + } + } + + if in.ApiVersions != nil { + combined := append([]string{}, in.ApiVersions...) + combined = append(combined, st.ApiVersions...) + st.ApiVersions = dedupStrings(combined) + } + + if in.KubeVersion != "" && st.KubeVersion == "" { + st.KubeVersion = in.KubeVersion + } + + return nil +} + +// WarnUninheritedRepos emits a one-time warning per repository that a release +// references, which is present in the parent's effective repository set +// (parentRepoNames — derived from the parent's st.Repositories, so it includes +// repositories brought in via bases: or inherits:) but missing from the child's +// effective repositories (after any inheritance merge). It is a footgun guard +// for issue #1495: when repositories are not inherited, a release chart like +// "release-charts/myapp" fails with a confusing "repo not found". The warning +// suggests `inherits: [repositories]`. +// +// When `inherits: [repositories]` is in effect, the parent's repos are already +// part of the child's st.Repositories, so the condition is naturally false and +// nothing is logged. +func (st *HelmState) WarnUninheritedRepos(parentRepoNames []string, logger *zap.SugaredLogger) { + if logger == nil || len(parentRepoNames) == 0 || len(st.Releases) == 0 { + return + } + + parentSet := make(map[string]bool, len(parentRepoNames)) + for _, n := range parentRepoNames { + parentSet[n] = true + } + childSet := make(map[string]bool, len(st.Repositories)) + for _, r := range st.Repositories { + childSet[r.Name] = true + } + + warned := map[string]bool{} + for _, rel := range st.Releases { + chart := rel.Chart + if chart == "" { + continue + } + parts := strings.SplitN(chart, "/", 2) + if len(parts) < 2 || parts[0] == "" { + continue // local path, URL, or bare chart name — not a named-repo ref + } + repo := parts[0] + // Skip schemes (oci://, https://, ...) and relative paths (./, ../): + // these are never named-repository references, so they must not match a + // parent repo name even by coincidence. + if strings.Contains(repo, ":") || repo == "." || repo == ".." { + continue + } + if parentSet[repo] && !childSet[repo] && !warned[repo] { + warned[repo] = true + logger.Warnf( + `release %q references repository %q which is available in the parent helmfile but not inherited. `+ + `Add "inherits: [repositories]" to this sub-helmfile entry, or declare the repository here.`, + rel.Name, repo, + ) + } + } +} + +// dedupReposByName de-duplicates a repository list by Name, keeping the LAST +// occurrence so a child entry overrides a parent entry with the same name. +func dedupReposByName(repos []RepositorySpec) []RepositorySpec { + if len(repos) == 0 { + return repos + } + idx := map[string]int{} + for i, r := range repos { + idx[r.Name] = i // last index wins + } + out := make([]RepositorySpec, 0, len(repos)) + for i, r := range repos { + if idx[r.Name] == i { + out = append(out, r) + } + } + return out +} + +// dedupStrings de-duplicates a string slice, preserving first-seen order. +func dedupStrings(in []string) []string { + if len(in) == 0 { + return in + } + seen := map[string]bool{} + out := make([]string, 0, len(in)) + for _, s := range in { + if !seen[s] { + seen[s] = true + out = append(out, s) + } + } + return out +} diff --git a/pkg/state/inherited_test.go b/pkg/state/inherited_test.go new file mode 100644 index 00000000..0818802f --- /dev/null +++ b/pkg/state/inherited_test.go @@ -0,0 +1,302 @@ +package state + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/helmfile/helmfile/pkg/environment" +) + +func TestBuildInheritedConfig_OnlyRequestedFields(t *testing.T) { + st := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "a"}, {Name: "b"}}, + HelmDefaults: HelmSpec{Timeout: 300, Atomic: true}, + CommonLabels: map[string]string{"team": "platform"}, + ApiVersions: []string{"v1"}, + KubeVersion: "1.30.0", + Templates: map[string]TemplateSpec{"t": {}}, + Env: environment.Environment{Name: "prod", Values: map[string]any{"k": "v"}}, + }} + + t.Run("repositories only", func(t *testing.T) { + in, err := st.BuildInheritedConfig([]string{"repositories"}) + require.NoError(t, err) + assert.Equal(t, []RepositorySpec{{Name: "a"}, {Name: "b"}}, in.Repositories) + assert.Nil(t, in.HelmDefaults) + assert.Nil(t, in.CommonLabels) + assert.Nil(t, in.Env) + }) + + t.Run("helmDefaults becomes pointer", func(t *testing.T) { + in, err := st.BuildInheritedConfig([]string{"helmDefaults"}) + require.NoError(t, err) + require.NotNil(t, in.HelmDefaults) + assert.Equal(t, 300, in.HelmDefaults.Timeout) + assert.True(t, in.HelmDefaults.Atomic) + assert.Nil(t, in.Repositories) + }) + + t.Run("environments deep-copies env", func(t *testing.T) { + in, err := st.BuildInheritedConfig([]string{"environments"}) + require.NoError(t, err) + require.NotNil(t, in.Env) + assert.Equal(t, "prod", in.Env.Name) + // mutating the copy must not affect the parent + in.Env.Values["k"] = "mutated" + assert.Equal(t, "v", st.Env.Values["k"]) + }) + + t.Run("nothing requested yields empty config", func(t *testing.T) { + in, err := st.BuildInheritedConfig(nil) + require.NoError(t, err) + require.NotNil(t, in) + assert.Nil(t, in.Repositories) + assert.Nil(t, in.HelmDefaults) + assert.Nil(t, in.Env) + }) +} + +// TestBuildInheritedConfig_PureFieldsAreDeepCopied verifies the returned config +// does not alias the parent's slices/maps — mutating the copy must not affect +// the parent state. This guards against the cross-state coupling noted in review. +func TestBuildInheritedConfig_PureFieldsAreDeepCopied(t *testing.T) { + st := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "a"}, {Name: "b"}}, + HelmDefaults: HelmSpec{Timeout: 300, Args: []string{"--parent-arg"}}, + CommonLabels: map[string]string{"team": "platform"}, + ApiVersions: []string{"v1"}, + Templates: map[string]TemplateSpec{"base": {ReleaseSpec: ReleaseSpec{Namespace: "parent-ns"}}}, + }} + in, err := st.BuildInheritedConfig([]string{ + "repositories", "helmDefaults", "commonLabels", "apiVersions", "templates", + }) + require.NoError(t, err) + + // Mutate every reference field on the copy. + in.Repositories = append(in.Repositories, RepositorySpec{Name: "c"}) + in.Repositories[0].Name = "mutated" + in.HelmDefaults.Args[0] = "--mutated" + in.CommonLabels["team"] = "mutated" + in.ApiVersions[0] = "mutated" + tb := in.Templates["base"] + tb.Namespace = "mutated" + in.Templates["base"] = tb + + // The parent must be unaffected. + assert.Equal(t, []RepositorySpec{{Name: "a"}, {Name: "b"}}, st.Repositories) + assert.Equal(t, []string{"--parent-arg"}, st.HelmDefaults.Args) + assert.Equal(t, "platform", st.CommonLabels["team"]) + assert.Equal(t, []string{"v1"}, st.ApiVersions) + assert.Equal(t, "parent-ns", st.Templates["base"].Namespace) +} + +func TestMergeInherited_NilIsNoop(t *testing.T) { + st := &HelmState{ReleaseSetSpec: ReleaseSetSpec{Repositories: []RepositorySpec{{Name: "a"}}}} + require.NoError(t, st.MergeInherited(nil)) + assert.Equal(t, []RepositorySpec{{Name: "a"}}, st.Repositories) +} + +func TestMergeInherited_RepositoriesAppendsAndDedupsChildWins(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "shared", URL: "parent-url"}, {Name: "only-parent"}}, + }} + in, err := parent.BuildInheritedConfig([]string{"repositories"}) + require.NoError(t, err) + + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "shared", URL: "child-url"}, {Name: "only-child"}}, + }} + require.NoError(t, child.MergeInherited(in)) + + names := repoNames(child.Repositories) + assert.ElementsMatch(t, []string{"shared", "only-parent", "only-child"}, names) + // child's "shared" wins over parent's + for _, r := range child.Repositories { + if r.Name == "shared" { + assert.Equal(t, "child-url", r.URL) + } + } +} + +func TestMergeInherited_HelmDefaultsParentFillsChildGaps(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + HelmDefaults: HelmSpec{Timeout: 300, Atomic: true}, + }} + in, err := parent.BuildInheritedConfig([]string{"helmDefaults"}) + require.NoError(t, err) + + t.Run("child omits helmDefaults entirely", func(t *testing.T) { + child := &HelmState{} + require.NoError(t, child.MergeInherited(in)) + assert.Equal(t, 300, child.HelmDefaults.Timeout) + assert.True(t, child.HelmDefaults.Atomic) + }) + + t.Run("child sets a non-zero field, parent fills the rest", func(t *testing.T) { + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{HelmDefaults: HelmSpec{Wait: true}}} + require.NoError(t, child.MergeInherited(in)) + assert.Equal(t, 300, child.HelmDefaults.Timeout, "parent fills child gap") + assert.True(t, child.HelmDefaults.Atomic, "parent fills child gap") + assert.True(t, child.HelmDefaults.Wait, "child non-zero field wins") + }) +} + +func TestMergeInherited_CommonLabelsUnionChildWins(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + CommonLabels: map[string]string{"team": "platform", "shared": "parent"}, + }} + in, err := parent.BuildInheritedConfig([]string{"commonLabels"}) + require.NoError(t, err) + + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + CommonLabels: map[string]string{"shared": "child", "local": "c"}, + }} + require.NoError(t, child.MergeInherited(in)) + + assert.Equal(t, "platform", child.CommonLabels["team"], "parent-only key added") + assert.Equal(t, "child", child.CommonLabels["shared"], "child wins on conflict") + assert.Equal(t, "c", child.CommonLabels["local"], "child-only key kept") +} + +func TestMergeInherited_TemplatesUnionChildWins(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Templates: map[string]TemplateSpec{"base": {ReleaseSpec: ReleaseSpec{Namespace: "a"}}, "shared": {ReleaseSpec: ReleaseSpec{Namespace: "p"}}}, + }} + in, err := parent.BuildInheritedConfig([]string{"templates"}) + require.NoError(t, err) + + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Templates: map[string]TemplateSpec{"shared": {ReleaseSpec: ReleaseSpec{Namespace: "c"}}, "local": {ReleaseSpec: ReleaseSpec{Namespace: "x"}}}, + }} + require.NoError(t, child.MergeInherited(in)) + + assert.Contains(t, child.Templates, "base", "parent-only template added") + assert.Contains(t, child.Templates, "local", "child-only template kept") + assert.Equal(t, "c", child.Templates["shared"].Namespace, "child wins on conflict") +} + +func TestMergeInherited_ApiVersionsAppendsAndDedups(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ApiVersions: []string{"v1", "v2"}}} + in, err := parent.BuildInheritedConfig([]string{"apiVersions"}) + require.NoError(t, err) + + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ApiVersions: []string{"v2", "v3"}}} + require.NoError(t, child.MergeInherited(in)) + + assert.Equal(t, []string{"v1", "v2", "v3"}, child.ApiVersions) +} + +func TestMergeInherited_KubeVersionChildWinsParentFillsGap(t *testing.T) { + t.Run("child empty inherits parent", func(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{KubeVersion: "1.30.0"}} + in, err := parent.BuildInheritedConfig([]string{"kubeVersion"}) + require.NoError(t, err) + child := &HelmState{} + require.NoError(t, child.MergeInherited(in)) + assert.Equal(t, "1.30.0", child.KubeVersion) + }) + t.Run("child set keeps its own", func(t *testing.T) { + parent := &HelmState{ReleaseSetSpec: ReleaseSetSpec{KubeVersion: "1.30.0"}} + in, err := parent.BuildInheritedConfig([]string{"kubeVersion"}) + require.NoError(t, err) + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{KubeVersion: "1.29.0"}} + require.NoError(t, child.MergeInherited(in)) + assert.Equal(t, "1.29.0", child.KubeVersion) + }) +} + +func newObservedLogger() (*zap.SugaredLogger, *observer.ObservedLogs) { + core, recorded := observer.New(zapcore.WarnLevel) + return zap.New(core).Sugar(), recorded +} + +func TestWarnUninheritedRepos_WarnsWhenParentHasRepoChildLacks(t *testing.T) { + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{{Name: "myapp", Chart: "release-charts/myapp"}}, + }} + logger, recorded := newObservedLogger() + + child.WarnUninheritedRepos([]string{"release-charts"}, logger) + + require.Len(t, recorded.All(), 1, "expected one warning") + assert.Contains(t, recorded.All()[0].Message, "release-charts") + assert.Contains(t, recorded.All()[0].Message, "inherits") +} + +func TestWarnUninheritedRepos_NoWarnWhenRepoInherited(t *testing.T) { + // child has the repo (e.g. because it was inherited and merged) -> no warn + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Repositories: []RepositorySpec{{Name: "release-charts"}}, + Releases: []ReleaseSpec{{Name: "myapp", Chart: "release-charts/myapp"}}, + }} + logger, recorded := newObservedLogger() + + child.WarnUninheritedRepos([]string{"release-charts"}, logger) + + assert.Empty(t, recorded.All()) +} + +func TestWarnUninheritedRepos_NoWarnForRepoNotInParent(t *testing.T) { + // repo absent from both -> helm will error separately, no inherit hint + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{{Name: "myapp", Chart: "other/myapp"}}, + }} + logger, recorded := newObservedLogger() + + child.WarnUninheritedRepos([]string{"release-charts"}, logger) + + assert.Empty(t, recorded.All()) +} + +func TestWarnUninheritedRepos_IgnoresLocalAndBareCharts(t *testing.T) { + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{ + {Name: "a", Chart: "./local/chart"}, + {Name: "b", Chart: "mychart"}, + {Name: "c", Chart: "oci://registry/chart"}, + {Name: "d", Chart: "https://host/charts/x"}, + {Name: "e", Chart: "../sibling/y"}, + }, + }} + logger, recorded := newObservedLogger() + + child.WarnUninheritedRepos([]string{"release-charts"}, logger) + + assert.Empty(t, recorded.All(), "local paths, bare names, oci://, https:// and ../ must not trigger") +} + +func TestWarnUninheritedRepos_WarnsOncePerRepo(t *testing.T) { + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{ + {Name: "a", Chart: "shared/x"}, + {Name: "b", Chart: "shared/y"}, + }, + }} + logger, recorded := newObservedLogger() + + child.WarnUninheritedRepos([]string{"shared"}, logger) + + assert.Len(t, recorded.All(), 1, "dedup by repo name") +} + +func TestWarnUninheritedRepos_NilLoggerAndEmptyInputsAreSafe(t *testing.T) { + child := &HelmState{ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{{Name: "a", Chart: "x/y"}}, + }} + assert.NotPanics(t, func() { child.WarnUninheritedRepos(nil, nil) }) + assert.NotPanics(t, func() { child.WarnUninheritedRepos(nil, zap.NewNop().Sugar()) }) + assert.NotPanics(t, func() { child.WarnUninheritedRepos([]string{"x"}, zap.NewNop().Sugar()) }) +} + +func repoNames(repos []RepositorySpec) []string { + out := make([]string, 0, len(repos)) + for _, r := range repos { + out = append(out, r.Name) + } + return out +} diff --git a/pkg/state/inherits_yaml_test.go b/pkg/state/inherits_yaml_test.go new file mode 100644 index 00000000..7fd386e5 --- /dev/null +++ b/pkg/state/inherits_yaml_test.go @@ -0,0 +1,99 @@ +package state + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/helmfile/helmfile/pkg/yaml" +) + +func TestSubHelmfileSpec_UnmarshalInherits(t *testing.T) { + t.Run("map form parses inherits", func(t *testing.T) { + var hf SubHelmfileSpec + require.NoError(t, yaml.Unmarshal([]byte(` +path: myapp.yaml +inherits: +- repositories +- helmDefaults +`), &hf)) + assert.Equal(t, "myapp.yaml", hf.Path) + assert.Equal(t, []string{"repositories", "helmDefaults"}, hf.Inherits) + }) + + t.Run("string shorthand leaves inherits nil", func(t *testing.T) { + var hf SubHelmfileSpec + require.NoError(t, yaml.Unmarshal([]byte(`myapp.yaml`), &hf)) + assert.Equal(t, "myapp.yaml", hf.Path) + assert.Nil(t, hf.Inherits) + }) + + t.Run("no inherits leaves it nil", func(t *testing.T) { + var hf SubHelmfileSpec + require.NoError(t, yaml.Unmarshal([]byte("path: myapp.yaml\n"), &hf)) + assert.Nil(t, hf.Inherits) + }) +} + +func TestSubHelmfileSpec_RejectsUnknownInheritsKey(t *testing.T) { + var hf SubHelmfileSpec + err := yaml.Unmarshal([]byte(` +path: myapp.yaml +inherits: +- repositories +- bunkKey +`), &hf) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid inherits entry") + assert.Contains(t, err.Error(), "bunkKey") +} + +func TestSubHelmfileSpec_RejectsInheritsWithoutPath(t *testing.T) { + var hf SubHelmfileSpec + err := yaml.Unmarshal([]byte(` +inherits: +- repositories +`), &hf) + require.Error(t, err) + assert.Contains(t, err.Error(), "found 'inherits' definition without path") +} + +func TestSubHelmfileSpec_AllAllowedKeysAccepted(t *testing.T) { + for _, key := range AllowedInherits() { + var hf SubHelmfileSpec + err := yaml.Unmarshal([]byte("path: x.yaml\ninherits:\n- "+key+"\n"), &hf) + require.NoErrorf(t, err, "key %q should be valid", key) + assert.Equal(t, []string{key}, hf.Inherits) + } +} + +func TestAllowedInherits_DefensiveCopy(t *testing.T) { + orig := AllowedInherits() + require.NotEmpty(t, orig) + + // Mutating the returned slice must not affect validation, which backs onto + // the unexported allowedInherits. + mutated := AllowedInherits() + mutated[0] = "tampered" + + // A genuinely valid key is still valid, the tampered value is not, and a fresh + // call still returns the pristine set. + assert.True(t, IsValidInherit("repositories")) + assert.False(t, IsValidInherit("tampered")) + assert.Equal(t, orig, AllowedInherits()) +} + +func TestSubHelmfileSpec_MarshalRoundTripInherits(t *testing.T) { + hf := SubHelmfileSpec{ + Path: "myapp.yaml", + Inherits: []string{"repositories", "environments"}, + } + out, err := yaml.Marshal(hf) + require.NoError(t, err) + + var got SubHelmfileSpec + require.NoError(t, yaml.Unmarshal(out, &got)) + assert.Equal(t, hf.Inherits, got.Inherits) + assert.Equal(t, hf.Path, got.Path) +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 6efc0047..5400ff74 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -189,6 +189,14 @@ type SubHelmfileSpec struct { //do the sub helmfiles inherits from parent selectors SelectorsInherited bool `yaml:"selectorsInherited,omitempty"` + // Inherits is the list of parent-helmfile config categories this + // sub-helmfile inherits. Allowed values are listed in AllowedInherits + // (repositories, helmDefaults, commonLabels, apiVersions, kubeVersion, + // templates, environments). Child values win; parent fills gaps. See + // MergeInherited. Empty (the default) preserves the historical behavior + // where sub-helmfiles are independent. + Inherits []string `yaml:"inherits,omitempty"` + Environment SubhelmfileEnvironmentSpec } @@ -5259,12 +5267,14 @@ func (p SubHelmfileSpec) MarshalYAML() (any, error) { Path string `yaml:"path,omitempty"` Selectors []string `yaml:"selectors,omitempty"` SelectorsInherited bool `yaml:"selectorsInherited,omitempty"` + Inherits []string `yaml:"inherits,omitempty"` OverrideValues []any `yaml:"values,omitempty"` } return &SubHelmfileSpecTmp{ Path: p.Path, Selectors: p.Selectors, SelectorsInherited: p.SelectorsInherited, + Inherits: p.Inherits, OverrideValues: p.Environment.OverrideValues, }, nil } @@ -5285,6 +5295,7 @@ func (hf *SubHelmfileSpec) UnmarshalYAML(unmarshal func(any) error) error { Path string `yaml:"path"` Selectors []string `yaml:"selectors"` SelectorsInherited bool `yaml:"selectorsInherited"` + Inherits []string `yaml:"inherits"` Environment SubhelmfileEnvironmentSpec `yaml:",inline"` } @@ -5294,6 +5305,7 @@ func (hf *SubHelmfileSpec) UnmarshalYAML(unmarshal func(any) error) error { hf.Path = subHelmfileSpecTmp.Path hf.Selectors = subHelmfileSpecTmp.Selectors hf.SelectorsInherited = subHelmfileSpecTmp.SelectorsInherited + hf.Inherits = subHelmfileSpecTmp.Inherits hf.Environment = subHelmfileSpecTmp.Environment } // since we cannot make sur the "console" string can be red after the "path" we must check we don't have @@ -5305,6 +5317,19 @@ func (hf *SubHelmfileSpec) UnmarshalYAML(unmarshal func(any) error) error { if hf.SelectorsInherited && len(hf.Selectors) > 0 { return fmt.Errorf("you cannot use 'SelectorsInherited: true' along with and explicit selector for path: %v", hf.Path) } + // inherits: only makes sense on a concrete sub-helmfile entry with a path; + // reject a map-form entry that sets inherits without a path, mirroring the + // selectors-without-path guard above. + if len(hf.Inherits) > 0 && hf.Path == "" { + return fmt.Errorf("found 'inherits' definition without path: %v", hf.Inherits) + } + // validate inherits: entries against the allowed set, failing fast on typos + // (an unknown key would otherwise silently do nothing) + for _, key := range hf.Inherits { + if !IsValidInherit(key) { + return fmt.Errorf("invalid inherits entry %q for path %q: allowed values are %v", key, hf.Path, AllowedInherits()) + } + } return nil } diff --git a/test/integration/run.sh b/test/integration/run.sh index 0d36e2c2..cfd1e8de 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -109,6 +109,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/helmfile-double-fetch.sh . ${dir}/test-cases/skip-diff-output.sh . ${dir}/test-cases/v1-subhelmfile-multi-bases-with-array-values.sh +. ${dir}/test-cases/inherits-subhelmfile.sh . ${dir}/test-cases/kustomized-fetch.sh . ${dir}/test-cases/issue-2503-kustomize-fetch.sh . ${dir}/test-cases/regression.sh diff --git a/test/integration/test-cases/inherits-subhelmfile.sh b/test/integration/test-cases/inherits-subhelmfile.sh new file mode 100644 index 00000000..9bd5e761 --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile.sh @@ -0,0 +1,66 @@ +inherits_input_dir="${cases_dir}/inherits-subhelmfile/input" +inherits_output_tmp=$(mktemp -d) + +# Run `helmfile template` on an input file; assert it succeeds and that the +# rendered output contains the given pattern. +expect_template_ok() { + local desc="$1" file="$2" pattern="$3" + info "${desc}" + code=0 + ${helmfile} -f ${inherits_input_dir}/${file} template &> ${inherits_output_tmp}/out.log || code=$? + if [ ${code} -ne 0 ]; then + cat ${inherits_output_tmp}/out.log + fail "${desc}: template should have succeeded, exit=${code}" + fi + grep -q "${pattern}" ${inherits_output_tmp}/out.log || { cat ${inherits_output_tmp}/out.log; fail "${desc}: expected '${pattern}' in output"; } + info "${desc}: OK" +} + +# --- Scenario 1: repositories (the issue #1495 regression) ------------------- +# Uses the remote incubator/raw chart so that real repository registration is +# exercised — the one thing only an integration test can cover. +test_start "inherits: repositories (issue #1495)" +${helm} repo remove incubator 2>/dev/null || true +expect_template_ok \ + "sub-helmfile resolves chart via repository inherited from parent" \ + "helmfile-inherits.yaml" \ + "kind: ConfigMap" +test_pass "inherits: repositories (issue #1495)" + +# --- Scenario 2: environments (resolved values flow to the sub-helmfile) ----- +# Uses the local raw chart (no repository needed) to isolate environments +# inheritance. The parent's resolved value must appear in the rendered output. +test_start "inherits: environments" +expect_template_ok \ + "sub-helmfile renders the parent's resolved environment value" \ + "helmfile-env.yaml" \ + "from-parent-env" +test_pass "inherits: environments" + +# --- Scenario 3: transitive inheritance (parent -> child -> grandchild) ------ +# The env value is inherited across two hops and must reach the grandchild's +# rendered output. Also uses the local raw chart. +test_start "inherits: transitive (parent -> child -> grandchild)" +expect_template_ok \ + "grandchild renders value transitively inherited across two levels" \ + "helmfile-transitive.yaml" \ + "from-parent-env" +test_pass "inherits: transitive (parent -> child -> grandchild)" + +# --- Scenario 4: validation (unknown key rejected at parse time) ------------- +test_start "inherits: rejects unknown key at parse time" +info "Expecting parse error for unknown inherits key" +code=0 +${helmfile} -f ${inherits_input_dir}/helmfile-bad-key.yaml template &> ${inherits_output_tmp}/bad.log || code=$? +if [ ${code} -eq 0 ]; then + cat ${inherits_output_tmp}/bad.log + fail "template should have failed for an unknown inherits key, but exited 0" +fi +grep -q "invalid inherits entry" ${inherits_output_tmp}/bad.log || { cat ${inherits_output_tmp}/bad.log; fail "expected 'invalid inherits entry' in the error"; } +grep -q "bogusKey" ${inherits_output_tmp}/bad.log || { cat ${inherits_output_tmp}/bad.log; fail "expected the offending key 'bogusKey' in the error"; } +info "unknown key rejected as expected (exit=${code})" +test_pass "inherits: rejects unknown key at parse time" + +# Cleanup so the registered repo does not leak into subsequent tests. +${helm} repo remove incubator 2>/dev/null || true +rm -rf ${inherits_output_tmp} diff --git a/test/integration/test-cases/inherits-subhelmfile/input/child-env.yaml.gotmpl b/test/integration/test-cases/inherits-subhelmfile/input/child-env.yaml.gotmpl new file mode 100644 index 00000000..902c4abd --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/child-env.yaml.gotmpl @@ -0,0 +1,18 @@ +# Sub-helmfile (templated) that consumes the parent's resolved environment +# value via .Values. This only resolves because the parent declared +# `inherits: [environments]`, which passes the parent's resolved env as the +# rendering context (see twoPassRenderTemplateToYaml). Uses the local raw chart +# so no repository or network is required. +releases: +- name: env-inherit-test + chart: ../../../charts/raw + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: env-inherit-cm + namespace: default + data: + inherited: {{ .Values.inheritedvalue }} diff --git a/test/integration/test-cases/inherits-subhelmfile/input/child-transitive.yaml b/test/integration/test-cases/inherits-subhelmfile/input/child-transitive.yaml new file mode 100644 index 00000000..771191dd --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/child-transitive.yaml @@ -0,0 +1,6 @@ +# Middle sub-helmfile: inherits environment values from the parent, then passes +# them further down to its own sub-helmfile. It has no releases of its own. +helmfiles: +- path: grandchild-transitive.yaml.gotmpl + inherits: + - environments diff --git a/test/integration/test-cases/inherits-subhelmfile/input/child.yaml b/test/integration/test-cases/inherits-subhelmfile/input/child.yaml new file mode 100644 index 00000000..d65ca9a4 --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/child.yaml @@ -0,0 +1,15 @@ +# Sub-helmfile that references a repository ("incubator") it does NOT declare. +# It only resolves when the parent passes the repository down via inherits:. +releases: +- name: inherits-test + chart: incubator/raw + version: 0.2.3 + values: + - resources: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: inherits-test-cm + namespace: default + data: + source: inherited-repository diff --git a/test/integration/test-cases/inherits-subhelmfile/input/env-values.yaml b/test/integration/test-cases/inherits-subhelmfile/input/env-values.yaml new file mode 100644 index 00000000..997320ab --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/env-values.yaml @@ -0,0 +1 @@ +inheritedvalue: from-parent-env diff --git a/test/integration/test-cases/inherits-subhelmfile/input/grandchild-transitive.yaml.gotmpl b/test/integration/test-cases/inherits-subhelmfile/input/grandchild-transitive.yaml.gotmpl new file mode 100644 index 00000000..b6fa2e3b --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/grandchild-transitive.yaml.gotmpl @@ -0,0 +1,17 @@ +# Leaf sub-helmfile (templated): consumes an environment value that was +# transitively inherited across two levels (parent -> middle -> here). Uses the +# local raw chart so no repository or network is required. +releases: +- name: transitive-test + chart: ../../../charts/raw + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: transitive-cm + namespace: default + data: + hop-count: "3" + source: {{ .Values.inheritedvalue }} diff --git a/test/integration/test-cases/inherits-subhelmfile/input/helmfile-bad-key.yaml b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-bad-key.yaml new file mode 100644 index 00000000..9e695f94 --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-bad-key.yaml @@ -0,0 +1,12 @@ +repositories: +- name: incubator + url: https://charts.helm.sh/incubator + +# An unknown inherits key must be rejected at parse time with a clear error, +# rather than silently doing nothing. The parse fails before any chart is +# fetched, so this scenario needs no network. +helmfiles: +- path: child.yaml + inherits: + - repositories + - bogusKey diff --git a/test/integration/test-cases/inherits-subhelmfile/input/helmfile-env.yaml b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-env.yaml new file mode 100644 index 00000000..89c3d48f --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-env.yaml @@ -0,0 +1,12 @@ +environments: + default: + values: + - env-values.yaml + +# The sub-helmfile inherits the parent's resolved environment values. It uses a +# LOCAL chart (no repository needed) so this scenario isolates the environments +# inheritance from the repositories scenario. +helmfiles: +- path: child-env.yaml.gotmpl + inherits: + - environments diff --git a/test/integration/test-cases/inherits-subhelmfile/input/helmfile-inherits.yaml b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-inherits.yaml new file mode 100644 index 00000000..a1368607 --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-inherits.yaml @@ -0,0 +1,11 @@ +repositories: +- name: incubator + url: https://charts.helm.sh/incubator + +# The sub-helmfile opts into inheriting the parent's repository, so the +# release in child.yaml (chart: incubator/raw) can resolve "incubator" even +# though child.yaml does not declare any repositories itself. See issue #1495. +helmfiles: +- path: child.yaml + inherits: + - repositories diff --git a/test/integration/test-cases/inherits-subhelmfile/input/helmfile-transitive.yaml b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-transitive.yaml new file mode 100644 index 00000000..3235dfd7 --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/input/helmfile-transitive.yaml @@ -0,0 +1,10 @@ +environments: + default: + values: + - env-values.yaml + +# Parent passes its resolved environment values to the middle sub-helmfile. +helmfiles: +- path: child-transitive.yaml + inherits: + - environments diff --git a/test/integration/test-cases/inherits-subhelmfile/readme b/test/integration/test-cases/inherits-subhelmfile/readme new file mode 100644 index 00000000..f1f3197e --- /dev/null +++ b/test/integration/test-cases/inherits-subhelmfile/readme @@ -0,0 +1 @@ +https://github.com/helmfile/helmfile/issues/1495