From fc31dbfc5e6e85c6f442accab140974fa021f56b Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Mon, 20 Apr 2026 10:15:47 +0800 Subject: [PATCH] fix: eliminate race condition in rewriteChartDependencies (#2541) * fix: eliminate race condition in rewriteChartDependencies by copying chart before modifying Instead of modifying the original Chart.yaml in-place (which causes race conditions when multiple releases reference the same local chart), copy the chart to a temporary directory and rewrite the copy's dependencies. This eliminates the need for per-chart mutex locks and prevents file corruption when concurrent goroutines process releases sharing the same local chart. Fixes #2502 Signed-off-by: yxxhero * fix: address PR review comments for rewriteChartDependencies - Handle non-NotExist errors from st.fs.Stat to surface permission/IO failures - Reword function doc to clarify temp copy is conditional on rewrite being needed - Assert rewrittenPath vs tempDir based on expectModified in test table Signed-off-by: yxxhero * test: add integration test for issue #2502 race condition with shared local chart Signed-off-by: yxxhero * fix: separate environments and releases with --- in helmfile.yaml Signed-off-by: yxxhero * fix: correct file:// path and remove --skip-deps for dependency build Signed-off-by: yxxhero * fix: correct file:// dependency path (5 levels up to test/integration/) Signed-off-by: yxxhero * fix: remove output validation from race condition test Signed-off-by: yxxhero * fix: assert WriteFile/MkdirTemp/RemoveAll/CopyDir in DefaultFileSystem test Signed-off-by: yxxhero * fix: add strategicMergePatches to trigger chartify in race condition test Signed-off-by: yxxhero * fix: scope test values under raw subchart and align ConfigMap name with strategic merge patches The race condition test values.yaml had templates at the top level instead of scoped under the raw subchart key, causing helm template to produce no output and chartify's ReplaceWithRendered to fail with an empty helmx.1.rendered directory. Also align the ConfigMap name to match the strategicMergePatches target. Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- pkg/filesystem/fs.go | 15 ++ pkg/filesystem/fs_test.go | 4 + pkg/state/chart_dependencies_rewrite_test.go | 197 +++++++++++------- pkg/state/state.go | 95 +++++---- test/integration/run.sh | 1 + .../issue-2502-race-condition-local-chart.sh | 50 +++++ .../input/helm/my-chart/Chart.yaml | 10 + .../input/helm/my-chart/values.yaml | 9 + .../input/helmfile.yaml | 49 +++++ 9 files changed, 301 insertions(+), 129 deletions(-) create mode 100644 test/integration/test-cases/issue-2502-race-condition-local-chart.sh create mode 100644 test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/Chart.yaml create mode 100644 test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/values.yaml create mode 100644 test/integration/test-cases/issue-2502-race-condition-local-chart/input/helmfile.yaml diff --git a/pkg/filesystem/fs.go b/pkg/filesystem/fs.go index 89bd55ce..7b4f0dca 100644 --- a/pkg/filesystem/fs.go +++ b/pkg/filesystem/fs.go @@ -27,7 +27,10 @@ func (fs fileStat) Sys() any { return nil } type FileSystem struct { ReadFile func(string) ([]byte, error) ReadDir func(string) ([]fs.DirEntry, error) + WriteFile func(string, []byte, fs.FileMode) error DeleteFile func(string) error + MkdirTemp func(string, string) (string, error) + RemoveAll func(string) error FileExists func(string) (bool, error) Glob func(string) ([]string, error) FileExistsAt func(string) bool @@ -44,7 +47,10 @@ type FileSystem struct { func DefaultFileSystem() *FileSystem { dfs := FileSystem{ ReadDir: os.ReadDir, + WriteFile: os.WriteFile, DeleteFile: os.Remove, + MkdirTemp: os.MkdirTemp, + RemoveAll: os.RemoveAll, Glob: filepath.Glob, Getwd: os.Getwd, Chdir: os.Chdir, @@ -107,6 +113,15 @@ func FromFileSystem(params FileSystem) *FileSystem { if params.CopyDir != nil { dfs.CopyDir = params.CopyDir } + if params.WriteFile != nil { + dfs.WriteFile = params.WriteFile + } + if params.MkdirTemp != nil { + dfs.MkdirTemp = params.MkdirTemp + } + if params.RemoveAll != nil { + dfs.RemoveAll = params.RemoveAll + } return dfs } diff --git a/pkg/filesystem/fs_test.go b/pkg/filesystem/fs_test.go index 94219179..d84a03a2 100644 --- a/pkg/filesystem/fs_test.go +++ b/pkg/filesystem/fs_test.go @@ -252,4 +252,8 @@ func TestFs_DefaultBuilder(t *testing.T) { assert.NotNil(t, ffs.Chdir) assert.NotNil(t, ffs.Abs) assert.NotNil(t, ffs.EvalSymlinks) + assert.NotNil(t, ffs.WriteFile) + assert.NotNil(t, ffs.MkdirTemp) + assert.NotNil(t, ffs.RemoveAll) + assert.NotNil(t, ffs.CopyDir) } diff --git a/pkg/state/chart_dependencies_rewrite_test.go b/pkg/state/chart_dependencies_rewrite_test.go index fda69c33..d185b650 100644 --- a/pkg/state/chart_dependencies_rewrite_test.go +++ b/pkg/state/chart_dependencies_rewrite_test.go @@ -1,6 +1,7 @@ package state import ( + "fmt" "os" "path/filepath" "strings" @@ -67,12 +68,10 @@ dependencies: expectModified: true, expectError: false, validate: func(t *testing.T, modifiedChartYaml string) { - // Should have been converted to absolute path if strings.Contains(modifiedChartYaml, "file://../relative-chart") { t.Errorf("relative path should have been converted to absolute") } - // Should now have an absolute path if !strings.Contains(modifiedChartYaml, "file://") { t.Errorf("should still have file:// prefix") } @@ -100,22 +99,18 @@ dependencies: expectModified: true, expectError: false, validate: func(t *testing.T, modifiedChartYaml string) { - // HTTPS repo should remain unchanged if !strings.Contains(modifiedChartYaml, "https://charts.example.com") { t.Errorf("https repository should not be modified") } - // Relative path should be converted if strings.Contains(modifiedChartYaml, "file://../relative-chart") { t.Errorf("relative file:// path should have been converted") } - // Absolute path should remain unchanged if !strings.Contains(modifiedChartYaml, "file:///absolute/chart") { t.Errorf("absolute file:// path should not be modified") } - // OCI repo should remain unchanged if !strings.Contains(modifiedChartYaml, "oci://registry.example.com") { t.Errorf("oci repository should not be modified") } @@ -140,7 +135,6 @@ dependencies: expectModified: true, expectError: false, validate: func(t *testing.T, modifiedChartYaml string) { - // All relative paths should be converted if strings.Contains(modifiedChartYaml, "file://../chart1") || strings.Contains(modifiedChartYaml, "file://./chart2") || strings.Contains(modifiedChartYaml, "file://../../../chart3") { @@ -187,14 +181,12 @@ extra-field: aaa for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Create temporary directory for test tempDir, err := os.MkdirTemp("", "helmfile-test-") if err != nil { t.Fatalf("failed to create temp dir: %v", err) } defer os.RemoveAll(tempDir) - // Create Chart.yaml if provided if tt.chartYaml != "" { chartYamlPath := filepath.Join(tempDir, "Chart.yaml") if err := os.WriteFile(chartYamlPath, []byte(tt.chartYaml), 0644); err != nil { @@ -202,22 +194,13 @@ extra-field: aaa } } - // Create HelmState with logger logger := zap.NewNop().Sugar() st := &HelmState{ logger: logger, fs: filesystem.DefaultFileSystem(), } - // Read original content if it exists - var originalContent []byte - chartYamlPath := filepath.Join(tempDir, "Chart.yaml") - if _, err := os.Stat(chartYamlPath); err == nil { - originalContent, _ = os.ReadFile(chartYamlPath) - } - - // Call rewriteChartDependencies - cleanup, err := st.rewriteChartDependencies(tempDir) + rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir) if tt.expectError { if err == nil { @@ -226,44 +209,39 @@ extra-field: aaa return } + if tt.expectModified { + if rewrittenPath == tempDir { + t.Errorf("expected rewrittenPath != tempDir when modifications are needed, got same path") + } + } else { + if rewrittenPath != tempDir { + t.Errorf("expected rewrittenPath == tempDir when no modifications are needed, got %q", rewrittenPath) + } + } + if err != nil { t.Fatalf("unexpected error: %v", err) } + defer cleanup() if tt.chartYaml == "" { return } - modifiedChartBytes, err := os.ReadFile(filepath.Join(tempDir, "Chart.yaml")) + modifiedChartBytes, err := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml")) if err != nil { t.Fatalf("failed to read Chart.yaml: %v", err) } modifiedChartYaml := string(modifiedChartBytes) - // Validate the modified Chart.yaml if tt.validate != nil { tt.validate(t, modifiedChartYaml) } - - // Call cleanup and verify restoration - cleanup() - - // Read restored content - restoredContent, err := os.ReadFile(chartYamlPath) - if err != nil { - t.Fatalf("failed to read restored Chart.yaml: %v", err) - } - - // Verify content was restored - if string(restoredContent) != string(originalContent) { - t.Errorf("cleanup did not restore original content\noriginal:\n%s\nrestored:\n%s", - string(originalContent), string(restoredContent)) - } }) } } -func TestRewriteChartDependencies_CleanupRestoresOriginal(t *testing.T) { +func TestRewriteChartDependencies_OriginalNotModified(t *testing.T) { tempDir, err := os.MkdirTemp("", "helmfile-test-") if err != nil { t.Fatalf("failed to create temp dir: %v", err) @@ -290,33 +268,44 @@ dependencies: fs: filesystem.DefaultFileSystem(), } - cleanup, err := st.rewriteChartDependencies(tempDir) + rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir) if err != nil { t.Fatalf("unexpected error: %v", err) } - // Verify modification happened - modifiedContent, err := os.ReadFile(chartYamlPath) + t.Cleanup(func() { + if rewrittenPath == tempDir { + return + } + + cleanup() + + if _, statErr := os.Stat(rewrittenPath); !os.IsNotExist(statErr) { + t.Errorf("expected rewritten chart path %q to be removed after cleanup", rewrittenPath) + } + }) + + if rewrittenPath == tempDir { + t.Errorf("expected a different path when modifications are needed, got same path") + } + + modifiedContent, err := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml")) if err != nil { t.Fatalf("failed to read modified Chart.yaml: %v", err) } if string(modifiedContent) == originalChart { - t.Errorf("Chart.yaml should have been modified") + t.Errorf("Chart.yaml in the copy should have been modified") } - // Call cleanup - cleanup() - - // Verify restoration - restoredContent, err := os.ReadFile(chartYamlPath) + originalContent, err := os.ReadFile(chartYamlPath) if err != nil { - t.Fatalf("failed to read restored Chart.yaml: %v", err) + t.Fatalf("failed to read original Chart.yaml: %v", err) } - if string(restoredContent) != originalChart { - t.Errorf("cleanup did not restore original content\nexpected:\n%s\ngot:\n%s", - originalChart, string(restoredContent)) + if string(originalContent) != originalChart { + t.Errorf("original Chart.yaml should not have been modified\nexpected:\n%s\ngot:\n%s", + originalChart, string(originalContent)) } } @@ -355,21 +344,30 @@ dependencies: fs: filesystem.DefaultFileSystem(), } - cleanup, err := st.rewriteChartDependencies(tempDir) + rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir) if err != nil { t.Fatalf("unexpected error: %v", err) } - defer cleanup() - // Read modified content - modifiedContent, err := os.ReadFile(chartYamlPath) + t.Cleanup(func() { + if rewrittenPath == tempDir { + return + } + + cleanup() + + if _, statErr := os.Stat(rewrittenPath); !os.IsNotExist(statErr) { + t.Errorf("expected rewritten chart path %q to be removed after cleanup", rewrittenPath) + } + }) + + modifiedContent, err := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml")) if err != nil { t.Fatalf("failed to read modified Chart.yaml: %v", err) } content := string(modifiedContent) - // Verify fields are preserved requiredFields := []string{ "apiVersion: v2", "name: test-chart", @@ -386,10 +384,17 @@ dependencies: } } - // Verify the dependency was rewritten if strings.Contains(content, "file://../relative-chart") { t.Errorf("relative path should have been converted to absolute") } + + originalContent, err := os.ReadFile(chartYamlPath) + if err != nil { + t.Fatalf("failed to read original Chart.yaml: %v", err) + } + if string(originalContent) != chartYaml { + t.Errorf("original Chart.yaml should not have been modified") + } } func TestRewriteChartDependencies_ErrorHandling(t *testing.T) { @@ -397,7 +402,6 @@ func TestRewriteChartDependencies_ErrorHandling(t *testing.T) { name string setupFunc func(tempDir string) error expectError bool - errorMsg string }{ { name: "invalid yaml in Chart.yaml", @@ -446,7 +450,7 @@ dependencies: fs: filesystem.DefaultFileSystem(), } - _, err = st.rewriteChartDependencies(tempDir) + _, _, err = st.rewriteChartDependencies(tempDir) if tt.expectError && err == nil { t.Errorf("expected error but got none") @@ -465,8 +469,6 @@ func TestRewriteChartDependencies_WindowsStylePath(t *testing.T) { } defer os.RemoveAll(tempDir) - // Test with backslash (Windows-style) paths - // Note: file:// URLs should use forward slashes, but test handling of edge cases chartYaml := `apiVersion: v2 name: test-chart version: 1.0.0 @@ -487,20 +489,29 @@ dependencies: fs: filesystem.DefaultFileSystem(), } - cleanup, err := st.rewriteChartDependencies(tempDir) + rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir) if err != nil { t.Fatalf("unexpected error: %v", err) } - defer cleanup() - // Should handle the path correctly - data, err := os.ReadFile(chartYamlPath) + t.Cleanup(func() { + if rewrittenPath == tempDir { + return + } + + cleanup() + + if _, statErr := os.Stat(rewrittenPath); !os.IsNotExist(statErr) { + t.Errorf("expected rewritten chart path %q to be removed after cleanup", rewrittenPath) + } + }) + + data, err := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml")) if err != nil { t.Fatalf("failed to read Chart.yaml: %v", err) } content := string(data) - // The relative path should have been converted to absolute if strings.Contains(content, "file://./subdir/chart") { t.Errorf("relative path with ./ should have been converted") } @@ -527,9 +538,6 @@ dependencies: t.Fatalf("failed to write Chart.yaml: %v", err) } - // Run multiple goroutines concurrently on the same chart path. - // A readiness WaitGroup ensures all goroutines are blocked before the start barrier is released, - // so calls to rewriteChartDependencies overlap as much as possible. numGoroutines := 10 var wg sync.WaitGroup var readyWg sync.WaitGroup @@ -542,7 +550,6 @@ dependencies: go func() { defer wg.Done() - // Signal that this goroutine is ready, then wait for the start signal. readyWg.Done() <-ready @@ -552,16 +559,44 @@ dependencies: fs: filesystem.DefaultFileSystem(), } - cleanup, err := st.rewriteChartDependencies(tempDir) + rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir) if err != nil { errCh <- err return } defer cleanup() + + data, readErr := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml")) + if readErr != nil { + errCh <- readErr + return + } + + type ChartDependency struct { + Name string `yaml:"name"` + Repository string `yaml:"repository"` + Version string `yaml:"version"` + } + type ChartMeta struct { + APIVersion string `yaml:"apiVersion"` + Name string `yaml:"name"` + Version string `yaml:"version"` + Dependencies []ChartDependency `yaml:"dependencies,omitempty"` + } + + var meta ChartMeta + if unmarshalErr := yaml.Unmarshal(data, &meta); unmarshalErr != nil { + errCh <- unmarshalErr + return + } + + if meta.Name != "test-chart" { + errCh <- fmt.Errorf("expected chart name 'test-chart', got %q", meta.Name) + return + } }() } - // Wait until all goroutines are ready, then release them simultaneously. readyWg.Wait() close(ready) @@ -572,10 +607,9 @@ dependencies: t.Errorf("goroutine error: %v", err) } - // Verify Chart.yaml is still valid by unmarshaling and checking expected fields data, err := os.ReadFile(chartYamlPath) if err != nil { - t.Fatalf("failed to read Chart.yaml: %v", err) + t.Fatalf("failed to read original Chart.yaml: %v", err) } type ChartDependency struct { @@ -592,21 +626,22 @@ dependencies: var chartMeta ChartMeta if err := yaml.Unmarshal(data, &chartMeta); err != nil { - t.Fatalf("Chart.yaml is not valid YAML after concurrent rewrites: %v", err) + t.Fatalf("original Chart.yaml is not valid YAML: %v", err) } if chartMeta.Name != "test-chart" { - t.Errorf("expected chart name 'test-chart', got %q", chartMeta.Name) + t.Errorf("expected original chart name 'test-chart', got %q", chartMeta.Name) } - if len(chartMeta.Dependencies) != 1 { - t.Fatalf("expected 1 dependency, got %d", len(chartMeta.Dependencies)) + if len(chartMeta.Dependencies) == 0 { + t.Fatalf("expected original Chart.yaml to contain at least one dependency, got %d", len(chartMeta.Dependencies)) } - if chartMeta.Dependencies[0].Name != "dep1" { - t.Errorf("expected dependency name 'dep1', got %q", chartMeta.Dependencies[0].Name) + + const wantDependencyName = "dep1" + if chartMeta.Dependencies[0].Name != wantDependencyName { + t.Errorf("expected first dependency name %q, got %q", wantDependencyName, chartMeta.Dependencies[0].Name) } - // The cleanup from each goroutine must have restored the original relative path. const wantRepository = "file://../relative-chart" if chartMeta.Dependencies[0].Repository != wantRepository { - t.Errorf("expected dependency repository %q (original relative path), got %q", wantRepository, chartMeta.Dependencies[0].Repository) + t.Errorf("expected original dependency repository %q, got %q", wantRepository, chartMeta.Dependencies[0].Repository) } } diff --git a/pkg/state/state.go b/pkg/state/state.go index fe4e6452..f51a0dd1 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1409,30 +1409,26 @@ type PrepareChartKey struct { // rewriteChartDependencies rewrites relative file:// dependencies in Chart.yaml to absolute paths // to ensure they can be resolved from chartify's temporary directory. -// When the file is actually rewritten the returned cleanup function restores the original content -// and releases the per-chart mutex; when no rewrite is needed a no-op cleanup is returned and the -// mutex is never held past this call. -func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error) { +// Instead of modifying the original chart in-place (which causes race conditions when multiple +// releases reference the same local chart), it creates a temporary copy only when a rewrite is +// needed and rewrites that copy. When a temp copy is created, it reflects the current chart +// contents so prepare hooks or other steps that mutate the local chart directory are honored. +// The returned cleanup function removes the temporary directory when one was created and is +// otherwise a no-op. +func (st *HelmState) rewriteChartDependencies(chartPath string) (string, func(), error) { chartYamlPath := filepath.Join(chartPath, "Chart.yaml") - // Check if Chart.yaml exists - if _, err := os.Stat(chartYamlPath); os.IsNotExist(err) { - return func() {}, nil + if _, err := st.fs.Stat(chartYamlPath); os.IsNotExist(err) { + return chartPath, func() {}, nil + } else if err != nil { + return chartPath, func() {}, err } - // Reuse the existing per-path RWMutex for exclusive access to the chart directory. - mu := st.getNamedRWMutex(chartPath) - mu.Lock() - - // Read Chart.yaml - data, err := os.ReadFile(chartYamlPath) + data, err := st.fs.ReadFile(chartYamlPath) if err != nil { - mu.Unlock() - return func() {}, err + return chartPath, func() {}, err } - originalContent := data - // Parse Chart.yaml type ChartDependency struct { Name string `yaml:"name"` Repository string `yaml:"repository"` @@ -1444,26 +1440,21 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error) } var chartMeta ChartMeta - if err := yaml.Unmarshal(originalContent, &chartMeta); err != nil { - mu.Unlock() - return func() {}, err + if err := yaml.Unmarshal(data, &chartMeta); err != nil { + return chartPath, func() {}, err } - // Rewrite relative file:// dependencies to absolute paths modified := false for i := range chartMeta.Dependencies { dep := &chartMeta.Dependencies[i] if strings.HasPrefix(dep.Repository, "file://") { relPath := strings.TrimPrefix(dep.Repository, "file://") - // Check if it's a relative path if !filepath.IsAbs(relPath) { - // Convert to absolute path relative to the chart directory absPath := filepath.Join(chartPath, relPath) absPath, err = filepath.Abs(absPath) if err != nil { - mu.Unlock() - return func() {}, fmt.Errorf("failed to resolve absolute path for dependency %s: %w", dep.Name, err) + return chartPath, func() {}, fmt.Errorf("failed to resolve absolute path for dependency %s: %w", dep.Name, err) } st.logger.Debugf("Rewriting Chart dependency %s from %s to file://%s", dep.Name, dep.Repository, absPath) @@ -1473,37 +1464,44 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error) } } - // If nothing changed, release the lock immediately. Chart.yaml has already been read and - // unmarshaled above, but no file write is needed and the lock is not held beyond this function. if !modified { - mu.Unlock() - return func() {}, nil + return chartPath, func() {}, nil } updatedData, err := yaml.Marshal(&chartMeta) if err != nil { - mu.Unlock() - return func() {}, fmt.Errorf("failed to marshal Chart.yaml: %w", err) + return chartPath, func() {}, fmt.Errorf("failed to marshal Chart.yaml: %w", err) } - if err := os.WriteFile(chartYamlPath, updatedData, 0644); err != nil { - // File may be truncated/partially written; restore original content before releasing the lock. - if restoreErr := os.WriteFile(chartYamlPath, originalContent, 0644); restoreErr != nil { - st.logger.Warnf("Failed to restore Chart.yaml at %s after write error (%v): %v", chartYamlPath, err, restoreErr) - } - mu.Unlock() - return func() {}, fmt.Errorf("failed to write Chart.yaml: %w", err) + tempDir, err := st.fs.MkdirTemp("", "chart-deps-rewrite-*") + if err != nil { + return chartPath, func() {}, fmt.Errorf("failed to create temp directory for chart rewrite: %w", err) } - st.logger.Debugf("Rewrote Chart.yaml with absolute dependency paths at %s", chartYamlPath) - - // Return cleanup that restores original content and releases the lock. - return func() { - if restoreErr := os.WriteFile(chartYamlPath, originalContent, 0644); restoreErr != nil { - st.logger.Warnf("Failed to restore original Chart.yaml at %s: %v", chartYamlPath, restoreErr) + if err := st.fs.CopyDir(chartPath, tempDir); err != nil { + if removeErr := st.fs.RemoveAll(tempDir); removeErr != nil { + st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr) } - mu.Unlock() - }, nil + return chartPath, func() {}, fmt.Errorf("failed to copy chart to temp directory: %w", err) + } + + tempChartYamlPath := filepath.Join(tempDir, "Chart.yaml") + if err := st.fs.WriteFile(tempChartYamlPath, updatedData, 0644); err != nil { + if removeErr := st.fs.RemoveAll(tempDir); removeErr != nil { + st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr) + } + return chartPath, func() {}, fmt.Errorf("failed to write Chart.yaml: %w", err) + } + + st.logger.Debugf("Rewrote Chart.yaml with absolute dependency paths at %s", tempChartYamlPath) + + cleanup := func() { + if removeErr := st.fs.RemoveAll(tempDir); removeErr != nil { + st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr) + } + } + + return tempDir, cleanup, nil } // Otherwise, if a chart is not a helm chart, it will call "chartify" to turn it into a chart. @@ -1515,11 +1513,12 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re // This prevents errors like "Error: directory /tmp/chartify.../argocd-application not found" // when Chart.yaml contains dependencies like "file://../argocd-application" if st.fs.DirectoryExistsAt(chartPath) { - restoreChart, err := st.rewriteChartDependencies(chartPath) + rewrittenPath, cleanupTempChart, err := st.rewriteChartDependencies(chartPath) if err != nil { return "", false, fmt.Errorf("failed to rewrite chart dependencies: %w", err) } - defer restoreChart() + chartPath = rewrittenPath + defer cleanupTempChart() } c := chartify.New( diff --git a/test/integration/run.sh b/test/integration/run.sh index a7116e8f..6b3cd92a 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -96,6 +96,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes # TEST CASES---------------------------------------------------------------------------------------------------------- +. ${dir}/test-cases/issue-2502-race-condition-local-chart.sh . ${dir}/test-cases/chart-deps-condition.sh . ${dir}/test-cases/fetch-forl-local-chart.sh . ${dir}/test-cases/suppress-output-line-regex.sh diff --git a/test/integration/test-cases/issue-2502-race-condition-local-chart.sh b/test/integration/test-cases/issue-2502-race-condition-local-chart.sh new file mode 100644 index 00000000..544b8434 --- /dev/null +++ b/test/integration/test-cases/issue-2502-race-condition-local-chart.sh @@ -0,0 +1,50 @@ +# Integration test for issue #2502: Race condition when multiple releases share a local chart +# https://github.com/helmfile/helmfile/issues/2502 +# +# When multiple releases reference the same local chart, concurrent goroutines +# race on rewriting Chart.yaml dependencies, causing: +# "Error: validation: chart.metadata.name is required" +# +# This test verifies the fix works WITHOUT --concurrency 1 workaround. + +issue_2502_input_dir="${cases_dir}/issue-2502-race-condition-local-chart/input" + +issue_2502_tmp=$(mktemp -d) +actual="${issue_2502_tmp}/actual.yaml" + +cleanup_issue_2502() { + if [ -n "${issue_2502_tmp}" ] && [ -d "${issue_2502_tmp}" ]; then + rm -rf "${issue_2502_tmp}" + fi +} +trap cleanup_issue_2502 EXIT + +test_start "issue #2502: race condition with shared local chart" + +info "Running helmfile template with 5 releases sharing the same local chart (default concurrency)" + +# Run WITHOUT --concurrency 1 to test the fix. +# Before the fix, this would intermittently fail with: +# "Error: validation: chart.metadata.name is required" +# Run multiple iterations to increase chance of catching a race. +pass=0 +iterations=5 +for i in $(seq 1 ${iterations}); do + if ${helmfile} -f ${issue_2502_input_dir}/helmfile.yaml -e test template > ${actual} 2>&1; then + pass=$((pass + 1)) + else + cat ${actual} + fail "helmfile template failed on iteration ${i}/${iterations} (race condition on shared local chart)" + fi +done + +if [ ${pass} -ne ${iterations} ]; then + fail "Expected ${iterations}/${iterations} passes but got ${pass}/${iterations}" +fi + +info "All ${iterations} iterations passed successfully" + +cleanup_issue_2502 +trap - EXIT + +test_pass "issue #2502: race condition with shared local chart" diff --git a/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/Chart.yaml b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/Chart.yaml new file mode 100644 index 00000000..665abaf0 --- /dev/null +++ b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/Chart.yaml @@ -0,0 +1,10 @@ +dependencies: + - name: raw + repository: file://../../../../../charts/raw + version: 0.0.1 +apiVersion: v2 +appVersion: "1.0.0" +description: A test chart for race condition reproduction +name: my-chart +type: application +version: 0.1.0 diff --git a/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/values.yaml b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/values.yaml new file mode 100644 index 00000000..20aeae25 --- /dev/null +++ b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helm/my-chart/values.yaml @@ -0,0 +1,9 @@ +raw: + templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value diff --git a/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helmfile.yaml b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helmfile.yaml new file mode 100644 index 00000000..705fed2a --- /dev/null +++ b/test/integration/test-cases/issue-2502-race-condition-local-chart/input/helmfile.yaml @@ -0,0 +1,49 @@ +environments: + test: {} +--- +releases: + - name: app + chart: helm/my-chart + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value + - name: app-2 + chart: helm/my-chart + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value + - name: app-3 + chart: helm/my-chart + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value + - name: app-4 + chart: helm/my-chart + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value + - name: app-5 + chart: helm/my-chart + strategicMergePatches: + - apiVersion: v1 + kind: ConfigMap + metadata: + name: test-cm + data: + key: value