- Notifications
You must be signed in to change notification settings - Fork69
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
ae471b339489bcf7244a8618c3214c739ea11ce53e19920fa255dceef8232cad8765ad9ac129106afd3eabb1ba3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Thanks for the info. I thought that referred to Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Spanner was using 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() { | ||
| @@ -111,7 +112,8 @@ public static GrpcCallContext createDefault() { | ||
| ApiCallContextOptions.getDefaultOptions(), | ||
| null, | ||
| null, | ||
| null, | ||
| false); | ||
| } | ||
| /** Returns an instance with the given channel and {@link CallOptions}. */ | ||
| @@ -128,7 +130,8 @@ public static GrpcCallContext of(Channel channel, CallOptions callOptions) { | ||
| ApiCallContextOptions.getDefaultOptions(), | ||
| null, | ||
| null, | ||
| null, | ||
| false); | ||
| } | ||
| private GrpcCallContext( | ||
| @@ -143,10 +146,14 @@ private GrpcCallContext( | ||
| ApiCallContextOptions options, | ||
| @Nullable RetrySettings retrySettings, | ||
| @Nullable Set<StatusCode.Code> retryableCodes, | ||
| @Nullable EndpointContext endpointContext, | ||
| boolean isDirectPath) { | ||
lqiu96 marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| this.channel = channel; | ||
| this.credentials = credentials; | ||
| 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; | ||
| @@ -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; | ||
| } | ||
| /** | ||
| @@ -199,7 +207,8 @@ public GrpcCallContext withCredentials(Credentials newCredentials) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @Override | ||
| @@ -210,7 +219,20 @@ public GrpcCallContext withTransportChannel(TransportChannel inputChannel) { | ||
| "Expected GrpcTransportChannel, got " + inputChannel.getClass().getName()); | ||
| } | ||
| GrpcTransportChannel transportChannel = (GrpcTransportChannel) inputChannel; | ||
| return new GrpcCallContext( | ||
| transportChannel.getChannel(), | ||
| credentials, | ||
| callOptions, | ||
| timeout, | ||
| streamWaitTimeout, | ||
| streamIdleTimeout, | ||
| channelAffinity, | ||
| extraHeaders, | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| transportChannel.isDirectPath()); | ||
| } | ||
| @Override | ||
| @@ -228,7 +250,8 @@ public GrpcCallContext withEndpointContext(EndpointContext endpointContext) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. */ | ||
| @@ -262,7 +285,8 @@ public GrpcCallContext withTimeoutDuration(@Nullable java.time.Duration timeout) | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| /** This method is obsolete. Use {@link #getTimeoutDuration()} instead. */ | ||
| @@ -310,7 +334,8 @@ public GrpcCallContext withStreamWaitTimeoutDuration( | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| /** | ||
| @@ -344,7 +369,8 @@ public GrpcCallContext withStreamIdleTimeoutDuration( | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @BetaApi("The surface for channel affinity is not stable yet and may change in the future.") | ||
| @@ -361,7 +387,8 @@ public GrpcCallContext withChannelAffinity(@Nullable Integer affinity) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @BetaApi("The surface for extra headers is not stable yet and may change in the future.") | ||
| @@ -382,7 +409,8 @@ public GrpcCallContext withExtraHeaders(Map<String, List<String>> extraHeaders) | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @Override | ||
| @@ -404,7 +432,8 @@ public GrpcCallContext withRetrySettings(RetrySettings retrySettings) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @Override | ||
| @@ -426,7 +455,8 @@ public GrpcCallContext withRetryableCodes(Set<StatusCode.Code> retryableCodes) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| @Override | ||
| @@ -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(); | ||
| @@ -525,7 +557,8 @@ public ApiCallContext merge(ApiCallContext inputCallContext) { | ||
| newOptions, | ||
| newRetrySettings, | ||
| newRetryableCodes, | ||
| endpointContext, | ||
| newIsDirectPath); | ||
| } | ||
| /** The {@link Channel} set on this context. */ | ||
| @@ -588,7 +621,11 @@ public Map<String, List<String>> getExtraHeaders() { | ||
| return extraHeaders; | ||
| } | ||
| /** | ||
| * 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, | ||
| @@ -602,7 +639,8 @@ public GrpcCallContext withChannel(Channel newChannel) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| /** Returns a new instance with the call options set to the given call options. */ | ||
| @@ -619,7 +657,8 @@ public GrpcCallContext withCallOptions(CallOptions newCallOptions) { | ||
| options, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| public GrpcCallContext withRequestParamsDynamicHeaderOption(String requestParams) { | ||
| @@ -663,7 +702,8 @@ public <T> GrpcCallContext withOption(Key<T> key, T value) { | ||
| newOptions, | ||
| retrySettings, | ||
| retryableCodes, | ||
| endpointContext, | ||
| isDirectPath); | ||
| } | ||
| /** {@inheritDoc} */ | ||
Uh oh!
There was an error while loading.Please reload this page.