Skip to content

Conversation

@jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Nov 28, 2025

remove Metadata field and use typed auth config

Remove the deprecated Metadata map[string]any field from BackendAuthStrategy and migrate all code to use typed fields (HeaderInjection, TokenExchange).

Large PR Justification

This is a refactoring that touches large number of files. I split it into 2 patches for easier review, the first one just moves constants from 1 package to another, the next patch removes

@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Nov 28, 2025
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.

@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 83.90805% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.55%. Comparing base (996f475) to head (980f0df).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/config/validator.go 31.25% 8 Missing and 3 partials ⚠️
pkg/vmcp/auth/converters/interface.go 47.05% 9 Missing ⚠️
cmd/thv-operator/pkg/vmcpconfig/converter.go 0.00% 4 Missing ⚠️
test/integration/vmcp/helpers/vmcp_server.go 0.00% 2 Missing ⚠️
pkg/vmcp/auth/strategies/tokenexchange.go 96.55% 1 Missing ⚠️
pkg/vmcp/client/client.go 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## main #2797 +/- ## ========================================== - Coverage 56.57% 56.55% -0.02%  ========================================== Files 320 320 Lines 30882 30874 -8 ========================================== - Hits 17470 17460 -10  Misses 11911 11911 - Partials 1501 1503 +2 

☔ 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 refactor/typed-backend-auth-strategy branch from d8bebfd to 43e900d Compare December 1, 2025 09:05
@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 1, 2025
@jhrozek jhrozek force-pushed the refactor/typed-backend-auth-strategy branch from 43e900d to 2b4a59e Compare December 1, 2025 09:29
@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 1, 2025
Create a new leaf package pkg/vmcp/auth/types containing: - Strategy type constants (StrategyTypeUnauthenticated, etc.) - BackendAuthStrategy struct with typed fields (HeaderInjection, TokenExchange) - HeaderInjectionConfig and TokenExchangeConfig structs The Metadata field is retained in BackendAuthStrategy for backward compatibility - all existing code continues to work unchanged. Update all files to import BackendAuthStrategy from auth/types instead of config package. This is a structural refactoring with zero behavior change, preparing for a follow-up PR that will remove the Metadata field.
@jhrozek jhrozek force-pushed the refactor/typed-backend-auth-strategy branch from 2b4a59e to 964e3fd Compare December 1, 2025 11:47
@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 1, 2025
Remove the deprecated Metadata map[string]any field from BackendAuthStrategy and migrate all code to use typed fields (HeaderInjection, TokenExchange). Key changes: - Remove Metadata field from authtypes.BackendAuthStrategy - Update Strategy interface to accept *BackendAuthStrategy instead of map - Update all strategies (header_injection, tokenexchange, unauthenticated) - Update converters to return typed BackendAuthStrategy - Change Backend/BackendTarget structs to use AuthConfig instead of AuthStrategy + AuthMetadata - Update ResolveForBackend to return *BackendAuthStrategy - Update all consumers and tests This provides type safety, better IDE support, and eliminates runtime type assertions throughout the auth subsystem.
@jhrozek jhrozek force-pushed the refactor/typed-backend-auth-strategy branch from 964e3fd to 980f0df Compare December 1, 2025 12:39
@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 1, 2025
@github-actions github-actions bot dismissed their stale review December 1, 2025 12:39

Large PR justification has been provided. Thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

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

@jhrozek jhrozek changed the title DRAFT: Refactor/typed backend auth strategy Refactor/typed backend auth strategy Dec 1, 2025
@amirejaz amirejaz merged commit 20ecb03 into main Dec 1, 2025
34 checks passed
@amirejaz amirejaz deleted the refactor/typed-backend-auth-strategy branch December 1, 2025 13:29
amirejaz added a commit that referenced this pull request Dec 1, 2025
- Update convertExternalAuthConfigToStrategy to use authtypes.BackendAuthStrategy - Update convertBackendAuthConfigToVMCP to use typed structures - Remove Metadata map usage in favor of typed fields (TokenExchange, HeaderInjection) - This ensures compatibility with the marshaling fix from PR #2797
amirejaz added a commit that referenced this pull request Dec 1, 2025
…alMCPServer This PR implements the core discovery feature that allows VirtualMCPServer to automatically discover ExternalAuthConfig from MCPServers in the group and include them in the outgoing auth configuration. Changes: - Add discoverExternalAuthConfigs() to discover auth configs from MCPServers - Add buildOutgoingAuthConfig() to build outgoing auth with discovery - Add convertExternalAuthConfigToStrategy() to convert CRD to strategy - Add convertBackendAuthConfigToVMCP() to convert backend auth config - Add comprehensive test suite for discovery and conversion logic - Integration with typed BackendAuthStrategy from PR #2797 Note: Secret management for discovered ExternalAuthConfigs will be added in a follow-up PR.
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

4 participants