fix: ensure OCI registry login when SkipRepos is set (#2701)

* fix: ensure OCI registry login when SkipRepos is set (#1847) Commands like build, status, list, and show-dag set SkipRepos: true to avoid slow helm repo add/update for classic repos. However, this also skipped helm registry login for OCI registries, causing 401 Unauthorized errors when pulling OCI charts. Add a variadic SyncOption parameter (backward compatible) with WithOCIOnly() that limits repo processing to OCI registries only. When skipRepos is true, callers now pass WithOCIOnly() so that OCI authentication still happens before chart pulls. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: reword OCI login comments per review feedback RegistryLogin is a no-op when credentials are not configured, so the word 'always' was misleading. Clarify that login is only needed when credentials are present. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: skip OCI login for commands that don't pull charts Commands like 'list' and 'write-values' skip chart preparation entirely, so OCI registry login is unnecessary for them. Extract the skip-command list into a shared variable and use it to gate OCI-only login in WithPreparedCharts. Signed-off-by: yxxhero <aiopsclub@163.com> * docs: clarify commandsSkipChartPrep comment per review feedback Clarify that these commands only skip OCI login when skipRepos is true; when skipRepos is false, SyncReposOnce still runs normally for all repos. Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com>
2026-09-30 05:42:37 +02:00 · 2026-07-12 09:51:24 +08:00
parent 09afa425d7
commit 43aafeaad1
4 changed files with 127 additions and 13 deletions
@@ -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
@@ -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)...)
@@ -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 {
@@ -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