only use provider auth if sa and secretRef aren't set

Signed-off-by: Somtochi Onyekwere <[email protected]>

Author: somtochiama

internal/helm/getter/client_opts.go

@@ -24,7 +24,6 @@ import (
 	"fmt"
 	"net/url"
 
-	"github.com/fluxcd/pkg/oci"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/authn/k8schain"
 	helmgetter "helm.sh/helm/v3/pkg/getter"
@@ -33,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/fluxcd/pkg/oci"
 	helmv1 "github.com/fluxcd/source-controller/api/v1beta2"
 	"github.com/fluxcd/source-controller/internal/helm/registry"
 	soci "github.com/fluxcd/source-controller/internal/oci"
@@ -82,15 +82,6 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
 	var authSecret *corev1.Secret
 	var deprecatedTLSConfig bool
 
-	if ociRepo {
-		if obj.Spec.ServiceAccountName != "" {
-			hrOpts.Keychain, err = getKeychainFromSAImagePullSecrets(ctx, c, obj.GetNamespace(), obj.Spec.ServiceAccountName)
-			if err != nil {
-				return nil, fmt.Errorf("failed to get keychain from service account: %w", err)
-			}
-		}
-	}
-
 	if obj.Spec.SecretRef != nil {
 		authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace())
 		if err != nil {
@@ -118,28 +109,43 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
 		}
 
 		if ociRepo {
-			keychain, err := registry.LoginOptionFromSecret(url, *authSecret)
+			hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret)
 			if err != nil {
 				return nil, fmt.Errorf("failed to configure login options: %w", err)
 			}
+		}
+	}
+
+	if ociRepo {
+		if obj.Spec.ServiceAccountName != "" {
+			keychain, err := getKeychainFromSAImagePullSecrets(ctx, c, obj.GetNamespace(), obj.Spec.ServiceAccountName)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get keychain from service account: %w", err)
+			}
 
 			if hrOpts.Keychain != nil {
-				hrOpts.Keychain = authn.NewMultiKeychain(keychain, hrOpts.Keychain)
+				hrOpts.Keychain = authn.NewMultiKeychain(hrOpts.Keychain, keychain)
 			} else {
 				hrOpts.Keychain = keychain
 			}
 		}
-	} else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI && ociRepo {
-		authenticator, authErr := soci.OIDCAuth(ctx, obj.Spec.URL, obj.Spec.Provider)
-		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
-			return nil, fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr)
+
+		var hasKeychain bool
+		if hrOpts.Keychain != nil {
+			_, ok := hrOpts.Keychain.(soci.Anonymous)
+			hasKeychain = !ok
 		}
-		if authenticator != nil {
-			hrOpts.Authenticator = authenticator
+
+		if !hasKeychain && obj.Spec.Provider != helmv1.GenericOCIProvider {
+			authenticator, authErr := soci.OIDCAuth(ctx, obj.Spec.URL, obj.Spec.Provider)
+			if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
+				return nil, fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr)
+			}
+			if authenticator != nil {
+				hrOpts.Authenticator = authenticator
+			}
 		}
-	}
 
-	if ociRepo {
 		hrOpts.RegLoginOpt, err = registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url)
 		if err != nil {
 			return nil, err

internal/helm/getter/client_opts_test.go

@@ -44,13 +44,15 @@ func TestGetClientOpts(t *testing.T) {
 	}
 
 	tests := []struct {
-		name           string
-		certSecret     *corev1.Secret
-		authSecret     *corev1.Secret
-		serviceAccount *corev1.ServiceAccount
-		afterFunc      func(t *WithT, hcOpts *ClientOpts)
-		oci            bool
-		err            error
+		name            string
+		certSecret      *corev1.Secret
+		authSecret      *corev1.Secret
+		imagePullSecret *corev1.Secret
+		serviceAccount  *corev1.ServiceAccount
+		provider        string
+		afterFunc       func(t *WithT, hcOpts *ClientOpts)
+		oci             bool
+		err             error
 	}{
 		{
 			name: "HelmRepository with certSecretRef discards TLS config in secretRef",
@@ -130,7 +132,41 @@ func TestGetClientOpts(t *testing.T) {
 					},
 				},
 			},
-			authSecret: &corev1.Secret{
+			imagePullSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pull-secret",
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+				Data: map[string][]byte{
+					corev1.DockerConfigJsonKey: []byte(`{"auths":{"ghcr.io":{"username":"user","password":"pass","auth":"dXNlcjpwYXNz"}}}`),
+				},
+			},
+			afterFunc: func(t *WithT, hcOpts *ClientOpts) {
+				repo, err := name.NewRepository("ghcr.io/dummy")
+				t.Expect(err).ToNot(HaveOccurred())
+				authenticator, err := hcOpts.Keychain.Resolve(repo)
+				t.Expect(err).ToNot(HaveOccurred())
+				config, err := authenticator.Authorization()
+				t.Expect(err).ToNot(HaveOccurred())
+				t.Expect(config.Username).To(Equal("user"))
+				t.Expect(config.Password).To(Equal("pass"))
+			},
+			oci: true,
+		},
+		{
+			name:     "OCI HelmRepository with serviceaccount name and provider (serviceaccount takes precedence)",
+			provider: helmv1.AzureOCIProvider,
+			serviceAccount: &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-sa",
+				},
+				ImagePullSecrets: []corev1.LocalObjectReference{
+					{
+						Name: "pull-secret",
+					},
+				},
+			},
+			imagePullSecret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pull-secret",
 				},
@@ -159,6 +195,7 @@ func TestGetClientOpts(t *testing.T) {
 
 			helmRepo := &helmv1.HelmRepository{
 				Spec: helmv1.HelmRepositorySpec{
+					Provider: tt.provider,
 					Timeout: &metav1.Duration{
 						Duration: time.Second,
 					},
@@ -181,6 +218,9 @@ func TestGetClientOpts(t *testing.T) {
 					Name: tt.certSecret.Name,
 				}
 			}
+			if tt.imagePullSecret != nil {
+				clientBuilder.WithObjects(tt.imagePullSecret.DeepCopy())
+			}
 			if tt.serviceAccount != nil {
 				clientBuilder.WithObjects(tt.serviceAccount.DeepCopy())
 				helmRepo.Spec.ServiceAccountName = tt.serviceAccount.Name