Skip to content

Add authServerRef CRD types, controller logic, and unit tests#4644

Draft
tgrunnagle wants to merge 4 commits intomainfrom
issue_4640_authServerRef
Draft

Add authServerRef CRD types, controller logic, and unit tests#4644
tgrunnagle wants to merge 4 commits intomainfrom
issue_4640_authServerRef

Conversation

@tgrunnagle
Copy link
Copy Markdown
Contributor

Summary

The embedded auth server currently competes with outgoing auth types (like AWS STS) for the single externalAuthConfigRef slot on MCPServer and MCPRemoteProxy CRDs. Because MCPExternalAuthConfig enforces mutually exclusive types, users cannot configure both an embedded auth server for incoming client authentication and an outgoing token exchange on the same resource. This PR adds a dedicated authServerRef field to both CRDs, separating the embedded auth server from externalAuthConfigRef so both can coexist. This is Phase 1 of RFC-0050.

Closes #4640

Type of change

  • New feature

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Changes

File Change
api/v1alpha1/mcpserver_types.go Add AuthServerRef field to MCPServerSpec and AuthServerConfigHash to MCPServerStatus
api/v1alpha1/mcpremoteproxy_types.go Add AuthServerRef field to MCPRemoteProxySpec and AuthServerConfigHash to MCPRemoteProxyStatus
api/v1alpha1/zz_generated.deepcopy.go Regenerated deepcopy for new fields
pkg/controllerutil/authserver.go Add AddAuthServerRefOptions, ValidateAndAddAuthServerRefOptions, and GenerateAuthServerConfigFromRef utility functions
controllers/mcpserver_runconfig.go Wire conflict validation and authServerRef resolution into MCPServer runconfig generation
controllers/mcpremoteproxy_runconfig.go Wire conflict validation and authServerRef resolution into MCPRemoteProxy runconfig generation
controllers/mcpserver_controller.go Add handleAuthServerRef for config hash tracking, extend watch handler and deployment builder for authServerRef
controllers/mcpremoteproxy_controller.go Add handleAuthServerRef for config hash tracking, extend watch handler for authServerRef
controllers/mcpremoteproxy_deployment.go Generate volumes/mounts/env vars from authServerRef in deployment builder
controllers/mcpexternalauthconfig_controller.go Extend findReferencingMCPServers and findReferencingWorkloads to check authServerRef with deduplication; add reverse watch for authServerRef in SetupWithManager
pkg/controllerutil/authserver_test.go Add unit tests for AddAuthServerRefOptions and ValidateAndAddAuthServerRefOptions covering nil ref, invalid kind, wrong type, valid ref, conflict detection, and combined auth paths
controllers/mcpexternalauthconfig_controller_test.go Add unit tests for referencing logic finding resources via authServerRef
CRD YAMLs (4 files) Regenerated CRD manifests with new fields

Does this introduce a user-facing change?

Yes. MCPServer and MCPRemoteProxy resources now accept an optional authServerRef field that references an MCPExternalAuthConfig of type embeddedAuthServer. This allows configuring the embedded auth server independently from externalAuthConfigRef, enabling use cases like embedded auth server + AWS STS token exchange on the same resource.

Special notes for reviewers

Generated with Claude Code

tgrunnagle and others added 3 commits April 7, 2026 09:32
Implements changes for issue #4640 (Phase 1 of RFC-0050):
- Add AuthServerRef (TypedLocalObjectReference) to MCPServerSpec and MCPRemoteProxySpec
- Add AuthServerConfigHash to MCPServerStatus and MCPRemoteProxyStatus
- Add AddAuthServerRefOptions and ValidateAndAddAuthServerRefOptions utility functions
- Wire conflict validation (authServerRef + externalAuthConfigRef both embedded = error)
- Extend watch handlers to reconcile on authServerRef config changes
- Extend MCPExternalAuthConfig controller to find resources via authServerRef
- Add handleAuthServerRef config hash tracking in both reconcile loops
- Add authServerRef volume/env generation for deployments
- Regenerate CRD YAML and deepcopy functions
- Add unit tests for AddAuthServerRefOptions and authServerRef referencing logic
Fixed issues from code review:
- HIGH: Return non-NotFound errors in conflict validation instead of silently ignoring
- HIGH: Add table-driven tests for ValidateAndAddAuthServerRefOptions
- HIGH: Add table-driven tests for GenerateAuthServerConfigFromRef
- MEDIUM: Return error for unsupported Kind in GenerateAuthServerConfigFromRef
- MEDIUM: Add comments explaining reconcile-time validation in deployment builders
- MEDIUM: Remove redundant name-matching in refExtractor closures

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The findReferencingMCPServers and findReferencingWorkloads closures used
an early-return pattern that only returned the externalAuthConfigRef name,
causing authServerRef changes to be missed when both fields were set on
the same server pointing to different MCPExternalAuthConfig resources.

Split each closure into two separate FindReferencingMCPServers calls (one
per ref field) and merge with deduplication. Also rewire
findReferencingWorkloads to delegate to findReferencingMCPServers to
avoid duplicating merge logic.

Add test cases covering:
- Server with both refs pointing to different configs (both trigger reconciliation)
- Server with both refs pointing to the same config (deduplicated to one entry)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Apr 7, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

❌ Patch coverage is 40.14599% with 164 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (1ee7107) to head (9f7ba26).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...or/controllers/mcpexternalauthconfig_controller.go 34.84% 80 Missing and 6 partials ⚠️
...d/thv-operator/controllers/mcpserver_controller.go 10.52% 31 Missing and 3 partials ⚠️
...-operator/controllers/mcpremoteproxy_controller.go 12.12% 27 Missing and 2 partials ⚠️
cmd/thv-operator/pkg/controllerutil/authserver.go 82.97% 4 Missing and 4 partials ⚠️
...v-operator/controllers/mcpremoteproxy_runconfig.go 60.00% 1 Missing and 1 partial ⚠️
...md/thv-operator/controllers/mcpserver_runconfig.go 60.00% 1 Missing and 1 partial ⚠️
cmd/thv-operator/pkg/controllerutil/config.go 77.77% 1 Missing and 1 partial ⚠️
...-operator/controllers/mcpremoteproxy_deployment.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4644      +/-   ##
==========================================
- Coverage   68.88%   68.82%   -0.07%     
==========================================
  Files         505      508       +3     
  Lines       52437    52762     +325     
==========================================
+ Hits        36121    36312     +191     
- Misses      13525    13641     +116     
- Partials     2791     2809      +18     

☔ 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 github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 7, 2026
@tgrunnagle tgrunnagle force-pushed the issue_4640_authServerRef branch from ce887cc to 30def11 Compare April 7, 2026 20:19
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 7, 2026
@tgrunnagle tgrunnagle force-pushed the issue_4640_authServerRef branch from 30def11 to 0234528 Compare April 7, 2026 20:37
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 7, 2026
@tgrunnagle tgrunnagle force-pushed the issue_4640_authServerRef branch from 0234528 to 5c69e4e Compare April 7, 2026 20:43
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 7, 2026
…rces

The MCPExternalAuthConfig controller only tracked MCPServer references,
missing MCPRemoteProxy resources that also have externalAuthConfigRef and
authServerRef fields. This adds:

- FindReferencingMCPRemoteProxies utility in controllerutil/config.go
- findReferencingMCPRemoteProxies and mapMCPRemoteProxyToExternalAuthConfig
  in the MCPExternalAuthConfig controller
- MCPRemoteProxy listing in findReferencingWorkloads
- Watches() registration for MCPRemoteProxy in SetupWithManager
- Kind check on AuthServerRef before treating it as MCPExternalAuthConfig
- Extracted mapMCPServerToExternalAuthConfig from anonymous closure
- Consolidated GenerateAuthServerConfig/FromRef into GenerateAuthServerConfigByName
- EmbeddedAuthServerConfigName helper for single auth config selection
- Fixed data race in parallel tests by using factory functions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tgrunnagle tgrunnagle force-pushed the issue_4640_authServerRef branch from 5c69e4e to 9f7ba26 Compare April 7, 2026 20:57
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add authServerRef CRD types, controller logic, and unit tests

1 participant