Skip to content

Fix resolving default ServiceAccount for providers that don't use it #2232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matheuscscp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix resolving default ServiceAccount for providers that don't use it #2232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions internal/controller/gitrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

Expand Down
58 changes: 50 additions & 8 deletions internal/controller/gitrepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 5 additions & 3 deletions internal/controller/ocirepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
37 changes: 25 additions & 12 deletions internal/controller/ocirepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -527,6 +528,17 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) {
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
},
},
{
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 '<revision>' for '<url>'"),
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
},
},
{
name: "HTTP with basic auth secret",
want: sreconcile.ResultSuccess,
Expand Down Expand Up @@ -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"),
}

Expand Down
Loading