Wire MCPOIDCConfig into MCPRemoteProxy controller

[ChrisJBurns]

Summary

• MCPRemoteProxy was the only workload CRD missing support for referencing shared MCPOIDCConfig resources. MCPServer (Wire MCPOIDCConfig into MCPServer controller #4481) and VirtualMCPServer (Wire MCPOIDCConfig into VirtualMCPServer controller #4493) already had this wiring. This PR completes the set so platform teams can define OIDC configuration once and reference it from all three workload types.
• Adds OIDCConfigRef field to MCPRemoteProxy spec, OIDCConfigHash to status, controller lifecycle (fail-closed validation, hash tracking, condition management), bidirectional watches between MCPOIDCConfig and MCPRemoteProxy controllers, and runconfig resolution supporting both the new ref path and legacy inline config.
• Makes oidcConfig optional (pointer) with deprecation notice and adds CEL mutual exclusivity validation with oidcConfigRef, matching the MCPServer pattern.

Ref #4275

Type of change

• New feature

Large PR Justification

This PR exceeds the normal line limit because:

• 838 lines are the new integration test file (18 test scenarios for cross-CRD lifecycle)
• ~64 lines are regenerated CRD manifests (Helm chart files/templates)
• ~35 lines are test fixture changes (value→pointer type migration across existing test files)
• ~3 lines are regenerated CRD API docs

The core production logic is ~330 lines across 4 files, within the normal limit. The tests and generated content are not meaningfully splittable.

Test plan

• Unit tests (task test)
• Linting (task lint-fix)
• Integration tests (new test file covering 7 scenarios)

Changes

File	Change
cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go	Add OIDCConfigRef spec field, make OIDCConfig optional pointer with deprecation, CEL mutual exclusivity rule, OIDCConfigHash status field
cmd/thv-operator/controllers/mcpremoteproxy_controller.go	Add handleOIDCConfig, updateOIDCConfigReferencingWorkloads, mapOIDCConfigToMCPRemoteProxy; RBAC markers; wire into validateAndHandleConfigs and SetupWithManager
cmd/thv-operator/controllers/mcpoidcconfig_controller.go	Add MCPRemoteProxy scanning in findReferencingWorkloads, mapMCPRemoteProxyToOIDCConfig watch handler, RBAC marker
cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go	Extract resolveAndAddOIDCConfig helper supporting both MCPOIDCConfigRef and legacy inline paths
cmd/thv-operator/controllers/mcpremoteproxy_deployment.go	Nil guards for optional OIDCConfig, extracted buildOIDCClientSecretEnvVars helper
cmd/thv-operator/api/v1alpha1/zz_generated.deepcopy.go	Regenerated DeepCopy for pointer OIDCConfig field
deploy/charts/operator-crds/	Regenerated CRD manifest and Helm template
docs/operator/crd-api.md	Regenerated CRD API reference docs
cmd/thv-operator/test-integration/mcp-oidc-config/mcpoidcconfig_mcpremoteproxy_integration_test.go	New: 7 integration test scenarios for cross-CRD lifecycle
cmd/thv-operator/test-integration/mcp-oidc-config/suite_test.go	Register MCPRemoteProxyReconciler in test suite
Various *_test.go files	Value→pointer migration for OIDCConfig field

Does this introduce a user-facing change?

MCPRemoteProxy resources can now reference a shared MCPOIDCConfig via spec.oidcConfigRef instead of only using inline spec.oidcConfig. The oidcConfig field is now optional and deprecated — existing manifests continue to work but should migrate to oidcConfigRef. The two fields are mutually exclusive (enforced by CEL validation).

Special notes for reviewers

• Follows the exact same pattern as MCPServer (Wire MCPOIDCConfig into MCPServer controller #4481) and VirtualMCPServer (Wire MCPOIDCConfig into VirtualMCPServer controller #4493) wiring PRs
• Reuses shared OIDC condition constants from mcpserver_types.go (no per-type duplicates)
• The GetOIDCConfigForServer helper is reused despite its MCPServer-flavored name — it works for any workload
• Integration tests cover: happy path, fail-closed (missing + not-ready), hash cascade, deletion protection, reference removal cleanup (including condition removal), and workload deletion cleanup
• Filed Status condition not persisted when OIDCConfigHash unchanged after transient error #4511 for the pre-existing status condition persistence gap (affects all three controllers)

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 10.82474% with 173 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.85%. Comparing base (2e8e487) to head (0af18a3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...-operator/controllers/mcpremoteproxy_controller.go	4.54%	101 Missing and 4 partials ⚠️
...v-operator/controllers/mcpoidcconfig_controller.go	2.85%	32 Missing and 2 partials ⚠️
...v-operator/controllers/mcpremoteproxy_runconfig.go	14.70%	26 Missing and 3 partials ⚠️
...-operator/controllers/mcpremoteproxy_deployment.go	64.28%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4509      +/-   ##
==========================================
- Coverage   69.06%   68.85%   -0.22%     
==========================================
  Files         502      505       +3     
  Lines       51997    52322     +325     
==========================================
+ Hits        35913    36026     +113     
- Misses      13300    13499     +199     
- Partials     2784     2797      +13     

☔ View full report in Codecov by Sentry.
📢 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.

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.

[ChrisJBurns]

@claude please review this PR. Only report on critical/high issues and non-controversial medium issues. Ignore low and informational issues related to style or preference. At the end, recommend either approval or rejection.

[claude]

Claude finished @ChrisJBurns's task in 4m 1s —— View job

---

PR Review: Wire MCPOIDCConfig into MCPRemoteProxy controller

• Gather context and understand request
• Read changed files
• Analyze for critical/high and medium issues
• Post review findings

---

Summary

This PR completes the OIDC config ref pattern across all three workload types (MCPServer, VirtualMCPServer, MCPRemoteProxy). The implementation is faithful to the sibling PRs and the overall structure is sound. No critical or high issues found.

---

Medium Issue: Stale condition not removed when OIDCConfigRef is cleared

File: cmd/thv-operator/controllers/mcpremoteproxy_controller.go:715-723

When proxy.Spec.OIDCConfigRef is set to nil (e.g., a user removes the ref), handleOIDCConfig clears the OIDCConfigHash but does not remove the OIDCConfigRefValidated condition. This leaves a stale condition in the proxy status indefinitely.

if proxy.Spec.OIDCConfigRef == nil {
    // No MCPOIDCConfig referenced, clear any stored hash
    if proxy.Status.OIDCConfigHash != "" {
        proxy.Status.OIDCConfigHash = ""
        ...
    }
    return nil  // ← missing: meta.RemoveStatusCondition(...)
}

Compare with the two other handlers in the same file, which both correctly remove their conditions:

// handleToolConfig (line ~569)
meta.RemoveStatusCondition(&proxy.Status.Conditions, mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated)

// handleExternalAuthConfig (line ~633)
meta.RemoveStatusCondition(&proxy.Status.Conditions, mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated)

The integration test for reference removal (line 587+) only asserts that OIDCConfigHash is cleared and ReferencingWorkloads is updated — it doesn't verify the condition is removed.

Note: The same omission exists in MCPServer and VirtualMCPServer controllers, but the inconsistency within MCPRemoteProxy itself (two handlers clean up, this one doesn't) makes it more likely to cause confusion.

Fix this →

---

Minor: Unreachable nil check

File: cmd/thv-operator/controllers/mcpremoteproxy_controller.go:742-754

The if oidcConfig == nil block after the GetOIDCConfigForServer call is dead code. GetOIDCConfigForServer only returns (nil, nil) when ref == nil (already guarded at line 715), and returns (nil, err) for all other failures (caught at line 728). This nil guard can never be reached. Same dead code exists in the MCPServer controller, so it's an inherited pattern — low priority.

---

Large PR Justification needed

The github-actions bot has blocked this PR (CHANGES_REQUESTED review) because it exceeds 1000 lines and requires a ## Large PR Justification section in the PR body. The PR is large because ~838 lines are the new integration test file, ~32+32 lines are regenerated CRD manifests, and 3 lines are regenerated docs — essentially all generated or test-only content. Adding the justification section will unblock the automated review.

---

Verdict: Approve (after addressing the medium bug and the bot blocker)

The implementation correctly follows the established pattern, the controller logic is sound (fail-closed OIDC validation, hash tracking, bidirectional watches, RBAC markers), and the integration tests cover the key scenarios well. The stale condition bug is a real issue but low operational impact (cosmetic status noise). The large PR bot blocker needs to be addressed regardless.

[ChrisJBurns]

@claude please re-review. The previous feedback has been addressed: stale condition removal on ref clear, large PR justification added, CEL mutual exclusivity validation, optional pointer type for oidcConfig, and shared condition constants.

Large PR justification has been provided. Thank you!

[github-actions]

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

[claude]

Claude finished @ChrisJBurns's task in 1m 22s —— View job

---

Re-review: Wire MCPOIDCConfig into MCPRemoteProxy controller

• Gather context and understand request
• Read changed files (controller, types, tests)
• Verify previous issues were addressed
• Check for any remaining issues
• Post review findings

---

Previous Issues — All Addressed

Issue	Status
Stale condition not removed when OIDCConfigRef is cleared	✅ Fixed — meta.RemoveStatusCondition(..., ConditionOIDCConfigRefValidated) added at mcpremoteproxy_controller.go:723
Integration test missing condition-removal assertion	✅ Fixed — test now asserts cond == nil at mcpoidcconfig_mcpremoteproxy_integration_test.go:732-734
Large PR Justification missing	✅ Added to PR body
CEL mutual exclusivity validation	✅ Present at mcpremoteproxy_types.go:38
Optional pointer type for oidcConfig	✅ OIDCConfig *OIDCConfigRef at mcpremoteproxy_types.go:62
Shared condition constants	✅ Using mcpv1alpha1.ConditionOIDCConfigRefValidated from mcpserver_types.go:72 — no per-type duplicates

---

No New Issues Found

The nil guards in buildOIDCClientSecretEnvVars (mcpremoteproxy_deployment.go:226) correctly handle the now-optional OIDCConfig pointer. The resolveAndAddOIDCConfig helper cleanly routes both the OIDCConfigRef and legacy inline paths. The bidirectional watches and ReferencingWorkloads tracking in mcpoidcconfig_controller.go follow the established pattern correctly.

The previously noted unreachable nil check at line ~749 is a cosmetic carry-over from the sibling controllers — not a blocker.

---

Verdict: Approve

All critical and medium issues from the previous review have been resolved. The implementation is correct, consistent with the MCPServer and VirtualMCPServer patterns, and the integration tests now cover the full lifecycle including condition cleanup on ref removal.