Skip to content

Unify subject provider defaulting, hard-error xaa#5708

Open
jhrozek wants to merge 1 commit into
stacklok:mainfrom
jhrozek:5687-5697-defaulting-and-error
Open

Unify subject provider defaulting, hard-error xaa#5708
jhrozek wants to merge 1 commit into
stacklok:mainfrom
jhrozek:5687-5697-defaulting-and-error

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

The YAML and operator paths each defaulted a backend's SubjectProviderName to the first configured upstream, but drifted apart:

This extracts a shared authtypes.DefaultSubjectProviderName helper used by both the YAML (pkg/vmcp/config/defaults.go) and operator (cmd/thv-operator/controllers/virtualmcpserver_controller.go) paths, covering token_exchange, aws_sts, and xaa.

  • xaa now hard-errors on ambiguous multi-upstream configs on both paths.
  • On the operator side this surfaces as a non-fatal per-backend AuthConfigError condition (AmbiguousSubjectProvider), consistent with the existing degraded-mode pattern for other auth config errors — the rest of the VirtualMCPServer keeps reconciling.
  • On the YAML/CLI side, InjectSubjectProviderNames now returns an error, so thv vmcp serve fails fast at startup rather than silently guessing.

Fixes #5687
Fixes #5697

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Does this introduce a user-facing change?

Yes, narrowly: a xaa backend auth strategy with an empty SubjectProviderName and 2+ configured upstreams now fails validation instead of silently defaulting to the first upstream. On the operator this surfaces as a per-backend status condition (degraded mode, rest of the server keeps running); on the YAML/CLI path this causes thv vmcp serve to fail at startup. Since xaa support merged the day before this change, there are no existing deployments this affects — new xaa adopters will simply need to set SubjectProviderName explicitly when configuring more than one upstream. token_exchange and aws_sts behavior is unaffected by this change (they continue to default silently); the only behavior fix for them is that the YAML path now also defaults aws_sts, matching the operator path.

Special notes for reviewers

The equivalent ambiguous-upstream defaulting in pkg/runner/middleware.go (Cedar's PrimaryUpstreamProvider) has the same footgun this PR fixes for xaa, but is intentionally out of scope here to keep this PR to one logical change. Worth a follow-up issue.

Generated with Claude Code

The YAML and operator paths each defaulted a backend's
SubjectProviderName to the first configured upstream, but drifted
apart: the YAML path was missing aws_sts entirely (stacklok#5687), so an
aws_sts backend with an unset field failed at request time instead
of being defaulted. Separately, both paths silently picked
upstream[0] for xaa even with multiple upstreams configured, a
security-relevant footgun since sending the wrong subject token to
Step A yields a confusing IdP error or a token minted for the wrong
subject (stacklok#5697).

Extract a shared authtypes.DefaultSubjectProviderName helper used by
both the YAML (pkg/vmcp/config/defaults.go) and operator
(cmd/thv-operator/controllers/virtualmcpserver_controller.go) paths,
covering token_exchange, aws_sts, and xaa. xaa now hard-errors on
ambiguous multi-upstream configs on both paths; on the operator side
this surfaces as a non-fatal per-backend AuthConfigError condition
rather than a Reconcile failure.

Fixes stacklok#5687, stacklok#5697

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.38202% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.68%. Comparing base (2aca563) to head (71e39a9).

Files with missing lines Patch % Lines
pkg/vmcp/cli/serve.go 0.00% 2 Missing ⚠️
pkg/vmcp/config/defaults.go 84.61% 1 Missing and 1 partial ⚠️
...perator/controllers/virtualmcpserver_controller.go 97.61% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5708      +/-   ##
==========================================
+ Coverage   70.63%   70.68%   +0.05%     
==========================================
  Files         667      668       +1     
  Lines       67607    67641      +34     
==========================================
+ Hits        47752    47815      +63     
+ Misses      16399    16356      -43     
- Partials     3456     3470      +14     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

1 participant