From 8b35a74ad5513a3dc0014f8a24a820fcecd259b6 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Tue, 16 Jun 2026 21:59:24 +0100 Subject: [PATCH 1/2] Consolidate MCPOIDCConfig ReferencingWorkloads ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCPServer, VirtualMCPServer, and MCPRemoteProxy controllers each wrote the referenced MCPOIDCConfig's status via a full r.Status().Update to append themselves to ReferencingWorkloads. This had two problems: - A full PUT carries the entire Status.Conditions array, so a write landing between the MCPOIDCConfig controller's Get and Patch clobbers the conditions that controller owns (the hazard #5511 tracks, exposed by the #5509 MutateAndPatchStatus migration). - The writes were append-only — they never removed stale entries. Only the MCPOIDCConfig controller, which watches all three workload kinds and recomputes the full list via findReferencingWorkloads, handles removals. Remove the three redundant cross-controller writers and let the MCPOIDCConfig controller be the sole owner of its ReferencingWorkloads. It already watches MCPServer/VirtualMCPServer/MCPRemoteProxy and recomputes the authoritative list (additions and removals) on every relevant change, so the field stays correct without the workload controllers touching the config's status. Drop the unit tests that exercised the removed helpers; the config controller's own reference-tracking tests (and the mcp-oidc-config envtest suites, which use Eventually) cover the consolidated behavior. Closes #5511 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/mcpremoteproxy_controller.go | 39 +------- .../mcpremoteproxy_controller_test.go | 61 ------------- .../controllers/mcpserver_controller.go | 41 ++------- .../controllers/mcpserver_oidcconfig_test.go | 91 +++---------------- .../virtualmcpserver_controller.go | 35 +------ .../virtualmcpserver_controller_test.go | 61 ------------- 6 files changed, 32 insertions(+), 296 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index fa0336f0d0..50bf04c543 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -920,11 +920,11 @@ func (r *MCPRemoteProxyReconciler) handleOIDCConfig(ctx context.Context, proxy * return err } - // Update ReferencingWorkloads on the MCPOIDCConfig status - if err := r.updateOIDCConfigReferencingWorkloads(ctx, oidcConfig, proxy.Name); err != nil { - ctxLogger.Error(err, "Failed to update MCPOIDCConfig ReferencingWorkloads") - // Non-fatal: continue with reconciliation - } + // ReferencingWorkloads on the MCPOIDCConfig is maintained solely by the + // MCPOIDCConfig controller, which watches the workload kinds and recomputes + // the full list. The MCPRemoteProxy controller must not write the config's + // status — a full r.Status().Update would clobber conditions the config + // controller owns. See #5511. // Detect whether the condition is transitioning to True (e.g. recovering from // a transient error). Without this check the status update is skipped when the @@ -1017,35 +1017,6 @@ func (r *MCPRemoteProxyReconciler) fetchAndValidateOIDCConfig( return oidcConfig, nil } -// updateOIDCConfigReferencingWorkloads ensures the MCPRemoteProxy is listed in -// the MCPOIDCConfig's ReferencingWorkloads status field. -func (r *MCPRemoteProxyReconciler) updateOIDCConfigReferencingWorkloads( - ctx context.Context, - oidcConfig *mcpv1beta1.MCPOIDCConfig, - proxyName string, -) error { - ref := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindMCPRemoteProxy, - Name: proxyName, - } - - // Check if already listed - for _, entry := range oidcConfig.Status.ReferencingWorkloads { - if entry.Kind == ref.Kind && entry.Name == ref.Name { - return nil - } - } - - // Add the workload reference - oidcConfig.Status.ReferencingWorkloads = append(oidcConfig.Status.ReferencingWorkloads, ref) - oidcConfig.Status.ReferenceCount = workloadReferenceCount(oidcConfig.Status.ReferencingWorkloads) - if err := r.Status().Update(ctx, oidcConfig); err != nil { - return fmt.Errorf("failed to update MCPOIDCConfig ReferencingWorkloads: %w", err) - } - - return nil -} - // validateGroupRef validates the GroupRef field of the MCPRemoteProxy. // This function only sets conditions on the proxy object - the caller is responsible // for persisting the status update to avoid multiple conflicting status updates. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index 69d715a4f3..8effa03b59 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -1439,64 +1439,3 @@ func TestGetExternalAuthConfigForMCPRemoteProxy(t *testing.T) { assert.NotNil(t, result) assert.Equal(t, "test-auth", result.Name) } - -func TestMCPRemoteProxyReconciler_updateOIDCConfigReferencingWorkloads(t *testing.T) { - t.Parallel() - - existingRef := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindMCPRemoteProxy, - Name: "existing", - } - newRef := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindMCPRemoteProxy, - Name: "new", - } - - tests := []struct { - name string - proxyName string - expectedRefs []mcpv1beta1.WorkloadReference - expectedCount int32 - }{ - { - name: "adds new proxy reference", - proxyName: "new", - expectedRefs: []mcpv1beta1.WorkloadReference{existingRef, newRef}, - expectedCount: 2, - }, - { - name: "does not duplicate existing reference", - proxyName: "existing", - expectedRefs: []mcpv1beta1.WorkloadReference{existingRef}, - expectedCount: 1, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctx := t.Context() - scheme := runtime.NewScheme() - require.NoError(t, mcpv1beta1.AddToScheme(scheme)) - - oidcConfig := &mcpv1beta1.MCPOIDCConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, - Status: mcpv1beta1.MCPOIDCConfigStatus{ - ReferencingWorkloads: []mcpv1beta1.WorkloadReference{existingRef}, - ReferenceCount: 1, - }, - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(oidcConfig). - WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}). - Build() - reconciler := &MCPRemoteProxyReconciler{Client: fakeClient, Scheme: scheme} - - require.NoError(t, reconciler.updateOIDCConfigReferencingWorkloads(ctx, oidcConfig, tt.proxyName)) - assert.ElementsMatch(t, tt.expectedRefs, oidcConfig.Status.ReferencingWorkloads) - assert.Equal(t, tt.expectedCount, oidcConfig.Status.ReferenceCount) - }) - } -} diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 734b704ab9..13f608fc3b 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -2276,11 +2276,13 @@ func (r *MCPServerReconciler) handleOIDCConfig(ctx context.Context, m *mcpv1beta return err } - // Update ReferencingWorkloads on the MCPOIDCConfig status - if err := r.updateOIDCConfigReferencingWorkloads(ctx, oidcConfig, m.Name); err != nil { - ctxLogger.Error(err, "Failed to update MCPOIDCConfig ReferencingWorkloads") - // Non-fatal: continue with reconciliation - } + // ReferencingWorkloads on the MCPOIDCConfig is maintained solely by the + // MCPOIDCConfig controller, which watches MCPServer/VirtualMCPServer/ + // MCPRemoteProxy and recomputes the full list (additions and removals). The + // MCPServer controller must not write the config's status: a full + // r.Status().Update here would clobber conditions the config controller + // owns, and the previous append-only write never removed stale entries. + // See #5511. // Detect whether the condition is transitioning to True (e.g. recovering from // a transient error). Without this check the status update is skipped when the @@ -2367,35 +2369,6 @@ func setOIDCConfigRefCondition(m *mcpv1beta1.MCPServer, status metav1.ConditionS }) } -// updateOIDCConfigReferencingWorkloads ensures the MCPServer is listed in -// the MCPOIDCConfig's ReferencingWorkloads status field. -func (r *MCPServerReconciler) updateOIDCConfigReferencingWorkloads( - ctx context.Context, - oidcConfig *mcpv1beta1.MCPOIDCConfig, - serverName string, -) error { - ref := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindMCPServer, - Name: serverName, - } - - // Check if already listed - for _, entry := range oidcConfig.Status.ReferencingWorkloads { - if entry.Kind == ref.Kind && entry.Name == ref.Name { - return nil - } - } - - // Add the workload reference - oidcConfig.Status.ReferencingWorkloads = append(oidcConfig.Status.ReferencingWorkloads, ref) - oidcConfig.Status.ReferenceCount = workloadReferenceCount(oidcConfig.Status.ReferencingWorkloads) - if err := r.Status().Update(ctx, oidcConfig); err != nil { - return fmt.Errorf("failed to update MCPOIDCConfig ReferencingWorkloads: %w", err) - } - - return nil -} - // handleWebhookConfig validates and tracks the hash of the referenced MCPWebhookConfig. func (r *MCPServerReconciler) handleWebhookConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error { ctxLogger := log.FromContext(ctx) diff --git a/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go b/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go index 96de0cb2e1..3758dc9822 100644 --- a/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go +++ b/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go @@ -29,16 +29,15 @@ func TestMCPServerReconciler_handleOIDCConfig(t *testing.T) { }} tests := []struct { - name string - mcpServer *mcpv1beta1.MCPServer - oidcConfig *mcpv1beta1.MCPOIDCConfig - expectError bool - expectErrorContains string - expectHash string - expectHashCleared bool - expectConditionStatus *metav1.ConditionStatus - expectConditionReason string - expectReferencingServer bool + name string + mcpServer *mcpv1beta1.MCPServer + oidcConfig *mcpv1beta1.MCPOIDCConfig + expectError bool + expectErrorContains string + expectHash string + expectHashCleared bool + expectConditionStatus *metav1.ConditionStatus + expectConditionReason string }{ { name: "no ref clears previously stored hash", @@ -109,10 +108,9 @@ func TestMCPServerReconciler_handleOIDCConfig(t *testing.T) { Conditions: validOIDCCondition, }, }, - expectHash: "hash-123", - expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue), - expectConditionReason: mcpv1beta1.ConditionReasonOIDCConfigRefValid, - expectReferencingServer: true, + expectHash: "hash-123", + expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue), + expectConditionReason: mcpv1beta1.ConditionReasonOIDCConfigRefValid, }, { name: "detects config hash change", @@ -137,10 +135,9 @@ func TestMCPServerReconciler_handleOIDCConfig(t *testing.T) { Conditions: validOIDCCondition, }, }, - expectHash: "new-hash", - expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue), - expectConditionReason: mcpv1beta1.ConditionReasonOIDCConfigRefValid, - expectReferencingServer: true, + expectHash: "new-hash", + expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue), + expectConditionReason: mcpv1beta1.ConditionReasonOIDCConfigRefValid, }, } @@ -200,68 +197,10 @@ func TestMCPServerReconciler_handleOIDCConfig(t *testing.T) { } assert.True(t, found, "expected %s condition", mcpv1beta1.ConditionOIDCConfigRefValidated) } - - if tt.expectReferencingServer && tt.oidcConfig != nil { - var updated mcpv1beta1.MCPOIDCConfig - require.NoError(t, fakeClient.Get(ctx, client.ObjectKeyFromObject(tt.oidcConfig), &updated)) - expectedRef := mcpv1beta1.WorkloadReference{Kind: "MCPServer", Name: tt.mcpServer.Name} - assert.Contains(t, updated.Status.ReferencingWorkloads, expectedRef) - } }) } } -func TestMCPServerReconciler_updateOIDCConfigReferencingWorkloads(t *testing.T) { - t.Parallel() - - existingRef := mcpv1beta1.WorkloadReference{Kind: "MCPServer", Name: "existing"} - - t.Run("adds new server reference", func(t *testing.T) { - t.Parallel() - ctx := t.Context() - scheme := runtime.NewScheme() - require.NoError(t, mcpv1beta1.AddToScheme(scheme)) - - cfg := &mcpv1beta1.MCPOIDCConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, - Status: mcpv1beta1.MCPOIDCConfigStatus{ - ReferencingWorkloads: []mcpv1beta1.WorkloadReference{existingRef}, - ReferenceCount: 1, - }, - } - fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cfg). - WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}).Build() - r := newTestMCPServerReconciler(fc, scheme, kubernetes.PlatformKubernetes) - - require.NoError(t, r.updateOIDCConfigReferencingWorkloads(ctx, cfg, "new")) - newRef := mcpv1beta1.WorkloadReference{Kind: "MCPServer", Name: "new"} - assert.ElementsMatch(t, []mcpv1beta1.WorkloadReference{existingRef, newRef}, cfg.Status.ReferencingWorkloads) - assert.EqualValues(t, 2, cfg.Status.ReferenceCount) - }) - - t.Run("does not duplicate existing reference", func(t *testing.T) { - t.Parallel() - ctx := t.Context() - scheme := runtime.NewScheme() - require.NoError(t, mcpv1beta1.AddToScheme(scheme)) - - cfg := &mcpv1beta1.MCPOIDCConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, - Status: mcpv1beta1.MCPOIDCConfigStatus{ - ReferencingWorkloads: []mcpv1beta1.WorkloadReference{existingRef}, - ReferenceCount: 1, - }, - } - fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cfg). - WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}).Build() - r := newTestMCPServerReconciler(fc, scheme, kubernetes.PlatformKubernetes) - - require.NoError(t, r.updateOIDCConfigReferencingWorkloads(ctx, cfg, "existing")) - assert.Len(t, cfg.Status.ReferencingWorkloads, 1) - assert.EqualValues(t, 1, cfg.Status.ReferenceCount) - }) -} - // TestMCPServerReconciler_handleOIDCConfig_ConditionPersistedOnRecovery verifies that the // OIDCConfigRefValidated condition is actually persisted to the API server (not just set // in memory) when recovering from a transient error with an unchanged config hash (#4511). diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 6ead41efc1..795942862c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -3433,11 +3433,11 @@ func (r *VirtualMCPServerReconciler) handleOIDCConfig( return fmt.Errorf("%s", msg) } - // Update ReferencingWorkloads on the MCPOIDCConfig status - if err := r.updateOIDCConfigReferencingWorkloads(ctx, oidcConfig, vmcp.Name); err != nil { - ctxLogger.Error(err, "Failed to update MCPOIDCConfig ReferencingWorkloads") - // Non-fatal: continue with reconciliation - } + // ReferencingWorkloads on the MCPOIDCConfig is maintained solely by the + // MCPOIDCConfig controller, which watches the workload kinds and recomputes + // the full list. The VirtualMCPServer controller must not write the config's + // status — a full r.Status().Update would clobber conditions the config + // controller owns. See #5511. // Set valid condition statusManager.SetCondition( @@ -3461,31 +3461,6 @@ func (r *VirtualMCPServerReconciler) handleOIDCConfig( return nil } -// updateOIDCConfigReferencingWorkloads ensures the VirtualMCPServer is listed in -// the MCPOIDCConfig's ReferencingWorkloads status field. -func (r *VirtualMCPServerReconciler) updateOIDCConfigReferencingWorkloads( - ctx context.Context, - oidcConfig *mcpv1beta1.MCPOIDCConfig, - vmcpName string, -) error { - ref := mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindVirtualMCPServer, Name: vmcpName} - // Check if already listed - for _, entry := range oidcConfig.Status.ReferencingWorkloads { - if entry.Kind == ref.Kind && entry.Name == ref.Name { - return nil - } - } - - // Add the workload reference - oidcConfig.Status.ReferencingWorkloads = append(oidcConfig.Status.ReferencingWorkloads, ref) - oidcConfig.Status.ReferenceCount = workloadReferenceCount(oidcConfig.Status.ReferencingWorkloads) - if err := r.Status().Update(ctx, oidcConfig); err != nil { - return fmt.Errorf("failed to update MCPOIDCConfig ReferencingWorkloads: %w", err) - } - - return nil -} - // mapOIDCConfigToVirtualMCPServer maps MCPOIDCConfig changes to VirtualMCPServer reconciliation requests. func (r *VirtualMCPServerReconciler) mapOIDCConfigToVirtualMCPServer( ctx context.Context, obj client.Object, diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index 8d69891965..721eddc290 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go @@ -4342,64 +4342,3 @@ func TestVirtualMCPServerReconciler_IdentitySynthesizedTransitionsOnValidationFa assert.NotContains(t, cond.Message, "atlassian", "stale message naming the now-removed upstream must not survive the broken edit") } - -func TestVirtualMCPServerReconciler_updateOIDCConfigReferencingWorkloads(t *testing.T) { - t.Parallel() - - existingRef := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindVirtualMCPServer, - Name: "existing", - } - newRef := mcpv1beta1.WorkloadReference{ - Kind: mcpv1beta1.WorkloadKindVirtualMCPServer, - Name: "new", - } - - tests := []struct { - name string - vmcpName string - expectedRefs []mcpv1beta1.WorkloadReference - expectedCount int32 - }{ - { - name: "adds new virtual server reference", - vmcpName: "new", - expectedRefs: []mcpv1beta1.WorkloadReference{existingRef, newRef}, - expectedCount: 2, - }, - { - name: "does not duplicate existing reference", - vmcpName: "existing", - expectedRefs: []mcpv1beta1.WorkloadReference{existingRef}, - expectedCount: 1, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctx := t.Context() - scheme := runtime.NewScheme() - require.NoError(t, mcpv1beta1.AddToScheme(scheme)) - - oidcConfig := &mcpv1beta1.MCPOIDCConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, - Status: mcpv1beta1.MCPOIDCConfigStatus{ - ReferencingWorkloads: []mcpv1beta1.WorkloadReference{existingRef}, - ReferenceCount: 1, - }, - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(oidcConfig). - WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}). - Build() - reconciler := &VirtualMCPServerReconciler{Client: fakeClient, Scheme: scheme} - - require.NoError(t, reconciler.updateOIDCConfigReferencingWorkloads(ctx, oidcConfig, tt.vmcpName)) - assert.ElementsMatch(t, tt.expectedRefs, oidcConfig.Status.ReferencingWorkloads) - assert.Equal(t, tt.expectedCount, oidcConfig.Status.ReferenceCount) - }) - } -} From f1f8501961185a7c331b676aa4da62c6add909a6 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 17 Jun 2026 15:35:32 +0100 Subject: [PATCH 2/2] Drop unused mcpoidcconfigs/status RBAC; guard the no-write boundary Follow-up to the ReferencingWorkloads consolidation, addressing review feedback: - Remove the now-unused +kubebuilder:rbac mcpoidcconfigs/status (get;update;patch) markers from the MCPServer, VirtualMCPServer, and MCPRemoteProxy controllers. They read the config's status via a plain Get on the main resource (covered by the mcpoidcconfigs get marker), so the /status subresource verbs are dead. This is marker-accuracy hygiene: the operator runs as a single ServiceAccount whose aggregated role still grants mcpoidcconfigs/status write via the MCPOIDCConfig controller's own marker, so the generated ClusterRole is unchanged. - Add TestMCPServerReconciler_handleOIDCConfig_DoesNotWriteConfigStatus, which asserts the MCPServer reconcile leaves the referenced config's status (ResourceVersion, ReferencingWorkloads, conditions) untouched. Since RBAC does not enforce the boundary under a shared ServiceAccount, this test is the actual guard against a future reintroduced cross-controller writer. - Align the VirtualMCPServer/MCPRemoteProxy call-site comments with the fuller MCPServer wording (clobber + append-only rationale). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/mcpremoteproxy_controller.go | 11 +-- .../controllers/mcpserver_controller.go | 1 - .../controllers/mcpserver_oidcconfig_test.go | 72 +++++++++++++++++++ .../virtualmcpserver_controller.go | 11 +-- 4 files changed, 84 insertions(+), 11 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 50bf04c543..ba5bdac6fc 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -54,7 +54,6 @@ type MCPRemoteProxyReconciler struct { // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptelemetryconfigs,verbs=get;list;watch -// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups="",resources=services,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources=roles,verbs=create;delete;get;list;patch;update;watch @@ -921,10 +920,12 @@ func (r *MCPRemoteProxyReconciler) handleOIDCConfig(ctx context.Context, proxy * } // ReferencingWorkloads on the MCPOIDCConfig is maintained solely by the - // MCPOIDCConfig controller, which watches the workload kinds and recomputes - // the full list. The MCPRemoteProxy controller must not write the config's - // status — a full r.Status().Update would clobber conditions the config - // controller owns. See #5511. + // MCPOIDCConfig controller, which watches MCPServer/VirtualMCPServer/ + // MCPRemoteProxy and recomputes the full list (additions and removals). The + // MCPRemoteProxy controller must not write the config's status: a full + // r.Status().Update here would clobber conditions the config controller + // owns, and the previous append-only write never removed stale entries. + // See #5511. // Detect whether the condition is transitioning to True (e.g. recovering from // a transient error). Without this check the status update is skipped when the diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 13f608fc3b..4b7736e191 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -153,7 +153,6 @@ func (r *MCPServerReconciler) detectPlatform(ctx context.Context) (kubernetes.Pl // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpservers/finalizers,verbs=update // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptoolconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs,verbs=get;list;watch -// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptelemetryconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups="",resources=services,verbs=create;delete;get;list;patch;update;watch diff --git a/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go b/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go index 3758dc9822..1d51471206 100644 --- a/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go +++ b/cmd/thv-operator/controllers/mcpserver_oidcconfig_test.go @@ -397,3 +397,75 @@ func TestMCPOIDCConfigReconciler_handleDeletion_IgnoresCrossNamespaceRef(t *test func conditionStatusPtr(s metav1.ConditionStatus) *metav1.ConditionStatus { return &s } + +// TestMCPServerReconciler_handleOIDCConfig_DoesNotWriteConfigStatus guards the +// trust boundary consolidated in #5511: the MCPServer controller may read the +// referenced MCPOIDCConfig but must never write its status. The MCPOIDCConfig +// controller is the sole owner of that status (conditions and +// ReferencingWorkloads). This unit test is the actual enforcement of that +// boundary — RBAC does not enforce it, because the operator runs as a single +// ServiceAccount whose aggregated role still grants mcpoidcconfigs/status write +// for the MCPOIDCConfig controller's own use. A future reintroduction of a +// cross-controller status write would flip the ResourceVersion and fail here. +func TestMCPServerReconciler_handleOIDCConfig_DoesNotWriteConfigStatus(t *testing.T) { + t.Parallel() + ctx := t.Context() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + // Seed a config whose status carries a condition and a ReferencingWorkloads + // entry owned by the MCPOIDCConfig controller — neither must be touched. + oidcConfig := &mcpv1beta1.MCPOIDCConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "default"}, + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{Issuer: "https://x", ClientID: "c"}, + }, + Status: mcpv1beta1.MCPOIDCConfigStatus{ + ConfigHash: "hash-123", + Conditions: []metav1.Condition{ + { + Type: mcpv1beta1.ConditionTypeOIDCConfigValid, Status: metav1.ConditionTrue, + Reason: mcpv1beta1.ConditionReasonOIDCConfigValid, + }, + { + Type: "ForeignControllerSays", Status: metav1.ConditionTrue, + Reason: "ExternallySet", LastTransitionTime: metav1.Now(), + }, + }, + ReferencingWorkloads: []mcpv1beta1.WorkloadReference{{Kind: "MCPServer", Name: "someone-else"}}, + ReferenceCount: 1, + }, + } + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "img", + OIDCConfigRef: &mcpv1beta1.MCPOIDCConfigReference{Name: "cfg", Audience: "aud"}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(oidcConfig, mcpServer). + WithStatusSubresource(&mcpv1beta1.MCPServer{}, &mcpv1beta1.MCPOIDCConfig{}). + Build() + r := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + var before mcpv1beta1.MCPOIDCConfig + require.NoError(t, fakeClient.Get(ctx, client.ObjectKeyFromObject(oidcConfig), &before)) + + require.NoError(t, r.handleOIDCConfig(ctx, mcpServer)) + + var after mcpv1beta1.MCPOIDCConfig + require.NoError(t, fakeClient.Get(ctx, client.ObjectKeyFromObject(oidcConfig), &after)) + + assert.Equal(t, before.ResourceVersion, after.ResourceVersion, + "MCPServer reconcile must not write the MCPOIDCConfig — its status is owned by the MCPOIDCConfig controller") + assert.Equal(t, before.Status.ReferencingWorkloads, after.Status.ReferencingWorkloads, + "MCPServer reconcile must not touch the config's ReferencingWorkloads") + assert.NotNil(t, meta.FindStatusCondition(after.Status.Conditions, "ForeignControllerSays"), + "config-owned conditions must remain untouched") +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 795942862c..653bc0d0c7 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -145,7 +145,6 @@ type VirtualMCPServerReconciler struct { // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs,verbs=get;list;watch -// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=embeddingservers,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=embeddingservers/status,verbs=get // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptelemetryconfigs,verbs=get;list;watch @@ -3434,10 +3433,12 @@ func (r *VirtualMCPServerReconciler) handleOIDCConfig( } // ReferencingWorkloads on the MCPOIDCConfig is maintained solely by the - // MCPOIDCConfig controller, which watches the workload kinds and recomputes - // the full list. The VirtualMCPServer controller must not write the config's - // status — a full r.Status().Update would clobber conditions the config - // controller owns. See #5511. + // MCPOIDCConfig controller, which watches MCPServer/VirtualMCPServer/ + // MCPRemoteProxy and recomputes the full list (additions and removals). The + // VirtualMCPServer controller must not write the config's status: a full + // r.Status().Update here would clobber conditions the config controller + // owns, and the previous append-only write never removed stale entries. + // See #5511. // Set valid condition statusManager.SetCondition(