Repository navigation
Re-check upstream DCR registrations while running - #6948
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6948 +/- ##
==========================================
+ Coverage 80.09% 80.17% +0.08%
==========================================
Files 839 846 +7
Lines 85339 85837 +498
==========================================
+ Hits 68351 68822 +471
- Misses 16983 17010 +27
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b0d06df to
011615d
Compare
jhrozek
left a comment
There was a problem hiding this comment.
Thanks for this, the swap itself is nicely done: one atomic load per request, the capability reached by type assertion, and Close stopping the loops before the server. Most of what follows is about what happens when the start-time resolution runs on a timer on every replica. The two I'd want addressed before merge are the OIDC client/verifier pairing and jitter on the ticks. Endpoint adoption and the per-tick stale warning are close behind.
The suggestions were applied together locally and pass build, the scoped race tests and the envtest. If you take the metav1.Duration one, task operator-generate operator-manifests crdref-gen regenerates the rest.
The embedded auth server resolves an upstream's DCR registration once, at start. A registration the upstream drops afterwards stays in use until the process restarts, so every sign-in in between fails at the upstream, and a replica that started earlier keeps a registration a later replica replaced (stacklok#6496). dcrConfig.registrationProbeInterval (a duration of at least 1m, empty by default) repeats the start-time resolution on that interval. Each tick re-reads the stored row, checks it with the configured registrationProbe, replaces a rejected registration, registers a missing one, and switches the running upstream provider to the result when it names a different client, including a row another replica replaced. Endpoints the run config left to discovery follow the new registration, and a file-based initial access token is re-read on every tick. The first tick waits a random part of the interval and later ones the interval +-10%, so replicas of one rollout do not probe and re-register together. A provider holds its client config and, for OIDC, the ID token verifier as one state behind an atomic pointer; a token request and the ID token check that follows it use the state they loaded, so a swap during the request does not fail it. A failed tick is logged and leaves the provider on its current client; a switch is logged at Warn because authorizations started under the previous client may fail their code exchange. Periodic ticks log the 90-day staleness warning at Debug. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Aron Gates <aron@muonspace.com>
011615d to
6898e0b
Compare
Covers operator.defaultRedis.tls (stacklok/toolhive#6838) and dcrConfig.registrationProbeInterval (stacklok/toolhive#6948) for v0.59.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Update stacklok/toolhive to v0.59.0 Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Refresh reference assets for toolhive v0.59.0 * Document Redis TLS default and DCR re-check Covers operator.defaultRedis.tls (stacklok/toolhive#6838) and dcrConfig.registrationProbeInterval (stacklok/toolhive#6948) for v0.59.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Clarify DCR registration re-check wording Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
The embedded auth server resolves an upstream's DCR registration once, when it starts. The
registrationProbecheck from #6822 and #6834 runs inside that step, so a registration the upstream drops while the proxy runs stays in use until the process restarts, and every sign-in in between fails at the upstream. In the deployment behind #6821 the upstream has dropped our registrations three times in under four weeks, most recently five days after a fresh registration, and proxy pods run for three to four days between restarts, so startup-only checking leaves sign-ins broken for days after each drop. The same single-shot resolution lets replicas drift apart: a replica that started earlier keeps a registration that a later replica replaced (#6496).dcrConfig.registrationProbeIntervalonMCPExternalAuthConfig(and the inlineVirtualMCPServerconfig), ametav1.Durationof at least1m, empty by default. The CRD restricts it to Go duration syntax and enforces the minimum with a CEL rule; run-config validation mirrors both for YAML and CLI configs.dcr.ResolveCredentialson that interval. That call re-reads the stored row, checks it with the configuredregistrationProbe, replaces a rejected registration throughReplaceIfUnchanged, and registers a missing one. When the result differs from the client in use, including a row another replica replaced, the running provider switches to it in place. Endpoints the run config left to discovery follow the new registration, as they do at start, and a file-based initial access token is re-read on every tick.clientStatebehind anatomic.Pointer, replaced whole under a mutex. A code exchange or refresh and the ID token check that follows it use the state they loaded, so a swap during the token request does not fail that sign-in or refresh. The newupstream.ClientCredentialsUpdatercapability performs the swap; the server exposes it as the optionalauthserver.UpstreamCredentialsUpdater, reached by type assertion likeCloseIdleConnections, so theServerinterface is unchanged.EmbeddedAuthServer.Closestops the loops before closing the server.Related to #6821 and #6496. Builds on #6834.
Type of change
Test plan
go test -raceonpkg/authserver/...,pkg/auth/...,pkg/vmcp/...andcmd/thv-operator/...outsidetest-integration)task test-e2e)task lint)The
mcp-external-authenvtest suite (Kubernetes 1.31.0) passes, including the newdcr_probe_interval_validation_test.go.task gen,task operator-generate operator-manifests crdref-genandtask docsleave no diff, andtask docs-verifypasses.New tests:
TestEmbeddedAuthServer_DCRRecheckRecoversWithoutRestart: two replicas of a downstream auth server share one Redis and a ToolHive upstream, and both present the same registration. The upstream evicts it. One re-check on the first replica re-registers and its next authorization reaches the upstream with the new client; one re-check on the second replica adopts the replaced row. A further re-check against a live registration changes nothing.TestEmbeddedAuthServer_DCRRecheckFollowsMovedTokenEndpoint: the upstream forgets its clients and moves its token endpoint. After one re-check a full sign-in, code redemption included, succeeds at the new token endpoint, and the re-registration presents the initial access token rewritten on disk.TestOIDCProvider_UpdateDuringTokenRequestKeepsOneClient: the token response is held open acrossUpdateClientCredentials; the code exchange and the refresh still verify the ID token issued to the client that made the request.TestBaseOAuth2Provider_UpdateClientCredentials,TestBaseOAuth2Provider_UpdateClientCredentialsWhileServing,TestOIDCProvider_UpdateClientCredentials: the authorization URL, code exchange and refresh use the new client, its auth method and its endpoints; OIDC providers keep body credentials and discovered endpoints and verify ID tokens for the new client_id only; invalid updates keep the previous client; concurrent swaps under-race.TestUpdateUpstreamClientCredentials: updates by upstream name and by an empty name for the single unnamed upstream; a provider without the capability, an unknown name and a server without the capability are refused.TestNewDCRRecheck,TestDCRRecheckTick_KeepsCurrentClientOnFailure,TestStartDCRRechecks_StopsLoops,TestJitterInterval: loop construction for OAuth2 and OIDC upstreams (periodic request copy, endpoint adoption, token file), a failing store, an unreadable token file and a failing switch keep the current client, stop waits for the loops, and the jitter stays within ±10%.TestLookupCachedResolution_StaleWarningOnlyAtStart: the staleness warning is Warn on a start-time lookup and Debug on a periodic one.dcr_probe_interval_validation_test.go(envtest): omitted,1m,90sand1hare admitted;30sand0are rejected with the minimum message; malformed5,1dandsoonare rejected; theVirtualMCPServercopy of the schema rejects30s.registration_probe_intervaland the operator's run-config mapping (5m0s).API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.goregistrationProbeInterval(metav1.Duration, Go duration pattern, CEL minimum of1m)cmd/thv-operator/pkg/controllerutil/authserver.gopkg/authserver/config.goRegistrationProbeInterval,ProbeIntervaland its validationpkg/authserver/upstream/oauth2.goatomic.Pointer[clientState]; addClientCredentials,ClientCredentialsUpdaterandUpdateClientCredentials; exchange and refresh take the state they loadedpkg/authserver/upstream/oidc.gopkg/authserver/server.go,pkg/authserver/server_impl.goUpstreamCredentialsUpdatercapability andUpdateUpstreamClientCredentials; document the loops in theCloseIdleConnectionscontractpkg/authserver/runner/dcr_recheck.gopkg/authserver/runner/embeddedauthserver.goClosepkg/auth/dcr/request.go,pkg/auth/dcr/resolver.goRequest.Periodic; staleness warning at Debug on periodic lookupsdocs/arch/11-auth-server-storage.mddocs/operator/crd-api.md, swagger, Go SDKDoes this introduce a user-facing change?
Yes, opt-in. Before: a cached upstream DCR registration is checked only when the proxy starts, and a registration the upstream drops later stays in use until the pods restart. After: with
dcrConfig.registrationProbeIntervalset, each replica re-checks on that interval and switches to a replacement registration without a restart, so sign-ins recover within about one interval. Each check sends at most oneregistrationProberequest to the upstream from every replica (none withregistrationProbe: none), plus a registration request when the probe rejects the client; thetokenEndpointprobe appears in upstream logs as a failed code redemption, so 5m to 15m suits most upstreams. Without the field nothing changes.Special notes for reviewers
ResolveCredentialsrather than a narrower re-check so the start-time and periodic paths share one implementation, including the singleflight and the compare-and-set replacement.registrationProbe: nonethe interval still helps: the tick adopts a row another replica replaced and registers a missing one, which covers the drift described in DCR credential resolution never refreshes after a replica's startup, allowing cross-replica generation drift #6496 for upstreams the probes cannot check.CloseIdleConnectionsdoes not stop the loops, for the same reason it leaves the JWKS workers running: it is safe to call on a server that keeps serving. A superseded server retired that way keeps re-checking until it is closed; both doc comments say so.registration_client_uriandregistration_access_token; requiring two rejections in a row before rotating while running; an RFC 7592 §2.1 read probe.🤖 Generated with Claude Code