Handle slow security policies without blocking gRPC threads. #10633

mateusazis · 2023-10-26T18:29:01Z

Introduce PendingAuthListener to handle a ListenableFuture, progressing the gRPC through each stage in sequence once the future completes and is OK.
Move unit tests away from checkAuthorizationForService and into checkAuthorizationForServiceAsync since that should be the only method called in production now.
Some tests in ServerSecurityPolicyTest had their expectations updated; they previously called synchornous APIs that transformed failed ListenableFuture<Status> into one or another status. Now, we call the sync API, so those transformations do not happen anymore, thus the test needs to deal with failed futures directly.
I couldn't figure out if this PR needs extra tests. AFAICT BinderSecurityTest should already cover the new codepaths, but please let me know otherwise.

This should be the last PR to address #10566.

- Introduce PendingAuthListener to handle a ListenableFuture<Status>, progressing the gRPC through each stage in sequence once the future completes and is OK. - Move unit tests away from `checkAuthorizationForService` and into `checkAuthorizationForServiceAsync` since that should be the only method called in production now. - Some tests in `ServerSecurityPolicyTest` had their expectations updated; they previously called synchornous APIs that transformed failed `ListenableFuture<Status>` into one or another status. Now, we call the sync API, so those transformations do not happen anymore, thus the test needs to deal with failed futures directly. - I couldn't figure out if this PR needs extra tests. AFAICT `BinderSecurityTest` should already cover the new codepaths, but please let me know otherwise. This should be the last PR to address grpc#10566.

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java

- Drop sequential executor from PendingAuthListener.

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java

binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java

- Factory method for PendingAuthListener. - Fix leak of executor when PendingAuthListener encountered an exception while starting the call. - Catching RuntimeException.

- Comment + TODO for multiple concurrent auth checks.

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java

jdcormie

LGTM

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

markb74

Getting really close.

I'm happy with this overall, but I'd like some changes to make this easier to review more completely.

I.e. Undo changes to the struture of interceptCall per my comment there, and undo all unrelated reformatting.

binder/src/main/java/io/grpc/binder/BinderServerBuilder.java

binder/src/main/java/io/grpc/binder/internal/BinderServer.java

binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java

binder/src/main/java/io/grpc/binder/internal/BinderServer.java

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

- define BinderTransportSecurity.ShutdownListener interface - take a non-close executor closeable in BinderServer's constructor

markb74

Thanks so much for your patience!

Just a couple of minor nits now, then we can go ahead and submit.

There's a couple more things we'll want to do in a follow up to handle nuance around policy decision caching, and server vs. transport shutdown, but this is in a good state.

binder/src/main/java/io/grpc/binder/internal/BinderServer.java

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

markb74 · 2023-11-30T11:25:15Z

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java

+   * resources to be potentially cleaned up.
+   */
+  public interface ShutdownListener {
+    void onShutdown();


Server shutdown is actually kind of complex, so if we're going to use the prefix "on" (which I agree makes sense in a listener), we probably want to clarify that this is the server shutdown, and rename this to onServerShutdown().

FWIW, there's actually a bit more nuance here we'll need to handle, but let's leave that for a follow up CL.

Renamed.

By "nuance", do you mean code style-wise or are we actually missing something from the implementation? Because if it's the later, I'm not aware of what that is.

jdcormie reviewed Oct 30, 2023

View reviewed changes

binder/src/main/java/io/grpc/binder/internal/PendingAuthListener.java Outdated Show resolved Hide resolved

markb74 requested changes Oct 31, 2023

View reviewed changes

mateusazis added 2 commits October 31, 2023 10:56

- Short-circuit immediately resolved auth futures.

54cb525

- Drop sequential executor from PendingAuthListener.

fix indentation

26b8cc2

mateusazis requested review from markb74 and jdcormie November 6, 2023 02:27

jdcormie reviewed Nov 7, 2023

View reviewed changes

Address jdcormie's comments

09a0190

mateusazis requested a review from jdcormie November 7, 2023 18:46

jdcormie requested changes Nov 11, 2023

View reviewed changes

- Fix indentation

438734a

- Factory method for PendingAuthListener. - Fix leak of executor when PendingAuthListener encountered an exception while starting the call. - Catching RuntimeException.

mateusazis requested a review from jdcormie November 14, 2023 22:08

Exception -> RuntimeException

28eccc5

mateusazis mentioned this pull request Nov 15, 2023

Ensure there is at most a single pending authorization check per (uid, serviceName) pair at any given time. #10669

Open

- Refactor test helpers.

8cf4885

- Comment + TODO for multiple concurrent auth checks.

mateusazis mentioned this pull request Nov 15, 2023

Race condition in BinderTransportSecurity #10670

Closed

markb74 reviewed Nov 15, 2023

View reviewed changes

jdcormie reviewed Nov 15, 2023

View reviewed changes

markb74 reviewed Nov 16, 2023

View reviewed changes

binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java Outdated Show resolved Hide resolved

Move ListenableFuture handling logic to BinderTransportSecurity

889c524

mateusazis requested a review from markb74 November 16, 2023 18:52

Move the lifecycle management of the executor to the BinderServer.

6a7fd67

markb74 requested changes Nov 29, 2023

View reviewed changes

- revert formatting changes in ServerSecurityPolicyTest

5559320

- define BinderTransportSecurity.ShutdownListener interface - take a non-close executor closeable in BinderServer's constructor

mateusazis requested a review from markb74 November 29, 2023 23:41

markb74 approved these changes Nov 30, 2023

View reviewed changes

extra lint and style fixes

6f6cde9

mateusazis requested review from markb74 and jdcormie November 30, 2023 17:40

markb74 approved these changes Dec 4, 2023

View reviewed changes

markb74 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 4, 2023

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 4, 2023

jdcormie approved these changes Dec 4, 2023

View reviewed changes

markb74 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 5, 2023

grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 5, 2023

markb74 merged commit a053889 into grpc:master Dec 6, 2023
6 checks passed

mateusazis mentioned this pull request Feb 6, 2024

binder: ServerAuthInterceptor might use an Executor that BinderServer has already returned to the pool #10897

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle slow security policies without blocking gRPC threads. #10633

Handle slow security policies without blocking gRPC threads. #10633

mateusazis commented Oct 26, 2023

jdcormie left a comment

markb74 left a comment

markb74 left a comment

markb74 Nov 30, 2023

mateusazis Nov 30, 2023

Handle slow security policies without blocking gRPC threads. #10633

Handle slow security policies without blocking gRPC threads. #10633

Conversation

mateusazis commented Oct 26, 2023

jdcormie left a comment

Choose a reason for hiding this comment

markb74 left a comment

Choose a reason for hiding this comment

markb74 left a comment

Choose a reason for hiding this comment

markb74 Nov 30, 2023

Choose a reason for hiding this comment

mateusazis Nov 30, 2023

Choose a reason for hiding this comment