Skip to content

googleapis: DirectPath over Interconnect #12760

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

Open
shivaspeaks wants to merge 8 commits into
base: master
Choose a base branch
from
+186 −81
Open

googleapis: DirectPath over Interconnect #12760

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
72b24f6
direct-path-interconnect
shivaspeaks Apr 15, 2026
9b65e38
direct-path-interconnect
shivaspeaks Apr 15, 2026
3aaa6ed
merge from master
shivaspeaks May 7, 2026
a52050e
rework with QueryParams
shivaspeaks May 7, 2026
45cf39c
use one bootstrap file
shivaspeaks May 7, 2026
39bce96
resolve comments
shivaspeaks May 7, 2026
8106aae
remove empty ampersand edge case
shivaspeaks May 7, 2026
20bf74a
use QueryParams instance to handle target
shivaspeaks May 8, 2026
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
125 changes: 90 additions & 35 deletions googleapis/src/main/java/io/grpc/googleapis/GoogleCloudToProdNameResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.grpc.MetricRecorder;
import io.grpc.NameResolver;
import io.grpc.NameResolverRegistry;
import io.grpc.QueryParams;
import io.grpc.Status;
import io.grpc.SynchronizationContext;
import io.grpc.Uri;
Expand All @@ -47,7 +48,6 @@
import java.io.Reader;
import java.net.HttpURLConnection;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.List;
Expand Down Expand Up @@ -76,23 +76,45 @@ final class GoogleCloudToProdNameResolver extends NameResolver {
private static final String serverUriOverride =
System.getenv("GRPC_TEST_ONLY_GOOGLE_C2P_RESOLVER_TRAFFIC_DIRECTOR_URI");

@GuardedBy("GoogleCloudToProdNameResolver.class")
private static final Object BOOTSTRAP_LOCK = new Object();
Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

Revert?

private static final Object FORCE_XDS_BOOTSTRAP_LOCK = new Object();

@GuardedBy("BOOTSTRAP_LOCK")
private static BootstrapInfo bootstrapInfo;
@GuardedBy("FORCE_XDS_BOOTSTRAP_LOCK")
private static BootstrapInfo forceXdsBootstrapInfo;
private static HttpConnectionProvider httpConnectionProvider = HttpConnectionFactory.INSTANCE;
private static int c2pId = new Random().nextInt();

private static synchronized BootstrapInfo getBootstrapInfo()
private static BootstrapInfo getBootstrapInfo(boolean isForcedXds)
Copy link
Copy Markdown
Member

@ejona86 ejona86 Apr 21, 2026

Choose a reason for hiding this comment

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

The design didn't call this out. We need to decode if it is "first one wins" or we need two different bootstraps. I've added a comment on the design.

Copy link
Copy Markdown
Member Author

@shivaspeaks shivaspeaks May 6, 2026

Choose a reason for hiding this comment

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

Per Mark's thoughts on the design, they'll share the bootstrap file and the first one wins? So we're good with the current implementation for bootstrapInfo right?

We are stripping the force-xds parameter from the URI before passing it to the pool (InternalSharedXdsClientPoolProvider) and the target strings will be identical. So whichhever channel starts first will set the "winning" bootstrap configuration.

Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

The current implementation does not share and does not have a "first one wins" behavior. We use two different variables to store the caches... they are completely separate.

Copy link
Copy Markdown
Member Author

@shivaspeaks shivaspeaks May 7, 2026

Choose a reason for hiding this comment

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

Oh, I was assuming that finalBootstrapInfo is what we cared about which will set the bootstrap config in sharedXdsClientPoolProvider.

I'll update to use one single file here as well, that will obviously make it simpler having one lock.

Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

Oh, that's fair, although the changes here no longer served any purpose.

throws XdsInitializationException, IOException {
if (bootstrapInfo != null) {
return bootstrapInfo;
}
BootstrapInfo bootstrapInfoTmp =
InternalGrpcBootstrapperImpl.parseBootstrap(generateBootstrap());
// Avoid setting global when testing
if (httpConnectionProvider == HttpConnectionFactory.INSTANCE) {
bootstrapInfo = bootstrapInfoTmp;
if (isForcedXds) {
synchronized (FORCE_XDS_BOOTSTRAP_LOCK) {
if (forceXdsBootstrapInfo != null) {
return forceXdsBootstrapInfo;
}
BootstrapInfo newInfo = InternalGrpcBootstrapperImpl.parseBootstrap(
generateBootstrap("", true, true));
// Avoid setting global when testing
if (httpConnectionProvider == HttpConnectionFactory.INSTANCE) {
forceXdsBootstrapInfo = newInfo;
}
return newInfo;
}
} else {
synchronized (BOOTSTRAP_LOCK) {
if (bootstrapInfo != null) {
return bootstrapInfo;
}
BootstrapInfo newInfo = InternalGrpcBootstrapperImpl.parseBootstrap(
generateBootstrap());
// Avoid setting global when testing
if (httpConnectionProvider == HttpConnectionFactory.INSTANCE) {
bootstrapInfo = newInfo;
}
return newInfo;
}
}
return bootstrapInfoTmp;
}

private final String authority;
Expand All @@ -102,7 +124,8 @@ private static synchronized BootstrapInfo getBootstrapInfo()
private final MetricRecorder metricRecorder;
private final NameResolver delegate;
private final boolean usingExecutorResource;
private final String schemeOverride = !isOnGcp ? "dns" : "xds";
private final boolean forceXds;
private final String schemeOverride;
private XdsClientResult xdsClientPool;
private XdsClient xdsClient;
private Executor executor;
Expand All @@ -122,16 +145,32 @@ private static synchronized BootstrapInfo getBootstrapInfo()
NameResolver.Factory nameResolverFactory) {
this.executorResource = checkNotNull(executorResource, "executorResource");
String targetPath = checkNotNull(checkNotNull(targetUri, "targetUri").getPath(), "targetPath");
Uri grpcUri = Uri.create(targetUri.toString());
String query = grpcUri.getRawQuery();
this.forceXds = checkForceXds(query);
this.schemeOverride = (forceXds || isOnGcp) ? "xds" : "dns";
String newQuery = stripForceXds(query);

Preconditions.checkArgument(
targetPath.startsWith("/"),
"the path component (%s) of the target (%s) must start with '/'",
targetPath,
targetUri);
authority = GrpcUtil.checkAuthority(targetPath.substring(1));
syncContext = checkNotNull(args, "args").getSynchronizationContext();
targetUri = overrideUriScheme(targetUri, schemeOverride);

Uri.Builder modifiedTargetBuilder = grpcUri.toBuilder().setScheme(schemeOverride);
if (newQuery != null) {
modifiedTargetBuilder.setRawQuery(newQuery);
} else {
modifiedTargetBuilder.setRawQuery(null);
}
if (schemeOverride.equals("xds")) {
modifiedTargetBuilder.setRawAuthority(C2P_AUTHORITY);
}
targetUri = URI.create(modifiedTargetBuilder.build().toString());

if (schemeOverride.equals("xds")) {
targetUri = overrideUriAuthority(targetUri, C2P_AUTHORITY);
args = args.toBuilder()
.setArg(XdsNameResolverProvider.XDS_CLIENT_SUPPLIER, () -> xdsClient)
.build();
Expand All @@ -155,6 +194,11 @@ private static synchronized BootstrapInfo getBootstrapInfo()
Resource<Executor> executorResource,
NameResolver.Factory nameResolverFactory) {
this.executorResource = checkNotNull(executorResource, "executorResource");
String query = targetUri.getRawQuery();
Copy link
Copy Markdown
Member

@jdcormie jdcormie May 7, 2026

Choose a reason for hiding this comment

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

Consider working with the query component as an instance of QueryParams rather than a string. That way you can parse it just once here, then read/modify it in both checkForceXds() and stripForceXds() without parsing it again.

this.forceXds = checkForceXds(query);
this.schemeOverride = (forceXds || isOnGcp) ? "xds" : "dns";
String newQuery = stripForceXds(query);

Preconditions.checkArgument(
targetUri.isPathAbsolute(),
"the path component of the target (%s) must start with '/'",
Expand All @@ -167,6 +211,12 @@ private static synchronized BootstrapInfo getBootstrapInfo()
authority = GrpcUtil.checkAuthority(pathSegments.get(0));
syncContext = checkNotNull(args, "args").getSynchronizationContext();
Uri.Builder modifiedTargetBuilder = targetUri.toBuilder().setScheme(schemeOverride);
if (newQuery != null) {
modifiedTargetBuilder.setRawQuery(newQuery);
} else {
modifiedTargetBuilder.setRawQuery(null);
}

if (schemeOverride.equals("xds")) {
modifiedTargetBuilder.setRawAuthority(C2P_AUTHORITY);
args =
Expand Down Expand Up @@ -226,7 +276,7 @@ class Resolve implements Runnable {
public void run() {
BootstrapInfo bootstrapInfo = null;
try {
bootstrapInfo = getBootstrapInfo();
bootstrapInfo = getBootstrapInfo(forceXds);
} catch (IOException e) {
listener.onError(
Status.INTERNAL.withDescription("Unable to get metadata").withCause(e));
Expand Down Expand Up @@ -263,16 +313,18 @@ public void run() {
static ImmutableMap<String, ?> generateBootstrap() throws IOException {
return generateBootstrap(
queryZoneMetadata(METADATA_URL_ZONE),
queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6));
queryIpv6SupportMetadata(METADATA_URL_SUPPORT_IPV6), false);
}

private static ImmutableMap<String, ?> generateBootstrap(String zone, boolean supportIpv6) {
static ImmutableMap<String, ?> generateBootstrap(
String zone, boolean supportIpv6, boolean isForcedXds) {
ImmutableMap.Builder<String, Object> nodeBuilder = ImmutableMap.builder();
nodeBuilder.put("id", "C2P-" + (c2pId & Integer.MAX_VALUE));
if (!zone.isEmpty()) {
String nodeIdPrefix = isOnGcp ? "C2P-" : "C2P-non-gcp-";
nodeBuilder.put("id", nodeIdPrefix + (c2pId & Integer.MAX_VALUE));
if (!isForcedXds && !zone.isEmpty()) {
Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

The zone is guaranteed to be empty when isForcedXds=true. And that is important, because we need to make sure not to look up the zone when forcing xds. Same for supportIpv6. We shouldn't have these conditions, because that should be handled before we get to this function. The isForcedXds argument should be removed. This should also return to being private, because the tests calling this directly don't actually test anything.

nodeBuilder.put("locality", ImmutableMap.of("zone", zone));
}
if (supportIpv6) {
if (isForcedXds || supportIpv6) {
nodeBuilder.put("metadata",
ImmutableMap.of("TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE", true));
}
Expand Down Expand Up @@ -373,24 +425,27 @@ static void setC2pId(int c2pId) {
GoogleCloudToProdNameResolver.c2pId = c2pId;
}

private static URI overrideUriScheme(URI uri, String scheme) {
URI res;
try {
res = new URI(scheme, uri.getAuthority(), uri.getPath(), uri.getQuery(), uri.getFragment());
} catch (URISyntaxException ex) {
throw new IllegalArgumentException("Invalid scheme: " + scheme, ex);
private static boolean checkForceXds(String query) {
if (query == null) {
return false;
}
return res;
QueryParams params = QueryParams.fromRawQuery(query);
for (QueryParams.Entry entry : params.asList()) {
if ("force-xds".equals(entry.getKey())) {
return true;
}
}
return false;
}

private static URI overrideUriAuthority(URI uri, String authority) {
URI res;
try {
res = new URI(uri.getScheme(), authority, uri.getPath(), uri.getQuery(), uri.getFragment());
} catch (URISyntaxException ex) {
throw new IllegalArgumentException("Invalid authority: " + authority, ex);
private static String stripForceXds(String query) {
if (query == null) {
return null;
}
return res;
QueryParams params = QueryParams.fromRawQuery(query);
params.asList().removeIf(entry -> "force-xds".equals(entry.getKey()));
params.asList().removeIf(entry -> entry.getKey().isEmpty() && !entry.hasValue());
Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

I don't think we need this line. It looks like it is to encourage empty query parameters to become null query parameters, but in what situation do we need that? If we just had ?force-xds, then removing the force-xds entry above would turn it into null query parameters.

Copy link
Copy Markdown
Member Author

@shivaspeaks shivaspeaks May 7, 2026

Choose a reason for hiding this comment

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

?&force-xds&

Check the testcase notOnGcpButForceXds_EdgeCaseAmpersands_DelegateToXds()

Copy link
Copy Markdown
Member

@ejona86 ejona86 May 7, 2026

Choose a reason for hiding this comment

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

What value is that providing? That seems unrelated to force-xds.

Copy link
Copy Markdown
Member Author

@shivaspeaks shivaspeaks May 7, 2026

Choose a reason for hiding this comment

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

Sure, I'll remove this edge case.

return params.toRawQuery();
}

private enum HttpConnectionFactory implements HttpConnectionProvider {
Expand Down
Loading
Loading