Skip to content

Consolidate MCPOIDCConfig ReferencingWorkloads ownership#5544

Merged
ChrisJBurns merged 2 commits into
mainfrom
cburns/consolidate-oidcconfig-refs-ownership
Jun 17, 2026
Merged

Consolidate MCPOIDCConfig ReferencingWorkloads ownership#5544
ChrisJBurns merged 2 commits into
mainfrom
cburns/consolidate-oidcconfig-refs-ownership

Conversation

@ChrisJBurns

@ChrisJBurns ChrisJBurns commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

The #5509 migration made each config controller patch-safe, but the MCPOIDCConfig controller is still not the sole owner of its status: the MCPServer, VirtualMCPServer, and MCPRemoteProxy controllers each do a full r.Status().Update against the referenced MCPOIDCConfig to append themselves to ReferencingWorkloads. A full PUT carries the whole Status.Conditions array, so a write landing between the config controller's Get and Patch clobbers the conditions that controller owns. The writes are also append-only — they never remove stale entries. This removes them and lets the MCPOIDCConfig controller (which already watches all three workload kinds and recomputes the full list with removals) be the sole owner.

Closes #5511

Medium level
  • Removed updateOIDCConfigReferencingWorkloads and its call site from the MCPServer, VirtualMCPServer, and MCPRemoteProxy controllers. Each call site keeps the rest of its OIDC handling (ref validation + hash tracking on the workload's own status); only the cross-controller write to the config object is gone.
  • The MCPOIDCConfig controller is unchanged: it already Watches MCPServer/VirtualMCPServer/MCPRemoteProxy and recomputes the authoritative ReferencingWorkloads (additions and removals) via findReferencingWorkloads on every relevant change.
  • Net effect: eliminates the merge-patch-vs-PUT clobber hazard and the append-only staleness bug; ReferencingWorkloads now has a single owner.
Low level
File Change
cmd/thv-operator/controllers/mcpserver_controller.go Remove updateOIDCConfigReferencingWorkloads + call; explanatory comment referencing #5511
cmd/thv-operator/controllers/virtualmcpserver_controller.go Same removal
cmd/thv-operator/controllers/mcpremoteproxy_controller.go Same removal
*_test.go (3 files) Drop the unit tests that exercised the removed helpers and the expectReferencingServer table assertion

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Documentation
  • Other

Test plan

  • task build passes
  • task test passes for cmd/thv-operator/controllers (unit)
  • Confirmed the MCPOIDCConfig controller retains ReferencingWorkloads coverage: TestMCPOIDCConfigReconciler_ReferenceCountUpdatedWithWorkloads (population) and TestMCPOIDCConfigReconciler_handleDeletion_BlocksWhenReferenced (deletion-block), plus the mcp-oidc-config envtest suites which assert population and cleanup via Eventually
  • Local task lint blocked by the known golangci-lint go1.24-vs-go1.26 toolchain mismatch; CI runs lint
  • gofmt clean

Special notes for reviewers

Behavioral change worth a careful look: MCPOIDCConfig.ReferencingWorkloads is now populated only by the config controller's watch-driven reconcile, not synchronously during the referencing workload's reconcile. This is eventually-consistent (the watch fires near-immediately) and is the intended design — the field is informational status, and deletion-blocking already recomputes the live list rather than reading stored status. The mcp-oidc-config envtest suites already assert this via Eventually, so they continue to exercise the consolidated path.

MCPExternalAuthConfig and MCPAuthzConfig never had cross-controller writers, so they needed no change.

Generated with Claude Code


Review follow-up (addressed in latest commit):

  • RBAC: Dropped the now-unused mcpoidcconfigs/status markers from the three workload controllers. Note this is marker-accuracy hygiene, not a privilege reduction — the operator is a single ServiceAccount and its aggregated role still grants mcpoidcconfigs/status write via the MCPOIDCConfig controller's own marker (regenerating role.yaml produces no diff).
  • Cross-controller full-PUT writers undercut config-controller sole-ownership of Status.Conditions #5511 item 2 (interceptor test): Satisfied by removing the writers rather than by a WithInterceptorFuncs test — there is no longer a full-PUT path in these controllers to regress against. Because RBAC does not enforce the boundary under a shared ServiceAccount, the new TestMCPServerReconciler_handleOIDCConfig_DoesNotWriteConfigStatus is the actual guard: it fails if a workload reconcile ever writes the config's status again.
  • Comments: Aligned all three call-site comments on the fuller clobber + append-only rationale.

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) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jun 16, 2026
@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.88%. Comparing base (141f9a4) to head (f1f8501).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5544      +/-   ##
==========================================
+ Coverage   69.87%   69.88%   +0.01%     
==========================================
  Files         648      648              
  Lines       65873    65870       -3     
==========================================
+ Hits        46029    46034       +5     
+ Misses      16500    16490      -10     
- Partials     3344     3346       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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) <noreply@anthropic.com>
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jun 17, 2026
@ChrisJBurns ChrisJBurns merged commit bcc9081 into main Jun 17, 2026
46 checks passed
@ChrisJBurns ChrisJBurns deleted the cburns/consolidate-oidcconfig-refs-ownership branch June 17, 2026 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cross-controller full-PUT writers undercut config-controller sole-ownership of Status.Conditions

2 participants