diff --git a/pkg/app/context.go b/pkg/app/context.go index 3f9c65db..48f4eb6d 100644 --- a/pkg/app/context.go +++ b/pkg/app/context.go @@ -17,11 +17,11 @@ func NewContext() Context { } } -func (ctx *Context) SyncReposOnce(st *state.HelmState, helm state.RepoUpdater) error { +func (ctx *Context) SyncReposOnce(st *state.HelmState, helm state.RepoUpdater, opts ...state.SyncOption) error { ctx.mu.Lock() defer ctx.mu.Unlock() - updated, err := st.SyncRepos(helm, ctx.updatedRepos) + updated, err := st.SyncRepos(helm, ctx.updatedRepos, opts...) for _, r := range updated { ctx.updatedRepos[r] = true diff --git a/pkg/app/run.go b/pkg/app/run.go index 590c10bc..323380a7 100644 --- a/pkg/app/run.go +++ b/pkg/app/run.go @@ -42,10 +42,15 @@ func (r *Run) askForConfirmation(msg string) bool { return AskForConfirmation(msg) } +// commandsSkipChartPrep lists commands that don't prepare or pull charts. +// These commands skip chart preparation, and when skipRepos is true they also +// skip the OCI-only registry login (since no chart pulls need authentication). +// When skipRepos is false, SyncReposOnce still runs normally for all repos. +var commandsSkipChartPrep = []string{"write-values", "list"} + func (r *Run) prepareChartsIfNeeded(helmfileCommand string, dir string, concurrency int, opts state.ChartPrepareOptions) (map[state.PrepareChartKey]string, error) { - // Skip chart preparation for certain commands - skipCommands := []string{"write-values", "list"} - if slices.Contains(skipCommands, strings.ToLower(helmfileCommand)) { + // Skip chart preparation for commands that don't need chart pulls + if slices.Contains(commandsSkipChartPrep, strings.ToLower(helmfileCommand)) { return nil, nil } @@ -67,10 +72,17 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare // - skipRefresh explicitly means "don't update repos" // - skipDeps implies "I have all dependencies locally" which means repo data isn't needed // This matches the CLI behavior where --skip-deps and --skip-refresh both skip repo operations. + // However, OCI registries need `helm registry login` before chart pulls when + // credentials are configured (issue #1847), so when skipRepos is true we still + // perform OCI-only login — but only for commands that actually pull charts. + needsChartPrep := !slices.Contains(commandsSkipChartPrep, strings.ToLower(helmfileCommand)) 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 { + if err := r.ctx.SyncReposOnce(r.state, r.helm); err != nil { + return err + } + } else if needsChartPrep { + if err := r.ctx.SyncReposOnce(r.state, r.helm, state.WithOCIOnly()); err != nil { return err } } @@ -140,12 +152,16 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare func (r *Run) Deps(c DepsConfigProvider) []error { // Check both CLI options and helmDefaults for skipping repos (issue #2296) - // Both skipDeps and skipRefresh cause repo sync to be skipped (see withPreparedCharts for rationale) + // Both skipDeps and skipRefresh cause repo sync to be skipped (see WithPreparedCharts for rationale). + // OCI registries need login before chart pulls when credentials are + // configured (issue #1847), so skipRepos=true still performs OCI-only login. 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} - } + var repoOpts []state.SyncOption + if skipRepos { + repoOpts = append(repoOpts, state.WithOCIOnly()) + } + if err := r.ctx.SyncReposOnce(r.state, r.helm, repoOpts...); err != nil { + return []error{err} } r.helm.SetExtraArgs(GetArgs(c.Args(), r.state)...) diff --git a/pkg/state/state.go b/pkg/state/state.go index 4ccf19f8..06c8b295 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -758,13 +758,37 @@ type RepoUpdater interface { RegistryLogin(name, username, password, caFile, certFile, keyFile string, skipTLSVerify bool) error } -func (st *HelmState) SyncRepos(helm RepoUpdater, shouldSkip map[string]bool) ([]string, error) { +// SyncOption configures SyncRepos behavior. +type SyncOption func(*syncConfig) + +type syncConfig struct { + ociOnly bool +} + +// WithOCIOnly limits repo processing to OCI registries (helm registry login), +// skipping classic repo add/update. This allows callers that skip classic repos +// to still authenticate with OCI registries before chart pulls (issue #1847). +func WithOCIOnly() SyncOption { + return func(c *syncConfig) { + c.ociOnly = true + } +} + +func (st *HelmState) SyncRepos(helm RepoUpdater, shouldSkip map[string]bool, opts ...SyncOption) ([]string, error) { + cfg := syncConfig{} + for _, o := range opts { + o(&cfg) + } + var updated []string for _, repo := range st.Repositories { if shouldSkip[repo.Name] { continue } + if cfg.ociOnly && !repo.OCI { + continue + } username, password := gatherUsernamePassword(repo.Name, repo.Username, repo.Password) var err error if repo.OCI { diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 62ecdc1a..2dd29dc0 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -4421,6 +4421,80 @@ func TestHelmState_SyncRepos_OCI(t *testing.T) { } } +func TestHelmState_SyncRepos_OCIOnly(t *testing.T) { + tests := []struct { + name string + repos []RepositorySpec + opts []SyncOption + wantRegistryLoginHost string + wantRepoSet bool + }{ + { + name: "WithOCIOnly logs into OCI registry", + repos: []RepositorySpec{ + { + Name: "ociregistry", + URL: "quay.io/myorg", + OCI: true, + Username: "user", + Password: "pass", + }, + }, + opts: []SyncOption{WithOCIOnly()}, + wantRegistryLoginHost: "quay.io", + wantRepoSet: false, + }, + { + name: "WithOCIOnly skips non-OCI repo", + repos: []RepositorySpec{ + { + Name: "stable", + URL: "https://charts.helm.sh/stable", + }, + }, + opts: []SyncOption{WithOCIOnly()}, + wantRegistryLoginHost: "", + wantRepoSet: false, + }, + { + name: "without options processes non-OCI repo via AddRepo", + repos: []RepositorySpec{ + { + Name: "stable", + URL: "https://charts.helm.sh/stable", + }, + }, + opts: nil, + wantRegistryLoginHost: "", + wantRepoSet: true, + }, + } + 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{}, tt.opts...) + if err != nil { + t.Errorf("SyncRepos() error = %v", err) + return + } + if tt.wantRegistryLoginHost != "" && helm.RegistryLoginHost != tt.wantRegistryLoginHost { + t.Errorf("RegistryLogin host = %q, want %q", helm.RegistryLoginHost, tt.wantRegistryLoginHost) + } + if tt.wantRegistryLoginHost == "" && helm.RegistryLoginHost != "" { + t.Errorf("RegistryLogin should not have been called, got host = %q", helm.RegistryLoginHost) + } + if len(helm.Repo) > 0 != tt.wantRepoSet { + t.Errorf("AddRepo called = %v, want %v", len(helm.Repo) > 0, tt.wantRepoSet) + } + }) + } +} + func TestGenerateOutputFilePath(t *testing.T) { tests := []struct { envName string