diff --git a/pkg/app/app.go b/pkg/app/app.go index f756f61d..4e68cfa1 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -171,14 +171,15 @@ func (a *App) Diff(c DiffConfigProvider) error { includeCRDs := !c.SkipCRDs() prepErr := run.WithPreparedCharts("diff", state.ChartPrepareOptions{ - SkipRepos: c.SkipRefresh() || c.SkipDeps(), - SkipRefresh: c.SkipRefresh(), - SkipDeps: c.SkipDeps(), - SkipSchemaValidation: c.SkipSchemaValidation(), - IncludeCRDs: &includeCRDs, - Validate: c.Validate(), - Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), + SkipRepos: c.SkipRefresh() || c.SkipDeps(), + SkipRefresh: c.SkipRefresh(), + SkipDeps: c.SkipDeps(), + SkipSchemaValidation: c.SkipSchemaValidation(), + IncludeCRDs: &includeCRDs, + Validate: c.Validate(), + Concurrency: c.Concurrency(), + IncludeTransitiveNeeds: c.IncludeNeeds(), + PrefetchSharedRemoteCharts: true, }, func() []error { msg, matched, affected, errs = a.diff(run, c) return errs @@ -501,18 +502,19 @@ func (a *App) Sync(c SyncConfigProvider) error { includeCRDs := !c.SkipCRDs() prepErr := run.WithPreparedCharts("sync", state.ChartPrepareOptions{ - SkipRepos: c.SkipRefresh() || c.SkipDeps(), - SkipRefresh: c.SkipRefresh(), - SkipDeps: c.SkipDeps(), - SkipSchemaValidation: c.SkipSchemaValidation(), - Wait: c.Wait(), - WaitRetries: c.WaitRetries(), - WaitForJobs: c.WaitForJobs(), - IncludeCRDs: &includeCRDs, - IncludeTransitiveNeeds: c.IncludeNeeds(), - Validate: c.Validate(), - Concurrency: c.Concurrency(), - TemplateArgs: c.TemplateArgs(), + SkipRepos: c.SkipRefresh() || c.SkipDeps(), + SkipRefresh: c.SkipRefresh(), + SkipDeps: c.SkipDeps(), + SkipSchemaValidation: c.SkipSchemaValidation(), + Wait: c.Wait(), + WaitRetries: c.WaitRetries(), + WaitForJobs: c.WaitForJobs(), + IncludeCRDs: &includeCRDs, + IncludeTransitiveNeeds: c.IncludeNeeds(), + Validate: c.Validate(), + Concurrency: c.Concurrency(), + TemplateArgs: c.TemplateArgs(), + PrefetchSharedRemoteCharts: true, }, func() []error { matched, updated, es := a.SyncState(run, c) @@ -558,19 +560,20 @@ func (a *App) Apply(c ApplyConfigProvider) error { includeCRDs := !c.SkipCRDs() prepErr := run.WithPreparedCharts("apply", state.ChartPrepareOptions{ - SkipRepos: c.SkipRefresh() || c.SkipDeps(), - SkipRefresh: c.SkipRefresh(), - SkipDeps: c.SkipDeps(), - SkipSchemaValidation: c.SkipSchemaValidation(), - Wait: c.Wait(), - WaitRetries: c.WaitRetries(), - WaitForJobs: c.WaitForJobs(), - IncludeCRDs: &includeCRDs, - SkipCleanup: c.SkipCleanup(), - Validate: c.Validate(), - Concurrency: c.Concurrency(), - IncludeTransitiveNeeds: c.IncludeNeeds(), - TemplateArgs: c.TemplateArgs(), + SkipRepos: c.SkipRefresh() || c.SkipDeps(), + SkipRefresh: c.SkipRefresh(), + SkipDeps: c.SkipDeps(), + SkipSchemaValidation: c.SkipSchemaValidation(), + Wait: c.Wait(), + WaitRetries: c.WaitRetries(), + WaitForJobs: c.WaitForJobs(), + IncludeCRDs: &includeCRDs, + SkipCleanup: c.SkipCleanup(), + Validate: c.Validate(), + Concurrency: c.Concurrency(), + IncludeTransitiveNeeds: c.IncludeNeeds(), + TemplateArgs: c.TemplateArgs(), + PrefetchSharedRemoteCharts: true, }, func() []error { matched, updated, es := a.apply(run, c) diff --git a/pkg/app/issue_2741_test.go b/pkg/app/issue_2741_test.go new file mode 100644 index 00000000..0e4de425 --- /dev/null +++ b/pkg/app/issue_2741_test.go @@ -0,0 +1,137 @@ +package app + +import ( + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + + "github.com/helmfile/vals" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/exectest" + ffs "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/helmexec" +) + +// prefetchTrackingHelm wraps exectest.Helm to verify the real production +// wiring behind issue #2741 (app.go's PrefetchSharedRemoteCharts:true on +// Diff/Sync/Apply -> state.PrepareCharts -> state.withChartOperationLock): +// it counts Fetch calls and records the chart argument each DiffRelease call +// actually received. +type prefetchTrackingHelm struct { + *exectest.Helm + + fetchCount atomic.Int32 + + mu sync.Mutex + diffedChart map[string]string +} + +func (m *prefetchTrackingHelm) Fetch(chart string, flags ...string) error { + untarDir := "" + for i, f := range flags { + if f == "--untardir" && i+1 < len(flags) { + untarDir = flags[i+1] + break + } + } + if untarDir != "" { + chartDir := filepath.Join(untarDir, chart) + _ = os.MkdirAll(chartDir, 0755) + _ = os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte("apiVersion: v2\nname: test\nversion: 1.0.0\n"), 0644) + } + m.fetchCount.Add(1) + return nil +} + +func (m *prefetchTrackingHelm) DiffRelease(context helmexec.HelmContext, name, chart, namespace string, suppressDiff bool, flags ...string) error { + m.mu.Lock() + defer m.mu.Unlock() + if m.diffedChart == nil { + m.diffedChart = map[string]string{} + } + m.diffedChart[name] = chart + return nil +} + +// TestDiffPrefetchesSharedRemoteChart is the end-to-end regression test for +// issue #2741, exercising the real production wiring rather than a hand +// simulation of it: app.Diff -> run.WithPreparedCharts(..., +// PrefetchSharedRemoteCharts: true) -> state.PrepareCharts -> +// withChartOperationLock -> DiffRelease. Two releases share a chart+version +// from a declared repository; without the fix they'd each hand +// "myrepo/mychart" to helm-diff untouched (safe, but serialized by +// withChartOperationLock). With the fix, the chart is fetched once and both +// releases receive the same materialized local chart path, so +// withChartOperationLock's ChartPath != "" guard lets them run concurrently. +// +// This test uses a real on-disk helmfile.yaml and the real filesystem +// (ffs.DefaultFileSystem()), not the in-memory testhelper.TestFs used by +// sibling tests in this package: the chart-download cache's fast path +// (forcedDownloadChart, see state.go) corroborates a cache hit against disk +// via st.fs.DirectoryExistsAt, and the in-memory TestFs has no way to see +// files a mocked `helm fetch` writes to real disk, which would make every +// call look like a cache miss regardless of this fix. +func TestDiffPrefetchesSharedRemoteChart(t *testing.T) { + tempDir := t.TempDir() + helmfilePath := filepath.Join(tempDir, "helmfile.yaml") + helmfileContent := []byte(` +repositories: +- name: myrepo + url: https://example.com/charts + +releases: +- name: release-a + chart: myrepo/mychart + version: 1.0.0 +- name: release-b + chart: myrepo/mychart + version: 1.0.0 +`) + require.NoError(t, os.WriteFile(helmfilePath, helmfileContent, 0644)) + + logger := zap.NewExample().Sugar() + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + require.NoError(t, err) + + helm := &prefetchTrackingHelm{Helm: &exectest.Helm{ + ChartsMutex: &sync.Mutex{}, + DiffMutex: &sync.Mutex{}, + ReleasesMutex: &sync.Mutex{}, + }} + + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + FileOrDir: helmfilePath, + Logger: logger, + fs: ffs.DefaultFileSystem(), + Set: map[string]any{}, + helms: map[helmKey]helmexec.Interface{ + createHelmKey(DefaultHelmBinary, "default"): helm, + }, + valsRuntime: valsRuntime, + } + + diffErr := app.Diff(diffConfig{ + concurrency: 5, + logger: logger, + }) + require.NoError(t, diffErr) + + require.Equal(t, int32(1), helm.fetchCount.Load(), + "myrepo/mychart shared by 2 releases must be fetched exactly once") + + require.Len(t, helm.diffedChart, 2) + chartA := helm.diffedChart["release-a"] + chartB := helm.diffedChart["release-b"] + require.NotEmpty(t, chartA) + require.NotEqual(t, "myrepo/mychart", chartA, "release-a should receive the materialized local chart path, not the bare chart reference") + require.Equal(t, chartA, chartB, "both releases sharing the chart should receive the identical materialized path") +} diff --git a/pkg/state/issue_2741_test.go b/pkg/state/issue_2741_test.go new file mode 100644 index 00000000..e6f888eb --- /dev/null +++ b/pkg/state/issue_2741_test.go @@ -0,0 +1,454 @@ +package state + +import ( + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/exectest" + "github.com/helmfile/helmfile/pkg/helmexec" +) + +const sharedChartHelmfile = ` +repositories: + - name: myrepo + url: https://example.com/charts + +releases: + - name: release-a + chart: myrepo/mychart + version: 1.0.0 + - name: release-b + chart: myrepo/mychart + version: 1.0.0 + - name: release-c + chart: myrepo/mychart + version: 1.0.0 + - name: release-d + chart: myrepo/mychart + version: 1.0.0 + - name: release-e + chart: myrepo/mychart + version: 1.0.0 +` + +const mixedVerifyFlagsHelmfile = ` +repositories: + - name: myrepo + url: https://example.com/charts + +releases: + - name: release-a + chart: myrepo/mychart + version: 1.0.0 + verify: true + - name: release-b + chart: myrepo/mychart + version: 1.0.0 + verify: false +` + +const uniformVerifyFlagsHelmfile = ` +repositories: + - name: myrepo + url: https://example.com/charts + +releases: + - name: release-a + chart: myrepo/mychart + version: 1.0.0 + verify: true + - name: release-b + chart: myrepo/mychart + version: 1.0.0 + verify: true +` + +const localStyleChartNoRepoHelmfile = ` +releases: + - name: frontend-v2 + chart: charts/frontend + - name: frontend-v3 + chart: charts/frontend +` + +const uniqueChartsHelmfile = ` +repositories: + - name: myrepo + url: https://example.com/charts + +releases: + - name: release-a + chart: myrepo/chart-a + version: 1.0.0 + - name: release-b + chart: myrepo/chart-b + version: 1.0.0 + - name: release-c + chart: myrepo/chart-c + version: 1.0.0 +` + +// TestPrepareChartsPrefetchesSharedRemoteChart verifies that when +// PrefetchSharedRemoteCharts is set, releases sharing a remote chart+version +// trigger exactly one `helm fetch`, and all of them are handed a materialized +// local chart path. This is the fix for issue #2741. +func TestPrepareChartsPrefetchesSharedRemoteChart(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(sharedChartHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + mockHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 5, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(mockHelm, tempDir, 5, "sync", opts) + require.Empty(t, errs, "PrepareCharts should not return errors") + + assert.Equal(t, int32(1), mockHelm.fetchCount.Load(), + "helm.Fetch must be called exactly once for 5 releases sharing the same chart+version") + + require.Len(t, releaseToChart, 5) + + var paths []string + for _, name := range []string{"release-a", "release-b", "release-c", "release-d", "release-e"} { + path, ok := releaseToChart[PrepareChartKey{Name: name}] + require.True(t, ok, "release %s should have a prepared chart", name) + assert.NotEqual(t, "myrepo/mychart", path, "release %s should have a materialized local chart path", name) + paths = append(paths, path) + } + for _, p := range paths[1:] { + assert.Equal(t, paths[0], p, "all releases sharing the chart should be prepared to the same local path") + } +} + +// TestPrepareChartsSkipsPrefetchForUniqueCharts verifies that +// PrefetchSharedRemoteCharts leaves charts used by only one release +// untouched: no forced download, no local ChartPath materialized. Preserves +// current behavior for the common single-release-per-chart case. +func TestPrepareChartsSkipsPrefetchForUniqueCharts(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(uniqueChartsHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + mockHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 3, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(mockHelm, tempDir, 3, "sync", opts) + require.Empty(t, errs, "PrepareCharts should not return errors") + + assert.Equal(t, int32(0), mockHelm.fetchCount.Load(), + "helm.Fetch must not be called for charts used by only one release") + + for name, chart := range map[string]string{ + "release-a": "myrepo/chart-a", + "release-b": "myrepo/chart-b", + "release-c": "myrepo/chart-c", + } { + path, ok := releaseToChart[PrepareChartKey{Name: name}] + require.True(t, ok) + assert.Equal(t, chart, path, "unique chart should be handed to helm unmodified, not materialized locally") + } +} + +// TestPrepareChartsSkipsPrefetchWhenFetchFlagsDiffer verifies that releases +// sharing a chart+version are NOT prefetched if they resolve to different +// chart acquisition flags (verify/keyring/plain-http/insecure-skip-tls-verify). +// The download cache is keyed by chart+version alone, so deduplicating a +// fetch across releases with different --verify settings would let whichever +// release's worker wins the race silently decide verification for the other. +func TestPrepareChartsSkipsPrefetchWhenFetchFlagsDiffer(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(mixedVerifyFlagsHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + mockHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 2, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(mockHelm, tempDir, 2, "sync", opts) + require.Empty(t, errs) + + assert.Equal(t, int32(0), mockHelm.fetchCount.Load(), + "releases with differing verify/keyring/TLS flags must not be deduplicated into a single fetch") + + for _, name := range []string{"release-a", "release-b"} { + path, ok := releaseToChart[PrepareChartKey{Name: name}] + require.True(t, ok) + assert.Equal(t, "myrepo/mychart", path, "chart should be handed to helm unmodified when flags differ across releases") + } +} + +// TestPrepareChartsSkipsPrefetchWhenVerifyEnabled verifies that releases +// sharing a chart+version are NOT prefetched when verify is enabled, even +// when every release agrees on identical flags. forcedDownloadChart untars +// the chart into a local directory, but flagsForUpgrade unconditionally +// re-adds --verify for the later `helm upgrade` regardless of ChartPath — +// and Helm's VerifyChart only accepts a packaged .tgz/provenance pair, not +// an unpacked directory, so upgrading a prefetched+verified chart would fail +// with a real helm binary. Confirms both that prefetch is skipped and that +// the resulting flagsForUpgrade output for the affected release still +// carries --verify unchanged, i.e. behaves exactly as it did before this +// feature existed. +func TestPrepareChartsSkipsPrefetchWhenVerifyEnabled(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(uniformVerifyFlagsHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + mockHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 2, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(mockHelm, tempDir, 2, "sync", opts) + require.Empty(t, errs) + + assert.Equal(t, int32(0), mockHelm.fetchCount.Load(), + "verify-enabled releases must not be prefetched, even with identical flags across the group") + + var release *ReleaseSpec + for i := range st.Releases { + if st.Releases[i].Name == "release-a" { + release = &st.Releases[i] + } + } + require.NotNil(t, release) + + path, ok := releaseToChart[PrepareChartKey{Name: "release-a"}] + require.True(t, ok) + assert.Equal(t, "myrepo/mychart", path, "chart should be handed to helm unmodified, not materialized locally") + assert.Empty(t, release.ChartPath, "ChartPath must stay unset so withChartOperationLock keeps serializing this chart") + + // flagsForUpgrade's own --verify emission is unconditional and untouched by + // this feature (see state.go:4052-4055) — it's driven purely by + // release.Verify/repo.Verify/HelmDefaults.Verify, not by ChartPath. Because + // prefetch was skipped above, that pre-existing logic is exactly what runs + // for this release, exactly as it did before this feature existed. + flags := st.appendVerifyFlags(nil, release) + assert.Contains(t, flags, "--verify", "verify must still reach the upgrade command exactly as before this feature existed") +} + +// TestPrepareChartsSkipsPrefetchForUnknownRepoChart verifies that a +// "dir/chart"-shaped chart string with no matching `repositories:` entry +// (the conventional way to reference a local chart, e.g. "charts/frontend") +// is never force-downloaded even when shared by multiple releases. Locks in +// the fix for the regression this caused in pkg/app's diff/sync fixtures, +// where such releases must be handed unmodified to helm/diff, not routed +// through forcedDownloadChart. +func TestPrepareChartsSkipsPrefetchForUnknownRepoChart(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(localStyleChartNoRepoHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + mockHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 2, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(mockHelm, tempDir, 2, "diff", opts) + require.Empty(t, errs) + + assert.Equal(t, int32(0), mockHelm.fetchCount.Load(), + "a chart string with no matching repository must not be treated as a remote chart") + + for _, name := range []string{"frontend-v2", "frontend-v3"} { + path, ok := releaseToChart[PrepareChartKey{Name: name}] + require.True(t, ok) + assert.Equal(t, "charts/frontend", path) + } +} + +// concurrencyTrackingHelm wraps exectest.Helm to record the maximum number of +// concurrent SyncRelease/DiffRelease calls observed. +type concurrencyTrackingHelm struct { + *exectest.Helm + + syncConcurrent atomic.Int32 + syncMax atomic.Int32 + diffConcurrent atomic.Int32 + diffMax atomic.Int32 +} + +func bumpMax(cur *atomic.Int32, max *atomic.Int32) { + c := cur.Add(1) + for { + old := max.Load() + if c <= old || max.CompareAndSwap(old, c) { + break + } + } + time.Sleep(20 * time.Millisecond) + cur.Add(-1) +} + +func (m *concurrencyTrackingHelm) SyncRelease(context helmexec.HelmContext, name, chart, namespace string, flags ...string) error { + bumpMax(&m.syncConcurrent, &m.syncMax) + return nil +} + +func (m *concurrencyTrackingHelm) DiffRelease(context helmexec.HelmContext, name, chart, namespace string, suppressDiff bool, flags ...string) error { + bumpMax(&m.diffConcurrent, &m.diffMax) + return nil +} + +// TestPrefetchedSharedChartAllowsConcurrentSyncRelease is the end-to-end +// regression test for issue #2741: it runs PrepareCharts with +// PrefetchSharedRemoteCharts on 5 releases sharing a remote chart, applies the +// resulting chart paths to each release (mirroring what +// app.Run.WithPreparedCharts does), then drives them through +// withChartOperationLock + SyncRelease exactly like the production sync path +// (see state.go's SyncRelease call sites). Without the prefetch, this would +// serialize to max concurrency 1 (see TestWithChartOperationLockSerializesSameChart); +// with it, all 5 should run concurrently. +func TestPrefetchedSharedChartAllowsConcurrentSyncRelease(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(sharedChartHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + fetchHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 5, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(fetchHelm, tempDir, 5, "sync", opts) + require.Empty(t, errs) + require.Equal(t, int32(1), fetchHelm.fetchCount.Load()) + + // Mirror app.Run.WithPreparedCharts: only set ChartPath when the prepared + // chart differs from the original chart reference. + for i := range st.Releases { + rel := &st.Releases[i] + if chart, ok := releaseToChart[PrepareChartKey{Name: rel.Name}]; ok && chart != rel.Chart { + rel.ChartPath = chart + } + } + + trackingHelm := &concurrencyTrackingHelm{Helm: &exectest.Helm{}} + + var wg sync.WaitGroup + for i := range st.Releases { + rel := &st.Releases[i] + require.NotEmpty(t, rel.ChartPath, "release %s should have a materialized ChartPath after prefetch", rel.Name) + wg.Add(1) + go func(release *ReleaseSpec) { + defer wg.Done() + _ = st.withChartOperationLock(release, release.Chart, func() error { + return trackingHelm.SyncRelease(helmexec.HelmContext{}, release.Name, release.ChartPath, release.Namespace) + }) + }(rel) + } + wg.Wait() + + assert.Equal(t, int32(5), trackingHelm.syncMax.Load(), + "prefetched shared-chart releases must run SyncRelease concurrently, not serialized") +} + +// TestPrefetchedSharedChartAllowsConcurrentDiffRelease is the DiffRelease +// analog of TestPrefetchedSharedChartAllowsConcurrentSyncRelease, mirroring +// the withChartOperationLock(release, chartPath, DiffRelease) call site in +// prepareDiffReleases. +func TestPrefetchedSharedChartAllowsConcurrentDiffRelease(t *testing.T) { + resetChartCacheForTest() + + logger := zap.NewExample().Sugar() + st, err := createFromYaml([]byte(sharedChartHelmfile), "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + tempDir := t.TempDir() + + fetchHelm := &mockFetchHelm{Helm: &exectest.Helm{Helm3: true, ChartsMutex: &sync.Mutex{}}} + + opts := ChartPrepareOptions{ + SkipResolve: true, + PrefetchSharedRemoteCharts: true, + Concurrency: 5, + OutputDirTemplate: "{{ .OutputDir }}/{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(fetchHelm, tempDir, 5, "diff", opts) + require.Empty(t, errs) + require.Equal(t, int32(1), fetchHelm.fetchCount.Load()) + + for i := range st.Releases { + rel := &st.Releases[i] + if chart, ok := releaseToChart[PrepareChartKey{Name: rel.Name}]; ok && chart != rel.Chart { + rel.ChartPath = chart + } + } + + trackingHelm := &concurrencyTrackingHelm{Helm: &exectest.Helm{}} + + var wg sync.WaitGroup + for i := range st.Releases { + rel := &st.Releases[i] + require.NotEmpty(t, rel.ChartPath) + wg.Add(1) + go func(release *ReleaseSpec) { + defer wg.Done() + _ = st.withChartOperationLock(release, release.ChartPath, func() error { + return trackingHelm.DiffRelease(helmexec.HelmContext{}, release.Name, release.ChartPath, release.Namespace, false) + }) + }(rel) + } + wg.Wait() + + assert.Equal(t, int32(5), trackingHelm.diffMax.Load(), + "prefetched shared-chart releases must run DiffRelease concurrently, not serialized") +} diff --git a/pkg/state/state.go b/pkg/state/state.go index e00f5cd9..00557081 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -15,6 +15,7 @@ import ( "path/filepath" "regexp" "runtime" + "slices" "sort" "strconv" "strings" @@ -1526,11 +1527,15 @@ func filterReleasesForBuild(releases []ReleaseSpec) []ReleaseSpec { type ChartPrepareOptions struct { ForceDownload bool - SkipRepos bool - SkipDeps bool - SkipRefresh bool - SkipResolve bool - SkipCleanup bool + // PrefetchSharedRemoteCharts forces a single materialized download for any remote chart + // (chart+version) used by more than one selected release, so those releases stop being + // serialized by withChartOperationLock during sync/diff. See issue #2741. + PrefetchSharedRemoteCharts bool + SkipRepos bool + SkipDeps bool + SkipRefresh bool + SkipResolve bool + SkipCleanup bool // SkipSchemaValidation configures chartify to pass --skip-schema-validation to helm-template run by it. SkipSchemaValidation bool // Validate configures chartify to pass --validate to helm-template run by it. @@ -2043,6 +2048,25 @@ func (st *HelmState) processLocalChart(normalizedChart, dir string, release *Rel return chartPath, nil } +// chartFetchFlags builds the `helm fetch` flags for a remote chart download, mirroring +// the flags flagsForUpgrade applies for chart acquisition (version, verify, keyring, +// TLS/plain-http) so a prefetched chart behaves identically to one helm would have +// downloaded itself during `helm upgrade`. See issue #2741. +func (st *HelmState) chartFetchFlags(release *ReleaseSpec) []string { + var flags []string + flags = st.appendChartVersionFlags(flags, release) + + // non-OCI chart should be verified here, matching flagsForUpgrade. + if !st.IsOCIChart(release.Chart) { + flags = st.appendVerifyFlags(flags, release) + flags = st.appendKeyringFlags(flags, release) + } + + flags = st.appendChartDownloadFlags(flags, release) + + return flags +} + // forcedDownloadChart handles forced chart downloads. // Locks are acquired during download and released immediately after. // A per-chart+version mutex serializes downloads within the process so that @@ -2100,8 +2124,7 @@ func (st *HelmState) forcedDownloadChart(chartName, dir string, release *Release } // Download the chart - var fetchFlags []string - fetchFlags = st.appendChartVersionFlags(fetchFlags, release) + fetchFlags := st.chartFetchFlags(release) fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath) if err := helm.Fetch(chartName, fetchFlags...); err != nil { lockResult.Release(st.logger) @@ -2258,10 +2281,78 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre releases = filterReleasesForBuild(releases) } + // Apply -chart override here, before the shared-chart grouping below reads + // release.Chart. It was previously applied later inside prepareChartForRelease, + // which meant grouping ran on each release's original chart and never noticed + // that all of them resolve to the same overridden chart. + if st.OverrideChart != "" { + for i := range releases { + releases[i].Chart = st.OverrideChart + } + } + // Initialize the chartify temp dir tracker before concurrent workers start, // so that all workers share a single tracker instance. See issue #1799. st.chartifyTempDirs = &chartifyTempDirTracker{} + // Issue #2741: releases sharing a remote chart+version are otherwise serialized by + // withChartOperationLock for the whole sync/diff, not just the download. Force a + // single materialized download per shared chart key here so release.ChartPath gets + // populated and withChartOperationLock's ChartPath != "" guard skips the lock for them. + // Charts unique to one release are left alone to preserve current behavior. + sharedChartKeys := map[ChartCacheKey]bool{} + if opts.PrefetchSharedRemoteCharts { + type prefetchStats struct { + count int + flagSigs map[string]bool + sampleFlags []string + } + stats := map[ChartCacheKey]*prefetchStats{} + for i := range releases { + release := &releases[i] + key := st.getChartCacheKey(release) + s, ok := stats[key] + if !ok { + s = &prefetchStats{flagSigs: map[string]bool{}} + stats[key] = s + } + s.count++ + flags := st.chartFetchFlags(release) + // NUL can't appear in an OS argument, so this join is injective — + // unlike a space-joined signature, it can't collide two different + // flag slices (e.g. a keyring path containing a space and a + // flag-like token) into the same string. + s.flagSigs[strings.Join(flags, "\x00")] = true + s.sampleFlags = flags + } + for key, s := range stats { + // The download cache (checkChartCache/addToChartCache) is keyed by + // chart+version alone, not by acquisition flags. If releases sharing a + // chart+version disagree on --verify/--keyring/--plain-http/ + // --insecure-skip-tls-verify/--devel, whichever release's worker wins the + // download race would silently decide those settings for the others. Only + // prefetch when every release sharing this key resolves to identical flags; + // otherwise fall back to today's per-release fetch (still correct, just not + // deduplicated). + // + // Only chart strings that resolve to a configured repository (or an OCI + // reference) are unambiguously remote. A bare "dir/chart"-shaped string with + // no matching `repositories:` entry is conventionally a local chart path + // (e.g. "charts/frontend") and must be left to the existing local-directory + // resolution, not force-fetched as if it were a registry chart. + // + // --verify is also disqualifying: forcedDownloadChart untars the chart, and + // flagsForUpgrade later re-adds --verify for the upgrade itself (it can't + // tell the chart was already verified at fetch time). Helm's VerifyChart only + // accepts a packaged .tgz/provenance pair, not an unpacked directory, so + // upgrading a prefetched chart with verify enabled would fail. Leave verified + // charts on the existing serialized remote-chart path instead. + if s.count > 1 && len(s.flagSigs) == 1 && !slices.Contains(s.sampleFlags, "--verify") && st.isPrefetchEligibleChart(key.Chart) { + sharedChartKeys[key] = true + } + } + } + var prepareChartInfoMutex sync.Mutex prepareChartInfo := make(map[PrepareChartKey]string, len(releases)) @@ -2284,7 +2375,11 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre }, func(workerIndex int) { for release := range jobQueue { - result := st.prepareChartForRelease(release, helm, dir, helmfileCommand, opts, workerIndex) + releaseOpts := opts + if sharedChartKeys[st.getChartCacheKey(release)] { + releaseOpts.ForceDownload = true + } + result := st.prepareChartForRelease(release, helm, dir, helmfileCommand, releaseOpts, workerIndex) results <- result } }, @@ -6096,6 +6191,20 @@ func (st *HelmState) IsOCIChart(chart string) bool { return repo.OCI } +// isPrefetchEligibleChart returns true if chart unambiguously refers to a +// remote source: an OCI reference, or a "repo/chart" string whose repo prefix +// matches a configured `repositories:` entry. Used by PrepareCharts to decide +// which shared chart keys are safe to force-download (see issue #2741) - +// local chart paths conventionally shaped like "dir/chart" (e.g. +// "charts/frontend") must not be mistaken for registry references. +func (st *HelmState) isPrefetchEligibleChart(chart string) bool { + if strings.HasPrefix(chart, "oci://") { + return true + } + repo, _ := st.GetRepositoryAndNameFromChartName(chart) + return repo != nil +} + // NeedsRepoUpdate returns true if there are any repositories that require `helm repo update`. // OCI repositories don't need `helm repo update` as they use `helm registry login` instead. func (st *HelmState) NeedsRepoUpdate() bool { diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index dc8d6b42..40bbd51b 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -6279,6 +6279,89 @@ func TestAppendVerifyFlags(t *testing.T) { } } +// TestChartFetchFlags verifies that chartFetchFlags (used to prefetch shared +// remote charts, see issue #2741) matches the chart-acquisition flags +// flagsForUpgrade applies for a normal (non-prefetched) remote chart: version, +// verify, keyring, and TLS/plain-http flags. +func TestChartFetchFlags(t *testing.T) { + tests := []struct { + name string + repo []RepositorySpec + helmDefaults HelmSpec + release *ReleaseSpec + expected []string + }{ + { + name: "exact version", + release: &ReleaseSpec{Chart: "myrepo/mychart", Version: "1.2.3"}, + expected: []string{"--version", "1.2.3"}, + }, + { + name: "version range", + release: &ReleaseSpec{Chart: "myrepo/mychart", Version: ">=1.0.0 <2.0.0"}, + expected: []string{"--version", ">=1.0.0 <2.0.0"}, + }, + { + name: "no version means latest", + release: &ReleaseSpec{Chart: "myrepo/mychart"}, + expected: nil, + }, + { + name: "devel release", + release: &ReleaseSpec{Chart: "myrepo/mychart", Devel: boolValue(true)}, + expected: []string{"--devel"}, + }, + { + name: "release-level verify and keyring", + release: &ReleaseSpec{Chart: "myrepo/mychart", Verify: boolValue(true), Keyring: "/keys/release.gpg"}, + expected: []string{"--verify", "--keyring", "/keys/release.gpg"}, + }, + { + name: "repo-level verify and keyring", + repo: []RepositorySpec{{Name: "myrepo", Verify: true, Keyring: "/keys/repo.gpg"}}, + release: &ReleaseSpec{ + Chart: "myrepo/mychart", + }, + expected: []string{"--verify", "--keyring", "/keys/repo.gpg"}, + }, + { + name: "helmDefaults-level verify and keyring", + helmDefaults: HelmSpec{Verify: true, Keyring: "/keys/default.gpg"}, + release: &ReleaseSpec{Chart: "myrepo/mychart"}, + expected: []string{"--verify", "--keyring", "/keys/default.gpg"}, + }, + { + name: "release-level plain http", + release: &ReleaseSpec{Chart: "myrepo/mychart", PlainHttp: true}, + expected: []string{"--plain-http"}, + }, + { + name: "release-level insecure skip tls verify", + release: &ReleaseSpec{Chart: "myrepo/mychart", InsecureSkipTLSVerify: true}, + expected: []string{"--insecure-skip-tls-verify"}, + }, + { + name: "OCI chart skips verify and keyring but keeps download flags", + repo: []RepositorySpec{{Name: "myrepo", OCI: true, Verify: true, Keyring: "/keys/repo.gpg"}}, + release: &ReleaseSpec{ + Chart: "myrepo/mychart", + PlainHttp: true, + }, + expected: []string{"--plain-http"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := &HelmState{} + st.ReleaseSetSpec.Repositories = tt.repo + st.ReleaseSetSpec.HelmDefaults = tt.helmDefaults + flags := st.chartFetchFlags(tt.release) + assert.Equal(t, tt.expected, flags) + }) + } +} + // TestHelmState_setStringFlags tests the setStringFlags method func TestHelmState_setStringFlags(t *testing.T) { tests := []struct {