Skip to content

Conversation

@yrobla
Copy link
Contributor

@yrobla yrobla commented Dec 5, 2025

Implements support for the 'unauthenticated' and header injection authentication strategy to align MCPExternalAuthConfig CRD with vMCP's supported auth strategies.

This change adds the third and final auth strategy type:

  • tokenExchange (CRD) -> token_exchange (vMCP)
  • headerInjection (CRD) -> header_injection (vMCP)
  • unauthenticated (CRD) -> unauthenticated (vMCP)

Large PR Justification

This is a complete PR adding missing modes to external auth. It adds support for unauthenticated, and properly adds missing tests for this mode and for header injection. The PR cannot be splitted if we want it to be atomic.

@github-actions github-actions bot added the size/L Large PR: 600-999 lines changed label Dec 5, 2025
@yrobla yrobla force-pushed the feat/vmcp_auth_strategies_incompatible branch from 7e798f7 to 26df11e Compare December 5, 2025 11:30
@github-actions github-actions bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Dec 5, 2025
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 23.36957% with 141 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.23%. Comparing base (afe9b41) to head (8cfe29b).

Files with missing lines Patch % Lines
cmd/thv-operator/pkg/vmcpconfig/converter.go 0.00% 127 Missing ⚠️
...ator/api/v1alpha1/mcpexternalauthconfig_webhook.go 80.00% 6 Missing ⚠️
...perator/controllers/virtualmcpserver_deployment.go 0.00% 4 Missing ⚠️
cmd/thv-operator/main.go 0.00% 2 Missing ⚠️
...d/thv-operator/pkg/controllerutil/tokenexchange.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2915      +/-   ##
==========================================
- Coverage   56.36%   56.23%   -0.14%     
==========================================
  Files         323      326       +3     
  Lines       31763    31910     +147     
==========================================
+ Hits        17904    17943      +39     
- Misses      12330    12435     +105     
- Partials     1529     1532       +3     

☔ 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.

@yrobla yrobla force-pushed the feat/vmcp_auth_strategies_incompatible branch from 26df11e to 41d89b5 Compare December 5, 2025 11:38
@yrobla yrobla requested a review from Copilot December 5, 2025 11:39
Copy link
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.

@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/L Large PR: 600-999 lines changed labels Dec 5, 2025
Copilot finished reviewing on behalf of yrobla December 5, 2025 11:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements support for the 'unauthenticated' authentication strategy in the MCPExternalAuthConfig CRD, completing the alignment with vMCP's three supported authentication strategies: token_exchange, header_injection, and unauthenticated.

Key Changes:

  • Added ExternalAuthTypeUnauthenticated type constant to the CRD API
  • Implemented UnauthenticatedConverter with appropriate no-op behavior for secret resolution
  • Added comprehensive validation webhook to ensure mutual exclusivity of auth type configurations

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go Added ExternalAuthTypeUnauthenticated constant with documentation explaining it should only be used for trusted networks
cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go Implemented validation webhook ensuring unauthenticated type has no conflicting auth configs
cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook_test.go Added comprehensive validation tests covering all auth types and invalid configurations
pkg/vmcp/auth/converters/unauthenticated.go Implemented converter that returns strategy with no additional auth fields
pkg/vmcp/auth/converters/unauthenticated_test.go Added unit tests for converter including integration with registry
pkg/vmcp/auth/converters/interface.go Registered the new unauthenticated converter in the default registry
cmd/thv-operator/pkg/controllerutil/tokenexchange.go Added case to handle unauthenticated type with no-op behavior
cmd/thv-operator/controllers/virtualmcpserver_deployment.go Added case to skip secret mounting for unauthenticated type
test/e2e/thv-operator/virtualmcp/virtualmcp_external_auth_test.go Added comprehensive e2e tests for both discovered and inline unauthenticated auth modes
deploy/charts/operator-crds/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml Updated CRD enum to include unauthenticated type
docs/operator/crd-api.md Updated API documentation with unauthenticated type description
deploy/charts/operator-crds/Chart.yaml Bumped chart version from 0.0.74 to 0.0.75
deploy/charts/operator-crds/README.md Updated version badge to reflect chart version bump
config/webhook/manifests.yaml Added webhook configuration for MCPExternalAuthConfig validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@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 Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

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

@github-actions github-actions bot dismissed their stale review December 5, 2025 12:01

Large PR justification has been provided. Thank you!

@yrobla yrobla requested a review from Copilot December 5, 2025 12:01
Copilot finished reviewing on behalf of yrobla December 5, 2025 12:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@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 Dec 5, 2025
@yrobla yrobla force-pushed the feat/vmcp_auth_strategies_incompatible branch 2 times, most recently from 7c7ed34 to cd83151 Compare December 5, 2025 14:40
Implements support for the 'unauthenticated' authentication strategy
to align MCPExternalAuthConfig CRD with vMCP's supported auth strategies.

This change adds the third and final auth strategy type:
- tokenExchange (CRD) -> token_exchange (vMCP)
- headerInjection (CRD) -> header_injection (vMCP)
- unauthenticated (CRD) -> unauthenticated (vMCP)
@yrobla yrobla force-pushed the feat/vmcp_auth_strategies_incompatible branch from cd83151 to ae87961 Compare December 5, 2025 14:43
@yrobla yrobla changed the title Add unauthenticated auth strategy to MCPExternalAuthConfig Add unauthenticated and headerinjection auth strategy to MCPExternalAuthConfig Dec 5, 2025
@github-actions github-actions bot removed the size/XL Extra large PR: 1000+ lines changed label Dec 5, 2025
@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Dec 5, 2025
@yrobla yrobla requested a review from Copilot December 5, 2025 14:43
Copilot finished reviewing on behalf of yrobla December 5, 2025 14:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@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 Dec 5, 2025
@yrobla yrobla force-pushed the feat/vmcp_auth_strategies_incompatible branch from 2f2b56b to 8cfe29b Compare December 5, 2025 15:03
@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 Dec 5, 2025
@yrobla yrobla requested review from JAORMX, amirejaz and jhrozek December 5, 2025 15:20
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.

3 participants