From 6b27a22d81aa3873fb38642b4ba93ef8cce88dd5 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 27 Aug 2026 12:51:58 +0100 Subject: [PATCH 1/2] Fetch secret from cluster instead of cache Signed-off-by: Kushal Harish Naidu --- pkg/externalrepo/git/git.go | 44 +++----- pkg/externalrepo/git/git_test.go | 185 +++++++++++++++++++++++++++++++ 2 files changed, 198 insertions(+), 31 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 5374008e3..fddeeac84 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -257,10 +257,6 @@ type gitRepository struct { // TODO: Better caching here, support repository spec changes deployment bool - // credential contains the information needed to authenticate against - // a git repository. - credential repository.Credential - // deletionProposedCache contains the deletionProposed branches that // exist in the repo so that we can easily check them without iterating // through all the refs each time @@ -1095,9 +1091,10 @@ func (r *gitRepository) dumpAllRefs() { } } -// getAuthMethod fetches the credentials for authenticating to git. It caches the -// credentials between calls and refresh credentials when the tokens have expired. -func (r *gitRepository) getAuthMethod(ctx context.Context, forceRefresh bool) (transport.AuthMethod, error) { +// getAuthMethod fetches the credentials for authenticating to git. +// The secret is re-read on every call to ensure changes to secret +// data are picked up promptly. +func (r *gitRepository) getAuthMethod(ctx context.Context) (transport.AuthMethod, error) { // If no secret is provided, we try without any auth. if r.secret == "" { return nil, nil @@ -1106,15 +1103,12 @@ func (r *gitRepository) getAuthMethod(ctx context.Context, forceRefresh bool) (t r.mutex.Lock() defer r.mutex.Unlock() - if r.credential == nil || !r.credential.Valid() || forceRefresh { - if cred, err := r.credentialResolver.ResolveCredential(ctx, r.Key().Namespace, r.secret); err != nil { - return nil, fmt.Errorf("failed to obtain credential from secret %s/%s: %w", r.Key().Namespace, r.secret, err) - } else { - r.credential = cred - } + cred, err := r.credentialResolver.ResolveCredential(ctx, r.Key().Namespace, r.secret) + if err != nil { + return nil, fmt.Errorf("failed to obtain credential from secret %s/%s: %w", r.Key().Namespace, r.secret, err) } - return r.credential.ToAuthMethod(), nil + return cred.ToAuthMethod(), nil } func (r *gitRepository) GetRepo() (string, error) { @@ -1969,27 +1963,15 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos }, nil } -// doGitWithAuth fetches auth information for git and provides it -// to the provided function which performs the operation against a git repo. +// doGitWithAuth fetches auth credentials and provides them to the +// operation. Retries are handled by the outer retry loop (e.g. +// fetchRemoteRepositoryWithRetry, pushAndCleanup). func (r *gitRepository) doGitWithAuth(ctx context.Context, op func(transport.AuthMethod) error) error { - auth, err := r.getAuthMethod(ctx, false) + auth, err := r.getAuthMethod(ctx) if err != nil { return fmt.Errorf("failed to obtain git credentials: %w", err) } - err = op(auth) - if err != nil { - if !pkgerrors.Is(err, transport.ErrAuthenticationRequired) { - return err - } - klog.Infof("Authentication failed. Trying to refresh credentials") - // TODO: Consider having some kind of backoff here. - auth, err := r.getAuthMethod(ctx, true) - if err != nil { - return fmt.Errorf("failed to obtain git credentials: %w", err) - } - return op(auth) - } - return nil + return op(auth) } // findPackage finds the packages in the git repository, under commit, if it is exists at path. diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index ae0c79dce..49aa79580 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -23,6 +23,7 @@ import ( "path/filepath" "reflect" "strings" + "sync" "testing" "time" @@ -30,6 +31,8 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v5/plumbing/transport/http" "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" @@ -2559,3 +2562,185 @@ func TestAppendRetryableErrors(t *testing.T) { }) } } + +// --- getAuthMethod tests --- + +// testCredential is a simple credential for testing getAuthMethod. +type testCredential struct { + username string + password string +} + +func (c *testCredential) Valid() bool { return true } +func (c *testCredential) ToAuthMethod() transport.AuthMethod { + return &http.BasicAuth{Username: c.username, Password: c.password} +} +func (c *testCredential) ToString() string { return c.username } + +// mutableCredentialResolver allows changing the returned credential between calls. +type mutableCredentialResolver struct { + mu sync.Mutex + username string + password string + calls int +} + +func (r *mutableCredentialResolver) ResolveCredential(_ context.Context, _, _ string) (repository.Credential, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls++ + return &testCredential{username: r.username, password: r.password}, nil +} + +func (r *mutableCredentialResolver) setCredentials(username, password string) { + r.mu.Lock() + defer r.mu.Unlock() + r.username = username + r.password = password +} + +func (r *mutableCredentialResolver) callCount() int { + r.mu.Lock() + defer r.mu.Unlock() + return r.calls +} + +// failingCredentialResolver returns an error on resolve. +type failingCredentialResolver struct{} + +func (r *failingCredentialResolver) ResolveCredential(_ context.Context, _, _ string) (repository.Credential, error) { + return nil, fmt.Errorf("secret not found") +} + +func newTestGitRepo(secret string, resolver repository.CredentialResolver) *gitRepository { + return &gitRepository{ + key: repository.RepositoryKey{ + Name: "test-repo", + Namespace: "test-ns", + }, + secret: secret, + credentialResolver: resolver, + } +} + +func TestGetAuthMethod_NoSecret(t *testing.T) { + repo := newTestGitRepo("", nil) + + auth, err := repo.getAuthMethod(context.Background()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if auth != nil { + t.Fatalf("expected nil auth for no secret, got %v", auth) + } +} + +func TestGetAuthMethod_ReturnsCredentials(t *testing.T) { + resolver := &mutableCredentialResolver{username: "user1", password: "pass1"} + repo := newTestGitRepo("my-secret", resolver) + + auth, err := repo.getAuthMethod(context.Background()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + basicAuth, ok := auth.(*http.BasicAuth) + if !ok { + t.Fatalf("expected *http.BasicAuth, got %T", auth) + } + if basicAuth.Username != "user1" || basicAuth.Password != "pass1" { + t.Fatalf("expected user1/pass1, got %s/%s", basicAuth.Username, basicAuth.Password) + } +} + +func TestGetAuthMethod_PicksUpSecretChanges(t *testing.T) { + resolver := &mutableCredentialResolver{username: "user1", password: "pass1"} + repo := newTestGitRepo("my-secret", resolver) + ctx := context.Background() + + // First call: returns user1 + auth, err := repo.getAuthMethod(ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + basicAuth := auth.(*http.BasicAuth) + if basicAuth.Username != "user1" { + t.Fatalf("expected user1, got %s", basicAuth.Username) + } + + // Simulate secret update: change to user2 + resolver.setCredentials("user2", "pass2") + + // Second call: should return user2 immediately (no auth failure needed) + auth, err = repo.getAuthMethod(ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + basicAuth = auth.(*http.BasicAuth) + if basicAuth.Username != "user2" { + t.Fatalf("expected user2 after secret change, got %s", basicAuth.Username) + } +} + +func TestGetAuthMethod_AlwaysCallsResolver(t *testing.T) { + resolver := &mutableCredentialResolver{username: "user1", password: "pass1"} + repo := newTestGitRepo("my-secret", resolver) + ctx := context.Background() + + for i := 0; i < 5; i++ { + _, err := repo.getAuthMethod(ctx) + if err != nil { + t.Fatalf("unexpected error on call %d: %v", i, err) + } + } + + if got := resolver.callCount(); got != 5 { + t.Fatalf("expected resolver to be called 5 times, got %d", got) + } +} + +func TestGetAuthMethod_ResolverError(t *testing.T) { + repo := newTestGitRepo("my-secret", &failingCredentialResolver{}) + + _, err := repo.getAuthMethod(context.Background()) + if err == nil { + t.Fatal("expected error, got nil") + } + if got := err.Error(); got != "failed to obtain credential from secret test-ns/my-secret: secret not found" { + t.Fatalf("unexpected error message: %s", got) + } +} + +func TestGetAuthMethod_ConcurrentAccess(t *testing.T) { + resolver := &mutableCredentialResolver{username: "user1", password: "pass1"} + repo := newTestGitRepo("my-secret", resolver) + ctx := context.Background() + + var wg sync.WaitGroup + errs := make(chan error, 100) + + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + auth, err := repo.getAuthMethod(ctx) + if err != nil { + errs <- err + return + } + if auth == nil { + errs <- fmt.Errorf("got nil auth") + } + }() + } + wg.Wait() + close(errs) + + for err := range errs { + t.Fatalf("concurrent access error: %v", err) + } + + if got := resolver.callCount(); got != 100 { + t.Fatalf("expected 100 resolver calls, got %d", got) + } +} From e0a71ccb4cd758ca6d546e0fdf4b8a18c72a396f Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 27 Aug 2026 13:07:11 +0100 Subject: [PATCH 2/2] Remove unnecessary mutex and fix type assertions in getAuthMethod tests Signed-off-by: Kushal Harish Naidu --- pkg/externalrepo/git/git.go | 3 --- pkg/externalrepo/git/git_test.go | 10 ++++++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index fddeeac84..84c37eb37 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -1100,9 +1100,6 @@ func (r *gitRepository) getAuthMethod(ctx context.Context) (transport.AuthMethod return nil, nil } - r.mutex.Lock() - defer r.mutex.Unlock() - cred, err := r.credentialResolver.ResolveCredential(ctx, r.Key().Namespace, r.secret) if err != nil { return nil, fmt.Errorf("failed to obtain credential from secret %s/%s: %w", r.Key().Namespace, r.secret, err) diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index 49aa79580..da7734750 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -2663,7 +2663,10 @@ func TestGetAuthMethod_PicksUpSecretChanges(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - basicAuth := auth.(*http.BasicAuth) + basicAuth, ok := auth.(*http.BasicAuth) + if !ok { + t.Fatalf("expected *http.BasicAuth, got %T", auth) + } if basicAuth.Username != "user1" { t.Fatalf("expected user1, got %s", basicAuth.Username) } @@ -2676,7 +2679,10 @@ func TestGetAuthMethod_PicksUpSecretChanges(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - basicAuth = auth.(*http.BasicAuth) + basicAuth, ok = auth.(*http.BasicAuth) + if !ok { + t.Fatalf("expected *http.BasicAuth after secret change, got %T", auth) + } if basicAuth.Username != "user2" { t.Fatalf("expected user2 after secret change, got %s", basicAuth.Username) }