Skip to content

Propagate protectedResourceAllowPrivateIP through OIDC resolver and converter#4784

Merged
ChrisJBurns merged 7 commits intomainfrom
chrisburns/fix-protectedresource-allowprivateip
Apr 14, 2026
Merged

Propagate protectedResourceAllowPrivateIP through OIDC resolver and converter#4784
ChrisJBurns merged 7 commits intomainfrom
chrisburns/fix-protectedresource-allowprivateip

Conversation

@ChrisJBurns
Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns commented Apr 13, 2026

Summary

The protectedResourceAllowPrivateIP CRD field was defined on both MCPServer and VirtualMCPServer but never actually propagated through the OIDC resolver, making the field a no-op. Additionally, the vmcpconfig converter incorrectly mapped it from jwksAllowPrivateIP, meaning the two fields could not be controlled independently.

Fixes bugs 2 and 3 from #3142 (Bug 1 was fixed separately via #4250).
The MCPServer/MCPRemoteProxy runner path remains out of scope — tracked in #4787.

Medium level
  • Add ProtectedResourceAllowPrivateIP field to the internal oidc.OIDCConfig struct so it can carry the value from CRD types through to consumers
  • Propagate the field through all three user-facing resolve paths: inline, shared inline (MCPOIDCConfig), and ConfigMap
  • Fix the vmcpconfig converter to map ProtectedResourceAllowPrivateIP from the correct source field (was incorrectly using JWKSAllowPrivateIP)
  • Add the missing JwksAllowPrivateIP mapping in the converter so the auth factory's OR logic (ProtectedResourceAllowPrivateIP || JwksAllowPrivateIP) works correctly
  • Add missing JWKSURL and IntrospectionURL mappings in mapResolvedOIDCToVmcpConfigFromRef (pre-existing gap fixed opportunistically)
  • Document runtime OR behavior on both CRD fields
Low level
File Change
cmd/thv-operator/pkg/oidc/resolver.go Add ProtectedResourceAllowPrivateIP to OIDCConfig struct; propagate in resolveInlineConfig, resolveFromInlineSharedConfig, resolveConfigMapConfig; extract configMapBoolTrue constant
cmd/thv-operator/pkg/vmcpconfig/converter.go Fix mapResolvedOIDCToVmcpConfig and mapResolvedOIDCToVmcpConfigFromRef to use correct source field; add missing JwksAllowPrivateIP, JWKSURL, IntrospectionURL mappings
cmd/thv-operator/api/v1alpha1/mcpoidcconfig_types.go Document runtime OR behavior on private IP fields
cmd/thv-operator/api/v1alpha1/mcpserver_types.go Document runtime OR behavior on private IP fields
cmd/thv-operator/pkg/oidc/resolver_test.go Update "all fields" tests; add independent propagation test cases for inline and ConfigMap
cmd/thv-operator/pkg/oidc/resolver_configref_test.go Add shared inline config propagation test
cmd/thv-operator/pkg/vmcpconfig/converter_test.go Update "maps all fields" test; add independent field mapping test

Type of change

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

Does this introduce a user-facing change?

Setting protectedResourceAllowPrivateIP: true on VirtualMCPServer OIDC config now works as documented. Previously the field was silently ignored.

Note: The field remains a no-op for MCPServer and MCPRemoteProxy — those resource types use a different runner path that only supports jwksAllowPrivateIP. This is tracked in #4787.

Behavioral note: Setting jwksAllowPrivateIP: true previously had the unintended side effect of also enabling private IPs for the protected resource endpoint. After this fix, users who need both must set both fields explicitly.

Test plan

  • task lint-fix passes (0 issues on affected packages)
  • task test passes on cmd/thv-operator/pkg/oidc/... and cmd/thv-operator/pkg/vmcpconfig/...
  • task build passes for cmd/thv-operator/...
  • New unit tests: independent propagation (resolver inline, ConfigMap, shared inline), independent field mapping (converter)
  • Existing tests updated to cover both fields

Special notes for reviewers

Generated with Claude Code

…onverter

The protectedResourceAllowPrivateIP CRD field was defined but never
propagated through the OIDC resolver, and the vmcpconfig converter
incorrectly mapped it from jwksAllowPrivateIP. This meant the field
had no effect and could not be controlled independently.

Add ProtectedResourceAllowPrivateIP to the resolver OIDCConfig struct,
propagate it through inline, shared inline, and ConfigMap resolve paths,
fix the converter to use the correct source field, and add the missing
JwksAllowPrivateIP mapping.

Fixes bugs 2 and 3 from #3142.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the size/S Small PR: 100-299 lines changed label Apr 13, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.92%. Comparing base (6a79259) to head (77ac79c).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
cmd/thv-operator/pkg/vmcpconfig/converter.go 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4784      +/-   ##
==========================================
- Coverage   68.94%   68.92%   -0.03%     
==========================================
  Files         517      517              
  Lines       54741    54749       +8     
==========================================
- Hits        37742    37735       -7     
- Misses      14095    14107      +12     
- Partials     2904     2907       +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.

Copy link
Copy Markdown
Collaborator Author

@ChrisJBurns ChrisJBurns left a comment

Choose a reason for hiding this comment

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

Multi-Agent Consensus Review

Agents consulted: kubernetes-expert, go-security-reviewer, code-reviewer, toolhive-expert
Recommendation: ✅ APPROVE

Consensus Summary

# Finding Consensus Severity Action
1 MCPServer/MCPRemoteProxy paths drop ProtectedResourceAllowPrivateIP 8/10 HIGH Comment (pre-existing)
2 Downstream factory ORs both fields into single AllowPrivateIP 7/10 MEDIUM Comment (pre-existing)
3 Missing ConfigMap independence test 7/10 LOW Suggest

Overall

This is a clean, well-scoped bug fix. The core change -- adding ProtectedResourceAllowPrivateIP to the resolver's OIDCConfig struct, propagating it through all three resolution paths (inline, shared inline, ConfigMap), and fixing the converter mapping -- is correct and addresses both bugs from #3142.

All three findings that survived consensus scoring are either pre-existing issues (F1, F2) or a minor test gap (F3). No agent identified any issues introduced by this PR. The test suite includes independence assertions that verify the two fields can be controlled separately, which directly guards against regression of the original bug. The behavioral change (separating jwksAllowPrivateIP from protectedResourceAllowPrivateIP) is clearly documented in the PR description.

The main pre-existing gaps to track: (1) MCPServer and MCPRemoteProxy CRD paths still don't propagate ProtectedResourceAllowPrivateIP through runner.WithOIDCConfig, and (2) the downstream auth factory ORs both fields into a single AllowPrivateIP, limiting the practical granularity of the separation.


Generated with Claude Code

Comment thread cmd/thv-operator/pkg/vmcpconfig/converter.go
Comment thread cmd/thv-operator/pkg/vmcpconfig/converter.go
Comment thread cmd/thv-operator/pkg/oidc/resolver_test.go
Comment thread cmd/thv-operator/pkg/oidc/resolver.go
Comment thread cmd/thv-operator/pkg/vmcpconfig/converter.go
Address review feedback (F3): add a ConfigMap test case where
jwksAllowPrivateIP is true but protectedResourceAllowPrivateIP is
absent, verifying they resolve independently.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 13, 2026
…verter mappings

Add doc comments to JWKSAllowPrivateIP and ProtectedResourceAllowPrivateIP
on both InlineOIDCSharedConfig and InlineOIDCConfig noting that at runtime
either field being true enables private IPs for all OIDC HTTP requests.

Also add missing JWKSURL and IntrospectionURL mappings in
mapResolvedOIDCToVmcpConfigFromRef, which silently dropped these fields
for the MCPOIDCConfig reference path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 13, 2026
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 13, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 13, 2026
@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 Apr 13, 2026
Co-Authored-By: Claude Opus 4.6 (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 Apr 13, 2026
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

LGTM -- clean bug fix with good test coverage

@ChrisJBurns ChrisJBurns merged commit c63b10e into main Apr 14, 2026
41 checks passed
@ChrisJBurns ChrisJBurns deleted the chrisburns/fix-protectedresource-allowprivateip branch April 14, 2026 10:11
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.

2 participants