Handle slow security policies without blocking gRPC threads. - #10633
Merged
Conversation
- 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.
jdcormie
reviewed
Oct 30, 2023
markb74
suggested changes
Oct 31, 2023
- Drop sequential executor from PendingAuthListener.
jdcormie
reviewed
Nov 7, 2023
jdcormie
requested changes
Nov 11, 2023
- 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.
markb74
reviewed
Nov 15, 2023
markb74
reviewed
Nov 16, 2023
markb74
suggested changes
Nov 29, 2023
markb74
left a comment
markb74
left a comment
Contributor
There was a problem hiding this comment.
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.
- define BinderTransportSecurity.ShutdownListener interface - take a non-close executor closeable in BinderServer's constructor
markb74
approved these changes
Nov 30, 2023
markb74
left a comment
markb74
left a comment
Contributor
There was a problem hiding this comment.
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.
| * resources to be potentially cleaned up. | ||
| */ | ||
| public interface ShutdownListener { | ||
| void onShutdown(); |
Contributor
There was a problem hiding this comment.
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.
Collaborator
Author
There was a problem hiding this comment.
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.
markb74
approved these changes
Dec 4, 2023
markb74
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
jdcormie
approved these changes
Dec 4, 2023
markb74
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
checkAuthorizationForServiceand intocheckAuthorizationForServiceAsyncsince that should be the only method called in production now.ServerSecurityPolicyTesthad their expectations updated; they previously called synchornous APIs that transformed failedListenableFuture<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.BinderSecurityTestshould already cover the new codepaths, but please let me know otherwise.This should be the last PR to address #10566.