Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: add support for custom, per-target credentials on the transport. #11919

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
50 changes: 36 additions & 14 deletions xds/src/main/java/io/grpc/xds/GrpcXdsTransportFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.annotations.VisibleForTesting;
import io.grpc.CallCredentials;
import io.grpc.CallOptions;
import io.grpc.ChannelCredentials;
import io.grpc.ClientCall;
Expand All @@ -35,22 +36,33 @@
final class GrpcXdsTransportFactory implements XdsTransportFactory {

static final GrpcXdsTransportFactory DEFAULT_XDS_TRANSPORT_FACTORY =
Copy link
Member

Choose a reason for hiding this comment

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

Move this into the tests that use it? It isn't used any more in production. (I probably wouldn't make it a CONSTANT like it is here, at it is no longer the DEFAULT and such. I'd probably just create a new instance when needed.)

new GrpcXdsTransportFactory();
new GrpcXdsTransportFactory(null);

private final CallCredentials callCredentials;

GrpcXdsTransportFactory(CallCredentials callCredentials) {
this.callCredentials = callCredentials;
}

@Override
public XdsTransport create(Bootstrapper.ServerInfo serverInfo) {
return new GrpcXdsTransport(serverInfo);
GrpcXdsTransport transport = new GrpcXdsTransport(serverInfo);
transport.setCallCredentials(callCredentials);
return transport;
}

@VisibleForTesting
public XdsTransport createForTest(ManagedChannel channel) {
return new GrpcXdsTransport(channel);
GrpcXdsTransport transport = new GrpcXdsTransport(channel);
transport.setCallCredentials(callCredentials);
return transport;
}

@VisibleForTesting
static class GrpcXdsTransport implements XdsTransport {

private final ManagedChannel channel;
private CallCredentials callCredentials;
Copy link
Member

Choose a reason for hiding this comment

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

final


public GrpcXdsTransport(Bootstrapper.ServerInfo serverInfo) {
String target = serverInfo.target();
Expand All @@ -65,14 +77,19 @@ public GrpcXdsTransport(ManagedChannel channel) {
this.channel = checkNotNull(channel, "channel");
}

void setCallCredentials(CallCredentials callCredentials) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass this in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced the setter with a CallCredentials parameter in the constructors. I originally used a setter here to avoid impacting GrpcXdsTransport constructor callsites (e.g. across tests), but I have updated those now.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, you could have left the old constructors in place and them just becoming something like this(serverInfo, null). If there's only a few callsites, it is nice to update the old callsites with the new parameters. But if there are a lot, then leaving the old constructors in place is easy and simple.

this.callCredentials = callCredentials;
}

@Override
public <ReqT, RespT> StreamingCall<ReqT, RespT> createStreamingCall(
String fullMethodName,
MethodDescriptor.Marshaller<ReqT> reqMarshaller,
MethodDescriptor.Marshaller<RespT> respMarshaller) {
Context prevContext = Context.ROOT.attach();
try {
return new XdsStreamingCall<>(fullMethodName, reqMarshaller, respMarshaller);
return new XdsStreamingCall<>(
fullMethodName, reqMarshaller, respMarshaller, callCredentials);
} finally {
Context.ROOT.detach(prevContext);
}
Expand All @@ -89,16 +106,21 @@ private class XdsStreamingCall<ReqT, RespT> implements

private final ClientCall<ReqT, RespT> call;

public XdsStreamingCall(String methodName, MethodDescriptor.Marshaller<ReqT> reqMarshaller,
MethodDescriptor.Marshaller<RespT> respMarshaller) {
this.call = channel.newCall(
MethodDescriptor.<ReqT, RespT>newBuilder()
.setFullMethodName(methodName)
.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
.setRequestMarshaller(reqMarshaller)
.setResponseMarshaller(respMarshaller)
.build(),
CallOptions.DEFAULT); // TODO(zivy): support waitForReady
public XdsStreamingCall(
String methodName,
MethodDescriptor.Marshaller<ReqT> reqMarshaller,
MethodDescriptor.Marshaller<RespT> respMarshaller,
CallCredentials callCredentials) {
this.call =
channel.newCall(
MethodDescriptor.<ReqT, RespT>newBuilder()
.setFullMethodName(methodName)
.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
.setRequestMarshaller(reqMarshaller)
.setResponseMarshaller(respMarshaller)
.build(),
CallOptions.DEFAULT.withCallCredentials(
callCredentials)); // TODO(zivy): support waitForReady
}

@Override
Expand Down
27 changes: 16 additions & 11 deletions xds/src/main/java/io/grpc/xds/SharedXdsClientPoolProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
package io.grpc.xds;

import static com.google.common.base.Preconditions.checkNotNull;
import static io.grpc.xds.GrpcXdsTransportFactory.DEFAULT_XDS_TRANSPORT_FACTORY;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.concurrent.GuardedBy;
import io.grpc.CallCredentials;
import io.grpc.MetricRecorder;
import io.grpc.internal.ExponentialBackoffPolicy;
import io.grpc.internal.GrpcUtil;
Expand Down Expand Up @@ -153,16 +153,21 @@ public XdsClient getObject() {
}
scheduler = SharedResourceHolder.get(GrpcUtil.TIMER_SERVICE);
metricReporter = new XdsClientMetricReporterImpl(metricRecorder, target);
xdsClient = new XdsClientImpl(
DEFAULT_XDS_TRANSPORT_FACTORY,
bootstrapInfo,
scheduler,
BACKOFF_POLICY_PROVIDER,
GrpcUtil.STOPWATCH_SUPPLIER,
TimeProvider.SYSTEM_TIME_PROVIDER,
MessagePrinter.INSTANCE,
new TlsContextManagerImpl(bootstrapInfo),
metricReporter);
CallCredentials transportCallCreds =
XdsTransportCallCredentialsProvider.getTransportCallCredentials(target);
GrpcXdsTransportFactory xdsTransportFactory =
new GrpcXdsTransportFactory(transportCallCreds);
xdsClient =
new XdsClientImpl(
xdsTransportFactory,
bootstrapInfo,
scheduler,
BACKOFF_POLICY_PROVIDER,
GrpcUtil.STOPWATCH_SUPPLIER,
TimeProvider.SYSTEM_TIME_PROVIDER,
MessagePrinter.INSTANCE,
new TlsContextManagerImpl(bootstrapInfo),
metricReporter);
metricReporter.setXdsClient(xdsClient);
}
refCount++;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright 2025 The gRPC Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.grpc.xds;

import io.grpc.CallCredentials;
import io.grpc.Internal;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

/**
* Stores custom call credentials to use on the xDS transport for specific targets, and supplies
* them to the xDS client.
*/
@Internal
public final class XdsTransportCallCredentialsProvider {
private static final Map<String, CallCredentials> xdsTransportCallCredentials =
new ConcurrentHashMap<>();

public static CallCredentials getTransportCallCredentials(String target) {
return xdsTransportCallCredentials.get(target);
}

/**
* Registers a custom {@link CallCredentials} that is to be used on the xDS transport when
* resolving the given target. Must be called before the xDS client for the target is created
* (i.e. before the application creates a channel to the target).
*
* <p>A custom {@code CallCredentials} can only be set once on the xDS transport; in other words,
* it is not possible to change the custom transport {@code CallCredentials} for an existing xDS
* client. If {@code setTransportCallCredentials} is called when there is already an existing xDS
* client for the target, then this does nothing and returns false.
Copy link
Member

Choose a reason for hiding this comment

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

The false return is not reliable. So we should weaken the language here a bit to say it "attempts" or "tries" to return false, or say it is "best effort".

Are the callers actually going to use the return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, reworded the javadoc.

The return value is informational for callers to know whether their custom transport per-RPC credential is actually being used. A false return value gives clients the choice to handle it in any way - e.g. print a warning log, fail, or ignore.

*/
public static boolean setTransportCallCredentials(String target, CallCredentials creds) {
if (SharedXdsClientPoolProvider.getDefaultProvider().get(target) != null) {
return false;
}
xdsTransportCallCredentials.put(target, creds);
Copy link
Member

Choose a reason for hiding this comment

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

So once registered this will exist for the life of the process. Are there memory usage concerns there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated RefCountedXdsClientObjectPool to remove the (target, CallCredentials) entry when the xDS client is cleaned up at refcount 0.

Copy link
Member

Choose a reason for hiding this comment

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

Is that what we're doing in the other languages? That actually sounds racy for this configuration, as a channel could be shutting down when you configuring the CallCreds.

return true;
}

private XdsTransportCallCredentialsProvider() {}
}
Loading