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_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()) 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) + }) }