public_git
/
helmfile
mirror of https://github.com/helmfile/helmfile.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <aiopsclub@163.com>
This commit is contained in:
yxxhero 2026-04-17 21:23:22 +08:00 committed by yxxhero
parent cf542df19f
commit c532e73096
3 changed files with 165 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
pkg/filesystem/fs.go
View File

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

187
pkg/state/chart_dependencies_rewrite_test.go
View File

@ -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 {
@ -229,41 +212,26 @@ extra-field: aaa
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 +258,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 +334,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 +374,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 +392,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 +440,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 +459,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 +479,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 +528,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 +540,6 @@ dependencies:
go func() {
defer wg.Done()
// Signal that this goroutine is ready, then wait for the start signal.
readyWg.Done()
<-ready
@ -552,16 +549,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 +597,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 +616,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)
}
}

92
pkg/state/state.go
View File

@ -1409,30 +1409,23 @@ 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 copies the chart to a temporary directory and
// rewrites the copy. Each call creates a fresh temp copy reflecting the current chart contents,
// so prepare hooks or other steps that mutate the local chart directory are always honored.
// The returned cleanup function removes the temporary directory when called.
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
}
// 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 +1437,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 +1461,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 +1510,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(