From 30459ec5f176ccc0e66c7d7b5d7dead6fafa8d6f Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Mon, 9 Nov 2026 16:33:24 +0000 Subject: [PATCH] Fix resolving default ServiceAccount for providers that don't use it Signed-off-by: Matheus Pimenta --- .../controller/gitrepository_controller.go | 36 +++++++----- .../gitrepository_controller_test.go | 58 ++++++++++++++++--- .../controller/ocirepository_controller.go | 8 ++- .../ocirepository_controller_test.go | 37 ++++++++---- 4 files changed, 100 insertions(+), 39 deletions(-) diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index bd45f2fff..119b4170e 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -712,23 +712,27 @@ func (r *GitRepositoryReconciler) getAuthOpts(ctx context.Context, obj *sourcev1 (opts.Username != "" || opts.Password != "" || opts.BearerToken != "" || len(opts.Identity) > 0 || len(authData[githubapp.KeyAppID]) > 0 || len(authData[githubapp.KeyAppClientID]) > 0) - // Resolve the ServiceAccount once, applying the default ServiceAccount when - // the object does not specify one. - saName := obj.Spec.ServiceAccountName - if saName == "" { - saName = r.DefaultServiceAccount - } + // Resolve the ServiceAccount only for the providers that use it for + // authentication. The default ServiceAccount does not apply to the generic + // and github providers, which do not use a ServiceAccount. var sa *corev1.ServiceAccount - if saName != "" { - sa = &corev1.ServiceAccount{} - saRef := types.NamespacedName{Namespace: obj.GetNamespace(), Name: saName} - if err := r.Get(ctx, saRef, sa); err != nil { - e := serror.NewGeneric( - fmt.Errorf("failed to get service account '%s': %w", saRef, err), - sourcev1.AuthenticationFailedReason, - ) - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) - return nil, e + switch provider { + case sourcev1.GitProviderAzure, sourcev1.GitProviderAWS: + saName := obj.Spec.ServiceAccountName + if saName == "" { + saName = r.DefaultServiceAccount + } + if saName != "" { + sa = &corev1.ServiceAccount{} + saRef := types.NamespacedName{Namespace: obj.GetNamespace(), Name: saName} + if err := r.Get(ctx, saRef, sa); err != nil { + e := serror.NewGeneric( + fmt.Errorf("failed to get service account '%s': %w", saRef, err), + sourcev1.AuthenticationFailedReason, + ) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e) + return nil, e + } } } diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 38aadca4d..44e908fa7 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -938,11 +938,12 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { func TestGitRepositoryReconciler_getAuthOpts_provider(t *testing.T) { tests := []struct { - name string - url string - secret *corev1.Secret - beforeFunc func(obj *sourcev1.GitRepository) - wantErr string + name string + url string + secret *corev1.Secret + beforeFunc func(obj *sourcev1.GitRepository) + defaultServiceAccount string + wantErr string }{ { name: "azure provider", @@ -1054,6 +1055,46 @@ func TestGitRepositoryReconciler_getAuthOpts_provider(t *testing.T) { obj.Spec.Provider = sourcev1.GitProviderGeneric }, }, + { + name: "generic provider ignores default service account", + url: "https://example.com/org/repo", + defaultServiceAccount: "missing-sa", + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.Provider = sourcev1.GitProviderGeneric + }, + }, + { + name: "no provider ignores default service account", + url: "https://example.com/org/repo", + defaultServiceAccount: "missing-sa", + }, + { + name: "github provider ignores default service account", + url: "https://github.com/org/repo.git", + defaultServiceAccount: "missing-sa", + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.Provider = sourcev1.GitProviderGitHub + }, + wantErr: "secretRef with github app data must be specified when provider is set to github", + }, + { + name: "aws provider resolves default service account", + url: "https://git-codecommit.us-east-1.amazonaws.com/v1/repos/my-repo", + defaultServiceAccount: "missing-sa", + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.Provider = sourcev1.GitProviderAWS + }, + wantErr: "failed to get service account", + }, + { + name: "azure provider resolves default service account", + url: "https://dev.azure.com/foo/bar/_git/baz", + defaultServiceAccount: "missing-sa", + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.Provider = sourcev1.GitProviderAzure + }, + wantErr: "failed to get service account", + }, { name: "secret ref defined for non existing secret", url: "https://github.com/org/repo.git", @@ -1083,9 +1124,10 @@ func TestGitRepositoryReconciler_getAuthOpts_provider(t *testing.T) { obj := &sourcev1.GitRepository{} r := &GitRepositoryReconciler{ - Recorder: events.NewFakeRecorder(32, false), - Client: clientBuilder.Build(), - patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), + Recorder: events.NewFakeRecorder(32, false), + Client: clientBuilder.Build(), + DefaultServiceAccount: tt.defaultServiceAccount, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), } url, err := url.Parse(tt.url) diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 63b430a52..8bbd8b26b 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -347,10 +347,12 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch provider := obj.GetProvider() - // Apply the default ServiceAccount when the object does not specify one, so - // that both the keychain and the auth library are aware of it. + // Apply the default ServiceAccount when the object does not specify one. + // The generic provider only uses a ServiceAccount when explicitly + // referenced by the object. Applying the default ServiceAccount to the + // generic provider would be a breaking change, so it is not supported. saName := obj.Spec.ServiceAccountName - if saName == "" { + if saName == "" && provider != sourcev1.GenericOCIProvider { saName = r.DefaultServiceAccount } diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index 8907d84d0..2e181042e 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -504,18 +504,19 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { pool.AppendCertsFromPEM(tlsCA) tests := []struct { - name string - url string - registryOpts registryOptions - craneOpts []crane.Option - secretOpts secretOptions - tlsCertSecret *corev1.Secret - insecure bool - provider string - providerImg string - want sreconcile.Result - wantErr bool - assertConditions []metav1.Condition + name string + url string + registryOpts registryOptions + craneOpts []crane.Option + secretOpts secretOptions + tlsCertSecret *corev1.Secret + insecure bool + provider string + providerImg string + defaultServiceAccount string + want sreconcile.Result + wantErr bool + assertConditions []metav1.Condition }{ { name: "HTTP without basic auth", @@ -527,6 +528,17 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, + { + name: "HTTP without basic auth ignores default service account", + want: sreconcile.ResultSuccess, + craneOpts: []crane.Option{crane.Insecure}, + insecure: true, + defaultServiceAccount: "missing-sa", + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + }, + }, { name: "HTTP with basic auth secret", want: sreconcile.ResultSuccess, @@ -888,6 +900,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { Recorder: events.NewFakeRecorder(32, false), Storage: testStorage, CosignVerifierFactory: testCosignVerifierFactory, + DefaultServiceAccount: tt.defaultServiceAccount, patchOptions: getPatchOptions(ociRepositoryReadyCondition.Owned, "sc"), }