refactor: eliminate hidden mutations in HelmRepository client opts

Replace hidden mutation functions with explicit configuration
pattern to eliminate side effects where ClientOpts was modified
through configuration functions. Adds CertsTempDir field to ClientOpts struct.

Signed-off-by: cappyzawa <cappyzawa@gmail.com>

internal/controller/helmchart_controller.go

@@ -523,7 +523,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		return chartRepoConfigErrorReturn(err, obj)
 	}
 
-	clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
+	clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL)
 	if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 		e := serror.NewGeneric(
 			err,
@@ -532,9 +532,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
 		return sreconcile.ResultEmpty, e
 	}
-	if certsTmpDir != "" {
+	if clientOpts.CertsTempDir != "" {
 		defer func() {
-			if err := os.RemoveAll(certsTmpDir); err != nil {
+			if err := os.RemoveAll(clientOpts.CertsTempDir); err != nil {
 				r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason,
 					"failed to delete temporary certificates directory: %s", err)
 			}
@@ -1018,7 +1018,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 		ctxTimeout, cancel := context.WithTimeout(ctx, obj.GetTimeout())
 		defer cancel()
 
-		clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
+		clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL)
 		if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			return nil, err
 		}
@@ -1037,7 +1037,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters),
 				repository.WithOCIGetterOptions(getterOpts),
 				repository.WithOCIRegistryClient(registryClient),
-				repository.WithCertificatesStore(certsTmpDir),
+				repository.WithCertificatesStore(clientOpts.CertsTempDir),
 				repository.WithCredentialsFile(credentialsFile))
 			if err != nil {
 				errs = append(errs, fmt.Errorf("failed to create OCI chart repository: %w", err))

internal/controller/helmchart_controller_test.go

@@ -1035,12 +1035,12 @@ func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) {
 				}
 			},
 			want:    sreconcile.ResultEmpty,
-			wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret: secrets \"invalid\" not found")},
+			wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid': secrets \"invalid\" not found")},
 			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
 				g.Expect(build.Complete()).To(BeFalse())
 
 				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
-					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret: secrets \"invalid\" not found"),
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid': secrets \"invalid\" not found"),
 				}))
 			},
 		},
@@ -1304,12 +1304,12 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 				}
 			},
 			want:    sreconcile.ResultEmpty,
-			wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret: secrets \"invalid\" not found")},
+			wantErr: &serror.Generic{Err: errors.New("failed to get authentication secret '/invalid': secrets \"invalid\" not found")},
 			assertFunc: func(g *WithT, obj *sourcev1.HelmChart, build chart.Build) {
 				g.Expect(build.Complete()).To(BeFalse())
 
 				g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
-					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret: secrets \"invalid\" not found"),
+					*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get authentication secret '/invalid': secrets \"invalid\" not found"),
 				}))
 			},
 		},

internal/controller/helmrepository_controller.go

@@ -415,7 +415,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc
 		return sreconcile.ResultEmpty, e
 	}
 
-	clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
+	clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL)
 	if err != nil {
 		if errors.Is(err, getter.ErrDeprecatedTLSConfig) {
 			ctrl.LoggerFrom(ctx).

internal/helm/getter/client_opts.go

@@ -22,7 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path"
+	"path/filepath"
 
 	"github.com/google/go-containerregistry/pkg/authn"
 	helmgetter "helm.sh/helm/v3/pkg/getter"
@@ -46,6 +46,9 @@ const (
 
 var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner")
 
+// option applies configuration to ClientOpts.
+type option func(*ClientOpts) error
+
 // ClientOpts contains the various options to use while constructing
 // a Helm repository client.
 type ClientOpts struct {
@@ -55,25 +58,26 @@ type ClientOpts struct {
 	TlsConfig     *tls.Config
 	GetterOpts    []helmgetter.Option
 	Insecure      bool
+	CertsTempDir  string
 }
 
 // MustLoginToRegistry returns true if the client options contain at least
-// one registry login option.
+// one registry login option. This indicates that registry authentication
+// has been configured, regardless of the specific login option type.
 func (o ClientOpts) MustLoginToRegistry() bool {
-	return len(o.RegLoginOpts) > 0 && o.RegLoginOpts[0] != nil
+	return len(o.RegLoginOpts) > 0
 }
 
 // GetClientOpts uses the provided HelmRepository object and a normalized
 // URL to construct a HelmClientOpts object. If obj is an OCI HelmRepository,
 // then the returned options object will also contain the required registry
 // auth mechanisms.
-// A temporary directory is created to store the certs files if needed and its path is returned along with the options object. It is the
+// A temporary directory is created to store the certs files if needed and its path is available via ClientOpts.CertsTempDir.
-// caller's responsibility to clean up the directory.
+// It is the caller's responsibility to clean up the directory.
-func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string) (*ClientOpts, string, error) {
+func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, url string) (*ClientOpts, error) {
-	// This function configures authentication for Helm repositories based on the provided secrets:
+	var certSecret, authSecret *corev1.Secret
-	// - CertSecretRef: TLS client certificates (always takes priority)
+	var err error
-	// - SecretRef: Can contain Basic Auth or TLS certificates (deprecated)
+
-	// For OCI repositories, additional registry-specific authentication is configured (including Docker config)
 	opts := &ClientOpts{
 		GetterOpts: []helmgetter.Option{
 			helmgetter.WithURL(url),
@@ -83,117 +87,111 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *sourcev1.HelmRepos
 		Insecure: obj.Spec.Insecure,
 	}
 
-	// Process secrets and configure authentication
+	if obj.Spec.CertSecretRef != nil {
-	deprecatedTLS, certSecret, authSecret, err := configureAuthentication(ctx, c, obj, opts, url)
+		certSecret, err = fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace())
 		if err != nil {
-		return nil, "", err
+			return nil, fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err)
 		}
-
+		certOpt := withAuthFromCertSecret(ctx, certSecret, url, obj.Spec.Insecure)
-	// Setup OCI registry specific configurations if needed
+		if err := certOpt(opts); err != nil {
-	var tempCertDir string
+			return nil, err
-	if obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
-		tempCertDir, err = configureOCIRegistryWithSecrets(ctx, obj, opts, url, certSecret, authSecret)
-		if err != nil {
-			return nil, "", err
 		}
 	}
 
 	var deprecatedErr error
-	if deprecatedTLS {
-		deprecatedErr = ErrDeprecatedTLSConfig
-	}
-
-	return opts, tempCertDir, deprecatedErr
-}
-
-// configureAuthentication processes all secret references and sets up authentication.
-// Returns (deprecatedTLS, certSecret, authSecret, error) where:
-// - deprecatedTLS: true if TLS config comes from SecretRef (deprecated pattern)
-// - certSecret: the secret from CertSecretRef (nil if not specified)
-// - authSecret: the secret from SecretRef (nil if not specified)
-func configureAuthentication(ctx context.Context, c client.Client, obj *sourcev1.HelmRepository, opts *ClientOpts, url string) (bool, *corev1.Secret, *corev1.Secret, error) {
-	var deprecatedTLS bool
-	var certSecret, authSecret *corev1.Secret
-
-	if obj.Spec.CertSecretRef != nil {
-		secret, err := fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace())
-		if err != nil {
-			return false, nil, nil, fmt.Errorf("failed to get TLS authentication secret: %w", err)
-		}
-		certSecret = secret
-
-		tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, obj.Spec.URL, obj.Spec.Insecure)
-		if err != nil {
-			return false, nil, nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err)
-		}
-		opts.TlsConfig = tlsConfig
-	}
-
-	// Extract all authentication methods from SecretRef.
-	// This secret may contain multiple auth types (Basic Auth, TLS).
 	if obj.Spec.SecretRef != nil {
-		secret, err := fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace())
+		authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace())
 		if err != nil {
-			return false, nil, nil, fmt.Errorf("failed to get authentication secret: %w", err)
+			return nil, fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err)
 		}
-		authSecret = secret
 
-		methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(obj.Spec.URL, obj.Spec.Insecure))
+		secretRefOpt := withAuthFromSecret(ctx, authSecret, url, obj.Spec.Insecure)
+		if err := secretRefOpt(opts); err != nil {
+			if errors.Is(err, ErrDeprecatedTLSConfig) {
+				deprecatedErr = err
+			} else {
+				return nil, err
+			}
+		}
+	}
+
+	if obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+		ociOpt := withOCIRegistry(ctx, obj, url, certSecret, authSecret)
+		if err := ociOpt(opts); err != nil {
+			return nil, err
+		}
+	}
+
+	return opts, deprecatedErr
+}
+
+// withAuthFromCertSecret applies TLS config from a pre-fetched secret.
+func withAuthFromCertSecret(ctx context.Context, secret *corev1.Secret, url string, insecure bool) option {
+	return func(o *ClientOpts) error {
+		tlsConfig, err := secrets.TLSConfigFromSecret(ctx, secret, url, insecure)
 		if err != nil {
-			return false, nil, nil, fmt.Errorf("failed to detect authentication methods: %w", err)
+			return fmt.Errorf("failed to construct Helm client's TLS config: %w", err)
+		}
+		o.TlsConfig = tlsConfig
+		return nil
+	}
+}
+
+// withAuthFromSecret applies BasicAuth or TLS from a pre-fetched secret.
+// Returns ErrDeprecatedTLSConfig if TLS config comes from SecretRef (deprecated pattern).
+func withAuthFromSecret(ctx context.Context, secret *corev1.Secret, url string, insecure bool) option {
+	return func(o *ClientOpts) error {
+		methods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTLS(url, insecure))
+		if err != nil {
+			return fmt.Errorf("failed to detect authentication methods: %w", err)
 		}
 
 		if methods.HasBasicAuth() {
-			opts.GetterOpts = append(opts.GetterOpts,
+			o.GetterOpts = append(o.GetterOpts,
 				helmgetter.WithBasicAuth(methods.Basic.Username, methods.Basic.Password))
 		}
 
-		// Use TLS from SecretRef only if CertSecretRef is not specified (CertSecretRef takes priority)
+		if o.TlsConfig == nil && methods.HasTLS() {
-		if opts.TlsConfig == nil && methods.HasTLS() {
+			o.TlsConfig = methods.TLS
-			opts.TlsConfig = methods.TLS
+			return ErrDeprecatedTLSConfig
-			deprecatedTLS = true
+		}
+		return nil
 	}
 }
 
-	return deprecatedTLS, certSecret, authSecret, nil
+// withOCIRegistry applies OCI-specific login and TLS file handling.
-}
+func withOCIRegistry(ctx context.Context, obj *sourcev1.HelmRepository, url string, certSecret, authSecret *corev1.Secret) option {
-
+	return func(o *ClientOpts) error {
-// configureOCIRegistryWithSecrets sets up OCI-specific configurations using pre-fetched secrets
-func configureOCIRegistryWithSecrets(ctx context.Context, obj *sourcev1.HelmRepository, opts *ClientOpts, url string, certSecret, authSecret *corev1.Secret) (string, error) {
-	// Configure OCI authentication from authSecret if available
 		if authSecret != nil {
 			keychain, err := registry.LoginOptionFromSecret(url, *authSecret)
 			if err != nil {
-			return "", fmt.Errorf("failed to configure login options: %w", err)
+				return fmt.Errorf("failed to configure login options: %w", err)
 			}
-		opts.Keychain = keychain
+			o.Keychain = keychain
 		}
 
-	// Handle OCI provider authentication if no SecretRef
 		if obj.Spec.SecretRef == nil && obj.Spec.Provider != "" && obj.Spec.Provider != sourcev1.GenericOCIProvider {
 			authenticator, err := soci.OIDCAuth(ctx, url, obj.Spec.Provider)
 			if err != nil {
-			return "", fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, err)
+				return fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, err)
 			}
-		opts.Authenticator = authenticator
+			o.Authenticator = authenticator
 		}
 
-	// Setup registry login options
+		loginOpt, err := registry.NewLoginOption(o.Authenticator, o.Keychain, url)
-	loginOpt, err := registry.NewLoginOption(opts.Authenticator, opts.Keychain, url)
 		if err != nil {
-		return "", err
+			return err
 		}
 
 		if loginOpt != nil {
-		opts.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)}
+			// NOTE: RegLoginOpts is rebuilt here intentionally (overwrites any existing entries).
+			o.RegLoginOpts = []helmreg.LoginOption{loginOpt, helmreg.LoginOptInsecure(obj.Spec.Insecure)}
 		}
 
-	// Handle TLS certificate files for OCI
+		if o.TlsConfig != nil {
-	var tempCertDir string
+			tempCertDir, err := os.MkdirTemp("", "helm-repo-oci-certs")
-	if opts.TlsConfig != nil {
-		tempCertDir, err = os.MkdirTemp("", "helm-repo-oci-certs")
 			if err != nil {
-			return "", fmt.Errorf("cannot create temporary directory: %w", err)
+				return fmt.Errorf("cannot create temporary directory: %w", err)
 			}
 
 			var tlsSecret *corev1.Secret
@@ -203,18 +201,21 @@ func configureOCIRegistryWithSecrets(ctx context.Context, obj *sourcev1.HelmRepo
 				tlsSecret = authSecret
 			}
 
-		certFile, keyFile, caFile, err := storeTLSCertificateFilesForOCI(ctx, tlsSecret, nil, tempCertDir)
+			certFile, keyFile, caFile, err := storeTLSCertificateFilesForOCI(tlsSecret, nil, tempCertDir)
 			if err != nil {
-			return "", fmt.Errorf("cannot write certs files to path: %w", err)
+				return fmt.Errorf("cannot write certs files to path: %w", err)
 			}
 
 			tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile)
 			if tlsLoginOpt != nil {
-			opts.RegLoginOpts = append(opts.RegLoginOpts, tlsLoginOpt)
+				o.RegLoginOpts = append(o.RegLoginOpts, tlsLoginOpt)
-		}
 			}
 
-	return tempCertDir, nil
+			o.CertsTempDir = tempCertDir
+		}
+
+		return nil
+	}
 }
 
 func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) {
@@ -233,7 +234,7 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (
 // Helm OCI registry client requires certificate file paths rather than in-memory data,
 // so we need to temporarily write the certificate data to disk.
 // Returns paths to the written cert, key, and CA files (any of which may be empty if not present).
-func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret *corev1.Secret, path string) (string, string, string, error) {
+func storeTLSCertificateFilesForOCI(certSecret, authSecret *corev1.Secret, dir string) (string, string, string, error) {
 	var (
 		certFile string
 		keyFile  string
@@ -241,7 +242,6 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret
 		err      error
 	)
 
-	// Try to get TLS data from certSecret first, then authSecret
 	var tlsSecret *corev1.Secret
 	if certSecret != nil {
 		tlsSecret = certSecret
@@ -252,11 +252,11 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret
 	if tlsSecret != nil {
 		if certData, exists := tlsSecret.Data[secrets.KeyTLSCert]; exists {
 			if keyData, keyExists := tlsSecret.Data[secrets.KeyTLSPrivateKey]; keyExists {
-				certFile, err = writeToFile(certData, certFileName, path)
+				certFile, err = writeToFile(certData, certFileName, dir)
 				if err != nil {
 					return "", "", "", err
 				}
-				keyFile, err = writeToFile(keyData, keyFileName, path)
+				keyFile, err = writeToFile(keyData, keyFileName, dir)
 				if err != nil {
 					return "", "", "", err
 				}
@@ -264,7 +264,7 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret
 		}
 
 		if caData, exists := tlsSecret.Data[secrets.KeyCACert]; exists {
-			caFile, err = writeToFile(caData, caFileName, path)
+			caFile, err = writeToFile(caData, caFileName, dir)
 			if err != nil {
 				return "", "", "", err
 			}
@@ -275,7 +275,7 @@ func storeTLSCertificateFilesForOCI(ctx context.Context, certSecret, authSecret
 }
 
 func writeToFile(data []byte, filename, tmpDir string) (string, error) {
-	file := path.Join(tmpDir, filename)
+	file := filepath.Join(tmpDir, filename)
 	err := os.WriteFile(file, data, 0o600)
 	if err != nil {
 		return "", err

internal/helm/getter/client_opts_test.go

@@ -164,7 +164,7 @@ func TestGetClientOpts(t *testing.T) {
 			}
 			c := clientBuilder.Build()
 
-			clientOpts, _, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy")
+			clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy")
 			if tt.err != nil {
 				g.Expect(err).To(Equal(tt.err))
 			} else {
@@ -271,7 +271,7 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) {
 			}
 			c := clientBuilder.Build()
 
-			clientOpts, tmpDir, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy")
+			clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy")
 			if tt.wantErrMsg != "" {
 				if err == nil {
 					t.Errorf("GetClientOpts() expected error but got none")
@@ -287,8 +287,8 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) {
 				t.Errorf("GetClientOpts() error = %v", err)
 				return
 			}
-			if tmpDir != "" {
+			if clientOpts.CertsTempDir != "" {
-				defer os.RemoveAll(tmpDir)
+				defer os.RemoveAll(clientOpts.CertsTempDir)
 			}
 			if tt.loginOptsN != len(clientOpts.RegLoginOpts) {
 				// we should have a login option but no TLS option