fix: DirectPath calls do not duplicate the request metadata #3663
Conversation
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java
Outdated
Show resolved
Hide resolved
| // Use default TLS credentials if we cannot initialize channel credentials via DCA or S2A. | ||
| channelCredentials = | ||
| CompositeChannelCredentials.create( | ||
| TlsChannelCredentials.create(), MoreCallCredentials.from(credentials)); |
There was a problem hiding this comment.
qq, for this flow, shouldn't it be just TlsChannelCredentials.create() for the channelCredentials?
Why does it need to be done via CompositeChannelCredentials?
There was a problem hiding this comment.
Oh wait.. This is because we were sending the GoogleCredentials as part of CallOptions. NVM I think this is correct.
| @@ -185,21 +184,8 @@ public GrpcCallContext nullToSelf(ApiCallContext inputContext) { | |||
|
|
|||
| @Override | |||
| public GrpcCallContext withCredentials(Credentials newCredentials) { | |||
There was a problem hiding this comment.
I think the one main concern about this is that this is a behavior breaking change if there are uses that manually create a GrpcCallContext and use withCredentials. I don't think there are really any valid uses cases for this, but I will need to check for this.
There was a problem hiding this comment.
Yes that makes sense.
What's also concerning is, by looking at the CI failures, it seems many tests have been building the channel from InstantiatingGrpcChannelProvider without actually giving it a credentials. And MoreCallCredentials.from() throws exception for null credentials.
There was a problem hiding this comment.
Those showcase tests seems to be fixable within this PR. But I'm unsure about the downstream compatibility tests and those on Cloud Build (I lack permission to view logs).
There was a problem hiding this comment.
The showcase tests were following this: https://github.com/googleapis/gapic-showcase?tab=readme-ov-file#example-for-java-grpc
Essentially it's a local server that is not expected to do any authentication. I think we just need a way to be able to hit the local server with no credentials provided.
The downstream tests for our partner teams is a bit concerning. I think we'll need to investigate them to see if they're just a testing configuration issue or a logic issue.
There was a problem hiding this comment.
Essentially it's a local server that is not expected to do any authentication. I think we just need a way to be able to hit the local server with no credentials provided.
Would it make sense if we allow null credentials in the case of one-way TLS? I believe there are no valid use cases to go without credentials here when mTLS or DirectPath are used.
The downstream tests for our partner teams is a bit concerning. I think we'll need to investigate them to see if they're just a testing configuration issue or a deeper issue.
Agreed.
There was a problem hiding this comment.
I realize a tricky thing we missed earlier.
The API setChannelConfigurator takes an option that gets applied to the channel builder. In various tests (downstream or showcase), ManagedChannelBuilder::usePlainText is given. While users are not supposed to use insecure channels in production, this is reasonable and often convenient in tests when the server is turned on locally. This change is breaking that behavior.
Basically, either we have to change all tests (setting up TLS can be difficult), or we need a way to allow insecure channels w/ credentials, and TlsChannelCredentials.create() doesn't support that. For the second approach, IIUC, it goes back to the need of supporting adding call credentials to a managed channel builder that is possibly configured to build insecure channels. cc: @ejona86
There was a problem hiding this comment.
Using InsecureChannelCredentials.create() instead of TlsChannelCredentials would be fine for those specific tests. But the existing sdk API works poorly for that.
You can add CallCredentials per-RPC, either on the stub, in the CallOptions, or with an interceptor (that adds it to the CallOptions).
There was a problem hiding this comment.
Yes unfortunately the existing SDK API doesn't take InsecureChannelCredentials explicilty.
The client SDK has been attaching call credentials to the CallOptions. Now the problem is that DirectPath (via GoogleDefaultChannelCredentials) gets or creates its own call credentials. So currently, this is causing the same request metadata to appear twice. Because we want to adopt bound credentials in DirectPath, it's not ideal to change this GoogleDefaultChannelCredentials to build without call credentials. Nor would it be good for backward compatibility reasons.
So as this PR is trying to do, we want to stop attaching call credentials to CallOptions in all gRPC cases. But then we need to make sure all gRPC cases can get the call credentials from the channel. ManagedChannelBuilder doesn't allow it. This is where we are stuck.
There was a problem hiding this comment.
You can add an interceptor to the channel that attaches CallCreds to the CallOptions.
There was a problem hiding this comment.
You can add an interceptor to the channel that attaches CallCreds to the CallOptions.
Ah. I didn't realize this from your first comment. Will try it. Thanks!
gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
Show resolved
Hide resolved
|
/gcbrun |
rockspore
force-pushed
the
grpc-creds
branch
from
06e4620 to
264a123
Compare
|
@lqiu96 Thanks for the patch. I applied it and it was able to solve almost all the test failures except for one in java-bigtable. I was guessing maybe the test there wasn't giving Now I suspect that the failing case may be truly relying on the call credentials attached to the client context, and then call options, which this PR stops doing. The investigation is still WIP. |
|
Perhaps it's just this one test file that's configuring the channel with proxy: https://github.com/googleapis/java-bigtable/blob/cd7f82e4b66dc3c34262c73b26afc2fdfd1deed7/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java#L256 We could try fixing that test by attaching credentials to the channel provider there if that's the only failure. And we can talk more about the last commit (add interceptor with |
|
Hmm, I think this may be a new edge case we haven't considered. For the test, BigTable is passing in a FixedTransportChannel with a gRPC Transport Channel already configured (it's plaintext and no CallCredentials is used during channel creation). They also pass in a FixedCredentialProvider with a credential to be used. The test expects that the FixedCredentialProvider Cred value will be passed to the CallOptions (or be used), but since we changed the logic so that only the channel creds are used, it ends up not using the creds at all (no bearer token). |
|
Actually after another look, I think this test is a bit confusing and there is probably more going on in the BigTable library. FixedTransportChannelProvider's |
If their test is relying on the existing behavior (prior to this PR) that the credentials will be passed to the |
Yes, unfortunately I believe so. At least for the test, it looks like they have FixedTransportChannel (with no Creds) and a FixedCredentialProvider with the creds that they want to apply. I think there are certain ways we can work with this inside Gax to pass this over. I think I'll need to talk to a BigTable expert to confirm this to ensure we're not breaking anything.
I don't believe so. There are other TransportChannelProviders, but they're used in testing so I don't think we should be impacted by them. |
This is achieved by:
InstantiatingGrpcChannelProvideralways uses the given credentials when creating the gRPCManagedChannel.GrpcCallContextdoes not put call credentials into the call options duringwithCredentials()calls.So far the DirectPath flow is verified to work properly in local test setups.