-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Enable MTLS_S2A bound token by default for gRPC S2A enabled flows #3591
Conversation
cc: @rockspore |
@@ -592,6 +593,41 @@ ChannelCredentials createS2ASecuredChannelCredentials() { | |||
} | |||
} | |||
|
|||
boolean isMtlsS2AHardBoundTokensEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a thought (nothing that needs to be changed in this PR): With how many helper methods we have for S2A and hard bound tokens, I wonder if we can split these methods into a helper class in Gax-Grpc (something like S2AMtlsContext or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this would be help to reduce the complexity in the InstantiatingGrpcChannelProvider
file. I'm happy to do the cleanup of that in a followup CL.
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
...ava/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can the title be updated to be something like:
feat: Enable MTLS_S2A bound token by default for gRPC S2A enabled flows
So users who use gRPC don't need to worry.
Thanks @lqiu96 for the review! |
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. In a future PR, I think we can try and see if we can clean up the S2A logic and move it into a helper/ util class.
/gcbrun |
Similar to implementation for DirectPath in #3572.
This is part of the experimental S2A feature (see #3400)