Add Channel Credentials #7294
Add Channel Credentials #7294
Conversation
alts/src/main/java/io/grpc/alts/ComputeEngineChannelBuilder.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/GoogleDefaultChannelCredentials.java
Outdated
Show resolved
Hide resolved
| if (CheckGcpEnvironment.isOnGcp()) { | ||
| callCredentials = MoreCallCredentials.from(ComputeEngineCredentials.create()); | ||
| } else { | ||
| callCredentials = new FailingCallCredentials( |
There was a problem hiding this comment.
Have been meaning to ask this. I understand this matches the old behavior but if you are not on Gcp is there a reason to fail each call instead of failing channel creation? Wouldn't it be better to fail early when the environment is wrong?
There was a problem hiding this comment.
We don't tend to fail channel creation based on I/O; we do it when you use the API incorrectly. So I think we did it this way to fail in the normal way we fail for I/O.
| try { | ||
| callCredentials = MoreCallCredentials.from(GoogleCredentials.getApplicationDefault()); | ||
| } catch (IOException e) { | ||
| // TODO(ejona): Should this just throw? |
There was a problem hiding this comment.
As I commented elsewhere throwing sounds better than FailingCallCredentials unless you want to match existing behavior.
There was a problem hiding this comment.
The other case is a bit different. We can check if we're on GCP by looking at the BIOS information, which would be pretty quick and reliable. And ComputeEngineCredentials doesn't do any I/O when created.
In this case getApplicationDefault() itself does network I/O and can fail for reasons other than misconfiguration. It seems wrong to permanently break a channel in the case of that failure without providing a good way for users to manage the error themselves. So that's why I suggested throwing here.
Actually, I guess there is another option here: create CallCredentials that call getApplicationDefault() as part of its loading of credentials and if it fails it could continue retrying.
In either case, though, I don't think it is appropriate to change the behavior in this PR. And we probably can't change the existing builder. But we do really want to decide the API for GoogleDefaultChannelCredentials before stabilizing it.
There was a problem hiding this comment.
It looks like we shall also call CheckGcpEnvironment.isOnGcp() for GoogleDefaultChannelCredentials.
Do we expect caller of GoogleDefaultChannelCredentials outside GCP? It is not safe to call ALTS without GCP platform check.
If we do plan to support GoogleDefaultChannelCredentials outside GCP, we can do platform check, if running outside GCP, disable ALTS, only TLS is allowed. WDYT?
There was a problem hiding this comment.
Do we expect caller of GoogleDefaultChannelCredentials outside GCP? It is not safe to call ALTS without GCP platform check.
Absolutely we do. In that case we won't get any gRPCLB addresses and so won't trigger the ALTS code path.
The entire point of GoogleDefaultChannelCredentials (which mirrors C core) is to auto-select the proper credentials to use when communicating with Google independent of where you are. Just like GoogleCredentials.getApplicationDefault().
In any case, I do not want to change existing behavior in this PR.
There was a problem hiding this comment.
IIUC, google_default_credentials in c core can only be run on GCP. Agree this PR should not change existing behavior.
It is reasonable to not perform GCP check for GoogleDefaultCredentials, as we trust DNS to signal if we enable ALTS or not. If GDC runs outside GCP, ALTS code path won't be triggered.
There was a problem hiding this comment.
ComputeEngineChannelCredentials is the one restricted to GCP. No reason to restrict GoogleDefaultChannelCredentials to GCP (similar to the behavior of GoogleDefaultChannelBuilder)
|
@ejona86 Thanks much for implementation! Security team is long waiting for this PR. |
core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/grpc/internal/CallCredentialsApplyingTransportFactory.java
Show resolved
Hide resolved
netty/src/main/java/io/grpc/netty/InternalNettyChannelCredentials.java
Outdated
Show resolved
Hide resolved
netty/src/main/java/io/grpc/netty/InternalNettyChannelCredentials.java
Outdated
Show resolved
Hide resolved
okhttp/src/main/java/io/grpc/okhttp/SslSocketFactoryChannelCredentials.java
Outdated
Show resolved
Hide resolved
|
Finished this round of review. It will also be good to resolve the merge conflicts that exist now. |
|
@sanjaypujare, I've pushed individual commits with the changes. I'm working on rebasing next, but when I do so it will be harder to see what new changes I've made. I also still need to update the TODO experimental APIs to their actual URL. |
Okay. I went thru the latest changes/comments and added a few comments myself. Will take a look again once the new changes are in. |
AltsChannelBuilder could be improved a bit more by removing the call to InternalNettyChannelBuilder.setProtocolNegotiatorFactory. However, to do that cleanest would require reworking how port is plumbed in NettyChannelBuilder and potentially AbstractManagedChannelImplBuilder to move getDefaultPort() to ProtocolNegotiator from ClientFactory. Saving that for another day.
Since this is huge I'm very willing to split this into multiple PRs and have this be an "overview" of all the changes. My main concern with that is how many back-and-forths that may require, since so much of this is connected and the commit "Migrate users of ManagedChannelBuilder.{forTarget,forAddress} to ChannelCredentials" is important to see how this plays out in real code. I'd suggest "skipping ahead" and looking at that commit first.
Notably, it is annoying to have to remember which ChannelCredentials have public constructors and which have a
create()method. I think it would probably be best to get rid of the public constructors and always havecreate()/newBuilder(). If I did that, then it's not 100% clear to me whether we should havecreate()returnChannelCredentialsor the concrete type (e.g.,TlsChannelCredentials).ChannelCredentialsshould be fully adequate for the producers of the credentials (the application) and might make it easier to migrate the internal credential implementations. The best example of this is OkHttp's SslSocketFactoryChannelCredentials; it seemed beneficial to hide the internal data structure to allow it to morph over time. That doesn't really apply the same to the credentials in io.grpc though.