L74: Java Channel Credentials

[ejona86]

Take special note of how ManagedChannelBuilders are created.

[jiangtaoli2016]

@ZhenLian FYI

[sanjaypujare]

Put Java here?

[ejona86]

It's not implemented yet, so I was leaving it as-is. I expected to update the gRFC after adding it to grpc-java to update this line. I'd also update the "Implementation" section to say which version it was added to.

[ejona86]

I've sent out the changes for this proposal in grpc/grpc-java#7294 .

One thing came out of that when using the unstable ChannelCredentials as provided by the transports: they won't have constructors. I don't care too much about users that use the classes in io.grpc.netty or io.grpc.okhttp directly, since they are few and can "deal with it." But it is a bigger concern for more user-facing things like GoogleDefaultChannelCredentials. We have a few of those today and will get more, like XdsChannelCredentials. That could cause "normal users" to see a mix of both construction styles.

[ejona86]

API design review, just talking about whether to have create() methods on things like TlsChannelCredentials:

• There was some concern about *ChannelCreds that don't actually extend ChannelCreds (like GoogleDefaultChannelCreds) (same concern raised by @carl-mastrangelo)
• There did not seem to be a problem having a separation between producers (applications) and consumers (transports). We did not find any need for users to refer to concrete classes like TlsChannelCreds in their own APIs; they can simply use ChannelCreds and be better off for it. Consumers will pretty much always need to consume the base ChannelCreds class and use instanceof checks
• (1) If we had a create() method for all “ChannelCreds” and had that method return ChannelCredentials (not the concrete type), then that helps some. This mainly impacts those in io.grpc (Tls, Insecure, etc). We are seeing it as a feature that it returns ChannelCreds instead of the concrete type
• (2) Going a step further, we could make ChannelCreds final and not have any extending classes. That guarantees that all credentials are uniform. To achieve that, we move the current hierarchy to a “Config” set of classes, and have ChannelCredentials just be a final class holder for that config. So TlsChannelCredentials.Config would be the concrete type within a ChannelCredentials. There would be a tree of configs instead a tree of credentials
• We did discuss the possibility of making GoogleDefaultChannelCredentials actually extend ChannelCredentials. We could make NettyChannelCredentials itself an abstract factory for ProtocolNegotiators (instead of a holder of a factory) and then GoogleDefaultChannelCredentials would extend it to provide its implementation. To support CallCredentials on that same object we’d need to add a CallCredentials.getCallCredentials() method, which could be used instead of CompositeChannelCredentials. But we had a design goal of not exposing the Netty-specific implementation detail of those classes, and this would expose users directly to ProtocolNegotiator and expose GoogleDefaultChannelCredentials forever as an NettyChannelCredential

(2) has more complexity, via an extra level of indirection, but is clear. (1) suffices, but has some awkwardness.

@carl-mastrangelo, do you want to provide a vote? We all could accept either way (1 or 2), but had different preferences.

[ejona86]

I should also mention, from the API review:

• We discussed renaming the CallCredentials-that-aren't-credentials. Using "Factory" in the same was suggested, and it make sense. But it is not a normal "Java Factory" (or producer, or whatever else), since the class would not be instantiated and it doesn't implement an interface.
• We discussed using "Util" but were split on how we felt about that. The create() method then should probably change, to like createChannelCredentials()
• We discussed moving the create() methods into factory methods on new classes named OkHttp, Netty, and Alts; for example, Alts.googleDefaultChannelCreds(). However, we would prefer to keep the classes separate. We've experienced cases where users want to limit usage of certain classes (like InsecureChannelCreds, for security reasons) and multiple credentials in one class makes that difficult.

So those were things considered, but did not settle on them.

[carl-mastrangelo]

My leaning is more towards (2). I didnt have any other feedback at this point.

[ejona86]

I made the changes for approach (1), which is a prerequisite for (2) anyway.

I thought I had been convinced to go with approach (2), and I started with making the changes for (2). But it seemed to mostly add confusion and goo. The documentation in particular had to become more precise in a way that seemed unhelpful. And users were going to need to "see" the Config object places and know to just ignore it.

I'm going to try again tomorrow with the changes to (2) and see how it compares a bit more directly.

[sanjaypujare]

We are not hiding concrete types (like ChoiceChannelCredentials or CompositeChannelCredentials) but expect users to do instanceof and a type-cast to be able to use methods like getChannelCredentials() of CompositeChannelCredentials (for example). How does it support the view that "concrete type should not be relevant to API users and may be an implementation decision"?

Wouldn't it then be better for the create() methods to return the actual public concrete type, but making sure that most of the functionality is at the ChannelCredentials level and we only expose the minimum necessary functionality in the exposed concrete type?

[ejona86]

Most users don't need to call any methods on the credentials. They just 1) create them and 2) pass them around. The transports are the main thing that needs to call methods on the credentials.

Wouldn't it then be better for the create() methods to return the actual public concrete type

Better in what way?

but making sure that most of the functionality is at the ChannelCredentials level and we only expose the minimum necessary functionality in the exposed concrete type?

I don't understand what that would look like. Could you provide a sketch of what ChannelCredentials would look like in that approach?

[sanjaypujare]

Wouldn't it then be better for the create() methods to return the actual public concrete type

Better in what way?

Compared to what was described in the original text "...Concrete types would only be used after {@code instanceof} checks..."

So with the current proposal one would do:

ChannelCredentials channelCreds = ChoiceChannelCredentials.create(...);
...
// somewhere later in the code
if (channelCreds instanceof ChoiceChannelCredentials) {
    ChoiceChannelCredentials choiceChannelCreds = (ChoiceChannelCredentials)channelCreds;
    // do something with choiceChannelCreds
}

Whereas if create returned the actual concrete type you wouldn't need the check and the cast:

ChoiceChannelCredentials choiceChannelCreds = ChoiceChannelCredentials.create(...);
...
// somewhere later in the code
// do something with choiceChannelCreds

All I am saying is that exposing/making public concrete subclasses but not have create return them looks a little inconsistent but I do understand the argument about encouraging the pattern.

but making sure that most of the functionality is at the ChannelCredentials level and we only expose the minimum necessary functionality in the exposed concrete type?

I don't understand what that would look like. Could you provide a sketch of what ChannelCredentials would look like in that approach?

Nothing different from what is currently being proposed. As you said users would mostly create creds and pass them around because the ChannelCredentials interface/functionality should work for most cases.

[ejona86]

There is a very strong producer/consumer split with this API. The producer doesn't have any need to look at the configuration; it provided it. The consumer is what needs to look at the configuration, and if it only supports one specific credential type that sort of defeats the purpose. So I don't see your example as reflecting any actual use case for this API.

As you said users would mostly create creds and pass them around because the ChannelCredentials interface/functionality should work for most cases.

No, I don't understand what configuration you think should be within ChannelCredentials.

[sanjaypujare]

LGTM