Skip to content

fix: remove call credentials from call options if DirectPath #3670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 10, 2025
Merged

fix: remove call credentials from call options if DirectPath #3670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ae471b3
remove call credentials from call options if DirectPath
rockspore Mar 1, 2025
39489bc
fix ctor issue
rockspore Mar 1, 2025
f7244a8
add tests
rockspore Mar 3, 2025
618c321
fix tests
rockspore Mar 3, 2025
4c739ea
fix tests
rockspore Mar 3, 2025
11ce53e
fix typo and slightly more coverage
rockspore Mar 3, 2025
19920fa
make the getter dumb
rockspore Mar 5, 2025
255dcee
fix tests
rockspore Mar 5, 2025
f8232ca
address comment
rockspore Mar 5, 2025
d8765ad
Merge branch 'main' into grpc-creds-alt
rockspore Mar 5, 2025
9ac1291
address comment
rockspore Mar 7, 2025
06afd3e
Merge branch 'main' of https://github.com/googleapis/sdk-platform-jav…
rockspore Mar 7, 2025
abb1ba3
Merge branch 'grpc-creds-alt' of github.com:rockspore/sdk-platform-ja…
rockspore Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 59 additions & 19 deletions gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public final class GrpcCallContext implements ApiCallContext {
private final ImmutableMap<String, List<String>> extraHeaders;
private final ApiCallContextOptions options;
private final EndpointContext endpointContext;
private final boolean isDirectPath;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an issue reported that isDirectPath may not be correct populated in some cases. @lqiu96 is looking into it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. I thought that referred to canUseDirectPath() which could be invoked multiple times before and after the credentials was set so might be no accurate, but by the time the ClientContext got this boolean from the channel, it should already be final.

Copy link
Contributor

@lqiu96 lqiu96 Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @surbhigarg92 regarding this to get some more information about the issue. This issue from Spanner's POV is not so much that the canUseDirectPath value that is client is initialized with and used by the TransportChannel is incorrect, it's that Spanner doesn't have a way to get the canUseDirectPath value when initializing the client for Otel.

Spanner was using GrpcSpannerStub.create(StubSettings) and didn't have an easy way to access to TransportChannel's fields (StubSettings only exposes getTransportChannelProvider(). Spanner could use GrpcSpannerStub.create(ClientContext), but that creates the Stub with a new StubSettings and not the one they manually configured.

For them, the DirectPath transportchannel was always created correctly and the value the client uses is correct. They used this workaround to be able to access the field for their use case.

I think their issue is a valid concern, but is different from this.


/** Returns an empty instance with a null channel and default {@link CallOptions}. */
public static GrpcCallContext createDefault() {
Expand All @@ -111,7 +112,8 @@ public static GrpcCallContext createDefault() {
ApiCallContextOptions.getDefaultOptions(),
null,
null,
null);
null,
false);
}

/** Returns an instance with the given channel and {@link CallOptions}. */
Expand All @@ -128,7 +130,8 @@ public static GrpcCallContext of(Channel channel, CallOptions callOptions) {
ApiCallContextOptions.getDefaultOptions(),
null,
null,
null);
null,
false);
}

private GrpcCallContext(
Expand All @@ -143,10 +146,14 @@ private GrpcCallContext(
ApiCallContextOptions options,
@Nullable RetrySettings retrySettings,
@Nullable Set<StatusCode.Code> retryableCodes,
@Nullable EndpointContext endpointContext) {
@Nullable EndpointContext endpointContext,
boolean isDirectPath) {
this.channel = channel;
this.credentials = credentials;
this.callOptions = Preconditions.checkNotNull(callOptions);
Preconditions.checkNotNull(callOptions);
// CallCredentials is stripped from CallOptions because CallCredentials are attached
// to ChannelCredentials in DirectPath flows. Adding it again would duplicate the headers.
this.callOptions = isDirectPath ? callOptions.withCallCredentials(null) : callOptions;
this.timeout = timeout;
this.streamWaitTimeout = streamWaitTimeout;
this.streamIdleTimeout = streamIdleTimeout;
Expand All @@ -159,6 +166,7 @@ private GrpcCallContext(
// a valid EndpointContext with user configurations after the client has been initialized.
this.endpointContext =
endpointContext == null ? EndpointContext.getDefaultInstance() : endpointContext;
this.isDirectPath = isDirectPath;
}

/**
Expand Down Expand Up @@ -199,7 +207,8 @@ public GrpcCallContext withCredentials(Credentials newCredentials) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@Override
Expand All @@ -210,7 +219,20 @@ public GrpcCallContext withTransportChannel(TransportChannel inputChannel) {
"Expected GrpcTransportChannel, got " + inputChannel.getClass().getName());
}
GrpcTransportChannel transportChannel = (GrpcTransportChannel) inputChannel;
return withChannel(transportChannel.getChannel());
return new GrpcCallContext(
transportChannel.getChannel(),
credentials,
callOptions,
timeout,
streamWaitTimeout,
streamIdleTimeout,
channelAffinity,
extraHeaders,
options,
retrySettings,
retryableCodes,
endpointContext,
transportChannel.isDirectPath());
}

@Override
Expand All @@ -228,7 +250,8 @@ public GrpcCallContext withEndpointContext(EndpointContext endpointContext) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unfortunate that we have to change all the places that use the constructor. Ideally, if we had a builder for GrpcCallContext, the code here would be simplified to this.toBuilder().setEndpointContext(endpointContext).build(), and we don't have to change the code here at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. But I'd prefer to leave the refactoring with a builder out of this PR since it would look cleaner. I can open an issue for it. Let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Yes please create a separate issue and we can put it in our backlog.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Created #3681

isDirectPath);
}

/** This method is obsolete. Use {@link #withTimeoutDuration(java.time.Duration)} instead. */
Expand Down Expand Up @@ -262,7 +285,8 @@ public GrpcCallContext withTimeoutDuration(@Nullable java.time.Duration timeout)
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

/** This method is obsolete. Use {@link #getTimeoutDuration()} instead. */
Expand Down Expand Up @@ -310,7 +334,8 @@ public GrpcCallContext withStreamWaitTimeoutDuration(
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

/**
Expand Down Expand Up @@ -344,7 +369,8 @@ public GrpcCallContext withStreamIdleTimeoutDuration(
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@BetaApi("The surface for channel affinity is not stable yet and may change in the future.")
Expand All @@ -361,7 +387,8 @@ public GrpcCallContext withChannelAffinity(@Nullable Integer affinity) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@BetaApi("The surface for extra headers is not stable yet and may change in the future.")
Expand All @@ -382,7 +409,8 @@ public GrpcCallContext withExtraHeaders(Map<String, List<String>> extraHeaders)
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@Override
Expand All @@ -404,7 +432,8 @@ public GrpcCallContext withRetrySettings(RetrySettings retrySettings) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@Override
Expand All @@ -426,7 +455,8 @@ public GrpcCallContext withRetryableCodes(Set<StatusCode.Code> retryableCodes) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

@Override
Expand Down Expand Up @@ -456,6 +486,8 @@ public ApiCallContext merge(ApiCallContext inputCallContext) {
newDeadline = callOptions.getDeadline();
}

boolean newIsDirectPath = grpcCallContext.isDirectPath;

CallCredentials newCallCredentials = grpcCallContext.callOptions.getCredentials();
if (newCallCredentials == null) {
newCallCredentials = callOptions.getCredentials();
Expand Down Expand Up @@ -525,7 +557,8 @@ public ApiCallContext merge(ApiCallContext inputCallContext) {
newOptions,
newRetrySettings,
newRetryableCodes,
endpointContext);
endpointContext,
newIsDirectPath);
}

/** The {@link Channel} set on this context. */
Expand Down Expand Up @@ -588,7 +621,11 @@ public Map<String, List<String>> getExtraHeaders() {
return extraHeaders;
}

/** Returns a new instance with the channel set to the given channel. */
/**
* This method is obsolete. Use {@link #withTransportChannel()} instead. Returns a new instance
* with the channel set to the given channel.
*/
@ObsoleteApi("Use withTransportChannel() instead")
public GrpcCallContext withChannel(Channel newChannel) {
return new GrpcCallContext(
newChannel,
Expand All @@ -602,7 +639,8 @@ public GrpcCallContext withChannel(Channel newChannel) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

/** Returns a new instance with the call options set to the given call options. */
Expand All @@ -619,7 +657,8 @@ public GrpcCallContext withCallOptions(CallOptions newCallOptions) {
options,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

public GrpcCallContext withRequestParamsDynamicHeaderOption(String requestParams) {
Expand Down Expand Up @@ -663,7 +702,8 @@ public <T> GrpcCallContext withOption(Key<T> key, T value) {
newOptions,
retrySettings,
retryableCodes,
endpointContext);
endpointContext,
isDirectPath);
}

/** {@inheritDoc} */
Expand Down
42 changes: 42 additions & 0 deletions gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,14 @@
import com.google.api.gax.rpc.testing.FakeTransportChannel;
import com.google.api.gax.tracing.ApiTracer;
import com.google.auth.Credentials;
import com.google.auth.oauth2.GoogleCredentials;
import com.google.common.collect.ImmutableMap;
import com.google.common.truth.Truth;
import io.grpc.CallCredentials;
import io.grpc.CallOptions;
import io.grpc.ManagedChannel;
import io.grpc.Metadata.Key;
import io.grpc.auth.MoreCallCredentials;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -100,6 +103,26 @@ void testWithTransportChannelWrongType() {
}
}

@Test
void testWithTransportChannelIsDirectPath() {
ManagedChannel channel = Mockito.mock(ManagedChannel.class);
Credentials credentials = Mockito.mock(GoogleCredentials.class);
GrpcCallContext context = GrpcCallContext.createDefault().withCredentials(credentials);
assertNotNull(context.getCallOptions().getCredentials());
context =
context.withTransportChannel(
GrpcTransportChannel.newBuilder()
.setDirectPath(true)
.setManagedChannel(channel)
.build());
assertNull(context.getCallOptions().getCredentials());

// Call credentials from the call options will be stripped.
context.withCallOptions(
CallOptions.DEFAULT.withCallCredentials(MoreCallCredentials.from(credentials)));
assertNull(context.getCallOptions().getCredentials());
}

@Test
void testMergeWrongType() {
try {
Expand Down Expand Up @@ -320,6 +343,25 @@ void testMergeWithCustomCallOptions() {
.isEqualTo(ctx2.getCallOptions().getOption(key));
}

@Test
void testMergeWithIsDirectPath() {
ManagedChannel channel = Mockito.mock(ManagedChannel.class);
CallCredentials callCredentials = Mockito.mock(CallCredentials.class);
GrpcCallContext ctx1 =
GrpcCallContext.createDefault()
.withCallOptions(CallOptions.DEFAULT.withCallCredentials(callCredentials));
GrpcCallContext ctx2 =
GrpcCallContext.createDefault()
.withTransportChannel(
GrpcTransportChannel.newBuilder()
.setDirectPath(true)
.setManagedChannel(channel)
.build());

GrpcCallContext merged = (GrpcCallContext) ctx1.merge(ctx2);
assertNull(merged.getCallOptions().getCredentials());
}

@Test
void testWithExtraHeaders() {
Map<String, List<String>> extraHeaders =
Expand Down
Loading