From 6ccfe559b15265e682ab5fadb0074b08ec608788 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Wed, 1 Jul 2026 21:50:08 +0800 Subject: [PATCH] fix: route prebuild dep builds through the helmexec runner Address Copilot review comments on preBuildTransitiveSubchartDeps: - execHelmDep shelled out via os/exec, bypassing Helmfile's standard command wiring. Thread the helmexec.Interface (already available in prepareChartForRelease) through processChartification into the prebuild helpers and dispatch via helm.BuildDeps/helm.UpdateDeps. Drop the os/exec call and the DefaultHelmBinary re-resolution. - isLockOutOfSync now inspects the helmexec ExitError message (which embeds the combined output) instead of raw []byte output. - DoesNotRecurseIntoRemoteDeps used a real helm binary, risking network I/O to https://charts.example.com. Switch it to a stub helm whose BuildDeps returns an error so it fails fast and deterministically, and assert it visits only the chart dir (no recursion). Signed-off-by: yxxhero --- pkg/state/prebuild_subchart_deps_test.go | 84 ++++++++++++++++++++---- pkg/state/state.go | 57 ++++++---------- 2 files changed, 92 insertions(+), 49 deletions(-) diff --git a/pkg/state/prebuild_subchart_deps_test.go b/pkg/state/prebuild_subchart_deps_test.go index cfb1a245..d525e110 100644 --- a/pkg/state/prebuild_subchart_deps_test.go +++ b/pkg/state/prebuild_subchart_deps_test.go @@ -1,6 +1,8 @@ package state import ( + "context" + "errors" "os" "os/exec" "path/filepath" @@ -11,11 +13,12 @@ import ( "go.uber.org/zap" "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/helmexec" ) // skipIfNoHelm skips the calling test when the helm binary is not available on -// PATH. Tests that exercise runHelmDepBuild need helm installed to produce -// meaningful results. +// PATH. Tests that exercise runHelmDepBuild through a real helmexec need helm +// installed to produce meaningful results. func skipIfNoHelm(t *testing.T) { t.Helper() if _, err := exec.LookPath("helm"); err != nil { @@ -23,6 +26,42 @@ func skipIfNoHelm(t *testing.T) { } } +// newRealHelmExec builds a helmexec.Interface backed by the real helm binary on +// PATH, for tests that assert actual `helm dependency build` side effects. +func newRealHelmExec(t *testing.T) helmexec.Interface { + t.Helper() + logger := zap.NewNop().Sugar() + runner := &helmexec.ShellRunner{Ctx: context.Background(), Logger: logger} + helm, err := helmexec.New("helm", helmexec.HelmExecOptions{}, logger, "", "", runner) + if err != nil { + t.Fatalf("helmexec.New: %v", err) + } + return helm +} + +// stubHelm is a minimal helmexec.Interface for prebuild tests that should NOT +// invoke a real helm binary (avoids network I/O and the helm dependency). It +// records BuildDeps/UpdateDeps calls so tests can assert which charts were +// visited. Only BuildDeps/UpdateDeps are exercised by the prebuild path, so the +// embedded nil Interface satisfies the rest of the contract without being +// dereferenced. +type stubHelm struct { + helmexec.Interface + buildErr error + builds []string + updates []string +} + +func (s *stubHelm) BuildDeps(name, chart string, flags ...string) error { + s.builds = append(s.builds, chart) + return s.buildErr +} + +func (s *stubHelm) UpdateDeps(chart string) error { + s.updates = append(s.updates, chart) + return s.buildErr +} + // writeChart creates a chart directory with Chart.yaml and the given templates. // deps maps dependency name → repository URL (skipped if empty). func writeChart(t *testing.T, dir, name string, deps map[string]string, templates map[string]string) { @@ -97,7 +136,7 @@ func TestPreBuildTransitiveSubchartDeps_VisitsAllTransitiveDeps(t *testing.T) { }, } - st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "envelope")) + st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "envelope"), newRealHelmExec(t)) // The critical assertion: sub2/charts/ should now contain nested (as // .tgz or unpacked). Without the fix, helm dep build on the parent chart @@ -138,7 +177,8 @@ func hasNestedTgzPrefix(name string) bool { } // TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml verifies the -// function is a no-op (no panic) when the chart dir has no Chart.yaml. +// function is a no-op (no panic) when the chart dir has no Chart.yaml. A stub +// helm is used since the missing Chart.yaml short-circuits before any helm call. func TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml(t *testing.T) { rootDir, err := os.MkdirTemp("", "helmfile-prebuild-noyaml-") if err != nil { @@ -155,7 +195,7 @@ func TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml(t *testing.T) { } // Should not panic. - st.preBuildTransitiveSubchartDeps(rootDir) + st.preBuildTransitiveSubchartDeps(rootDir, &stubHelm{}) } // TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps verifies the function @@ -190,7 +230,7 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) { done := make(chan struct{}) go func() { defer close(done) - st.preBuildTransitiveSubchartDeps(aDir) + st.preBuildTransitiveSubchartDeps(aDir, newRealHelmExec(t)) }() select { case <-done: @@ -202,8 +242,12 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) { // TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps verifies the // function does not recurse into dependencies declared with non-file:// -// repositories (https://, oci://). It still runs helm dep build on the chart -// dir itself, but the failure to resolve the remote is logged and swallowed. +// repositories (https://, oci://). It still runs helm dep build on the chart dir +// itself, but does not follow the remote dependency. +// +// A stub helm whose BuildDeps returns an error is used so the test fails fast +// and deterministically without performing any network I/O (a real helm would +// try to fetch https://charts.example.com). func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing.T) { rootDir, err := os.MkdirTemp("", "helmfile-prebuild-remote-") if err != nil { @@ -212,9 +256,9 @@ func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing. defer os.RemoveAll(rootDir) // Chart with only a remote dep. preBuildTransitiveSubchartDeps should not - // recurse into anything (no file:// deps to follow) and should not panic - // when helm dep build can't resolve the remote dep in the test environment. - writeChart(t, filepath.Join(rootDir, "chart"), "chart", + // recurse into anything (no file:// deps to follow). + chartDir := filepath.Join(rootDir, "chart") + writeChart(t, chartDir, "chart", map[string]string{"remote": "https://charts.example.com"}, nil, ) @@ -227,6 +271,20 @@ func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing. }, } - // Should not panic; helm dep build failure is logged and swallowed. - st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "chart")) + helm := &stubHelm{buildErr: errors.New("stub: dependency build unavailable")} + st.preBuildTransitiveSubchartDeps(chartDir, helm) + + // BuildDeps must be called exactly once, on the chart dir itself — proving + // the function did not recurse into the remote dep. + if len(helm.builds) != 1 { + t.Fatalf("expected exactly one BuildDeps call (chart dir, no recursion into remote), got %v", helm.builds) + } + if got, want := filepath.Base(helm.builds[0]), "chart"; got != want { + t.Fatalf("BuildDeps called on %q, want chart dir %q", helm.builds[0], want) + } + // The stub error is not a lock-out-of-sync signal, so there must be no + // fallback to UpdateDeps. + if len(helm.updates) != 0 { + t.Fatalf("expected no UpdateDeps fallback, got %v", helm.updates) + } } diff --git a/pkg/state/state.go b/pkg/state/state.go index d1bb0983..0be917ac 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -12,7 +12,6 @@ import ( "io" "net/url" "os" - "os/exec" "path/filepath" "regexp" "runtime" @@ -1846,13 +1845,13 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (string, func(), // // Best-effort: errors are logged, not returned, so a single broken subchart // doesn't abort the entire release. -func (st *HelmState) preBuildTransitiveSubchartDeps(chartPath string) { - st.prebuildChartDeps(chartPath, make(map[string]bool)) +func (st *HelmState) preBuildTransitiveSubchartDeps(chartPath string, helm helmexec.Interface) { + st.prebuildChartDeps(chartPath, make(map[string]bool), helm) } // prebuildChartDeps is the recursive worker. It visits each file:// dependency // depth-first, then runs helm dependency build on chartDir itself. -func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool) { +func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool, helm helmexec.Interface) { absChartDir, err := filepath.Abs(chartDir) if err != nil || visited[absChartDir] { return @@ -1864,9 +1863,9 @@ func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool) return } for _, depDir := range deps { - st.prebuildChartDeps(depDir, visited) + st.prebuildChartDeps(depDir, visited, helm) } - st.runHelmDepBuild(absChartDir) + st.runHelmDepBuild(absChartDir, helm) } // parseLocalFileDeps reads chartDir/Chart.yaml and returns the absolute paths @@ -1920,48 +1919,34 @@ func (st *HelmState) resolveFileDep(repo, parentDir string) (string, bool) { // runHelmDepBuild runs `helm dependency build` on chartDir with a per-dir lock // to prevent concurrent releases from corrupting charts/*.tgz (same pattern as -// forcedDownloadChart for #768). Falls back to `helm dependency update` when -// Chart.lock is missing or stale. +// forcedDownloadChart for #768). It dispatches through the shared helmexec +// runner so the command inherits Helmfile's standard wiring (binary resolution, +// logging, flag handling, context cancellation) instead of shelling out +// directly. Falls back to `helm dependency update` when Chart.lock is missing or +// stale. // // --skip-refresh avoids a slow `helm repo update` per subchart; file:// deps // (the envelope-chart use case) don't need the repo cache. For mixed file:// + // https:// deps, chartify's own helm dep build (without --skip-refresh) runs // afterwards as a safety net. -func (st *HelmState) runHelmDepBuild(chartDir string) { +func (st *HelmState) runHelmDepBuild(chartDir string, helm helmexec.Interface) { mu := st.getNamedRWMutex("prebuild-deps:" + chartDir) mu.Lock() defer mu.Unlock() - helmBin := st.DefaultHelmBinary - if helmBin == "" { - helmBin = "helm" - } - // Try `dependency build` first (honors Chart.lock). Fall back to `update` - // (re-resolves from Chart.yaml) when the lock is missing or stale. - output, err := st.execHelmDep(helmBin, "build", chartDir) - if err != nil && isLockOutOfSync(output) { + // (re-resolves from Chart.yaml) when the lock is missing or stale. BuildDeps + // returns a helmexec.ExitError whose message embeds the combined output, so + // isLockOutOfSync can inspect err.Error() without a separate os/exec call. + if err := helm.BuildDeps(filepath.Base(chartDir), chartDir, "--skip-refresh"); err != nil && isLockOutOfSync(err.Error()) { st.logger.Debugf("runHelmDepBuild: falling back to `dependency update` for %s", chartDir) - _, _ = st.execHelmDep(helmBin, "update", chartDir) + _ = helm.UpdateDeps(chartDir) } } -// execHelmDep executes `helm dependency --skip-refresh`, -// logs the result, and returns the combined output and error. -func (st *HelmState) execHelmDep(helmBin, subcmd, chartDir string) ([]byte, error) { - output, err := exec.Command(helmBin, "dependency", subcmd, chartDir, "--skip-refresh").CombinedOutput() - if err != nil { - st.logger.Debugf("execHelmDep: helm dependency %s failed for %s: %v\n%s", subcmd, chartDir, err, string(output)) - } else { - st.logger.Debugf("execHelmDep: helm dependency %s succeeded for %s", subcmd, chartDir) - } - return output, err -} - -// isLockOutOfSync returns true when helm's output indicates Chart.lock is +// isLockOutOfSync returns true when helm's error/output indicates Chart.lock is // missing or stale — the signal to fall back from `build` to `update`. -func isLockOutOfSync(output []byte) bool { - msg := string(output) +func isLockOutOfSync(msg string) bool { return strings.Contains(msg, "out of sync") || strings.Contains(msg, "lock file is out of date") || strings.Contains(msg, "no lock file") || @@ -1972,7 +1957,7 @@ func isLockOutOfSync(output []byte) bool { // // If exists, it will also patch resources by json patches, strategic-merge patches, and injectors. // processChartification handles the chartification process -func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) { +func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, helm helmexec.Interface, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) { // Pre-build transitive local file:// subchart dependencies before chartify runs. // helm's `dependency build` on the parent chart fetches each file:// subchart and // packages it as .tgz, but does NOT recursively build the subchart's own deps. @@ -1981,7 +1966,7 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re // strategicMergePatches/jsonPatches targeting those resources fail with // "no resource matches". See https://github.com/helmfile/helmfile/issues/851. if !skipDeps && st.fs.DirectoryExistsAt(chartPath) { - st.preBuildTransitiveSubchartDeps(chartPath) + st.preBuildTransitiveSubchartDeps(chartPath, helm) } // Rewrite relative file:// dependencies in Chart.yaml to absolute paths before chartify processes them @@ -2258,7 +2243,7 @@ func (st *HelmState) prepareChartForRelease(release *ReleaseSpec, helm helmexec. if isLocal { chartPath = normalizeChart(st.basePath, chartPath) } - chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, opts, skipDeps, helmfileCommand) + chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, helm, opts, skipDeps, helmfileCommand) if err != nil { return &chartPrepareResult{err: err} }