stacklok / toolhive / 27649785079

67%

Build Type

push

github

Committed by web-flow

Commit Message

Wire OBO SecretEnvVars into MCPServer and MCPRemoteProxy (#5540)

* Wire OBO SecretEnvVars into MCPServer and proxy

The OBOHandler exposes three operator dispatch points, but SecretEnvVars
was dispatched from VirtualMCPServer only. For a build with a registered
OBO handler, an obo-typed MCPExternalAuthConfig referenced from an
MCPServer or MCPRemoteProxy got its middleware wired into the RunConfig
yet never received the env var(s) the handler asks for, so the OBO
middleware could not read its credential at startup.

Add AddExternalAuthConfigSecretEnvVars in controllerutil — the secret-env
sibling of AddExternalAuthConfigOptions — and call it from MCPServer (both
the deployment builder and the drift check) and MCPRemoteProxy. The
dispatcher handles only the obo type, whose env var name is defined by the
handler and is therefore identical across consumers; every other type
keeps its existing per-consumer helper so pod env stays byte-identical and
VirtualMCPServer is untouched. ErrEnterpriseRequired is treated as "no env
vars" so builder and drift agree in handler-less builds (no hot-loop).

Implements changes for issue #5537:
- Add the shared obo-scoped secret-env dispatcher with unit tests covering
  every ExternalAuthType plus the registered-handler and inert cases
- Inject OBO env vars in MCPServer builder + drift and MCPRemoteProxy
- Add controller tests asserting the env var is present and does not drift

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Address code review feedback

Fixed issues from code review (comment-only, no behavior change):
- MEDIUM: Document the known builder/drift divergence on a genuine OBO
  handler error at the MCPServer drift site; the inert/handler-less path
  stays symmetric, and unifying the transient-error case is out of scope
- LOW: Note in AddExternalAuthConfigSecretEnvVars that VirtualMCPServer's
  firstOBOSecretEnvVar keeps only the first env var, so the two are
  reconciled if multi-var... (continued)

Coverage Stats

77 of 84 new or added lines in 4 files covered. (91.67%)

8 existing lines in 4 files now uncovered.

69030 of 103018 relevant lines covered (67.01%)

64.19 hits per line