#1148 - Fetch secret from cluster instead of cache on every api request

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,26 +1091,21 @@ 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
 	}
 
-	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 +1960,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.

pkg/externalrepo/git/git_test.go

@@ -23,13 +23,16 @@ import (
 	"path/filepath"
 	"reflect"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	gogit "github.com/go-git/go-git/v5"
 	"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,191 @@ 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, 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)
+	}
+
+	// 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, 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)
+	}
+}
+
+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)
+	}
+}