Skip to content

Re-check upstream DCR registrations while running - #6948

Merged
jhrozek merged 1 commit into
stacklok:mainfrom
aron-muon:dcr-runtime-recheck
Oct 28, 2026
Merged

jhrozek merged 1 commit into
stacklok:mainfrom
aron-muon:dcr-runtime-recheck

Conversation

@aron-muon

@aron-muon aron-muon commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The embedded auth server resolves an upstream's DCR registration once, when it starts. The registrationProbe check 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).

  • New optional dcrConfig.registrationProbeInterval on MCPExternalAuthConfig (and the inline VirtualMCPServer config), a metav1.Duration of at least 1m, 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.
  • When set, a background loop per DCR upstream re-runs dcr.ResolveCredentials on that interval. That call re-reads the stored row, checks it with the configured registrationProbe, replaces a rejected registration through ReplaceIfUnchanged, 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.
  • The first tick waits a random part of the interval and later ticks the interval ±10%, so replicas of one rollout do not probe, and on a rejection register, at the same moment.
  • A provider holds its client config and, for OIDC, the ID token verifier as one clientState behind an atomic.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 new upstream.ClientCredentialsUpdater capability performs the swap; the server exposes it as the optional authserver.UpstreamCredentialsUpdater, reached by type assertion like CloseIdleConnections, so the Server interface is unchanged.
  • A failed tick is logged once and leaves the provider on its current client; the next tick tries again. A switch is logged at Warn with both client_ids, because authorizations started under the previous client may fail their code exchange. On periodic ticks the resolver logs its 90-day staleness warning at Debug, so it still appears once at start. EmbeddedAuthServer.Close stops the loops before closing the server.

Related to #6821 and #6496. Builds on #6834.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (go test -race on pkg/authserver/..., pkg/auth/..., pkg/vmcp/... and cmd/thv-operator/... outside test-integration)
  • E2E tests (task test-e2e)
  • Linting (task lint)
  • Manual testing (describe below)

The mcp-external-auth envtest suite (Kubernetes 1.31.0) passes, including the new dcr_probe_interval_validation_test.go. task gen, task operator-generate operator-manifests crdref-gen and task docs leave no diff, and task docs-verify passes.

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 across UpdateClientCredentials; 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, 90s and 1h are admitted; 30s and 0 are rejected with the minimum message; malformed 5, 1d and soon are rejected; the VirtualMCPServer copy of the schema rejects 30s.
  • Validation cases for registration_probe_interval and the operator's run-config mapping (5m0s).

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go Add registrationProbeInterval (metav1.Duration, Go duration pattern, CEL minimum of 1m)
cmd/thv-operator/pkg/controllerutil/authserver.go Map it into the run-config
pkg/authserver/config.go Add RegistrationProbeInterval, ProbeInterval and its validation
pkg/authserver/upstream/oauth2.go Hold the client in an atomic.Pointer[clientState]; add ClientCredentials, ClientCredentialsUpdater and UpdateClientCredentials; exchange and refresh take the state they loaded
pkg/authserver/upstream/oidc.go Keep the verifier in the client state; validate ID tokens with the state that obtained them
pkg/authserver/server.go, pkg/authserver/server_impl.go Optional UpstreamCredentialsUpdater capability and UpdateUpstreamClientCredentials; document the loops in the CloseIdleConnections contract
pkg/authserver/runner/dcr_recheck.go The per-upstream re-check loop
pkg/authserver/runner/embeddedauthserver.go Collect re-checks while building upstreams, start them after the server, stop them in Close
pkg/auth/dcr/request.go, pkg/auth/dcr/resolver.go Request.Periodic; staleness warning at Debug on periodic lookups
docs/arch/11-auth-server-storage.md Document the re-check
CRDs, deepcopy, docs/operator/crd-api.md, swagger, Go SDK Regenerated

Does 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.registrationProbeInterval set, 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 one registrationProbe request to the upstream from every replica (none with registrationProbe: none), plus a registration request when the probe rejects the client; the tokenEndpoint probe 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

  • A switch has the same cost as the restart it replaces: an authorization started under the previous client and completed after the switch redeems its code with the new client, which an upstream that binds codes to the client rejects once, and upstream refresh tokens bound to the previous client stop refreshing. The Warn line on the switch names both client_ids.
  • The loop calls ResolveCredentials rather than a narrower re-check so the start-time and periodic paths share one implementation, including the singleflight and the compare-and-set replacement.
  • With registrationProbe: none the 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.
  • The operator injects the initial access token as an env var, fixed for the life of the pod, so an upstream that issues single-use initial access tokens still needs a restart to recover there; the CRD field documents this. File-based run configs re-read the token every tick.
  • CloseIdleConnections does 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.
  • Follow-ups, not in this PR: deleting a losing registration through RFC 7592 §2.3 when the upstream returned registration_client_uri and registration_access_token; requiring two rejections in a row before rotating while running; an RFC 7592 §2.1 read probe.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.46809% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.17%. Comparing base (9726b3f) to head (6898e0b).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
pkg/authserver/runner/embeddedauthserver.go 90.90% 1 Missing ⚠️
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.
📢 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.

@aron-muon
aron-muon force-pushed the dcr-runtime-recheck branch 2 times, most recently from b0d06df to 011615d Compare October 26, 2026 19:19

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread pkg/authserver/upstream/oidc.go Resolved
Comment thread pkg/authserver/runner/dcr_recheck.go Resolved
Comment thread pkg/authserver/runner/dcr_recheck.go Resolved
Comment thread pkg/authserver/runner/dcr_recheck.go Resolved
Comment thread pkg/authserver/runner/dcr_recheck.go Resolved
Comment thread pkg/authserver/upstream/oauth2.go Resolved
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>
@aron-muon
aron-muon force-pushed the dcr-runtime-recheck branch from 011615d to 6898e0b Compare October 27, 2026 14:18
@aron-muon
aron-muon requested a review from jhrozek October 28, 2026 10:59
@jhrozek
jhrozek merged commit 7ce28f2 into stacklok:main Oct 28, 2026
80 of 81 checks passed
@github-actions github-actions Bot mentioned this pull request Oct 28, 2026
2 tasks
github-actions Bot added a commit to stacklok/docs-website that referenced this pull request Oct 28, 2026
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>
jerm-dro pushed a commit to stacklok/docs-website that referenced this pull request Oct 30, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants