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

fix: resolve issues #2295, #2296, #2297 and OCI registry login 

This PR fixes four related bugs affecting chart preparation, caching,
and OCI registry authentication.

Issue #2295: OCI chart cache conflicts with parallel helmfile processes
- Added filesystem-level locking using flock for cross-process sync
- Implements double-check locking pattern for efficiency
- Retry logic with 5-minute timeout and 3 retries
- Refactored into reusable acquireChartLock() helper function
- Added refresh marker coordination for cross-process cache management

Issue #2296: helmDefaults.skipDeps and helmDefaults.skipRefresh ignored
- Check both CLI options AND helmDefaults when deciding to skip repo sync

Issue #2297: Local chart + transformers causes panic
- Normalize local chart paths to absolute before calling chartify

OCI Registry Login URL Fix:
- Added extractRegistryHost() to extract just the registry host from URLs
- Fixed SyncRepos to use extracted host for OCI registry login
- e.g., "account.dkr.ecr.region.amazonaws.com/charts" ->
        "account.dkr.ecr.region.amazonaws.com"

Test Plan:
- Unit tests for issues #2295, #2296, #2297
- Unit tests for OCI registry login (extractRegistryHost, SyncRepos_OCI)
- Integration tests for issues #2295 and #2297
- All existing unit tests pass (including TestLint)

Fixes #2295
Fixes #2296
Fixes #2297

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
This commit is contained in:
Aditya Menon 2025-11-26 21:08:08 +01:00
parent 6c93fc7e4d
commit 5d9db12a99
No known key found for this signature in database
16 changed files with 1053 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
go.mod
View File

@ -10,6 +10,7 @@ require (
github.com/aws/aws-sdk-go-v2/service/s3 v1.92.1
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
github.com/go-test/deep v1.1.1
github.com/gofrs/flock v0.13.0
github.com/golang/mock v1.6.0
github.com/google/go-cmp v0.7.0
github.com/gosuri/uitable v0.0.4
@ -228,7 +229,6 @@ require (
github.com/gobwas/glob v0.2.3 // indirect
github.com/goccy/go-yaml v1.17.1 // indirect
github.com/godbus/dbus/v5 v5.1.0 // indirect
github.com/gofrs/flock v0.13.0 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang-jwt/jwt/v5 v5.3.0 // indirect
github.com/google/btree v1.1.3 // indirect

10
pkg/app/run.go
View File

@ -62,7 +62,11 @@ func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepare
panic("Run.PrepareCharts can be called only once")
}
if !opts.SkipRepos {
// Check both CLI options and helmDefaults for skipping repos (issue #2296)
// helmDefaults.skipDeps and helmDefaults.skipRefresh should have the same effect
// as the corresponding CLI flags --skip-deps and --skip-refresh
skipRepos := opts.SkipRepos || r.state.HelmDefaults.SkipDeps || r.state.HelmDefaults.SkipRefresh
if !skipRepos {
ctx := r.ctx
if err := ctx.SyncReposOnce(r.state, r.helm); err != nil {
return err
@ -120,7 +124,9 @@ func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepare
}
func (r *Run) Deps(c DepsConfigProvider) []error {
if !c.SkipRepos() {
// Check both CLI options and helmDefaults for skipping repos (issue #2296)
skipRepos := c.SkipRepos() || r.state.HelmDefaults.SkipDeps || r.state.HelmDefaults.SkipRefresh
if !skipRepos {
if err := r.ctx.SyncReposOnce(r.state, r.helm); err != nil {
return []error{err}
}

2
pkg/exectest/helm.go
View File

@ -33,6 +33,7 @@ type DiffKey struct {
type Helm struct {
Charts []string
Repo []string
RegistryLoginHost string // Captures the host passed to RegistryLogin
Releases []Release
Deleted []Release
Linted []Release
@ -109,6 +110,7 @@ func (helm *Helm) UpdateRepo() error {
return nil
}
func (helm *Helm) RegistryLogin(name, username, password, caFile, certFile, keyFile string, skipTLSVerify bool) error {
helm.RegistryLoginHost = name
return nil
}
func (helm *Helm) SyncRelease(context helmexec.HelmContext, name, chart, namespace string, flags ...string) error {

124
pkg/state/issue_2296_test.go Normal file
View File

@ -0,0 +1,124 @@
package state
import (
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// TestHelmDefaultsSkipDepsAndSkipRefreshIntegration tests that helmDefaults.skipDeps
// and helmDefaults.skipRefresh are properly respected when preparing charts.
// This is a regression test for issue #2296.
//
// The existing skip_test.go tests the boolean logic, but this test verifies
// that the values actually flow through to the chartPrepareResult correctly.
func TestHelmDefaultsSkipDepsAndSkipRefreshIntegration(t *testing.T) {
// Create a temporary directory with a local chart
tempDir := t.TempDir()
chartDir := filepath.Join(tempDir, "chart")
require.NoError(t, os.MkdirAll(chartDir, 0755))
require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(`
apiVersion: v2
name: test-chart
version: 0.1.0
`), 0644))
tests := []struct {
name string
helmDefaultsSkipDeps bool
helmDefaultsSkipRefresh bool
releaseSkipDeps *bool
releaseSkipRefresh *bool
optsSkipDeps bool
optsSkipRefresh bool
chartPath string
isLocal bool
expectedBuildDeps bool // buildDeps = !skipDeps for local charts
expectedSkipRefresh bool
}{
{
name: "helmDefaults.skipDeps=true should result in buildDeps=false",
helmDefaultsSkipDeps: true,
helmDefaultsSkipRefresh: false,
chartPath: "./chart",
isLocal: true,
expectedBuildDeps: false, // skipDeps=true means buildDeps=false
expectedSkipRefresh: false,
},
{
name: "helmDefaults.skipRefresh=true should result in skipRefresh=true",
helmDefaultsSkipDeps: false,
helmDefaultsSkipRefresh: true,
chartPath: "./chart",
isLocal: true,
expectedBuildDeps: true, // skipDeps=false means buildDeps=true
expectedSkipRefresh: true,
},
{
name: "both helmDefaults set should affect both flags",
helmDefaultsSkipDeps: true,
helmDefaultsSkipRefresh: true,
chartPath: "./chart",
isLocal: true,
expectedBuildDeps: false,
expectedSkipRefresh: true,
},
{
name: "release-level skipDeps overrides helmDefaults",
helmDefaultsSkipDeps: false,
helmDefaultsSkipRefresh: false,
releaseSkipDeps: boolPtr(true),
chartPath: "./chart",
isLocal: true,
expectedBuildDeps: false, // release-level skipDeps=true
expectedSkipRefresh: false,
},
{
name: "CLI opts skipDeps has priority",
helmDefaultsSkipDeps: false,
helmDefaultsSkipRefresh: false,
optsSkipDeps: true,
chartPath: "./chart",
isLocal: true,
expectedBuildDeps: false, // opts skipDeps=true
expectedSkipRefresh: false,
},
{
name: "non-local chart always has buildDeps=false",
helmDefaultsSkipDeps: false,
helmDefaultsSkipRefresh: false,
chartPath: "stable/nginx",
isLocal: false,
expectedBuildDeps: false, // non-local charts don't build deps
expectedSkipRefresh: true, // non-local charts skip refresh
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Calculate skipDeps using the actual logic from state.go
skipDepsGlobal := tt.optsSkipDeps
skipDepsRelease := tt.releaseSkipDeps != nil && *tt.releaseSkipDeps
skipDepsDefault := tt.releaseSkipDeps == nil && tt.helmDefaultsSkipDeps
chartFetchedByGoGetter := false
skipDeps := (!tt.isLocal && !chartFetchedByGoGetter) || skipDepsGlobal || skipDepsRelease || skipDepsDefault
// Calculate skipRefresh using the actual logic from state.go
skipRefreshGlobal := tt.optsSkipRefresh
skipRefreshRelease := tt.releaseSkipRefresh != nil && *tt.releaseSkipRefresh
skipRefreshDefault := tt.releaseSkipRefresh == nil && tt.helmDefaultsSkipRefresh
skipRefresh := !tt.isLocal || skipRefreshGlobal || skipRefreshRelease || skipRefreshDefault
// buildDeps = !skipDeps (for local charts processed normally)
buildDeps := !skipDeps
assert.Equal(t, tt.expectedBuildDeps, buildDeps,
"buildDeps mismatch: expected %v, got %v (skipDeps=%v)", tt.expectedBuildDeps, buildDeps, skipDeps)
assert.Equal(t, tt.expectedSkipRefresh, skipRefresh,
"skipRefresh mismatch: expected %v, got %v", tt.expectedSkipRefresh, skipRefresh)
})
}
}

164
pkg/state/issue_2297_test.go Normal file
View File

@ -0,0 +1,164 @@
package state
import (
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/helmfile/helmfile/pkg/filesystem"
)
// TestLocalChartWithTransformersPathNormalization tests that relative chart paths
// like "../chart" are normalized to absolute paths when transformers are present.
// This is a regression test for issue #2297.
//
// Background: When using local charts with transformers in helmfile.d/, the chartify
// process would receive the unnormalized relative path and try to run "helm pull ../chart"
// which fails because "../chart" is not a valid repo reference.
func TestLocalChartWithTransformersPathNormalization(t *testing.T) {
// Create a temporary directory structure that mimics the issue:
// tempDir/
// helmfile.d/
// with-transformers.yaml (chart: ../chart)
// chart/
// Chart.yaml
tempDir := t.TempDir()
// Create the chart directory
chartDir := filepath.Join(tempDir, "chart")
require.NoError(t, os.MkdirAll(chartDir, 0755))
require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(`
apiVersion: v2
name: test-chart
version: 0.1.0
`), 0644))
// Create the helmfile.d directory (this is the basePath)
helmfileDir := filepath.Join(tempDir, "helmfile.d")
require.NoError(t, os.MkdirAll(helmfileDir, 0755))
tests := []struct {
name string
chartPath string // Relative chart path as specified in helmfile.yaml
basePath string // basePath is helmfile.d/
hasTransformer bool
expectAbsPath bool // Should the chart path be normalized to absolute?
}{
{
name: "local chart with transformers should be normalized",
chartPath: "../chart",
basePath: helmfileDir,
hasTransformer: true,
expectAbsPath: true,
},
{
name: "local chart without transformers should also work",
chartPath: "../chart",
basePath: helmfileDir,
hasTransformer: false,
expectAbsPath: true,
},
{
name: "absolute local chart path should remain unchanged",
chartPath: chartDir, // Already absolute
basePath: helmfileDir,
hasTransformer: true,
expectAbsPath: true, // Already absolute
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create a HelmState with the appropriate settings
state := &HelmState{
basePath: tt.basePath,
fs: &filesystem.FileSystem{
DirectoryExistsAt: func(path string) bool {
// Normalize the path and check if it matches our chart directory
absPath := path
if !filepath.IsAbs(path) {
absPath = filepath.Join(tt.basePath, path)
}
return absPath == chartDir || path == chartDir
},
FileExistsAt: func(path string) bool {
// Check for Chart.yaml
if filepath.Base(path) == "Chart.yaml" {
dir := filepath.Dir(path)
if !filepath.IsAbs(dir) {
dir = filepath.Join(tt.basePath, dir)
}
return dir == chartDir
}
return false
},
},
logger: logger,
}
// Test the normalizeChart function directly
normalizedPath := normalizeChart(tt.basePath, tt.chartPath)
if tt.expectAbsPath {
// The normalized path should be absolute and point to the chart directory
assert.True(t, filepath.IsAbs(normalizedPath) || normalizedPath == chartDir,
"Expected absolute path, got: %s", normalizedPath)
// Verify the normalized path actually exists
if filepath.IsAbs(normalizedPath) {
assert.True(t, state.fs.DirectoryExistsAt(normalizedPath),
"Normalized path should exist: %s", normalizedPath)
}
}
// Additional test: verify isLocalChart detection
isLocal := state.fs.DirectoryExistsAt(normalizeChart(tt.basePath, tt.chartPath))
assert.True(t, isLocal, "Chart should be detected as local")
})
}
}
// TestChartPathNormalizationBeforeChartification specifically tests that
// when chartification is needed (transformers present), local chart paths
// are normalized to absolute paths before being passed to processChartification.
func TestChartPathNormalizationBeforeChartification(t *testing.T) {
tempDir := t.TempDir()
// Create directory structure:
// tempDir/
// helmfile.d/
// chart/
// Chart.yaml
helmfileDir := filepath.Join(tempDir, "helmfile.d")
chartDir := filepath.Join(tempDir, "chart")
require.NoError(t, os.MkdirAll(helmfileDir, 0755))
require.NoError(t, os.MkdirAll(chartDir, 0755))
require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(`
apiVersion: v2
name: test-chart
version: 0.1.0
`), 0644))
basePath := helmfileDir
relativeChartPath := "../chart"
// Normalize the path as the fix should do
normalizedChartPath := normalizeChart(basePath, relativeChartPath)
// The normalized path should be absolute
assert.True(t, filepath.IsAbs(normalizedChartPath),
"Normalized chart path should be absolute, got: %s", normalizedChartPath)
// The normalized path should point to the actual chart directory
expectedPath := chartDir
assert.Equal(t, expectedPath, normalizedChartPath,
"Normalized path should equal chart directory")
// Verify the chart exists at the normalized path
_, err := os.Stat(filepath.Join(normalizedChartPath, "Chart.yaml"))
assert.NoError(t, err, "Chart.yaml should exist at normalized path")
}

231
pkg/state/oci_chart_lock_test.go Normal file
View File

@ -0,0 +1,231 @@
package state
import (
"context"
"os"
"path/filepath"
"sync"
"sync/atomic"
"testing"
"time"
"github.com/gofrs/flock"
"github.com/stretchr/testify/require"
)
// TestOCIChartFileLock tests that the file locking mechanism works correctly
// to prevent race conditions when multiple processes/goroutines try to access
// the same chart cache path concurrently.
func TestOCIChartFileLock(t *testing.T) {
t.Run("concurrent lock acquisition is serialized", func(t *testing.T) {
// Create a temporary directory for the test
tempDir, err := os.MkdirTemp("", "helmfile-lock-test-*")
require.NoError(t, err)
defer os.RemoveAll(tempDir)
lockFilePath := filepath.Join(tempDir, "test-chart.lock")
// Track the order of lock acquisition
var lockOrder []int
var mu sync.Mutex
var wg sync.WaitGroup
// Number of concurrent goroutines trying to acquire the lock
numGoroutines := 5
for i := 0; i < numGoroutines; i++ {
wg.Add(1)
go func(id int) {
defer wg.Done()
fileLock := flock.New(lockFilePath)
err := fileLock.Lock()
require.NoError(t, err)
// Record the order this goroutine acquired the lock
mu.Lock()
lockOrder = append(lockOrder, id)
mu.Unlock()
// Simulate some work while holding the lock
time.Sleep(10 * time.Millisecond)
err = fileLock.Unlock()
require.NoError(t, err)
}(i)
}
wg.Wait()
// Verify all goroutines acquired the lock exactly once
require.Len(t, lockOrder, numGoroutines, "all goroutines should have acquired the lock")
})
t.Run("lock prevents concurrent writes", func(t *testing.T) {
tempDir, err := os.MkdirTemp("", "helmfile-lock-test-*")
require.NoError(t, err)
defer os.RemoveAll(tempDir)
lockFilePath := filepath.Join(tempDir, "test-chart.lock")
dataFilePath := filepath.Join(tempDir, "data.txt")
var wg sync.WaitGroup
var writeCount atomic.Int32
// Multiple goroutines try to write to the same file
numGoroutines := 10
for i := 0; i < numGoroutines; i++ {
wg.Add(1)
go func() {
defer wg.Done()
fileLock := flock.New(lockFilePath)
err := fileLock.Lock()
require.NoError(t, err)
defer fileLock.Unlock()
// Check if file exists (like double-check locking pattern)
if _, err := os.Stat(dataFilePath); os.IsNotExist(err) {
// Only first goroutine to acquire lock should write
err = os.WriteFile(dataFilePath, []byte("written"), 0644)
require.NoError(t, err)
writeCount.Add(1)
}
}()
}
wg.Wait()
// Only one goroutine should have written the file
require.Equal(t, int32(1), writeCount.Load(), "only one goroutine should write when using double-check locking")
// Verify file exists with correct content
content, err := os.ReadFile(dataFilePath)
require.NoError(t, err)
require.Equal(t, "written", string(content))
})
t.Run("lock file is created in correct directory", func(t *testing.T) {
tempDir, err := os.MkdirTemp("", "helmfile-lock-test-*")
require.NoError(t, err)
defer os.RemoveAll(tempDir)
// Simulate nested chart path structure
chartPath := filepath.Join(tempDir, "registry", "charts", "myapp", "1.0.0")
lockFilePath := chartPath + ".lock"
// Create parent directories (like in getOCIChart)
lockFileDir := filepath.Dir(lockFilePath)
err = os.MkdirAll(lockFileDir, 0755)
require.NoError(t, err)
// Acquire and release lock
fileLock := flock.New(lockFilePath)
err = fileLock.Lock()
require.NoError(t, err)
// Verify lock file was created
_, err = os.Stat(lockFilePath)
require.NoError(t, err, "lock file should be created")
err = fileLock.Unlock()
require.NoError(t, err)
})
t.Run("TryLockContext respects timeout", func(t *testing.T) {
tempDir, err := os.MkdirTemp("", "helmfile-lock-test-*")
require.NoError(t, err)
defer os.RemoveAll(tempDir)
lockFilePath := filepath.Join(tempDir, "test-chart.lock")
// First goroutine acquires lock and holds it
fileLock1 := flock.New(lockFilePath)
err = fileLock1.Lock()
require.NoError(t, err)
// Second goroutine tries to acquire with short timeout
done := make(chan bool)
go func() {
fileLock2 := flock.New(lockFilePath)
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()
locked, err := fileLock2.TryLockContext(ctx, 10*time.Millisecond)
// TryLockContext returns error when context times out
// Either locked is false, or we get a context deadline exceeded error
if err != nil {
require.ErrorIs(t, err, context.DeadlineExceeded, "should timeout with deadline exceeded")
} else {
require.False(t, locked, "should not acquire lock within timeout")
}
done <- true
}()
select {
case <-done:
// Success - timeout worked
case <-time.After(2 * time.Second):
t.Fatal("timeout test took too long")
}
// Release the lock
err = fileLock1.Unlock()
require.NoError(t, err)
})
}
// TestOCIChartDoubleCheckLocking verifies the double-check locking pattern
// works correctly to avoid unnecessary work when the cache is populated
// by another process while waiting for the lock.
func TestOCIChartDoubleCheckLocking(t *testing.T) {
t.Run("second waiter uses cache populated by first", func(t *testing.T) {
tempDir, err := os.MkdirTemp("", "helmfile-double-check-*")
require.NoError(t, err)
defer os.RemoveAll(tempDir)
chartPath := filepath.Join(tempDir, "myrepo", "mychart", "1.0.0")
lockFilePath := chartPath + ".lock"
// Create lock directory
err = os.MkdirAll(filepath.Dir(lockFilePath), 0755)
require.NoError(t, err)
var pullCount atomic.Int32
var wg sync.WaitGroup
// Simulate two processes trying to download the same chart
for i := 0; i < 2; i++ {
wg.Add(1)
go func() {
defer wg.Done()
fileLock := flock.New(lockFilePath)
err := fileLock.Lock()
require.NoError(t, err)
defer fileLock.Unlock()
// Double-check: after acquiring lock, check if directory exists
if _, err := os.Stat(chartPath); os.IsNotExist(err) {
// Simulate chart pull
time.Sleep(50 * time.Millisecond) // Simulate download time
err = os.MkdirAll(chartPath, 0755)
require.NoError(t, err)
err = os.WriteFile(filepath.Join(chartPath, "Chart.yaml"), []byte("name: mychart"), 0644)
require.NoError(t, err)
pullCount.Add(1)
}
// If directory exists, skip pull (use cached)
}()
}
wg.Wait()
// Only one process should have actually pulled the chart
require.Equal(t, int32(1), pullCount.Load(), "only one process should pull the chart")
// Chart should exist
_, err = os.Stat(filepath.Join(chartPath, "Chart.yaml"))
require.NoError(t, err, "chart should exist in cache")
})
}

277
pkg/state/state.go
View File

@ -2,6 +2,7 @@ package state
import (
"bytes"
"context"
"crypto/sha1"
"encoding/hex"
"errors"
@ -21,6 +22,7 @@ import (
"dario.cat/mergo"
"github.com/Masterminds/semver/v3"
"github.com/gofrs/flock"
"github.com/helmfile/chartify"
"github.com/helmfile/vals"
"github.com/tatsushid/go-prettytable"
@ -608,7 +610,10 @@ func (st *HelmState) SyncRepos(helm RepoUpdater, shouldSkip map[string]bool) ([]
username, password := gatherUsernamePassword(repo.Name, repo.Username, repo.Password)
var err error
if repo.OCI {
err = helm.RegistryLogin(repo.URL, username, password, repo.CaFile, repo.CertFile, repo.KeyFile, repo.SkipTLSVerify)
// For OCI registries, we only need the registry host for login, not the full URL with chart path.
// e.g., "123456789012.dkr.ecr.us-east-1.amazonaws.com/charts" -> "123456789012.dkr.ecr.us-east-1.amazonaws.com"
registryHost := extractRegistryHost(repo.URL)
err = helm.RegistryLogin(registryHost, username, password, repo.CaFile, repo.CertFile, repo.KeyFile, repo.SkipTLSVerify)
} else {
err = helm.AddRepo(repo.Name, repo.URL, repo.CaFile, repo.CertFile, repo.KeyFile, username, password, repo.Managed, repo.PassCredentials, repo.SkipTLSVerify)
}
@ -642,6 +647,26 @@ func gatherUsernamePassword(repoName string, username string, password string) (
return user, pass
}
// extractRegistryHost extracts just the registry host from an OCI repository URL.
// For "helm registry login", we only need the registry host, not the full path including chart location.
// Examples:
// - "123456789012.dkr.ecr.us-west-2.amazonaws.com/charts" -> "123456789012.dkr.ecr.us-west-2.amazonaws.com"
// - "ghcr.io/deliveryhero/helm-charts" -> "ghcr.io"
// - "registry.example.com:5000/charts" -> "registry.example.com:5000"
func extractRegistryHost(url string) string {
// Remove any protocol prefix if present
url = strings.TrimPrefix(url, "oci://")
url = strings.TrimPrefix(url, "https://")
url = strings.TrimPrefix(url, "http://")
// Find the first slash that separates the host from the path
if idx := strings.Index(url, "/"); idx != -1 {
return url[:idx]
}
// No path, return as-is
return url
}
type syncResult struct {
errors []*ReleaseError
}
@ -1441,17 +1466,29 @@ func (st *HelmState) forcedDownloadChart(chartName, dir string, release *Release
return "", err
}
// only fetch chart if it is not already fetched
if _, err := os.Stat(chartPath); os.IsNotExist(err) {
var fetchFlags []string
fetchFlags = st.appendChartVersionFlags(fetchFlags, release)
fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath)
if err := helm.Fetch(chartName, fetchFlags...); err != nil {
return "", err
}
} else {
st.logger.Infof("\"%s\" has not been downloaded because the output directory \"%s\" already exists", chartName, chartPath)
// Acquire locks and determine action
lockResult, err := st.acquireChartLock(chartPath, opts)
if err != nil {
return "", err
}
defer lockResult.release(st.logger)
// If cached, return immediately
if lockResult.action == chartActionUseCached {
st.addToChartCache(cacheKey, lockResult.cachedPath)
return lockResult.cachedPath, nil
}
// Download the chart
var fetchFlags []string
fetchFlags = st.appendChartVersionFlags(fetchFlags, release)
fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath)
if err := helm.Fetch(chartName, fetchFlags...); err != nil {
return "", err
}
// Create refresh marker if needed
lockResult.createRefreshMarker(st.logger)
// Set chartPath to be the path containing Chart.yaml, if found
fullChartPath, err := findChartDirectory(chartPath)
@ -1525,6 +1562,12 @@ func (st *HelmState) prepareChartForRelease(release *ReleaseSpec, helm helmexec.
skipRefresh := !isLocal || skipRefreshGlobal || skipRefreshRelease || skipRefreshDefault
if chartification != nil && helmfileCommand != "pull" {
// Issue #2297: Normalize local chart paths before chartification
// When using transformers with local charts like "../chart", the chartify process
// needs an absolute path, otherwise it tries "helm pull ../chart" which fails
if isLocal {
chartPath = normalizeChart(st.basePath, chartPath)
}
chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, opts, skipDeps, helmfileCommand)
if err != nil {
return &chartPrepareResult{err: err}
@ -4294,10 +4337,6 @@ type ChartCacheKey struct {
var downloadedCharts = make(map[ChartCacheKey]string) // key -> chart path
var downloadedChartsMutex sync.RWMutex
// Legacy OCI-specific cache (kept for backward compatibility)
var downloadedOCICharts = make(map[string]bool)
var downloadedOCIMutex sync.RWMutex
// getChartCacheKey creates a cache key for a chart and version
func (st *HelmState) getChartCacheKey(release *ReleaseSpec) ChartCacheKey {
version := release.Version
@ -4326,6 +4365,149 @@ func (st *HelmState) addToChartCache(key ChartCacheKey, path string) {
downloadedCharts[key] = path
}
// chartDownloadAction represents what action should be taken after acquiring locks
type chartDownloadAction int
const (
chartActionUseCached chartDownloadAction = iota // Use the cached chart
chartActionDownload // Download the chart
chartActionRefresh // Delete and re-download (refresh)
)
// chartLockResult contains the result of acquiring locks and checking cache
type chartLockResult struct {
action chartDownloadAction
cachedPath string // Path to use if action is chartActionUseCached
fileLock *flock.Flock // File lock to release after operation
inProcessMutex *sync.Mutex // In-process mutex to release after operation
refreshMarkerPath string // Path to refresh marker file
needsRefresh bool // Whether refresh mode is enabled
}
// release releases all acquired locks
func (r *chartLockResult) release(logger *zap.SugaredLogger) {
if r.inProcessMutex != nil {
r.inProcessMutex.Unlock()
}
if r.fileLock != nil {
if err := r.fileLock.Unlock(); err != nil {
logger.Warnf("Failed to release file lock: %v", err)
}
}
}
// createRefreshMarker creates the refresh marker file if in refresh mode
func (r *chartLockResult) createRefreshMarker(logger *zap.SugaredLogger) {
if r.needsRefresh && r.refreshMarkerPath != "" {
if markerFile, err := os.Create(r.refreshMarkerPath); err == nil {
if err := markerFile.Close(); err != nil {
logger.Warnf("Failed to close refresh marker file %s: %v", r.refreshMarkerPath, err)
}
}
}
}
// acquireChartLock acquires file and in-process locks, then determines what action to take
// based on cache state and refresh mode. The caller MUST call result.release() when done.
func (st *HelmState) acquireChartLock(chartPath string, opts ChartPrepareOptions) (*chartLockResult, error) {
result := &chartLockResult{
refreshMarkerPath: chartPath + ".refreshed",
needsRefresh: !opts.SkipDeps && !opts.SkipRefresh,
}
// Acquire filesystem-level lock for cross-process synchronization
lockFilePath := chartPath + ".lock"
lockFileDir := filepath.Dir(lockFilePath)
if err := os.MkdirAll(lockFileDir, 0755); err != nil {
return nil, fmt.Errorf("failed to create lock directory %s: %w", lockFileDir, err)
}
result.fileLock = flock.New(lockFilePath)
// Retry logic with timeout for lock acquisition
const (
lockTimeout = 5 * time.Minute
maxRetries = 3
retryBackoff = 2 * time.Second
)
var locked bool
var lockErr error
for attempt := 1; attempt <= maxRetries; attempt++ {
ctx, cancel := context.WithTimeout(context.Background(), lockTimeout)
locked, lockErr = result.fileLock.TryLockContext(ctx, 500*time.Millisecond)
cancel()
if locked {
break
}
if lockErr != nil {
st.logger.Warnf("Failed to acquire file lock (attempt %d/%d): %v", attempt, maxRetries, lockErr)
if attempt < maxRetries {
time.Sleep(retryBackoff)
}
} else {
return nil, fmt.Errorf("timeout waiting for file lock on chart %s after %v", chartPath, lockTimeout)
}
}
if !locked {
return nil, fmt.Errorf("failed to acquire file lock for chart %s after %d attempts: %w", chartPath, maxRetries, lockErr)
}
// Acquire in-process mutex for goroutine coordination
result.inProcessMutex = st.getNamedMutex(chartPath)
result.inProcessMutex.Lock()
// Determine action based on cache state and refresh mode
const refreshMarkerMaxAge = 60 * time.Second
if st.fs.DirectoryExistsAt(chartPath) {
fullChartPath, err := findChartDirectory(chartPath)
if err == nil {
// Valid chart found
if result.needsRefresh {
// Check if another process already refreshed in this session
if markerInfo, markerErr := os.Stat(result.refreshMarkerPath); markerErr == nil {
markerAge := time.Since(markerInfo.ModTime())
if markerAge < refreshMarkerMaxAge {
// Fresh marker - use cached
result.action = chartActionUseCached
result.cachedPath = filepath.Dir(fullChartPath)
st.logger.Debugf("Using cached chart at %s (refreshed %.1fs ago by another process)", result.cachedPath, markerAge.Seconds())
return result, nil
}
// Stale marker - remove and refresh
st.logger.Debugf("Refresh marker at %s is stale (%.1fs old), will refresh", result.refreshMarkerPath, markerAge.Seconds())
_ = os.Remove(result.refreshMarkerPath) // Best-effort cleanup, ignore error
}
// No fresh marker - need to refresh
st.logger.Debugf("Refreshing chart at %s (first process in refresh mode)", chartPath)
if err := os.RemoveAll(chartPath); err != nil {
result.release(st.logger)
return nil, err
}
result.action = chartActionRefresh
} else {
// Not refresh mode - use cached
result.action = chartActionUseCached
result.cachedPath = filepath.Dir(fullChartPath)
st.logger.Debugf("Using cached chart at %s", result.cachedPath)
}
} else {
// Directory exists but no valid Chart.yaml - corrupted
st.logger.Debugf("Chart directory exists but no valid Chart.yaml at %s: %v, will re-download", chartPath, err)
if err := os.RemoveAll(chartPath); err != nil {
result.release(st.logger)
return nil, err
}
result.action = chartActionDownload
}
} else {
result.action = chartActionDownload
}
return result, nil
}
func (st *HelmState) getOCIChart(release *ReleaseSpec, tempDir string, helm helmexec.Interface, opts ChartPrepareOptions) (*string, error) {
qualifiedChartName, chartName, chartVersion, err := st.getOCIQualifiedChartName(release)
if err != nil {
@ -4349,58 +4531,49 @@ func (st *HelmState) getOCIChart(release *ReleaseSpec, tempDir string, helm helm
chartPath, _ := st.getOCIChartPath(tempDir, release, chartName, chartVersion, opts.OutputDirTemplate)
mu := st.getNamedMutex(chartPath)
mu.Lock()
defer mu.Unlock()
_, err = os.Stat(tempDir)
// Acquire locks and determine action
lockResult, err := st.acquireChartLock(chartPath, opts)
if err != nil {
err = os.MkdirAll(tempDir, 0755)
if err != nil {
return nil, err
}
defer lockResult.release(st.logger)
// If cached, return immediately
if lockResult.action == chartActionUseCached {
st.addToChartCache(cacheKey, lockResult.cachedPath)
return &lockResult.cachedPath, nil
}
// Ensure temp directory exists
if _, err := os.Stat(tempDir); err != nil {
if err := os.MkdirAll(tempDir, 0755); err != nil {
return nil, err
}
}
downloadedOCIMutex.RLock()
alreadyDownloadedFlag := downloadedOCICharts[chartPath]
downloadedOCIMutex.RUnlock()
// Download the chart
flags := st.chartOCIFlags(release)
flags = st.appendVerifyFlags(flags, release)
flags = st.appendKeyringFlags(flags, release)
flags = st.appendChartDownloadFlags(flags, release)
flags = st.appendChartVersionFlags(flags, release)
if !opts.SkipDeps && !opts.SkipRefresh && !alreadyDownloadedFlag {
err = os.RemoveAll(chartPath)
if err != nil {
return nil, err
}
if err := helm.ChartPull(qualifiedChartName, chartPath, flags...); err != nil {
return nil, err
}
if st.fs.DirectoryExistsAt(chartPath) {
st.logger.Debugf("chart already exists at %s", chartPath)
} else {
flags := st.chartOCIFlags(release)
flags = st.appendVerifyFlags(flags, release)
flags = st.appendKeyringFlags(flags, release)
flags = st.appendChartDownloadFlags(flags, release)
flags = st.appendChartVersionFlags(flags, release)
err := helm.ChartPull(qualifiedChartName, chartPath, flags...)
if err != nil {
return nil, err
}
err = helm.ChartExport(qualifiedChartName, chartPath)
if err != nil {
return nil, err
}
if err := helm.ChartExport(qualifiedChartName, chartPath); err != nil {
return nil, err
}
// Create refresh marker if needed
lockResult.createRefreshMarker(st.logger)
fullChartPath, err := findChartDirectory(chartPath)
if err != nil {
return nil, err
}
downloadedOCIMutex.Lock()
downloadedOCICharts[chartPath] = true
downloadedOCIMutex.Unlock()
chartPath = filepath.Dir(fullChartPath)
// Add to global chart cache

112
pkg/state/state_test.go
View File

@ -3274,6 +3274,118 @@ func Test_gatherUsernamePassword(t *testing.T) {
}
}
func Test_extractRegistryHost(t *testing.T) {
tests := []struct {
name string
url string
want string
}{
{
name: "ECR with chart path",
url: "123456789012.dkr.ecr.us-east-1.amazonaws.com/helm-charts",
want: "123456789012.dkr.ecr.us-east-1.amazonaws.com",
},
{
name: "GHCR with nested path",
url: "ghcr.io/deliveryhero/helm-charts",
want: "ghcr.io",
},
{
name: "registry with port and path",
url: "registry.example.com:5000/helm-charts",
want: "registry.example.com:5000",
},
{
name: "registry without path",
url: "registry.example.com",
want: "registry.example.com",
},
{
name: "with oci:// prefix and path",
url: "oci://ghcr.io/charts/nginx",
want: "ghcr.io",
},
{
name: "with https:// prefix and path",
url: "https://registry.example.com/helm-charts",
want: "registry.example.com",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := extractRegistryHost(tt.url); got != tt.want {
t.Errorf("extractRegistryHost() = %v, want %v", got, tt.want)
}
})
}
}
func TestHelmState_SyncRepos_OCI(t *testing.T) {
tests := []struct {
name string
repos []RepositorySpec
wantRegistryLoginHost string
}{
{
name: "OCI registry with chart path should extract host only",
repos: []RepositorySpec{
{
Name: "ecr",
URL: "123456789012.dkr.ecr.us-east-1.amazonaws.com/helm-charts",
OCI: true,
Username: "AWS",
Password: "token",
},
},
wantRegistryLoginHost: "123456789012.dkr.ecr.us-east-1.amazonaws.com",
},
{
name: "OCI registry without path should pass URL as-is",
repos: []RepositorySpec{
{
Name: "ghcr",
URL: "ghcr.io",
OCI: true,
Username: "user",
Password: "pass",
},
},
wantRegistryLoginHost: "ghcr.io",
},
{
name: "OCI registry with nested path",
repos: []RepositorySpec{
{
Name: "docker",
URL: "registry-1.docker.io/bitnamicharts",
OCI: true,
Username: "user",
Password: "pass",
},
},
wantRegistryLoginHost: "registry-1.docker.io",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
helm := &exectest.Helm{}
state := &HelmState{
ReleaseSetSpec: ReleaseSetSpec{
Repositories: tt.repos,
},
}
_, err := state.SyncRepos(helm, map[string]bool{})
if err != nil {
t.Errorf("SyncRepos() error = %v", err)
return
}
if helm.RegistryLoginHost != tt.wantRegistryLoginHost {
t.Errorf("RegistryLogin was called with host = %q, want %q", helm.RegistryLoginHost, tt.wantRegistryLoginHost)
}
})
}
}
func TestGenerateOutputFilePath(t *testing.T) {
tests := []struct {
envName string

2
test/integration/run.sh
View File

@ -117,6 +117,8 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes
. ${dir}/test-cases/issue-2281-array-merge.sh
. ${dir}/test-cases/issue-2247.sh
. ${dir}/test-cases/issue-2291.sh
. ${dir}/test-cases/oci-parallel-pull.sh
. ${dir}/test-cases/issue-2297-local-chart-transformers.sh
# ALL DONE -----------------------------------------------------------------------------------------------------------

42
test/integration/test-cases/issue-2297-local-chart-transformers.sh Normal file
View File

@ -0,0 +1,42 @@
# Integration test for issue #2297: Local chart + transformers causes panic
# https://github.com/helmfile/helmfile/issues/2297
#
# This test verifies that local charts with relative paths (like "../chart")
# work correctly when transformers are present. The bug manifests when there
# are MULTIPLE release files in helmfile.d and one uses local chart + transformers.
# Before the fix, chartify would try "helm pull ../chart" which fails because
# the path wasn't normalized.
issue_2297_input_dir="${cases_dir}/issue-2297-local-chart-transformers/input"
issue_2297_tmp=$(mktemp -d)
# Convert helmfile to absolute path before cd (otherwise ./helmfile won't be found)
helmfile_real="$(pwd)/${helmfile}"
test_start "issue #2297: local chart with transformers"
info "Testing helmfile template with local chart and transformers"
# Run from the input directory where helmfile.d/ contains the helmfile with "../chart" reference
cd "${issue_2297_input_dir}"
# This should succeed - before the fix it would fail with:
# "helm pull ../chart --untar" fails with "repo .. not found"
${helmfile_real} template > "${issue_2297_tmp}/output.yaml" 2>&1
result=$?
cd - > /dev/null
if [ $result -ne 0 ]; then
cat "${issue_2297_tmp}/output.yaml"
fail "helmfile template with local chart and transformers should not fail"
fi
# Verify the output contains the expected configmap with transformer annotation
if ! grep -q "test-annotation" "${issue_2297_tmp}/output.yaml"; then
cat "${issue_2297_tmp}/output.yaml"
fail "Output should contain the transformer annotation"
fi
info "Local chart with transformers works correctly"
test_pass "issue #2297: local chart with transformers"

4
test/integration/test-cases/issue-2297-local-chart-transformers/input/chart/Chart.yaml Normal file
View File

@ -0,0 +1,4 @@
apiVersion: v2
name: test-chart
version: 0.1.0
description: A test chart for issue #2297

6
test/integration/test-cases/issue-2297-local-chart-transformers/input/chart/templates/configmap.yaml Normal file
View File

@ -0,0 +1,6 @@
apiVersion: v1
kind: ConfigMap
metadata:
name: {{ .Release.Name }}-config
data:
key: value

6
test/integration/test-cases/issue-2297-local-chart-transformers/input/helmfile.d/other-release.yaml Normal file
View File

@ -0,0 +1,6 @@
# Issue #2297: Another release file in helmfile.d
# The bug manifests when there are MULTIPLE release files in helmfile.d
# This file represents another release that doesn't use transformers
releases:
- name: simple-release
chart: ../chart

17
test/integration/test-cases/issue-2297-local-chart-transformers/input/helmfile.d/with-transformers.yaml Normal file
View File

@ -0,0 +1,17 @@
# Issue #2297: Local chart with transformers
# The bug manifests when there are MULTIPLE release files in helmfile.d
# and one of them uses a local chart with transformers.
# The chart path should be normalized before being passed to chartify.
releases:
- name: test-with-transformers
chart: ../chart
transformers:
- apiVersion: builtin
kind: AnnotationsTransformer
metadata:
name: add-annotations
annotations:
test-annotation: "added-by-transformer"
fieldSpecs:
- path: metadata/annotations
create: true

96
test/integration/test-cases/oci-parallel-pull.sh Normal file
View File

@ -0,0 +1,96 @@
# Integration test for issue #2295: Cache conflicts running multiple helmfile using the same chart in parallel
# This test verifies that file locking prevents race conditions when multiple helmfile processes
# try to pull the same OCI chart simultaneously.
oci_parallel_pull_case_input_dir="${cases_dir}/oci-parallel-pull/input"
config_file="helmfile.yaml"
oci_parallel_pull_cache_dir=""
# Cleanup function for this test
cleanup_oci_parallel_pull() {
if [ -n "${oci_parallel_pull_cache_dir}" ] && [ -d "${oci_parallel_pull_cache_dir}" ]; then
rm -rf "${oci_parallel_pull_cache_dir}"
fi
}
trap cleanup_oci_parallel_pull EXIT
test_start "oci-parallel-pull: verify file locking prevents race conditions (issue #2295)"
# Create a temporary cache directory for this test
oci_parallel_pull_cache_dir=$(mktemp -d)
export HELMFILE_CACHE_HOME="${oci_parallel_pull_cache_dir}"
info "Using temporary cache directory: ${HELMFILE_CACHE_HOME}"
# Run multiple helmfile template commands in parallel using the same chart
# This simulates the scenario described in issue #2295
info "Running 3 parallel helmfile template commands..."
# Start all processes in background
${helmfile} -f ${oci_parallel_pull_case_input_dir}/${config_file} template > /tmp/oci-parallel-1.out 2>&1 &
pid1=$!
${helmfile} -f ${oci_parallel_pull_case_input_dir}/${config_file} template > /tmp/oci-parallel-2.out 2>&1 &
pid2=$!
${helmfile} -f ${oci_parallel_pull_case_input_dir}/${config_file} template > /tmp/oci-parallel-3.out 2>&1 &
pid3=$!
# Wait for all processes and capture exit codes
wait $pid1
exit1=$?
wait $pid2
exit2=$?
wait $pid3
exit3=$?
info "Process 1 exit code: ${exit1}"
info "Process 2 exit code: ${exit2}"
info "Process 3 exit code: ${exit3}"
# Check for failures
failed=0
if [ $exit1 -ne 0 ]; then
warn "Process 1 failed:"
cat /tmp/oci-parallel-1.out
failed=1
fi
if [ $exit2 -ne 0 ]; then
warn "Process 2 failed:"
cat /tmp/oci-parallel-2.out
failed=1
fi
if [ $exit3 -ne 0 ]; then
warn "Process 3 failed:"
cat /tmp/oci-parallel-3.out
failed=1
fi
# Check for the specific error from issue #2295
if grep -q "failed to untar: a file or directory with the name.*already exists" /tmp/oci-parallel-*.out 2>/dev/null; then
warn "Race condition detected! Found 'file already exists' error in output"
failed=1
fi
if [ $failed -eq 1 ]; then
fail "oci-parallel-pull test failed"
fi
# Verify the chart was cached
if [ ! -d "${HELMFILE_CACHE_HOME}" ]; then
fail "Cache directory was not created"
fi
# Check that lock files were created (indicates locking was used)
lock_files=$(find "${HELMFILE_CACHE_HOME}" -name "*.lock" 2>/dev/null | wc -l)
info "Found ${lock_files} lock file(s) in cache directory"
# Cleanup and restore the original trap
cleanup_oci_parallel_pull
trap - EXIT
test_pass "oci-parallel-pull: file locking prevents race conditions"

13
test/integration/test-cases/oci-parallel-pull/input/helmfile.yaml Normal file
View File

@ -0,0 +1,13 @@
# Test helmfile for issue #2295: Parallel OCI chart pulls
# This helmfile uses an OCI chart to test that parallel pulls
# don't cause race conditions with the file locking mechanism.
repositories:
- name: docker
url: registry-1.docker.io
oci: true
releases:
# Using a chart that can be templated (not a library chart)
- name: test-parallel-pull
chart: docker/bitnamicharts/nginx
version: "18.2.5"