Skip to content

xds: Add ExtAuthzClientCall - #12897

Open
sauravzg wants to merge 2 commits into
dev/sauravzg/response-handlingfrom
dev/sauravzg/client-interceptor
Open

xds: Add ExtAuthzClientCall#12897
sauravzg wants to merge 2 commits into
dev/sauravzg/response-handlingfrom
dev/sauravzg/client-interceptor

Conversation

@sauravzg

@sauravzg sauravzg commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Part 3 of the client-side ext_authz filter. Sits on top of the response handling PR.

Introduces the ClientInterceptor that performs async ext_authz checks on outgoing RPCs. Because the authorization call is asynchronous, outgoing sendMessage() and halfClose() frames are buffered in ExtAuthzClientCall until a decision arrives. On allow, buffered operations are replayed with any header mutations applied. On deny, the call is terminated immediately.

Key classes:

  • AuthzCallbackObserver — async listener on the authz gRPC stream that signals the buffering call on completion.
  • ExtAuthzClientCall — buffers outgoing frames while authz is pending; drains or cancels based on the decision.
  • MutatingClientCall — wraps the real call to inject header mutations from the authz response.
  • FailingClientCall / FailingCallWithTrailerMutations — immediate termination paths for denied or disabled-filter scenarios.

@sauravzg
sauravzg requested a review from kannanjgithub July 7, 2026 13:03
Comment thread xds/src/main/java/io/grpc/xds/internal/extauthz/AuthzCallbackObserver.java Outdated

@Override
public void onNext(CheckResponse value) {
// Note on exception safety: handleResponse() internally catches

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.

Uncaught exception from onNext causes the stub to cancel the stream and route directly into onError(), which closes authzContext. Its closure does not rely on GC of the stream observer, and it cannot, because it has a direct reference to it from ExtAuthzClientCall.

In a unary rpc, onNext() delivers an inbound response payload; it does not signify that the RPC is complete. A unary RPC completes only when onClose / onCompleted() / onError() is invoked.

The comment needs to be corrected on both the above fronts.

Is relying on this mechanism "Valid Code"?

Mechanically, yes, gRPC will catch the exception and route to onError(). If an unhandled RuntimeException escapes onNext(), ClientCallImpl cancels the stream and delivers onError(), which drains delayedCall and closes authzContext.

However, intentionally relying on this as a design pattern is fragile and carries real risks:

A. Security Risk: Accidentally Failing Open on a DENY

This is the biggest risk:

// in onNext():
AuthzResponse authzResponse = responseHandler.handleResponse(value);
if (authzResponse.decision() == AuthzResponse.Decision.ALLOW) { ... }
else {
  // Suppose decision is DENY, but building FailingCallWithTrailerMutations throws an exception
  ...
}

If the external authorization server returned an explicit DENY response, but an unexpected RuntimeException is thrown while preparing the rejection:

  1. The exception escapes onNext().
  2. The stub cancels the stream and invokes onError(t).
  3. In onError():
    if (config.failureModeAllow()) {
      // FAILS OPEN: creates next.newCall() and forwards request to backend!
    }

If failure_mode_allow: true is configured (which is only intended for network/server outages reaching the authz service), a request that the authz server explicitly denied would be forwarded to the backend!

B. The "Throwing to Trigger Another Callback on Yourself" Anti-Pattern

Relying on throwing an unhandled exception out of onNext() so that the underlying framework cancels the transport stream and calls back into your own onError() is an indirect, surprising control flow:

  • It generates unnecessary transport RST_STREAM frames and Failed to read message error logs from ClientCallImpl exception handling.
  • It makes debugging difficult because the root cause in onNext gets swallowed into a transport Status.CANCELLED exception passed to onError.

C. Partial Failure State

If an exception occurs after delayedCall.setCall(call) was executed (for example, if callExecutor.execute(drain) throws a RejectedExecutionException):

  • When onError() subsequently runs, its call to delayedCall.setCall(...) returns null because realCall != null was already set. The failure call cannot be attached, leaving the call in an inconsistent state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was interesting.
So, practically it largely doesn't matter right now, because the current code is exception safe. So, a runtime exception shouldn't happen.

But given the security considerations, maybe we should be defensive, I'll have to see if other implementations in flight are as defensive and make changes. Keeping his comment open for now.

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.

I agree the code is exception safe. We can just remove the comment.


private void setCallAndDrain(ClientCall<ReqT, RespT> call) {
Runnable drain = delayedCall.setCall(call);
if (drain != null) {

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.

Do we really need this check (and the corresponding unit test allow_whenDelayedCallNotStarted_setCallReturnsNull) considering that the only code using this class is ExtAuthzClientCall that always starts the DelayedClientCall before invoking the rpc on the ext_auth stub?

@sauravzg sauravzg Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's fair enough to be defensive and handle it nonetheless. I don't want to rely on the imlementation details of when the runnable is null and when not.

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.

It is in fact part of the implementation detail, since it is package private and not really a utility class that has other use cases. It falls under the YAGNI principle.

Comment thread xds/src/test/java/io/grpc/xds/internal/extauthz/ExtAuthzClientCallTest.java Outdated
Part 3 of the client-side ext_authz filter. Sits on top of the
response handling PR.

Introduces the ClientInterceptor that performs async ext_authz
checks on outgoing RPCs. Because the authorization call is
asynchronous, outgoing sendMessage() and halfClose() frames are
buffered in ExtAuthzClientCall until a decision arrives. On allow,
buffered operations are replayed with any header mutations applied.
On deny, the call is terminated immediately.

Key classes:
- AuthzCallbackObserver — async listener on the authz gRPC stream
  that signals the buffering call on completion.
- ExtAuthzClientCall — buffers outgoing frames while authz is
  pending; drains or cancels based on the decision.
- MutatingClientCall — wraps the real call to inject header
  mutations from the authz response.
- FailingClientCall / FailingCallWithTrailerMutations — immediate
  termination paths for denied or disabled-filter scenarios.
@sauravzg sauravzg changed the title xds: Add ExtAuthzClientInterceptor and call buffering xds: Add ExtAuthzClientCall Sep 4, 2026
@sauravzg
sauravzg force-pushed the dev/sauravzg/client-interceptor branch from 08fa030 to ac83094 Compare September 7, 2026 06:00
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