s2a: Add gRPC S2A

[rmehta19]

Add S2A Java client to gRPC Java.

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

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: rmehta19 / name: Riya Mehta (739ee23e03350104b0d60a4d1dbb14a6026b67af, b35f145be2cb13c6ef58159c3deefc55816efb64, 46691df78e14e059b579af454b4f965243791283, 2dd1c7e7befd4d1eb35e29ac5a8307d30a9aed6e, 72630d8d23ef6d1e8c384449977f85d97e26cd6e, 217a3e417fef5c2a3261b6fcd82efec7fa5b6ffa, 44fe552660ba1aa921c073e8605d99dc2361785c, f94cc100cbaff7ee0151ef923851018a715da987, 08f83421b3ece4a7e44a84637971451f141f5f9e, f47c560980124babdd5af4fa365762559bfbc64c, 1d41d1037b47e36abc335d5e2019afd1679d165b, 42cd3e8fd841322d6f92f45d0315d537dd53ed80, f96d3956c34af47f5cf214a79930726471d43ed7, 12586b11508fd5fca7058e8e414428b93d7e45f2, 35084afb3ab3615f949470c44af857e6496276fc, 7f267121aa662379a1c137de27bf0f73c8b76cda, d1f413b0768bdcbd4946390ef3aaa5dd52287d65, 655f0bd893629ac46484209e8164733f8fc58adf, da330cd1f7c495ddff53becdba8450b38ad83750, 50b7366f50ab6c5a930c053d75856e72dce49229, 0e059e4d1a3a4477abe53f7d9fd9f411a9395990, a8cacb0f14d1235adb47be598b8b49b5096ba778, 3198eec7ca420d098d0c37d793774bf84e72d2d0, 3184cdcfddda29ebcfe45b1011c882df1489f5b2, 38b0a3a2c254785f375d4c4e9407f6405bf7df08, b021a2128ec2dc92fa403c190d4aeaa60c259a78, 752627a8e5b383f9bb06c5296bd1691a5987f1da, 19583a7b136989d6c1d4b88be814cbe2e3e15cc4)

[matthewstevenson88]

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

[rmehta19]

@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: https://github.com/grpc/grpc-java/issues/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: https://github.com/grpc/grpc-java/blob/fb9a10809f5106333f447fe0cda1c761d5846f72/alts/src/generated/main/grpc/io/grpc/alts/internal/HandshakerServiceGrpc.java#L8-L9). This resolved the errors thrown by https://github.com/grpc/grpc-java/blob/3c867c9ab7db99509830d3a722fb6f9098a1217e/.github/workflows/testing.yml#L65-L66

• 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.

[matthewstevenson88]

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]

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!

[rmehta19]

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]

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.

[rmehta19]

@ejona86, PTAL when you get a chance, and leave any additional feedback. Thank you in advance!

@erm-g & @matthewstevenson88 have approved these changes.

[larry-safran]

On July 16th @ejona86 replied to an email requesting some major reworking in your implementation. Has there been any progress on that? Specifically:

1. No blocking on the event loop
2. Use Async, not the blocking stub
3. Change to a more standard way of using error classes
4. Have your tests specify reasonable deadlines

I've been waiting on reviewing until this rework was done.

From the email:

I'm not comfortable with that handlerAdded(). InterruptedException causes loud sirens; there must be no blocking on
the event loop. We were promised ALTS would stop blocking, but that was never implemented and it caused my team
lots of time on debugging and workarounds. Given you're able to use SSLHandler, I don't think there'd be any strong 
reason to allow blocking, even initially. I see the blocking stub, and that will probably need to be changed. 
IllegalArgumentException is just the wrong exception type; that means "the programmer made an error" and it should 
basically never be caught. But it also means an argument was wrong, and a KeyStoreException seems unrelated to the 
arguments. Mixing "ChannelHandler next" and "negotiator.newHandler()" also seems suspicious, as it seems you are 
delegating twice.

[rmehta19]

Thanks @larry-safran, the item's have been addressed

[rmehta19]

Thanks for the review @larry-safran ! Please let me know if there is anything else to address.

[rmehta19]

Thanks for the comments @ejona86; I'll address them in separate commits at https://github.com/grpc/grpc-java/pull/11534. Let me know if you prefer separate PRs for them, or any other way of organizing.

[ejona86]

There's plenty of smaller/isolated items. Feel free to group those together as you wish, as they are easy to review. A few of the comments might turn into larger/plumbing changes. Some of those may be best to be separate. But overall, if you do lots of the "easy" stuff together that'd be fine to review/merge while you work on something more difficult.

[rmehta19]

@ejona86 , @larry-safran I have addressed all the comments on this PR in the following 3 PRs:

• https://github.com/grpc/grpc-java/pull/11534
• https://github.com/grpc/grpc-java/pull/11539
• https://github.com/grpc/grpc-java/pull/11540

PTAL at these 3 and leave any comments.

I will address the comment to move things to internal package and combine the MTLS and non-MTLS apis in the following 2 PRs, once the above 3 are merged.

• https://github.com/grpc/grpc-java/pull/11541
• https://github.com/grpc/grpc-java/pull/11544

Besides these changes, please let me know anything else we can do to enable s2a to be included in a grpc-java release. Thank you!