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

fix: add mutex lock for concurrent rewriteChartDependencies access (#2509) 

* fix: add mutex lock for concurrent rewriteChartDependencies access

Prevent race conditions when multiple goroutines concurrently access
the same Chart.yaml by introducing a per-chart-path mutex via sync.Map.

Signed-off-by: yxxhero <yxxhero@users.noreply.github.com>
Signed-off-by: yxxhero <aiopsclub@163.com>

* fix: unlock mutex on all error paths and improve race condition test validation

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/135e06b8-99e8-42cb-859d-524b2d2b1907

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: remove duplicate mutex, reuse getNamedRWMutex, restore on write failure, add test barrier

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/f93746bf-82fa-46b1-b8f0-b34bc9aa749c

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: early unlock when unmodified, assert exact restore in race test

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/5981aea4-2799-4560-9272-315d28d4b7d7

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

* fix: improve comment wording and strengthen race test start barrier

Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/9e4d6f4f-cdc1-4e9e-bdc6-81061ebc1dcc

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

---------

Signed-off-by: yxxhero <yxxhero@users.noreply.github.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This commit is contained in:
yxxhero 2026-04-13 11:59:52 +08:00 committed by GitHub
parent eaf006a386
commit acb7ce36fc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 150 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

107
pkg/state/chart_dependencies_rewrite_test.go
View File

@ -4,11 +4,13 @@ import (
"os"
"path/filepath"
"strings"
"sync"
"testing"
"go.uber.org/zap"
"github.com/helmfile/helmfile/pkg/filesystem"
"github.com/helmfile/helmfile/pkg/yaml"
)
func TestRewriteChartDependencies(t *testing.T) {
@ -503,3 +505,108 @@ dependencies:
t.Errorf("relative path with ./ should have been converted")
}
}
func TestRewriteChartDependencies_RaceCondition(t *testing.T) {
tempDir, err := os.MkdirTemp("", "helmfile-test-")
if err != nil {
t.Fatalf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)
chartYaml := `apiVersion: v2
name: test-chart
version: 1.0.0
dependencies:
- name: dep1
repository: file://../relative-chart
version: 1.0.0
`
chartYamlPath := filepath.Join(tempDir, "Chart.yaml")
if err := os.WriteFile(chartYamlPath, []byte(chartYaml), 0644); err != nil {
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
errCh := make(chan error, numGoroutines)
ready := make(chan struct{})
for i := 0; i < numGoroutines; i++ {
wg.Add(1)
readyWg.Add(1)
go func() {
defer wg.Done()
// Signal that this goroutine is ready, then wait for the start signal.
readyWg.Done()
<-ready
logger := zap.NewNop().Sugar()
st := &HelmState{
logger: logger,
fs: filesystem.DefaultFileSystem(),
}
cleanup, err := st.rewriteChartDependencies(tempDir)
if err != nil {
errCh <- err
return
}
defer cleanup()
}()
}
// Wait until all goroutines are ready, then release them simultaneously.
readyWg.Wait()
close(ready)
wg.Wait()
close(errCh)
for err := range errCh {
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)
}
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 chartMeta ChartMeta
if err := yaml.Unmarshal(data, &chartMeta); err != nil {
t.Fatalf("Chart.yaml is not valid YAML after concurrent rewrites: %v", err)
}
if chartMeta.Name != "test-chart" {
t.Errorf("expected chart name 'test-chart', got %q", chartMeta.Name)
}
if len(chartMeta.Dependencies) != 1 {
t.Fatalf("expected 1 dependency, got %d", len(chartMeta.Dependencies))
}
if chartMeta.Dependencies[0].Name != "dep1" {
t.Errorf("expected dependency name 'dep1', got %q", 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)
}
}

78
pkg/state/state.go
View File

@ -1407,19 +1407,11 @@ type PrepareChartKey struct {
Namespace, Name, KubeContext string
}
// PrepareCharts creates temporary directories of charts.
//
// Each resulting "chart" can be one of the followings:
//
// (1) local chart
// (2) temporary local chart generated from kustomization or manifests
// (3) remote chart
//
// When running `helmfile template` on helm v2, or `helmfile lint` on both helm v2 and v3,
// PrepareCharts will download and untar charts for linting and templating.
//
// rewriteChartDependencies rewrites relative file:// dependencies in Chart.yaml to absolute paths
// to ensure they can be resolved from chartify's temporary directory
// 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) {
chartYamlPath := filepath.Join(chartPath, "Chart.yaml")
@ -1428,19 +1420,17 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error)
return 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)
if err != nil {
mu.Unlock()
return func() {}, err
}
originalContent := data
cleanup := func() {
// Restore original Chart.yaml
if err := os.WriteFile(chartYamlPath, originalContent, 0644); err != nil {
st.logger.Warnf("Failed to restore original Chart.yaml at %s: %v", chartYamlPath, err)
}
}
// Parse Chart.yaml
type ChartDependency struct {
@ -1454,8 +1444,9 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error)
}
var chartMeta ChartMeta
if err := yaml.Unmarshal(data, &chartMeta); err != nil {
return cleanup, err
if err := yaml.Unmarshal(originalContent, &chartMeta); err != nil {
mu.Unlock()
return func() {}, err
}
// Rewrite relative file:// dependencies to absolute paths
@ -1471,7 +1462,8 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error)
absPath := filepath.Join(chartPath, relPath)
absPath, err = filepath.Abs(absPath)
if err != nil {
return cleanup, fmt.Errorf("failed to resolve absolute path for dependency %s: %w", dep.Name, err)
mu.Unlock()
return 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)
@ -1481,21 +1473,37 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (func(), error)
}
}
// Write back if modified
if modified {
updatedData, err := yaml.Marshal(&chartMeta)
if err != nil {
return cleanup, fmt.Errorf("failed to marshal Chart.yaml: %w", err)
}
if err := os.WriteFile(chartYamlPath, updatedData, 0644); err != nil {
return cleanup, fmt.Errorf("failed to write Chart.yaml: %w", err)
}
st.logger.Debugf("Rewrote Chart.yaml with absolute dependency paths at %s", chartYamlPath)
// 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 cleanup, nil
updatedData, err := yaml.Marshal(&chartMeta)
if err != nil {
mu.Unlock()
return 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)
}
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)
}
mu.Unlock()
}, nil
}
// Otherwise, if a chart is not a helm chart, it will call "chartify" to turn it into a chart.