From 4e5cb3c9d929c90b07d5462fb1838a0ce4070c58 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 22 Jul 2026 09:47:15 +0000 Subject: [PATCH 1/2] fix(attribution): resolve the audit route from one ClusterProvider read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auditRouteFor re-read the GitTarget's ClusterProvider and, when that read failed, returned the provider NAME as the audit route. Its comment claimed this "never a route that silently matches no facts" — true only for a provider that declares no spec.attribution.auditRoute. A provider that declares `shared-route` was declared under its own name instead, filing this target's streams against a partition the audit webhook never writes to: attribution silently resolves nobody, or resolves another cluster's facts if some provider happens to be NAMED like this one's route, writing a wrong author into Git. One reconcile read the same ClusterProvider three times with three different failure policies (error, guess, Unknown), which is what let the wrong one in. It is now read once, after the Validated gate that already proved it exists and admits the namespace, and handed to both consumers: - auditRouteFor is gone; the call site uses AuditRoute() on that object. A read failure returns an error and requeues, the same policy the gate already applies to the identical failure — a route is never guessed. - clusterProviderReadiness takes the provider instead of reading its own, so it is a pure function with no ctx, client, or "cannot observe" branch. That branch was unreachable anyway: a missing provider is the gate's hard denial. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/controller/gittarget_controller.go | 11 ++- .../controller/gittarget_source_cluster.go | 56 ++++++++------- .../gittarget_source_cluster_test.go | 70 +++++++++---------- 3 files changed, 72 insertions(+), 65 deletions(-) diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 3596ad91..8c9ca1b1 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -201,6 +201,13 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } + // One read of the source ClusterProvider serves everything below it: the audit route captured on + // Declare and the ClusterProviderReady projection. Reading it twice invited the two to disagree. + sourceProvider, sourceProviderErr := r.resolveSourceClusterProvider(ctx, &target) + if sourceProviderErr != nil { + return ctrl.Result{}, sourceProviderErr + } + // Ensure watch-first data-plane streams exist, then project source and target readiness // into the kstatus trio. streamsSettling := false @@ -219,7 +226,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ctx, gitDest, target.SourceCluster(), - r.auditRouteFor(ctx, &target), + sourceProvider.AuditRoute(), target.EffectivePruneMode(), gitPathWasRefused, ); declareErr != nil { @@ -260,7 +267,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( sourceReach = r.EventRouter.WatchManager.SourceClusterReachable(target.SourceCluster()) } providerStatus, providerReason, providerMessage := r.gitProviderReadiness(ctx, &target, providerNS) - cpStatus, cpReason, cpMessage := r.clusterProviderReadiness(ctx, &target) + cpStatus, cpReason, cpMessage := clusterProviderReadiness(sourceProvider) r.projectSourceAndProvider(&target, sourceReach, providerStatus, providerReason, providerMessage, cpStatus, cpReason, cpMessage) streamsSettling = streamsSettling || sourceReach.State != "True" || diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index 9fef6ee8..616603d8 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -71,9 +71,9 @@ const ( // the GitTarget, so one `kubectl get gittarget` shows whether the SOURCE cluster's provider is // healthy — distinct from SourceClusterReachable (the data plane's runtime reach) and // GitProviderReady (the destination). It follows the GitProviderReady contract: only an - // EXPLICIT Ready=False downgrades the GitTarget; a not-found or not-yet-reported provider is - // Unknown and does not (so a single-cluster install that has not yet installed the "default" - // provider is not held down). + // EXPLICIT Ready=False downgrades the GitTarget; a provider that has not reported readiness yet + // is Unknown and does not. A MISSING provider never reaches this projection at all — that is the + // Validated gate's hard denial (GitTargetReasonClusterProviderNotFound), not a readiness axis. GitTargetConditionClusterProviderReady = "ClusterProviderReady" // GitTargetReasonClusterProviderNotReady is the ClusterProviderReady=False/Unknown reason. GitTargetReasonClusterProviderNotReady = "ClusterProviderNotReady" @@ -81,40 +81,42 @@ const ( GitTargetReasonClusterProviderReady = "ClusterProviderReady" ) -// auditRouteFor resolves the audit route this GitTarget's attribution facts are keyed under, from -// the referenced ClusterProvider's spec.attribution.auditRoute. It is read here, on the control -// plane, so the watch data plane can capture it on Declare and never needs a Kubernetes client of -// its own to attribute an event. +// resolveSourceClusterProvider reads the ClusterProvider this GitTarget mirrors through, ONCE per +// reconcile, for the two consumers that need it: the audit route captured on Declare, and the +// ClusterProviderReady projection. It runs AFTER the Validated gate, which already proved through +// authz.GitTargetAdmitted that this provider exists and admits this namespace — so a failure here +// means the provider vanished (or the apiserver blinked) mid-reconcile, and it is returned as an +// error so the reconcile requeues. // -// An unreadable provider falls back to the provider NAME, which is exactly what AuditRoute() -// defaults to. So a transient read failure resolves the same route a provider that sets nothing -// would, and never a route that silently matches no facts. -func (r *GitTargetReconciler) auditRouteFor( +// Requeuing rather than guessing a route is the point. spec.attribution.auditRoute may be ANY +// string, so a route inferred from the provider NAME is only correct for a provider that declares +// none: guess it for a provider that declares `shared-route` and this target's streams are declared +// under a partition the audit webhook never writes to. Attribution then resolves nobody — or, if +// some other provider is NAMED `shared-route`, resolves that cluster's facts and writes a wrong +// author into Git. +func (r *GitTargetReconciler) resolveSourceClusterProvider( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, -) string { +) (*configbutleraiv1alpha3.ClusterProvider, error) { name := target.SourceCluster() var cp configbutleraiv1alpha3.ClusterProvider if err := r.Get(ctx, k8stypes.NamespacedName{Name: name}, &cp); err != nil { - return name + return nil, fmt.Errorf("read source ClusterProvider %q: %w", name, err) } - return cp.AuditRoute() + return &cp, nil } -// clusterProviderReadiness reads the referenced ClusterProvider's Ready condition and projects it, -// mirroring gitProviderReadiness. It returns Unknown — which does NOT downgrade Ready — when the -// provider cannot be observed (not found, or no Ready condition yet), so a not-yet-installed -// "default" provider never blocks a local GitTarget; only an explicit Ready=False downgrades. -func (r *GitTargetReconciler) clusterProviderReadiness( - ctx context.Context, - target *configbutleraiv1alpha3.GitTarget, +// clusterProviderReadiness projects the referenced ClusterProvider's Ready condition, mirroring +// gitProviderReadiness. It takes the provider resolveSourceClusterProvider already read rather than +// reading its own, so the readiness reported and the audit route declared always describe the same +// observation of the same object. +// +// Only an EXPLICIT Ready=False downgrades the GitTarget; a provider that has not reported readiness +// yet is Unknown, which does not. +func clusterProviderReadiness( + cp *configbutleraiv1alpha3.ClusterProvider, ) (metav1.ConditionStatus, string, string) { - name := target.SourceCluster() - var cp configbutleraiv1alpha3.ClusterProvider - if err := r.Get(ctx, k8stypes.NamespacedName{Name: name}, &cp); err != nil { - return metav1.ConditionUnknown, GitTargetReasonClusterProviderNotReady, - fmt.Sprintf("referenced ClusterProvider %q readiness not observed: %v", name, err) - } + name := cp.Name c := findCondition(cp.Status.Conditions, ConditionTypeReady) switch { case c == nil: diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index 45b1af2e..26628667 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -412,6 +412,9 @@ func TestClusterProviderReadiness_AllScenarios(t *testing.T) { Message: "validating", } + // The absent-provider case is deliberately NOT here: this projection runs only after the + // Validated gate admitted the target through an existing ClusterProvider, and it is handed the + // provider that gate's reconcile already read. A provider that cannot be read never reaches it. tests := []struct { name string cp *configbutleraiv1alpha3.ClusterProvider @@ -420,7 +423,6 @@ func TestClusterProviderReadiness_AllScenarios(t *testing.T) { {"ready", provider([]metav1.Condition{ready}), metav1.ConditionTrue}, {"not ready -> False (downgrades)", provider([]metav1.Condition{notReady}), metav1.ConditionFalse}, {"no condition -> Unknown (does not downgrade)", provider(nil), metav1.ConditionUnknown}, - {"absent provider -> Unknown", nil, metav1.ConditionUnknown}, // Only an EXPLICIT Ready=False downgrades. A provider reporting Ready=Unknown is mid-flight, // not broken, so it must not hold its GitTargets down. { @@ -431,18 +433,7 @@ func TestClusterProviderReadiness_AllScenarios(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - builder := fake.NewClientBuilder().WithScheme(scScheme(t)) - if tc.cp != nil { - builder = builder.WithObjects(tc.cp) - } - r := &GitTargetReconciler{Client: builder.Build()} - target := &configbutleraiv1alpha3.GitTarget{ - ObjectMeta: metav1.ObjectMeta{Name: "gt", Namespace: "team-a"}, - Spec: configbutleraiv1alpha3.GitTargetSpec{ - ClusterProviderRef: &configbutleraiv1alpha3.ClusterProviderReference{Name: "prod-eu-1"}, - }, - } - status, _, msg := r.clusterProviderReadiness(context.Background(), target) + status, _, msg := clusterProviderReadiness(tc.cp) assert.Equal(t, tc.want, status) assert.NotEmpty(t, msg) }) @@ -494,14 +485,15 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { } } -// TestAuditRouteFor covers how a GitTarget learns the audit route its author facts are keyed under. -// The controller reads it here, on the control plane, so the watch data plane needs no Kubernetes -// client of its own to attribute an event. +// TestResolveSourceClusterProvider covers the single read that feeds both the audit route a +// GitTarget's streams are declared under and its ClusterProviderReady projection. // -// The fallback is the load-bearing case: an unreadable provider must resolve the provider NAME, -// which is exactly what AuditRoute() defaults to. Resolving anything else would key the read to a -// partition the audit handler never writes, which is the failure this whole change removes. -func TestAuditRouteFor(t *testing.T) { +// The load-bearing case is the unreadable provider: it must ERROR, so the reconcile requeues +// without declaring. Resolving a route by falling back to the provider NAME is right only for a +// provider that declares none — do it for one that declares `shared-route` and this target's +// streams are keyed to a partition the audit handler never writes, which is exactly the silent +// attribution loss this resolution exists to prevent. +func TestResolveSourceClusterProvider(t *testing.T) { const providerName = "srcns-delegating" target := func() *configbutleraiv1alpha3.GitTarget { @@ -520,38 +512,44 @@ func TestAuditRouteFor(t *testing.T) { } tests := []struct { - name string - objects []client.Object - want string + name string + objects []client.Object + wantRoute string }{ { name: "a declared route is used", objects: []client.Object{ provider(&configbutleraiv1alpha3.ClusterProviderAttribution{AuditRoute: "default"}), }, - want: "default", + wantRoute: "default", }, { - name: "an empty attribution block falls back to the provider name", - objects: []client.Object{provider(&configbutleraiv1alpha3.ClusterProviderAttribution{})}, - want: providerName, + name: "an empty attribution block resolves the provider name", + objects: []client.Object{provider(&configbutleraiv1alpha3.ClusterProviderAttribution{})}, + wantRoute: providerName, }, { - name: "no attribution block falls back to the provider name", - objects: []client.Object{provider(nil)}, - want: providerName, - }, - { - name: "an unreadable provider falls back to the provider name", - objects: nil, - want: providerName, + name: "no attribution block resolves the provider name", + objects: []client.Object{provider(nil)}, + wantRoute: providerName, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects(tc.objects...).Build() r := &GitTargetReconciler{Client: cl} - assert.Equal(t, tc.want, r.auditRouteFor(context.Background(), target())) + cp, err := r.resolveSourceClusterProvider(context.Background(), target()) + require.NoError(t, err) + assert.Equal(t, tc.wantRoute, cp.AuditRoute()) }) } + + t.Run("an unreadable provider errors instead of guessing a route", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).Build() + r := &GitTargetReconciler{Client: cl} + cp, err := r.resolveSourceClusterProvider(context.Background(), target()) + require.Error(t, err, "a guessed route silently mis-keys this target's attribution facts") + assert.Nil(t, cp) + assert.Contains(t, err.Error(), providerName) + }) } From 4c0d3280755bc451ae09b4e788a6b5feb060e473 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 22 Jul 2026 13:44:07 +0000 Subject: [PATCH 2/2] test(gittarget): wait a full requeue for the age-key Secret to be recreated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recreate-on-deletion spec waited the shared 10s `timeout` for a recovery that only the periodic requeue can deliver: this controller runs no control-plane Secret watch, so nothing enqueues the GitTarget when its age-key Secret is deleted, and the next reconcile is one RequeueStreamSettleInterval away — also 10s. The wait therefore equalled the interval it was waiting for, and a deletion landing just after a reconcile lost by milliseconds. It failed that way on this branch's CI run and passed on a plain re-run. Wait RequeueStreamSettleInterval + timeout instead, so one full requeue plus reconcile time fits inside the window, and say in the comment what actually drives the recovery — the old one credited a "secret watch" that does not exist. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/controller/gittarget_controller_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/controller/gittarget_controller_test.go b/internal/controller/gittarget_controller_test.go index b1b4db69..47dcceab 100644 --- a/internal/controller/gittarget_controller_test.go +++ b/internal/controller/gittarget_controller_test.go @@ -1114,7 +1114,11 @@ var _ = Describe("GitTarget Controller Security", func() { // Delete the secret to simulate accidental removal. Expect(k8sClient.Delete(ctx, &firstSecret)).Should(Succeed()) - // The secret watch should trigger re-reconciliation and recreate it. + // Recreation rides the PERIODIC requeue, not a watch: this controller deliberately runs + // no control-plane Secret watch (SetupWithManager explains why), so nothing enqueues the + // GitTarget when its age-key Secret disappears. The wait must therefore exceed a full + // RequeueStreamSettleInterval — with the shared 10s `timeout` it equalled one, so a + // deletion landing just after a reconcile lost the race by milliseconds. Eventually(func(g Gomega) { var recreated corev1.Secret err := k8sClient.Get(ctx, secretKey, &recreated) @@ -1123,7 +1127,7 @@ var _ = Describe("GitTarget Controller Security", func() { g.Expect(ageKeyName).NotTo(BeEmpty()) g.Expect(string(ageKeyValue)).To(ContainSubstring("AGE-SECRET-KEY-")) g.Expect(recreated.Annotations).To(HaveKey(encryptionSecretRecipientAnnoKey)) - }, timeout, interval).Should(Succeed()) + }, RequeueStreamSettleInterval+timeout, interval).Should(Succeed()) Expect(k8sClient.Delete(ctx, target)).Should(Succeed()) Expect(k8sClient.Delete(ctx, gitProvider)).Should(Succeed())