[fix][proxy] PIP-478: resolve the proxy's broker-client credential off the event loop

[lhotari]

Main Issue: #25890

PIP: #25890

Stacked on #26327 — this PR's base is lh-pip-478-admin-auth-executor, which sits on #26326 → #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

DirectProxyHandler.channelActive called the v4 authentication plugin inline — getAuthData() then
authenticate(INIT_AUTH_DATA) — on the thread delivering the channel-active event, and
handleAuthChallenge did the same for every challenge round.

That thread is a Netty I/O loop, and the code being called is arbitrary plugin code: an OAuth2 token
endpoint round trip, an Athenz ZTS fetch, a GSSAPI exchange with the KDC. While it ran, every
connection multiplexed onto that loop stalled — the exact hazard PIP-478 exists to remove, and the one
it already removed from the client.

pip-478.md:145 put the proxy's broker-client credential I/O out of scope, on the reasoning that the
client still had a synchronous path anyway. After the v5-native inversion (#26317) the client has no
synchronous path at all, so there is no longer a justification for keeping one here — this is the last
place in the codebase where plugin credential code runs on an event loop.

Modifications

ProxyService owns one V5BinaryAuthenticationDriver, built lazily from its already-started v4
plugin (the bridge must not run that lifecycle a second time, since ProxyService starts and closes
that instance itself). Each DirectProxyHandler opens its own exchange against it — the exchange is
what carries per-connection conversation state, so a multi-round mechanism still works across rounds.

No ClientAuthenticationServices are bound: the proxy is not a PulsarClient and has no client-owned
executor to lend. Credential work therefore lands on the framework's shared blocking pool, which is
the case V5AuthContexts documents for exactly this caller — the alternative, running it inline, is
the event loop this change exists to free.

Three properties are preserved deliberately:

• Command ordering is unchanged. The continuation is dispatched back onto the channel's own
  event loop, so the connect command and every auth response are still built and written there,
  whether the credential was already in memory or needed I/O.
• A resolution failure now closes the backend channel rather than only logging. The proxy has no
  credential to send; leaving the connection open would wait out the broker's timeout instead of
  letting the client retry. This mirrors what ClientCnx.completeAuthChallenge was fixed to do.
• The broker-pushed REFRESH sentinel starts a fresh exchange, per PIP-478 binary routing rule 2,
  mirroring ClientCnx — rather than being routed into the conversation it just terminated.

The now-unused AuthenticationDataProvider field is removed.

Verifying this change

This change is already covered by existing tests: the proxy authentication suites exercise both the
connect path and the broker-pushed REFRESH path end to end through the changed code —
ProxyRefreshAuthTest, ProxyForwardAuthDataTest, ProxyAuthenticatedProducerConsumerTest and
ProxyWithAuthorizationTest (19 tests, all passing locally).

Not yet covered: a dedicated assertion that no credential call lands on a proxy I/O thread. I
attempted one and could not get its fixture to authenticate in setup, so I removed it rather than
leave a half-working test in the PR. It is worth adding — reviewers should treat the event-loop
property as argued from the code rather than pinned by a test.

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: the proxy's broker-client credential resolution moves off the Netty event loop
onto the framework's shared blocking pool. Command construction and writes stay on the event loop, so
ordering on the channel is unchanged.

[david-streamlio]

The motivation is right and the shape of the fix is the one I'd want: ProxyService owns one driver, each DirectProxyHandler opens its own exchange, and the continuation is dispatched back onto the channel's event loop so command construction and writes stay where they were. I traced the load-bearing invariants rather than taking them from the description, and they hold:

• Same credential is still sent. ProxyConnection.getClientAuthentication() returns service.getProxyClientAuthenticationPlugin(), so the authentication field this class used before and the plugin the new driver is built from are the same instance. Routing through ProxyService doesn't change which credential goes to the broker. This is the non-obvious premise of the whole refactor and is worth a word in the PR body.
• Per-host scoping and the initial call are preserved verbatim. newAuthenticationExchange(remoteHostName) → V5AuthContexts.binaryCallContext(brokerHost) → LegacyV4CredentialAdapter.getAuthDataAsync does v4.getAuthData(callContext.brokerHost()) then d.authenticate(AuthData.INIT_AUTH_DATA) — exactly what channelActive did inline.
• Challenge rounds keep the same provider. The adapter stashes the AuthenticationDataProvider in the call-context state slot, which reproduces the old retained authenticationDataProvider field for multi-round mechanisms. Rounds stay serialized here because the next challenge only arrives after the previous response is written.
• The lifecycle claim checks out. forStartedV4Plugin → wrapAlreadyStarted → ownsLifecycle=false, and the adapter gates both v4.start() in initializeAsync and v4.close() in close() on that flag.
• Ordering is genuinely unchanged. Inbound client bytes reach the backend only through ProxyConnection.channelRead, gated on this.directProxyHandler != null, which is assigned only in handleBrokerConnected. So nothing can be written to the backend channel between channelActive and the deferred connect write. The HAProxy header still goes out synchronously first, and moving isTlsOutboundChannel above the send is correct.

Findings, in the order I'd act on them:

1. The REFRESH path isn't covered by the cited tests — and isn't reachable in this class.

ProxyBackendHandler.channelRead stops decoding once state == HandshakeCompleted (set in handleConnected) and forwards raw bytes to the client. On the broker side, ServerCnx.maybeScheduleAuthenticationCredentialsRefresh only schedules the refresh task at authenticationRefreshCheckSeconds (default 60s) after connect completes. So the broker's REFRESH sentinel can never reach DirectProxyHandler.handleAuthChallenge — it arrives long after the handler stopped decoding, and gets proxied straight through to the client.

ProxyRefreshAuthTest asserts on ClientCnx.getLastDisconnectedTimestamp() over pulsarClientImpl.getCnxPool().getConnections() — it exercises the client's refresh through the proxy, not this code path.

So "the proxy authentication suites exercise both the connect path and the broker-pushed REFRESH path end to end through the changed code" holds for connect but not for REFRESH. Since the REFRESH branch is where the semantics actually changed (old: new provider + authenticate(REFRESH_AUTH_DATA); new: fresh exchange + getAuthDataAsync() → authenticate(INIT_AUTH_DATA), the delta pip-478.md:972 documents), I'd rather the PR body said plainly that the branch is defensive parity with ClientCnx/ProxyClientCnx and is not reachable here, than claimed test coverage for it. Not a code change — a claim change.

2. getProxyClientAuthenticationDriver() puts a shared monitor on the per-connection hot path. (ProxyService.java:702)

Every backend channelActive, on every proxy event loop, takes the ProxyService monitor — which is also held by createMetricsServlet() (:400), resetMetricsServlet() (:601), getMetricsServlet() (:710) and addPrometheusRawMetricsProvider() (:714). Those are all cheap, so this isn't a correctness bug, but the sibling code in this same stack — ClientCnx.resolveAuthDriver — deliberately does a lock-free read first and only synchronizes on the miss, precisely to keep the connect path off a shared monitor. Matching that here (make proxyClientAuthenticationDriver volatile, then double-check) seems worth it in a PR about not making connections wait on shared machinery. Note volatile isn't optional if you do this — the field is currently plain, so an unsynchronized read would be a data race.

3. The command-build failure path also closes now, which the PR body doesn't mention.

checkState(!authData.isComplete()) (:418) used to throw into catch (Exception e) { log.error("Error mutual verify") } and leave the connection open. It now lands in sendWhenResolved's builder catch and closes the channel. I think that's the right call and consistent with the stated policy, but the body attributes the new close behaviour only to resolution failures. Worth one more sentence so it isn't discovered later as a surprise.

4. Auth method name is read from the v4 plugin, not the exchange. (:292, :422)

ClientCnx takes the name from the exchange that produced the credential; this class still reads authentication.getAuthMethodName(). For the proxy the two agree (same plugin instance, and the bridge's authMethodName() delegates to v4.getAuthMethodName()), so this is correct as written — and arguably better here, since it doesn't depend on a completed round. But it reads as an oversight next to ClientCnx. A short comment saying why would stop someone "fixing" it later.

5. Two silent paths in sendWhenResolved. (:314, :322)

• The future returned by whenCompleteAsync is discarded. If ctx.executor() rejects during event-loop shutdown, that rejection completes the discarded future and vanishes — no log, no close. Harmless in practice since the channel is dying anyway, but a // intentionally discarded would say so on purpose.
• The !ctx.channel().isActive() early return leaves no trace. A log.debug() there would make "backend connected but never sent CommandConnect" diagnosable; right now that outcome is invisible.

On the test you removed. Rather than a fixture that has to authenticate end to end, the property is pinnable much more cheaply: configure the proxy with a small v4 plugin whose getAuthData records Thread.currentThread().getName(), then assert the recorded name is pulsar-auth-blocking-shared (or at least isn't the Netty loop). That sidesteps the TLS/fixture setup entirely and pins exactly the invariant this PR exists to establish. Happy to write it if useful.

Nothing here is a blocker on the code as written — 1 is a claim in the description, 2–5 are polish. The functional core looks right to me.

[Copilot]

Pull request overview

Moves proxy-to-broker credential resolution off Netty event loops using the v5 authentication driver.

Changes:

• Adds a shared, lazily initialized authentication driver.
• Resolves connect and challenge credentials asynchronously.
• Closes backend channels when credential resolution fails.

Reviewed changes

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

File	Description
ProxyService.java	Owns the shared v5 authentication driver.
DirectProxyHandler.java	Asynchronously handles broker authentication exchanges.

---

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

[david-streamlio]

Following up on the Copilot review that landed alongside mine, because one of its comments is correct in a way my review could be misread to dismiss.

Its comment on DirectProxyHandler.java:408 asks for round-state guarding equivalent to ClientCnx. I checked, and the parity gap is real:

Guard	ClientCnx	DirectProxyHandler
authRoundInProgress serialize-or-drop	yes	no
generation guard suppressing superseded continuations	yes	no
MAX_AUTH_CHALLENGE_ROUNDS cap	yes	no

ClientCnx.handleAuthChallenge states the reason itself: servicing a challenge that arrives mid-round "would re-enter the same single-round, non-thread-safe exchange concurrently". AuthenticationExchange's own javadoc makes serialization a caller obligation — "its rounds are serialized by the caller ({@code ClientCnx} issues the next round only after the previous future completes), so an implementation needs no internal synchronization". DirectProxyHandler is now a second caller of that contract and does not honour it.

Scoping this against what I wrote, since the two reviews interact:

My review argued the broker-pushed REFRESH sentinel cannot reach handleAuthChallenge here — channelRead stops decoding once state == HandshakeCompleted, and ServerCnx.maybeScheduleAuthenticationCredentialsRefresh only arms the refresh task authenticationRefreshCheckSeconds (default 60s) after connect. I still believe that, and it does rule out the specific REFRESH-supersedes-an-in-flight-round scenario in Copilot's comment.

It does not rule out the rest, and I should have flagged that and didn't. During Init the handler decodes every frame, so two challenge frames arriving in one TCP read produce two handleAuthChallenge invocations in the same event-loop turn — before either resolution future has completed — and both then drive the same non-thread-safe exchange. Nothing in this class prevents it. Please don't take my unreachability argument as an answer to the whole comment; it answers one third of it.

The missing round cap is the part I'd act on first. It is not a concurrency question at all: a broker that keeps answering each CommandAuthResponse with another challenge will loop indefinitely against the proxy, where ClientCnx fails the connection once MAX_AUTH_CHALLENGE_ROUNDS is exceeded. Each round now also schedules work onto the shared blocking pool. The proxy is the more exposed of the two components, so it is the one that least wants the unbounded version of this loop.

Copilot's other comment — that the event-loop property isn't pinned by a test — restates what the PR body already discloses, so nothing new there; my suggestion about a recording plugin still stands as the cheap way to get it.