Skip to content

[fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor - #26327

Open
lhotari wants to merge 1 commit into
lh-pip-478-followupsfrom
lh-pip-478-admin-auth-executor
Open

[fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor#26327
lhotari wants to merge 1 commit into
lh-pip-478-followupsfrom
lh-pip-478-admin-auth-executor

Conversation

@lhotari

@lhotari lhotari commented Aug 13, 2026

Copy link
Copy Markdown
Member

Main Issue: #25890

PIP: #25890

Stacked on #26326 — this PR's base is lh-pip-478-followups, which sits on #26322#26319#26317 and thence on master. Review those first; the diff here shows only this part. A stacked PR runs only the semantic-title check until its base merges.

Motivation

PIP-478 states that every synchronous v4 authentication plugin call is off-loaded, and after the core
migration (#26282) that was true everywhere except one place: BaseResource resolved the credential
for each admin request on whatever thread issued the call.

That is the same hazard the lookup path already fixed. HttpClient.computeAuthHeaders off-loads the
identical composition and names the self-deadlock it avoids — a v4 OAuth2 or Athenz shim's
getAuthData() refreshes its credential over a synchronous HTTP exchange, and for a broker calling
its own admin client the caller is a request-handling thread. BaseResource was the last
caller-thread credential resolution left in the codebase, and the one place PIP-478's claim was still
an overstatement.

Separately, PulsarAdminImpl bound its framework authentication services with a null blocking
executor
. That stopped being a live defect once the init context grew a shared-pool fallback, but it
left the SASL-over-HTTP challenge rounds putting their GSSAPI work on one process-wide pool shared
with every other client in the JVM — so a stalled KDC reached by one admin throttled authentication
for all of them.

Finally, AuthenticationInitContext.scheduler() and blockingExecutor() are documented as never
null, and the no-services context honours that. The bound context did not: it returned whatever
the component supplied, so binding partial services was worse for a third-party plugin than binding
none at all.

Modifications

BaseResource off-loads the v4 composition. The deprecated authenticationStage(...) /
newRequestHeader(...) hooks now run on a blocking executor via V5AuthContexts.supplyBlocking, and
produce the plugin's headers verbatim — off-loading changes when the work runs, not what it returns.

The v4 composition goes to the framework's shared pool rather than the admin's own: BaseResource
is constructed by 30 subclasses that would each have to thread an executor through, and the shared pool
is exactly what PIP-478 provides for a component with none to lend. The trade-off is stated at the call
site rather than left silent.

The admin lends its own bounded pool to services-aware plugins. A small ThreadPoolExecutor
(ceiling 8 — an admin issues REST calls, not a fan-out of connection attempts), created lazily, core
threads timing out, daemon threads, shutdown() after auth.close() so a plugin shutting down over it
still has it. This keeps a stalled identity provider's blast radius inside the admin that owns the
plugin. The admin's request threads are still never lent out — that was the hazard the original
null was avoiding, and it remains avoided.

The scheduler stays unbound: nothing on the admin path schedules periodic authentication work, and
binding a scheduled pool per admin to sit idle would cost more than it buys.

The bound init context falls back. BoundInitContext.scheduler() / blockingExecutor() now
substitute the shared instances when the bound services leave one null, so the SPI's "never null"
contract holds on every path.

Verifying this change

This change added tests and can be verified as follows:

  • AdminAuthOffloadTest — pins that the v4 credential is resolved off the thread that issued the
    admin call, and that the composed headers are unchanged. Mutation-verified: reverting the
    off-load to a direct call fails exactly this assertion and nothing else.
  • BoundInitContextFallbackTest — pins that a context bound with a null scheduler still supplies
    one, and that the fallback does not shadow executors a component did bind.

./gradlew sanityCheck and quickCheck pass.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Threading model: admin requests no longer resolve the v4 credential on the calling thread — the
composition moves to a blocking executor, and the admin gains a small bounded pool of its own for
services-aware plugins. No behaviour change in what is sent; only where it is computed.

…a bounded auth executor

PIP-478 states that every synchronous v4 plugin call is off-loaded, and after the
core migration that was true everywhere except one place: BaseResource resolved
the credential for each admin request on whatever thread issued the call.
HttpClient.computeAuthHeaders off-loads the identical composition for the lookup
path and names the self-deadlock it avoids — an OAuth2 or Athenz shim's
getAuthData() refreshes its credential over synchronous HTTP, and for a broker
calling its own admin client the caller is a request-handling thread. The
composition now runs on a blocking executor, and produces the plugin's headers
verbatim.

The v4 composition is off-loaded to the framework's shared pool rather than the
admin's own, because BaseResource is constructed by 30 subclasses that would each
have to thread an executor through; the shared pool is exactly what PIP-478
provides for a component with none to lend. What a services-aware plugin gets is
different, and is the second half of this change.

PulsarAdminImpl bound its framework services with a null blocking executor. That
stopped being a live defect when the init context grew a shared-pool fallback, but
it left the SASL-over-HTTP challenge rounds putting their GSSAPI work on one
process-wide pool shared with every other client in the JVM, so a stalled KDC
reached by one admin throttled authentication for all of them. The admin now lends
a small bounded pool of its own, created lazily and shut down with the admin, which
keeps that blast radius inside the admin that owns the plugin. Its request threads
are still never lent out — that was the hazard the original null was avoiding.

The scheduler stays unbound: nothing on the admin path schedules periodic
authentication work. Instead the bound init context now falls back to the shared
scheduler and blocking pool when the bound services leave one null, so the SPI's
"never null" contract holds on every path — previously, binding partial services
was worse for a third-party plugin than binding none at all, since the no-services
context has always guaranteed non-null.

Copilot AI 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.

Pull request overview

Off-loads synchronous admin authentication work and improves executor isolation and fallback behavior.

Changes:

  • Off-loads legacy v4 admin credential resolution.
  • Adds an admin-owned authentication executor.
  • Adds non-null executor fallbacks and regression tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
BoundInitContextFallbackTest.java Tests bound-context executor fallbacks.
V5AuthContexts.java Adds shared scheduler and executor fallbacks.
AdminAuthOffloadTest.java Tests admin credential off-loading.
PulsarAdminImpl.java Adds the admin authentication executor lifecycle.
BaseResource.java Off-loads legacy authentication composition.
Suppressed comments (1)

pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java:167

  • This helper does not keep the whole composition on a blocking executor when authenticationStage completes asynchronously: the thenApply continuation below invokes synchronous newRequestHeader on the thread that completes stage, potentially a Jersey/Netty I/O thread. Offload that continuation too so every synchronous v4 hook is covered.
    private CompletableFuture<Map<String, String>> v4AuthHeaders(URI uri) {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// with none to lend. The admin's own bounded pool is still what a services-aware plugin gets, so
// the work that can actually stall — a KDC or IdP round trip inside the plugin — stays isolated
// per admin.
return V5AuthContexts.supplyBlocking(null, () -> v4AuthHeaders(uri)).thenCompose(headers -> headers);
Comment on lines +624 to +625
ThreadPoolExecutor executor = new ThreadPoolExecutor(AUTH_BLOCKING_MAX_THREADS,
AUTH_BLOCKING_MAX_THREADS, 60L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
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