s2a: Add gRPC S2A #11113

rmehta19 · 2024-04-18T17:24:28Z

Add S2A Java client to gRPC Java.

Context: https://github.com/google/s2a-go/blob/main/README.md

linux-foundation-easycla · 2024-04-18T17:24:32Z

The committers listed above are authorized under a signed CLA.

✅ login: rmehta19 / name: Riya Mehta (739ee23, 44fe552, f94cc10, b35f145, d1f413b, 46691df, f47c560, 72630d8, 42cd3e8, 2dd1c7e, 217a3e4)

matthewstevenson88 · 2024-04-22T15:56:17Z

Thanks @rmehta19. Can you PTAL at the test failures?

rmehta19 · 2024-04-22T18:34:59Z

@matthewstevenson88, I made 2 changes and it looks like 2/3 linux runs are passing. tests(11) is failing with an error in code that was not affected by this PR, so I don't think that failure is related.

The changes:

The build originally failed because the generated grpc code in this PR did not match the code generated by the codegen plugin. I couldn't easily compile the codegen myself(due to reasons explained in: protoc-gen-grpc-java not available on apple m1 #7690). So instead I used the latest published binary available on https://mvnrepository.com/artifact/io.grpc/protoc-gen-grpc-java (1.63) to generate S2AServiceGrpc.java (Created and used a separate simple maven project just to run the plugin and get grpc gen code). Then I changed line 8 & 9 of S2AServiceGrpc.java to match other generated code in this repo (ex:

grpc-java/alts/src/generated/main/grpc/io/grpc/alts/internal/HandshakerServiceGrpc.java

Lines 8 to 9 in fb9a108

    
           value = "by gRPC proto compiler", 
        
           comments = "Source: grpc/gcp/handshaker.proto")

). This resolved the errors thrown by

grpc-java/.github/workflows/testing.yml

Lines 65 to 66 in 3c867c9

    
           - name: Check for modified codegen 
        
             run: test -z "$(git status --porcelain)" || (git status && echo Error Working directory is not clean. Forget to commit generated files? && false)

A failure in the tests was caused when IntegrationTest.java is in package io.grpc.s2a: GetAuthenticationMechanismsTest.java fails because the flag is not being set by JCommander (or perhaps it is getting overwritten). When I moved IntegrationTest.java into the same package as GetAuthenticationMechanismsTest.java: io.grpc.s2a.handshaker, this error does not happen.

s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java

s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java

matthewstevenson88 · 2024-04-24T21:40:02Z

s2a/src/main/proto/grpc/gcp/common.proto

@@ -0,0 +1,79 @@
+// Copyright 2022 Google LLC


Just to confirm, this proto will be removed from this PR once the other PR is merged?

I believe we will maintain a copy of the protos in this directory. IIUC, there is an automated process that syncs protos in grpc-proto to protos here. Context: grpc/grpc-proto#12

E.g. ALTS protos are maintained here: alts/src/main/proto/grpc/gcp

Makes sense, thanks for clarifying!

You'll want to add s2a to the list of modules we copy protos for:

grpc-java/buildscripts/sync-protos.sh

Line 11 in 06df25b

for project in alts grpclb services rls interop-testing; do

matthewstevenson88 · 2024-04-25T21:42:34Z

LGTM, thanks @rmehta19! When you have a chance, please remove "draft" to the PR title and can we say a bit more in the CL description including pointing to the S2A-Go README (similar to what we did in the proto PR)?

After that, can we get sign-off from @erm-g and @ejona86?

rmehta19 · 2024-04-26T20:01:50Z

LGTM, thanks @rmehta19! When you have a chance, please remove "draft" to the PR title and can we say a bit more in the CL description including pointing to the S2A-Go README (similar to what we did in the proto PR)?

After that, can we get sign-off from @erm-g and @ejona86?

Done -- thank you for the review @matthewstevenson88!

erm-g · 2024-04-29T03:44:20Z

s2a/src/main/java/io/grpc/s2a/channel/S2AChannelPool.java

+  Channel getChannel();
+
+  /** Returns a channel to the channel pool. */
+  void returnChannel(Channel channel);


Not really sure about naming conventions here, but does 'releaseChannel' or 'returnToPool' sound better?

Changed naming to returnToPool.

See commit: S2AChannelPool returnChannel --> returnToPool name change.

cc: @matthewstevenson88

erm-g · 2024-04-29T03:46:46Z

s2a/src/main/java/io/grpc/s2a/channel/S2AGrpcChannelPool.java

+ */
+@ThreadSafe
+public final class S2AGrpcChannelPool implements S2AChannelPool {
+  private static final int MAX_NUMBER_USERS_OF_CACHED_CHANNEL = 100000;


Are we ok to keep it hardcoded instead of exposing it via some param/flag?

hardcoding 100000 seems fine to me, @matthewstevenson88 WDYT?

If it's OK with you all, I'd prefer to keep it hardcoded until we have a use case that requires us to expose it.

erm-g · 2024-04-29T03:50:47Z

s2a/src/main/java/io/grpc/s2a/channel/S2AGrpcChannelPool.java

+  public synchronized Channel getChannel() {
+    checkState(state.equals(State.OPEN), "Channel pool is not open.");
+    checkState(
+        numberOfUsersOfCachedChannel >= 0,


I'm not sure how numberOfUsersOfCachedChannel can be negative. It's a private var, and you decrease it by -1 in LOC 96 after checking that it's >0 in LOC 93. Am I missing something?

You are right, thanks for pointing this out -- removed this unnecessary state check.

erm-g · 2024-04-29T03:52:05Z

s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java

+public final class S2AHandshakerServiceChannel {
+  private static final ConcurrentMap<String, Resource<Channel>> SHARED_RESOURCE_CHANNELS =
+      Maps.newConcurrentMap();
+  private static final Duration DELEGATE_TERMINATION_TIMEOUT = Duration.ofSeconds(2);


Are we ok to keep these 2 hardcoded instead of exposing it via some param/flag?

Hardcoding these two timeouts is ok with me, keeping it as is might help readability. @matthewstevenson88 WDYT?

If it's OK with you all, I'd prefer to keep it hardcoded until we have a use case that requires us to expose it.

erm-g · 2024-04-29T03:55:00Z

s2a/src/main/java/io/grpc/s2a/handshaker/ConnectionIsClosedException.java

+
+/** Indicates that a connection has been closed. */
+@SuppressWarnings("serial") // This class is never serialized.
+final class ConnectionIsClosedException extends IOException {


Never seen such naming, typically in Java it's 'ConnectionClosedException'

Done. Changed to ConnectionClosedException.

erm-g · 2024-04-29T03:57:40Z

s2a/src/main/java/io/grpc/s2a/handshaker/S2AIdentity.java

+  }
+
+  /** Returns the proto {@link Identity} representation of this identity instance. */
+  public Identity identity() {


Looks like a getter, should it be 'getIdentity'?

erm-g · 2024-04-29T04:13:39Z

s2a/src/main/java/io/grpc/s2a/handshaker/tokenmanager/SingleTokenFetcher.java

+        description = "The access token used to authenticate to S2A.")
+    private static String accessToken = System.getenv(ENVIRONMENT_VARIABLE);
+
+    public synchronized void reset() {


I'm not sure I understand why 'synchronized' is needed here.

This code doesn't exist anymore, as we removed the dependency on Jcommander and replaced it with a private field & setter

erm-g · 2024-04-29T04:18:57Z

s2a/src/test/resources/config.cnf

+distinguished_name = req_distinguished_name
+req_extensions = req_ext
+
+[req_distinguished_name]


Could you add the values you entered while generating certs?

Done, as discussed offline, I updated the README with the the values used to populate these fields.

@ejona86, does this sound ok to you? @erm-g pointed me to something similar he has done in go: https://github.com/grpc/grpc-go/pull/6670/files#diff-ac85255b49c8d65b03b560d9959b99c6d9338de1400da6fc2d4133a434a376ad

ejona86 · 2024-04-29T17:21:58Z

s2a/src/test/java/io/grpc/s2a/handshaker/GetAuthenticationMechanismsTest.java

+  @BeforeClass
+  public static void setUpClass() {
+    // Set the token that the client will use to authenticate to the S2A.
+    JCommander.newBuilder().addObject(FLAGS).build().parse(SET_TOKEN);


You're using jcommander to just set a static field. This is many more steps than just having a package-private field/setter. Let's not add a dependency on jcommander.

Done: refactored and removed the JCommander dependency.

ejona86

To make sure I understand, s2a is the public API, and s2a/channel and s2a/handshaker are internal APIs. Are those internal APIs to be used anywhere, even within Google?

ejona86 · 2024-04-29T17:29:14Z

s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java

+ * Configures gRPC to use S2A for transport security when establishing a secure channel. Only for
+ * use on the client side of a gRPC connection.
+ */
+@NotThreadSafe


Credentials must be thread-safe. Should this be on the Builder instead?

Yes, the Builder is the one that's not thread safe -- done.

ejona86 · 2024-04-29T17:32:33Z

s2a/src/main/proto/grpc/gcp/common.proto

@@ -0,0 +1,79 @@
+// Copyright 2022 Google LLC


You'll want to add s2a to the list of modules we copy protos for:

grpc-java/buildscripts/sync-protos.sh

Line 11 in 06df25b

for project in alts grpclb services rls interop-testing; do

Context: - grpc/grpc-java#11113 - https://github.com/google/s2a-go/blob/main/README.md

rmehta19 · 2024-04-30T17:08:18Z

To make sure I understand, s2a is the public API, and s2a/channel and s2a/handshaker are internal APIs. Are those internal APIs to be used anywhere, even within Google?

Correct s2a is the public api, s2a/handshaker and s2a/channel are internal APIs to only be used within the s2a package.

cc: @matthewstevenson88

rmehta19 · 2024-04-30T17:51:05Z

Thank you for reviewing @erm-g, @ejona86 ! I have addressed the comments in separate commits. PTAL when you get a chance, and provide any additional feedback.

erm-g assigned erm-g and matthewstevenson88 Apr 18, 2024

rmehta19 mentioned this pull request Apr 18, 2024

gRPC Java S2A protos grpc/grpc-proto#149

Merged

rmehta19 force-pushed the s2a branch 7 times, most recently from e83fee8 to 1991e37 Compare April 20, 2024 00:22

rmehta19 force-pushed the s2a branch 5 times, most recently from 604f9a0 to 3c867c9 Compare April 22, 2024 18:14

matthewstevenson88 reviewed Apr 24, 2024

View reviewed changes

s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java Outdated Show resolved Hide resolved

matthewstevenson88 reviewed Apr 24, 2024

View reviewed changes

rmehta19 force-pushed the s2a branch 2 times, most recently from b7b51bc to 73cc37a Compare April 24, 2024 22:22

add s2a java client.

42cd3e8

rmehta19 force-pushed the s2a branch from 73cc37a to 42cd3e8 Compare April 25, 2024 16:49

rmehta19 changed the title ~~[DRAFT]s2a: Add gRPC S2A~~ s2a: Add gRPC S2A Apr 26, 2024

erm-g requested changes Apr 29, 2024

View reviewed changes

ejona86 reviewed Apr 29, 2024

View reviewed changes

rmehta19 added 7 commits April 29, 2024 13:20

update to use gRPC Authors with copyright.

739ee23

S2AChannelPool returnChannel --> returnToPool name change.

72630d8

add s2a to sync-protos script.

46691df

S2AGrpcChannelPool remove unnecessary state check.

44fe552

update proto package to grpc.gcp.s2a.

2dd1c7e

ConnectionIsClosedException --> ConnectionClosedException.

d1f413b

identity() --> getIdentity().

f94cc10

ejona86 pushed a commit to grpc/grpc-proto that referenced this pull request Apr 29, 2024

Add gRPC Java S2A protos (#149)

1dc480d

Context: - grpc/grpc-java#11113 - https://github.com/google/s2a-go/blob/main/README.md

rmehta19 added 2 commits April 29, 2024 16:49

annotate S2AChannelCredentials.Builder with NotThreadSafe.

217a3e4

add values entered when generating certs.

b35f145

rmehta19 force-pushed the s2a branch from b0858c0 to ba7fc39 Compare April 30, 2024 17:03

rmehta19 force-pushed the s2a branch 2 times, most recently from d52ee0c to 38f72be Compare April 30, 2024 17:27

remove JCommander dependency.

f47c560

rmehta19 force-pushed the s2a branch from 38f72be to f47c560 Compare April 30, 2024 17:28

s2a: Add gRPC S2A #11113

Are you sure you want to change the base?

s2a: Add gRPC S2A #11113

Conversation

rmehta19 commented Apr 18, 2024 • edited

linux-foundation-easycla bot commented Apr 18, 2024 • edited

matthewstevenson88 commented Apr 22, 2024

rmehta19 commented Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewstevenson88 commented Apr 25, 2024

rmehta19 commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Apr 30, 2024

rmehta19 commented Apr 30, 2024

rmehta19 commented Apr 18, 2024 •

edited

linux-foundation-easycla bot commented Apr 18, 2024 •

edited

rmehta19 commented Apr 22, 2024 •

edited