alts: add Google Default Channel implementation #4742

jiangtaoli2016 · 2018-08-08T21:11:37Z

experimenting

ejona86

A few drive-by comments.

ejona86 · 2018-08-09T16:20:34Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+        NettyChannelBuilder.forTarget(target)
+            .keepAliveTime(20, TimeUnit.SECONDS)
+            .keepAliveTimeout(10, TimeUnit.SECONDS)
+            .keepAliveWithoutCalls(true);


What's going on here?

It is the same as ALTS channel builder and internal version of channel builder. Is there a problem here to set keepAliveWithoutCalls(true)?

All three of these settings are inappropriate when using TLS. The keepalive time will cause failures when going through GFE.

I see Can I just delete these 3 lines (lines 77-79)? Or you have better suggestion?

@jiangtaoli2016, if you don't need them, removing them will work.

ejona86 · 2018-08-09T16:28:52Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+          @Override
+          public TsiHandshaker newHandshaker() {
+            // Used the shared grpc channel to connecting to the ALTS handshaker service.
+            ManagedChannel channel = HandshakerServiceChannel.get();


This needs to be referenced counted. We can't have resources/threads live forever. That'll cause memory leaks in environments with custom ClassLoaders. You can create a SharedResourceHolder.Resource for it.

Many environments are quite temperamental when it comes to creating new threads, so it looks like we'll also want it to be lazily initialized, the first time ALTS is used in this channel.

I've created #4755. Let fix this quarter in a separate PR. It would be fairly easy to unref the channel if nobody is using it. I like to keep the thread open until application closes. Otherwise keep creating/closing thread could also be expensive. Is there concept of fiber (i.e., lightweight thread) in Java?

I like to keep the thread open until application closes.

No. The threads are actually the primary problem.

Otherwise keep creating/closing thread could also be expensive.

This is what our SharedResourceHolder deals with, to a degree. It waits a second before actually freeing, in case there will be another use soon. In general though, we would expect users to have a long-lived Channel, so there shouldn't be rampant thread creation.

Is there concept of fiber (i.e., lightweight thread) in Java?

Nope. I wish.

Let me take a look at SharedResourceHolder. I may schedule a mtg with you and @carl-mastrangelo. I remember old time ago, @carl-mastrangelo and I thought about SharedResourceHolder, but we did not use it. I don't remember the exact reason.

jiangtaoli2016 · 2018-08-18T02:19:06Z

@ejona86 Code revised with unit tests. Could you please take a look?

ejona86 · 2018-08-20T22:44:16Z

:-/ You squashed the changes all together. Please don't do that next time, as it means I can't see any changes you've made since last time I looked.

jiangtaoli2016 · 2018-08-21T16:10:18Z

Sorry about squashing. In grpc c core PRs, reviewable is often used, thus squashing is not an issue there.

The difference between first version and a few days back is that I added unit tests. I also revised GoogleDefaultProtocolNegotiator.java to add a testing-only constructor.

PTAL. I will squash in the end before merge.

ejona86 · 2018-08-20T22:49:37Z

repositories.bzl

+def com_google_auth_google_auth_library_oauth2_http():
+    native.maven_jar(
+        name = "com_google_auth_google_auth_library_oauth2_http",
+        artifact = "com.google.auth:google-auth-library-oauth2-http:0.9.0",


This requires dependencies on google-http-client and google-http-client-jackson2, transitively. So it will also pull in jackson-core-asl, org.apache.httpcomponents:httpclient, and maybe j2objc-annotations.

Is there a better library to depends on? DefaultChannel needs to call GoogleCredentials.getApplicationDefault().

To my knowledge... no.

Shall we add a comment on ALTS that alts artifact will depend on all these?

No, it would just be a dependency in the BUILD.bzl file. Note that if we were doing this "right" we would have a third_party/ directory that would pull in the maven_jar and include all the proper dependencies.

Ack. Thanks for clarification.

ejona86 · 2018-08-22T22:42:52Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+  }
+
+  /** "Overrides" the static method in {@link ManagedChannelBuilder}. */
+  public static GoogleDefaultChannelBuilder forAddress(String name, int port) {


Is this version necessary/helpful? It seems just a target string would be more than enough. People wouldn't need to pass in ipv6 addresses here, nor would it even be common to specify a port.

Okay, let remove this forAddress()

Oops, if I remove this forAddress(), tests will fail on https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java#L91, because GoogleDefaultChannelBuilder extends ForwardingChannelBuilder and it supposes to define forAddress().

Oh, that's right, ManagedChannelBuilder.forAddress exists. Yeah, we will want this function to exist here.

ejona86 · 2018-08-22T22:44:26Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+
+  @Override
+  public ManagedChannel build() {
+    InternalNettyChannelBuilder.setDynamicTransportParamsFactory(delegate(), tcpfFactory);


I'd prefer this to be called in the constructor, to avoid mutating the builder during build().

ejona86 · 2018-08-22T22:46:04Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+    }
+  }
+
+  static final class GoogleDefaultChannel extends ManagedChannel {


Why isn't this an interceptor? It could be added to the underlying channel with delegate().intercept() (probably from the builder's constructor).

Is there benefit to use interceptor here?

You would only implement newCall (actually interceptCall, but it's basically the same thing). All the other methods aren't necessary.

ejona86 · 2018-08-22T22:48:35Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+    try {
+      return new GoogleDefaultChannel(
+          delegate().build(), GoogleCredentials.getApplicationDefault());
+    } catch (IOException e) {


This is a pretty common exception. I'm not wild about hiding it behind a RuntimeException. Options:

Throw from forTarget()

Throw from build()

If it throws, package that up into a CallCredentials or an Interceptor to fail calls on this Channel.

Maybe there's some other options.

We can throw IOException from build(). But we need to declare build() can throws IOException in ManagedChannelBuilder and ForwardingChannelBuilder. Revise.

But we need to declare build() can throws IOException in ManagedChannelBuilder and ForwardingChannelBuilder.

Oh, yeah, we would need to change those. Changing those would break API though. That means (2) is not possible.

ejona86 · 2018-08-22T23:20:06Z

interop-testing/src/main/java/io/grpc/testing/integration/TestServiceClient.java

          + "\n  --use_tls=true|false        Whether to use TLS. Default " + c.useTls
          + "\n  --use_alts=true|false       Whether to use ALTS. Enable ALTS will disable TLS."
          + "\n                              Default " + c.useAlts
+          + "\n  --custom_credentials_type   Custom credentials type to use. Default "


Is this the argument that's being used cross-language?

This naming of "custom_credentials_type" come from grpc c core. Although I don't like this argument name, it is better to keep it so that interop tests can work in the future.

jiangtaoli2016 · 2018-08-24T05:36:37Z

@ejona86 code revised. PTAL

ejona86 · 2018-08-24T15:05:47Z

core/src/main/java/io/grpc/ManagedChannelBuilder.java

   * @since 1.0.0
   */
-  public abstract ManagedChannel build();
+  public abstract ManagedChannel build() throws IOException;


As I said before, this change is not backward compatible and thus not acceptable. Please revert, along with all the other exception handling you added elsewhere to make this possible. One of the other options is still possible, but we can't add new exceptions to pre-existing stable methods.

This is reason I throw RuntimeException in the first place. We cannot throw at forTarget(), at build(). We cannot throw at ClientInterceptor.

It looks throwing RumtimeException is the best option, IMO.

Why can't we throw in forTarget()? While the ClientInterceptor can't throw, it can fail the RPC.

jiangtaoli2016 · 2018-08-24T16:11:06Z

In ALTS, we check and fail (throws runtime exception) when we detect app is not running on GCP. Similarly, when a user tries to use GoogleDefaultChannel and if it does not run on GCP and he/she does not provide google credentials, we shall fail early (e.g., through runtime exception) rather than waiting SSL handshake succeed and then fail during rpc call (e.g., through per call interceptor). What do you think @ejona86 ?

ejona86 · 2018-08-24T16:42:27Z

In ALTS, we check and fail (throws runtime exception) when we detect app is not running on GCP.

That sounds wrong. Can you point me to that code?

Similarly, when a user tries to use GoogleDefaultChannel and if it does not run on GCP and he/she does not provide google credentials, we shall fail early (e.g., through runtime exception)

I don't much problem with failing early. I have a problem with using a RuntimeException for a common case that the user should really handle.

jiangtaoli2016 · 2018-08-25T01:40:01Z

Code revised. If we fail to at getApplicationDefault(), instead of throws exception, we fail at each RPC call. @ejona86 If you are happy with the change, I can do similar trick on ALTS in the next PR.

ejona86 · 2018-08-27T14:47:39Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

    } catch (IOException e) {
-      throw new RuntimeException(e);
+      String message = "Failed to get Google default credentials: " + e.getMessage();
+      logger.log(Level.WARNING, message);


As a general rule, log an error or propagate the error, but not both. Remove this log statement.

Got it! log statement removed.

ejona86 · 2018-08-27T16:29:39Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

+      credentials = MoreCallCredentials.from(GoogleCredentials.getApplicationDefault());
+    } catch (IOException e) {
+      String message = "Failed to get Google default credentials: " + e.getMessage();
+      status = Status.FAILED_PRECONDITION.withDescription(message);


Don't throw away the exception. Instead, pass it via withCause(e). At that point you shouldn't include e.getMessage() in the description.

ejona86 · 2018-08-27T16:58:49Z

alts/src/main/java/io/grpc/alts/GoogleDefaultChannelBuilder.java

-      status = Status.FAILED_PRECONDITION.withDescription(message);
+      status =
+          Status.FAILED_PRECONDITION
+              .withDescription("Failed to get Google default credentials.")


Nit: we don't tend to end in punctuation.

jiangtaoli2016 · 2018-08-27T22:27:19Z

Thanks @ejona86 for review! Let me squash and merge. Two more PRs coming.

jiangtaoli2016 force-pushed the default_channel branch 4 times, most recently from ec18806 to 32fad37 Compare August 9, 2018 03:33

ejona86 reviewed Aug 9, 2018

View reviewed changes

jiangtaoli2016 force-pushed the default_channel branch from 32fad37 to 8a787a0 Compare August 18, 2018 02:17

jiangtaoli2016 changed the title ~~alts: add Google Default Channel implementation [do not merge]~~ alts: add Google Default Channel implementation Aug 18, 2018

jiangtaoli2016 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 21, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 21, 2018

ejona86 reviewed Aug 22, 2018

View reviewed changes

jiangtaoli2016 force-pushed the default_channel branch 3 times, most recently from 67863a5 to 3dfa45a Compare August 23, 2018 22:10

ejona86 reviewed Aug 24, 2018

View reviewed changes

jiangtaoli2016 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 25, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 25, 2018

ejona86 approved these changes Aug 27, 2018

View reviewed changes

jiangtaoli2016 force-pushed the default_channel branch from b4267eb to f9d3459 Compare August 27, 2018 16:20

alts: add Google Default Channel implementation

cbf3f7b

jiangtaoli2016 force-pushed the default_channel branch from f9d3459 to cbf3f7b Compare August 27, 2018 16:24

ejona86 reviewed Aug 27, 2018

View reviewed changes

jiangtaoli2016 force-pushed the default_channel branch from 460f1cc to 8195994 Compare August 27, 2018 16:51

jiangtaoli2016 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 27, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Aug 27, 2018

ejona86 approved these changes Aug 27, 2018

View reviewed changes

alts: use withCause to handle exception

6f7ee6c

jiangtaoli2016 force-pushed the default_channel branch from 8195994 to 6f7ee6c Compare August 27, 2018 17:00