From 3b8e1bc2769ba251760bba69dbfb98e82f44262b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 15:45:09 +0000 Subject: [PATCH] Address PR review comments: fix ref counting, dedup resolveAuth, remove unused CacheStatus - Fix Release() ref counting bug: avoid calling getRef() which increments count, instead look up ref directly and clean up when count reaches zero - Remove unpopulated CacheStatus struct and fields from types, deepcopy, and CRD manifest since nothing writes to them - Extract duplicated resolveAuth into shared pkg/gitauth package with explicit key validation for missing "username"/"password" keys - Fix GC() race condition by skipping paths with active in-process refs - Remove misleading WorkspaceCacheMiss metric increment for in-memory (non-cache) code paths - Change push controller from shallow to full clones to avoid silent failures when transactions reference historical commits - Add tests for ref count cleanup, ref leak prevention, GC active ref skipping, and gitauth key validation https://claude.ai/code/session_01WJE6ozYXZMn1CD6d4vy8b6 --- cmd/push-controller/main.go | 5 +- .../git-k8s.imjasonh.com_gitrepositories.yaml | 13 -- pkg/apis/git/v1alpha1/types.go | 19 -- .../git/v1alpha1/zz_generated.deepcopy.go | 28 --- pkg/gitauth/gitauth.go | 58 +++++ pkg/gitauth/gitauth_test.go | 200 ++++++++++++++++++ pkg/reconciler/push/push.go | 42 +--- pkg/reconciler/push/push_test.go | 9 +- pkg/reconciler/push/reconcile_test.go | 5 +- pkg/reconciler/resolver/resolver.go | 37 +--- pkg/reconciler/sync/sync.go | 37 +--- pkg/workspace/workspace.go | 35 ++- pkg/workspace/workspace_test.go | 88 ++++++++ 13 files changed, 390 insertions(+), 186 deletions(-) create mode 100644 pkg/gitauth/gitauth.go create mode 100644 pkg/gitauth/gitauth_test.go diff --git a/cmd/push-controller/main.go b/cmd/push-controller/main.go index dad0e42..a848b6f 100644 --- a/cmd/push-controller/main.go +++ b/cmd/push-controller/main.go @@ -17,9 +17,10 @@ func main() { go health.ServeMetrics(ctx, ":9090") //nolint:errcheck // GIT_CACHE_DIR enables PVC-backed workspace caching when set. - // Push controller uses shallow clones since it only needs to push refs. + // Push controller needs full history to resolve refs referenced by + // push transactions (e.g., older tags or historical commits). cacheDir := os.Getenv("GIT_CACHE_DIR") - wsMgr := workspace.NewManager(cacheDir, true /* shallow */) + wsMgr := workspace.NewManager(cacheDir, false) ctx = workspace.WithManager(ctx, wsMgr) sharedmain.MainWithContext(ctx, "push-controller", push.NewController) diff --git a/config/crds/git-k8s.imjasonh.com_gitrepositories.yaml b/config/crds/git-k8s.imjasonh.com_gitrepositories.yaml index 5a8c28a..e04c111 100644 --- a/config/crds/git-k8s.imjasonh.com_gitrepositories.yaml +++ b/config/crds/git-k8s.imjasonh.com_gitrepositories.yaml @@ -80,19 +80,6 @@ spec: lastFetchTime: type: string format: date-time - cache: - type: object - description: Cache health information. - properties: - lastCloneTime: - type: string - format: date-time - lastFetchTime: - type: string - format: date-time - sizeBytes: - type: integer - format: int64 additionalPrinterColumns: - name: URL type: string diff --git a/pkg/apis/git/v1alpha1/types.go b/pkg/apis/git/v1alpha1/types.go index 45e2ccc..f77d2bd 100644 --- a/pkg/apis/git/v1alpha1/types.go +++ b/pkg/apis/git/v1alpha1/types.go @@ -70,25 +70,6 @@ type GitRepositoryStatus struct { // LastFetchTime is the timestamp of the last successful fetch. // +optional LastFetchTime *metav1.Time `json:"lastFetchTime,omitempty"` - - // Cache contains cache health information. - // +optional - Cache *CacheStatus `json:"cache,omitempty"` -} - -// CacheStatus reports the current state of the on-disk cache for a repository. -type CacheStatus struct { - // LastCloneTime is the timestamp of the initial clone to disk. - // +optional - LastCloneTime *metav1.Time `json:"lastCloneTime,omitempty"` - - // LastFetchTime is the timestamp of the last git fetch on the cached repo. - // +optional - LastFetchTime *metav1.Time `json:"lastFetchTime,omitempty"` - - // SizeBytes is the approximate size of the cached repo on disk. - // +optional - SizeBytes int64 `json:"sizeBytes,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/git/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/git/v1alpha1/zz_generated.deepcopy.go index 362f60a..f109781 100644 --- a/pkg/apis/git/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/git/v1alpha1/zz_generated.deepcopy.go @@ -25,29 +25,6 @@ func (in *CacheConfig) DeepCopy() *CacheConfig { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CacheStatus) DeepCopyInto(out *CacheStatus) { - *out = *in - if in.LastCloneTime != nil { - in, out := &in.LastCloneTime, &out.LastCloneTime - *out = (*in).DeepCopy() - } - if in.LastFetchTime != nil { - in, out := &in.LastFetchTime, &out.LastFetchTime - *out = (*in).DeepCopy() - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CacheStatus. -func (in *CacheStatus) DeepCopy() *CacheStatus { - if in == nil { - return nil - } - out := new(CacheStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitAuth) DeepCopyInto(out *GitAuth) { *out = *in @@ -148,11 +125,6 @@ func (in *GitRepositoryStatus) DeepCopyInto(out *GitRepositoryStatus) { in, out := &in.LastFetchTime, &out.LastFetchTime *out = (*in).DeepCopy() } - if in.Cache != nil { - in, out := &in.Cache, &out.Cache - *out = new(CacheStatus) - (*in).DeepCopyInto(*out) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitRepositoryStatus. diff --git a/pkg/gitauth/gitauth.go b/pkg/gitauth/gitauth.go new file mode 100644 index 0000000..9236efc --- /dev/null +++ b/pkg/gitauth/gitauth.go @@ -0,0 +1,58 @@ +package gitauth + +import ( + "context" + "encoding/base64" + "fmt" + + "github.com/go-git/go-git/v5/plumbing/transport/http" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/dynamic" + + gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" +) + +// ResolveAuth retrieves Git authentication credentials from the Secret +// referenced by the GitRepository's auth configuration. Returns nil if +// no auth is configured. +func ResolveAuth(ctx context.Context, dynamicClient dynamic.Interface, namespace string, repo *gitv1alpha1.GitRepository) (*http.BasicAuth, error) { + if repo.Spec.Auth == nil || repo.Spec.Auth.SecretRef == nil { + return nil, nil + } + + secretGVR := corev1.SchemeGroupVersion.WithResource("secrets") + u, err := dynamicClient.Resource(secretGVR).Namespace(namespace).Get(ctx, repo.Spec.Auth.SecretRef.Name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("getting secret %q: %w", repo.Spec.Auth.SecretRef.Name, err) + } + + data, found, err := unstructured.NestedStringMap(u.Object, "data") + if err != nil || !found { + return nil, fmt.Errorf("reading secret data: found=%v err=%v", found, err) + } + + usernameB64, ok := data["username"] + if !ok { + return nil, fmt.Errorf("secret %q is missing required key %q", repo.Spec.Auth.SecretRef.Name, "username") + } + passwordB64, ok := data["password"] + if !ok { + return nil, fmt.Errorf("secret %q is missing required key %q", repo.Spec.Auth.SecretRef.Name, "password") + } + + username, err := base64.StdEncoding.DecodeString(usernameB64) + if err != nil { + return nil, fmt.Errorf("decoding username: %w", err) + } + password, err := base64.StdEncoding.DecodeString(passwordB64) + if err != nil { + return nil, fmt.Errorf("decoding password: %w", err) + } + + return &http.BasicAuth{ + Username: string(username), + Password: string(password), + }, nil +} diff --git a/pkg/gitauth/gitauth_test.go b/pkg/gitauth/gitauth_test.go new file mode 100644 index 0000000..74dd8c8 --- /dev/null +++ b/pkg/gitauth/gitauth_test.go @@ -0,0 +1,200 @@ +package gitauth + +import ( + "context" + "encoding/base64" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + fakedynamic "k8s.io/client-go/dynamic/fake" + + gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" +) + +func newFakeSecret(name, namespace, username, password string) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("v1") + obj.SetKind("Secret") + obj.SetName(name) + obj.SetNamespace(namespace) + obj.Object["data"] = map[string]interface{}{ + "username": base64.StdEncoding.EncodeToString([]byte(username)), + "password": base64.StdEncoding.EncodeToString([]byte(password)), + } + return obj +} + +func TestResolveAuth_NoAuth(t *testing.T) { + scheme := runtime.NewScheme() + client := fakedynamic.NewSimpleDynamicClient(scheme) + + repo := &gitv1alpha1.GitRepository{ + Spec: gitv1alpha1.GitRepositorySpec{URL: "https://example.com/repo.git"}, + } + auth, err := ResolveAuth(context.Background(), client, "default", repo) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if auth != nil { + t.Fatal("expected nil auth for repo without auth config") + } +} + +func TestResolveAuth_NoSecretRef(t *testing.T) { + scheme := runtime.NewScheme() + client := fakedynamic.NewSimpleDynamicClient(scheme) + + repo := &gitv1alpha1.GitRepository{ + Spec: gitv1alpha1.GitRepositorySpec{ + URL: "https://example.com/repo.git", + Auth: &gitv1alpha1.GitAuth{}, + }, + } + auth, err := ResolveAuth(context.Background(), client, "default", repo) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if auth != nil { + t.Fatal("expected nil auth for repo with nil secretRef") + } +} + +func TestResolveAuth_ValidSecret(t *testing.T) { + scheme := runtime.NewScheme() + secret := newFakeSecret("my-secret", "default", "myuser", "mypass") + client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, + map[schema.GroupVersionResource]string{ + {Group: "", Version: "v1", Resource: "secrets"}: "SecretList", + }, + secret, + ) + + repo := &gitv1alpha1.GitRepository{ + Spec: gitv1alpha1.GitRepositorySpec{ + URL: "https://example.com/repo.git", + Auth: &gitv1alpha1.GitAuth{ + SecretRef: &gitv1alpha1.SecretRef{Name: "my-secret"}, + }, + }, + } + auth, err := ResolveAuth(context.Background(), client, "default", repo) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if auth == nil { + t.Fatal("expected non-nil auth") + } + if auth.Username != "myuser" { + t.Errorf("username = %q, want %q", auth.Username, "myuser") + } + if auth.Password != "mypass" { + t.Errorf("password = %q, want %q", auth.Password, "mypass") + } +} + +func TestResolveAuth_MissingUsernameKey(t *testing.T) { + scheme := runtime.NewScheme() + secret := &unstructured.Unstructured{} + secret.SetAPIVersion("v1") + secret.SetKind("Secret") + secret.SetName("bad-secret") + secret.SetNamespace("default") + secret.Object["data"] = map[string]interface{}{ + "password": base64.StdEncoding.EncodeToString([]byte("pass")), + } + client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, + map[schema.GroupVersionResource]string{ + {Group: "", Version: "v1", Resource: "secrets"}: "SecretList", + }, + secret, + ) + + repo := &gitv1alpha1.GitRepository{ + Spec: gitv1alpha1.GitRepositorySpec{ + URL: "https://example.com/repo.git", + Auth: &gitv1alpha1.GitAuth{ + SecretRef: &gitv1alpha1.SecretRef{Name: "bad-secret"}, + }, + }, + } + _, err := ResolveAuth(context.Background(), client, "default", repo) + if err == nil { + t.Fatal("expected error for missing username key") + } + if got := err.Error(); !contains(got, "missing required key") { + t.Errorf("error = %q, want to contain %q", got, "missing required key") + } +} + +func TestResolveAuth_MissingPasswordKey(t *testing.T) { + scheme := runtime.NewScheme() + secret := &unstructured.Unstructured{} + secret.SetAPIVersion("v1") + secret.SetKind("Secret") + secret.SetName("bad-secret") + secret.SetNamespace("default") + secret.Object["data"] = map[string]interface{}{ + "username": base64.StdEncoding.EncodeToString([]byte("user")), + } + client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, + map[schema.GroupVersionResource]string{ + {Group: "", Version: "v1", Resource: "secrets"}: "SecretList", + }, + secret, + ) + + repo := &gitv1alpha1.GitRepository{ + Spec: gitv1alpha1.GitRepositorySpec{ + URL: "https://example.com/repo.git", + Auth: &gitv1alpha1.GitAuth{ + SecretRef: &gitv1alpha1.SecretRef{Name: "bad-secret"}, + }, + }, + } + _, err := ResolveAuth(context.Background(), client, "default", repo) + if err == nil { + t.Fatal("expected error for missing password key") + } + if got := err.Error(); !contains(got, "missing required key") { + t.Errorf("error = %q, want to contain %q", got, "missing required key") + } +} + +func TestResolveAuth_SecretNotFound(t *testing.T) { + scheme := runtime.NewScheme() + client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, + map[schema.GroupVersionResource]string{ + {Group: "", Version: "v1", Resource: "secrets"}: "SecretList", + }, + ) + + repo := &gitv1alpha1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "default"}, + Spec: gitv1alpha1.GitRepositorySpec{ + URL: "https://example.com/repo.git", + Auth: &gitv1alpha1.GitAuth{ + SecretRef: &gitv1alpha1.SecretRef{Name: "nonexistent"}, + }, + }, + } + _, err := ResolveAuth(context.Background(), client, "default", repo) + if err == nil { + t.Fatal("expected error for missing secret") + } +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr)) +} + +func containsSubstring(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/pkg/reconciler/push/push.go b/pkg/reconciler/push/push.go index 762d342..5a6d425 100644 --- a/pkg/reconciler/push/push.go +++ b/pkg/reconciler/push/push.go @@ -2,7 +2,6 @@ package push import ( "context" - "encoding/base64" "fmt" "time" @@ -10,15 +9,14 @@ import ( "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/transport/http" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/gitauth" "github.com/imjasonh/git-k8s/pkg/metrics" "github.com/imjasonh/git-k8s/pkg/workspace" ) @@ -61,7 +59,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, txn *gitv1alpha1.GitPush } // Resolve authentication. - auth, err := r.resolveAuth(ctx, namespace, repo) + auth, err := gitauth.ResolveAuth(ctx, r.dynamicClient, namespace, repo) if err != nil { return r.failTransaction(ctx, txn, fmt.Sprintf("resolving auth: %v", err)) } @@ -146,42 +144,6 @@ func (r *Reconciler) executePush(ctx context.Context, repoURL string, spec gitv1 return resultCommit, nil } -// resolveAuth retrieves Git authentication credentials from the referenced Secret. -func (r *Reconciler) resolveAuth(ctx context.Context, namespace string, repo *gitv1alpha1.GitRepository) (*http.BasicAuth, error) { - if repo.Spec.Auth == nil || repo.Spec.Auth.SecretRef == nil { - return nil, nil - } - - // Use the dynamic client to fetch the Secret. - secretGVR := corev1.SchemeGroupVersion.WithResource("secrets") - u, err := r.dynamicClient.Resource(secretGVR).Namespace(namespace).Get(ctx, repo.Spec.Auth.SecretRef.Name, metav1.GetOptions{}) - if err != nil { - return nil, fmt.Errorf("getting secret %q: %w", repo.Spec.Auth.SecretRef.Name, err) - } - - data, found, err := unstructured.NestedStringMap(u.Object, "data") - if err != nil || !found { - return nil, fmt.Errorf("reading secret data: found=%v err=%v", found, err) - } - - // Secret data values are base64-encoded in the API response when - // accessed via the dynamic client (unlike the typed client which - // decodes automatically into []byte fields). - username, err := base64.StdEncoding.DecodeString(data["username"]) - if err != nil { - return nil, fmt.Errorf("decoding username: %w", err) - } - password, err := base64.StdEncoding.DecodeString(data["password"]) - if err != nil { - return nil, fmt.Errorf("decoding password: %w", err) - } - - return &http.BasicAuth{ - Username: string(username), - Password: string(password), - }, nil -} - // failTransaction marks a GitPushTransaction as Failed with the given message. func (r *Reconciler) failTransaction(ctx context.Context, txn *gitv1alpha1.GitPushTransaction, message string) error { logger := logging.FromContext(ctx) diff --git a/pkg/reconciler/push/push_test.go b/pkg/reconciler/push/push_test.go index f46e5c6..0449d44 100644 --- a/pkg/reconciler/push/push_test.go +++ b/pkg/reconciler/push/push_test.go @@ -13,6 +13,7 @@ import ( gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/gitauth" "github.com/imjasonh/git-k8s/pkg/workspace" ) @@ -318,9 +319,9 @@ func TestResolveAuth_NoAuth(t *testing.T) { }, } - auth, err := r.resolveAuth(context.Background(), "default", repo) + auth, err := gitauth.ResolveAuth(context.Background(), r.dynamicClient, "default", repo) if err != nil { - t.Fatalf("resolveAuth() error = %v", err) + t.Fatalf("ResolveAuth() error = %v", err) } if auth != nil { t.Errorf("auth = %v, want nil for repo without auth", auth) @@ -354,9 +355,9 @@ func TestResolveAuth_WithSecret(t *testing.T) { }, } - auth, err := r.resolveAuth(context.Background(), "default", repo) + auth, err := gitauth.ResolveAuth(context.Background(), r.dynamicClient, "default", repo) if err != nil { - t.Fatalf("resolveAuth() error = %v", err) + t.Fatalf("ResolveAuth() error = %v", err) } if auth == nil { t.Fatal("auth = nil, want non-nil") diff --git a/pkg/reconciler/push/reconcile_test.go b/pkg/reconciler/push/reconcile_test.go index a068eea..d5f9942 100644 --- a/pkg/reconciler/push/reconcile_test.go +++ b/pkg/reconciler/push/reconcile_test.go @@ -9,6 +9,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" + "github.com/imjasonh/git-k8s/pkg/gitauth" ) func TestReconcileKind_PendingToInProgress(t *testing.T) { @@ -148,7 +149,7 @@ func TestResolveAuth_MissingSecret(t *testing.T) { }, } - _, err := r.resolveAuth(context.Background(), "default", repo) + _, err := gitauth.ResolveAuth(context.Background(), r.dynamicClient, "default", repo) if err == nil { t.Fatal("resolveAuth() should return error for missing secret") } @@ -176,7 +177,7 @@ func TestResolveAuth_InvalidBase64(t *testing.T) { }, } - _, err := r.resolveAuth(context.Background(), "default", repo) + _, err := gitauth.ResolveAuth(context.Background(), r.dynamicClient, "default", repo) if err == nil { t.Fatal("resolveAuth() should return error for invalid base64") } diff --git a/pkg/reconciler/resolver/resolver.go b/pkg/reconciler/resolver/resolver.go index c66dd9b..9f8efd5 100644 --- a/pkg/reconciler/resolver/resolver.go +++ b/pkg/reconciler/resolver/resolver.go @@ -2,7 +2,6 @@ package resolver import ( "context" - "encoding/base64" "fmt" "time" @@ -10,15 +9,14 @@ import ( "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/plumbing/storer" "github.com/go-git/go-git/v5/plumbing/transport/http" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/gitauth" "github.com/imjasonh/git-k8s/pkg/workspace" ) @@ -56,7 +54,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, syncObj *gitv1alpha1.Git } // Resolve auth for cloning. - auth, err := r.resolveAuth(ctx, namespace, repoA) + auth, err := gitauth.ResolveAuth(ctx, r.dynamicClient, namespace, repoA) if err != nil { return fmt.Errorf("resolving auth for repo A: %w", err) } @@ -353,34 +351,3 @@ func (r *Reconciler) markManualIntervention(ctx context.Context, syncObj *gitv1a return err } -// resolveAuth retrieves Git authentication credentials from the referenced Secret. -func (r *Reconciler) resolveAuth(ctx context.Context, namespace string, repo *gitv1alpha1.GitRepository) (*http.BasicAuth, error) { - if repo.Spec.Auth == nil || repo.Spec.Auth.SecretRef == nil { - return nil, nil - } - - secretGVR := corev1.SchemeGroupVersion.WithResource("secrets") - u, err := r.dynamicClient.Resource(secretGVR).Namespace(namespace).Get(ctx, repo.Spec.Auth.SecretRef.Name, metav1.GetOptions{}) - if err != nil { - return nil, fmt.Errorf("getting secret %q: %w", repo.Spec.Auth.SecretRef.Name, err) - } - - data, found, err := unstructured.NestedStringMap(u.Object, "data") - if err != nil || !found { - return nil, fmt.Errorf("reading secret data: found=%v err=%v", found, err) - } - - username, err := base64.StdEncoding.DecodeString(data["username"]) - if err != nil { - return nil, fmt.Errorf("decoding username: %w", err) - } - password, err := base64.StdEncoding.DecodeString(data["password"]) - if err != nil { - return nil, fmt.Errorf("decoding password: %w", err) - } - - return &http.BasicAuth{ - Username: string(username), - Password: string(password), - }, nil -} diff --git a/pkg/reconciler/sync/sync.go b/pkg/reconciler/sync/sync.go index 1365864..39a7070 100644 --- a/pkg/reconciler/sync/sync.go +++ b/pkg/reconciler/sync/sync.go @@ -2,20 +2,18 @@ package sync import ( "context" - "encoding/base64" "fmt" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/transport/http" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/dynamic" "knative.dev/pkg/logging" "knative.dev/pkg/reconciler" gitv1alpha1 "github.com/imjasonh/git-k8s/pkg/apis/git/v1alpha1" gitclient "github.com/imjasonh/git-k8s/pkg/client" + "github.com/imjasonh/git-k8s/pkg/gitauth" "github.com/imjasonh/git-k8s/pkg/workspace" ) @@ -64,7 +62,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, syncObj *gitv1alpha1.Git } // Resolve auth for cloning. - auth, err := r.resolveAuth(ctx, namespace, repoASpec) + auth, err := gitauth.ResolveAuth(ctx, r.dynamicClient, namespace, repoASpec) if err != nil { return fmt.Errorf("resolving auth for repo A: %w", err) } @@ -212,34 +210,3 @@ func (r *Reconciler) updateSyncStatus(ctx context.Context, syncObj *gitv1alpha1. return err } -// resolveAuth retrieves Git authentication credentials from the referenced Secret. -func (r *Reconciler) resolveAuth(ctx context.Context, namespace string, repo *gitv1alpha1.GitRepository) (*http.BasicAuth, error) { - if repo.Spec.Auth == nil || repo.Spec.Auth.SecretRef == nil { - return nil, nil - } - - secretGVR := corev1.SchemeGroupVersion.WithResource("secrets") - u, err := r.dynamicClient.Resource(secretGVR).Namespace(namespace).Get(ctx, repo.Spec.Auth.SecretRef.Name, metav1.GetOptions{}) - if err != nil { - return nil, fmt.Errorf("getting secret %q: %w", repo.Spec.Auth.SecretRef.Name, err) - } - - data, found, err := unstructured.NestedStringMap(u.Object, "data") - if err != nil || !found { - return nil, fmt.Errorf("reading secret data: found=%v err=%v", found, err) - } - - username, err := base64.StdEncoding.DecodeString(data["username"]) - if err != nil { - return nil, fmt.Errorf("decoding username: %w", err) - } - password, err := base64.StdEncoding.DecodeString(data["password"]) - if err != nil { - return nil, fmt.Errorf("decoding password: %w", err) - } - - return &http.BasicAuth{ - Username: string(username), - Password: string(password), - }, nil -} diff --git a/pkg/workspace/workspace.go b/pkg/workspace/workspace.go index fcc10c7..0b27516 100644 --- a/pkg/workspace/workspace.go +++ b/pkg/workspace/workspace.go @@ -107,10 +107,8 @@ func (m *Manager) acquireInMemory(ctx context.Context, repoURL string, auth *htt }) metrics.WorkspaceAcquireDuration.WithLabelValues("memory").Observe(time.Since(start).Seconds()) if err != nil { - metrics.WorkspaceCacheMiss.Inc() return nil, fmt.Errorf("in-memory clone: %w", err) } - metrics.WorkspaceCacheMiss.Inc() return &Workspace{ Repo: repo, @@ -210,8 +208,18 @@ func (m *Manager) Release(ws *Workspace) { if ws == nil || ws.manager == nil { return } - r := m.getRef(ws.path) + m.mu.Lock() + r, ok := m.active[ws.path] + m.mu.Unlock() + if !ok { + return + } r.count-- + if r.count == 0 { + m.mu.Lock() + delete(m.active, ws.path) + m.mu.Unlock() + } r.mu.Unlock() } @@ -229,6 +237,8 @@ func (m *Manager) getRef(path string) *ref { } // GC removes cached directories that are not in the activeRepos set. +// It skips paths that have active in-process references to avoid racing +// with ongoing reconciles. func (m *Manager) GC(ctx context.Context, activeRepos map[string]bool) { if m.basePath == "" { return @@ -246,11 +256,20 @@ func (m *Manager) GC(ctx context.Context, activeRepos map[string]bool) { continue } dirPath := filepath.Join(m.basePath, entry.Name()) - if !activeRepos[dirPath] { - logger.Infof("GC: removing stale cache dir %s", dirPath) - if err := os.RemoveAll(dirPath); err != nil { - logger.Warnf("GC: removing %s: %v", dirPath, err) - } + if activeRepos[dirPath] { + continue + } + // Skip paths with active in-process references. + m.mu.Lock() + r, hasRef := m.active[dirPath] + m.mu.Unlock() + if hasRef && r.count > 0 { + logger.Infof("GC: skipping %s (active references)", dirPath) + continue + } + logger.Infof("GC: removing stale cache dir %s", dirPath) + if err := os.RemoveAll(dirPath); err != nil { + logger.Warnf("GC: removing %s: %v", dirPath, err) } } } diff --git a/pkg/workspace/workspace_test.go b/pkg/workspace/workspace_test.go index 7db7bad..1d6e0f2 100644 --- a/pkg/workspace/workspace_test.go +++ b/pkg/workspace/workspace_test.go @@ -424,3 +424,91 @@ func TestInMemoryStorerCompatibility(t *testing.T) { t.Fatal("repo is nil") } } + +// Test that Release properly decrements ref count and cleans up the active map. +func TestRelease_RefCountCleanup(t *testing.T) { + sourceDir := t.TempDir() + barePath, _ := initBareRepo(t, sourceDir) + + cacheDir := t.TempDir() + m := NewManager(cacheDir, false) + ctx := testCtx(t) + + ws, err := m.Acquire(ctx, barePath, nil, true) + if err != nil { + t.Fatalf("Acquire: %v", err) + } + + // After acquire, ref should exist with count 1. + m.mu.Lock() + r, ok := m.active[ws.path] + m.mu.Unlock() + if !ok { + t.Fatal("expected active ref after acquire") + } + if r.count != 1 { + t.Errorf("ref count after acquire = %d, want 1", r.count) + } + + m.Release(ws) + + // After release, the ref should be cleaned up from the active map. + m.mu.Lock() + _, ok = m.active[ws.path] + m.mu.Unlock() + if ok { + t.Error("expected active ref to be cleaned up after release") + } +} + +// Test that multiple acquire/release cycles don't leak refs. +func TestRelease_NoRefLeak(t *testing.T) { + sourceDir := t.TempDir() + barePath, _ := initBareRepo(t, sourceDir) + + cacheDir := t.TempDir() + m := NewManager(cacheDir, false) + ctx := testCtx(t) + + for i := 0; i < 5; i++ { + ws, err := m.Acquire(ctx, barePath, nil, true) + if err != nil { + t.Fatalf("Acquire #%d: %v", i, err) + } + m.Release(ws) + } + + // After all cycles, active map should be empty. + m.mu.Lock() + activeCount := len(m.active) + m.mu.Unlock() + if activeCount != 0 { + t.Errorf("active map has %d entries, want 0 after all releases", activeCount) + } +} + +// Test that GC skips directories with active references. +func TestGC_SkipsActiveRefs(t *testing.T) { + sourceDir := t.TempDir() + barePath, _ := initBareRepo(t, sourceDir) + + cacheDir := t.TempDir() + m := NewManager(cacheDir, false) + ctx := testCtx(t) + + // Acquire a workspace (creates a cached dir and holds a ref). + ws, err := m.Acquire(ctx, barePath, nil, true) + if err != nil { + t.Fatalf("Acquire: %v", err) + } + + // Run GC with empty activeRepos — the cache dir should survive because + // there's an active in-process reference. + m.GC(ctx, map[string]bool{}) + + if _, err := os.Stat(ws.path); os.IsNotExist(err) { + t.Error("GC should not remove dir with active references") + } + + m.Release(ws) +}