Skip to content

Commit 03e0356

Browse files
committed
Fixup: Add in-flight cancellation tests and ensure super.cancel runs before authzContext cancel
1 parent cb724f9 commit 03e0356

3 files changed

Lines changed: 109 additions & 1 deletion

File tree

‎xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzClientCall.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
9898
@Override
9999
public void cancel(
100100
@Nullable String message, @Nullable Throwable cause) {
101-
authzContext.cancel(cause);
102101
super.cancel(message, cause);
102+
authzContext.cancel(cause);
103103
}
104104

105105
/**

‎xds/src/test/java/io/grpc/xds/internal/extauthz/AuthzCallbackObserverTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,73 @@ public HeaderMutations responseHeaderMutations() {
625625
observer.onNext(CheckResponse.getDefaultInstance());
626626
}
627627

628+
@Test
629+
public void allow_whenDelayedCallCancelledInFlight_setCallReturnsNull() {
630+
doAnswer(invocation -> {
631+
StreamObserver<CheckResponse> obs = invocation.getArgument(1);
632+
obs.onNext(CheckResponse.newBuilder()
633+
.setStatus(com.google.rpc.Status.newBuilder().setCode(0).build())
634+
.setOkResponse(OkHttpResponse.getDefaultInstance())
635+
.build());
636+
obs.onCompleted();
637+
return null;
638+
}).when(authzService).check(any(), any());
639+
640+
TestDelayedCall<SimpleRequest, SimpleResponse> delayedCall =
641+
new TestDelayedCall<>(MoreExecutors.directExecutor(), scheduler, null);
642+
CapturingListener<SimpleResponse> listener = new CapturingListener<>();
643+
delayedCall.start(listener, new Metadata());
644+
delayedCall.cancel("cancelled in flight", null);
645+
646+
Context.CancellableContext authzCtx = Context.current().withCancellation();
647+
AuthzCallbackObserver<SimpleRequest, SimpleResponse> observer =
648+
new AuthzCallbackObserver<>(
649+
delayedCall, channel,
650+
SimpleServiceGrpc.getUnaryRpcMethod(),
651+
CallOptions.DEFAULT,
652+
MoreExecutors.directExecutor(),
653+
responseHandler, failClosedConfig(), authzCtx);
654+
655+
authzCtx.run(() -> {
656+
AuthorizationGrpc.newStub(channel)
657+
.check(CheckRequest.getDefaultInstance(), observer);
658+
});
659+
660+
assertThat(capturedBackendHeaders).isNull();
661+
assertThat(listener.getCloseStatus().getCode()).isEqualTo(Status.Code.CANCELLED);
662+
}
663+
664+
@Test
665+
public void allow_whenDelayedCallNotStarted_setCallReturnsNull() {
666+
doAnswer(invocation -> {
667+
StreamObserver<CheckResponse> obs = invocation.getArgument(1);
668+
obs.onNext(CheckResponse.newBuilder()
669+
.setStatus(com.google.rpc.Status.newBuilder().setCode(0).build())
670+
.setOkResponse(OkHttpResponse.getDefaultInstance())
671+
.build());
672+
obs.onCompleted();
673+
return null;
674+
}).when(authzService).check(any(), any());
675+
676+
TestDelayedCall<SimpleRequest, SimpleResponse> delayedCall =
677+
new TestDelayedCall<>(MoreExecutors.directExecutor(), scheduler, null);
678+
Context.CancellableContext authzCtx = Context.current().withCancellation();
679+
AuthzCallbackObserver<SimpleRequest, SimpleResponse> observer =
680+
new AuthzCallbackObserver<>(
681+
delayedCall, channel,
682+
SimpleServiceGrpc.getUnaryRpcMethod(),
683+
CallOptions.DEFAULT,
684+
MoreExecutors.directExecutor(),
685+
responseHandler, failClosedConfig(), authzCtx);
686+
687+
authzCtx.run(() -> {
688+
AuthorizationGrpc.newStub(channel)
689+
.check(CheckRequest.getDefaultInstance(), observer);
690+
});
691+
692+
assertThat(capturedBackendHeaders).isNull();
693+
}
694+
628695

629696
private static final class TestDelayedCall<ReqT, RespT>
630697
extends DelayedClientCall<ReqT, RespT> {

‎xds/src/test/java/io/grpc/xds/internal/extauthz/ExtAuthzClientCallTest.java‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,47 @@ public void cancel_whileCheckInFlight_cancelsContext() {
194194
assertThat(authzContext.isCancelled()).isTrue();
195195
}
196196

197+
@Test
198+
public void cancel_whileCheckInFlight_responseAfterCancelDoesNotForwardToBackend()
199+
throws Exception {
200+
AtomicReference<StreamObserver<CheckResponse>> capturedObserver = new AtomicReference<>();
201+
CountDownLatch checkCalled = new CountDownLatch(1);
202+
doAnswer(invocation -> {
203+
capturedObserver.set(invocation.getArgument(1));
204+
checkCalled.countDown();
205+
return null;
206+
}).when(authzService).check(any(CheckRequest.class), any());
207+
208+
HeaderMutations emptyMutations = HeaderMutations.create(
209+
ImmutableList.of(), ImmutableList.of());
210+
AuthzResponse authzResponse =
211+
AuthzResponse.allow(emptyMutations)
212+
.setResponseHeaderMutations(emptyMutations)
213+
.build();
214+
when(mockResponseHandler.handleResponse(
215+
any(CheckResponse.class))).thenReturn(authzResponse);
216+
217+
ExtAuthzClientCall<SimpleRequest, SimpleResponse> call = createCall(
218+
com.google.common.util.concurrent.MoreExecutors.directExecutor(), channel, config);
219+
CapturingListener<SimpleResponse> listener = new CapturingListener<>();
220+
call.start(listener, new Metadata());
221+
222+
assertThat(checkCalled.await(5, TimeUnit.SECONDS)).isTrue();
223+
224+
// Cancel while check is pending
225+
call.cancel("client cancelled", null);
226+
assertThat(listener.getCloseStatus().getCode()).isEqualTo(Status.Code.CANCELLED);
227+
228+
lastBackendHeaders = null;
229+
230+
// Authz responds after cancellation
231+
capturedObserver.get().onNext(CheckResponse.getDefaultInstance());
232+
capturedObserver.get().onCompleted();
233+
234+
// Verify backend received nothing
235+
assertThat(lastBackendHeaders).isNull();
236+
}
237+
197238
@Test
198239
public void start_dispatchesAuthzCheckUnderChildContext() throws Exception {
199240
Context.Key<String> testKey = Context.key("test-key");

0 commit comments

Comments
 (0)