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 2 commits into
base: master
Choose a base branch
from
+333 −36
Open

googleapis: DirectPath over Interconnect #12760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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
2 changes: 1 addition & 1 deletion api/src/main/java/io/grpc/Uri.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ public Builder setQuery(@Nullable String query) {
}

@CanIgnoreReturnValue
Builder setRawQuery(String query) {
public Builder setRawQuery(String query) {
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.

@jdcormie, setQuery() is another case where the auto-encoding/decoding seems to cause trouble, and it seems people shouldn't actually call the non-raw version of the method. I feel like we'll need to remove many of these before we stabilize the API (basically any of them which say "NB: This method's decoding is lossy"). In this case, it seems people probably shouldn't even use setQuery(), because the string should almost always have percent-encoding performed before combining with = and &

@shivaspeaks, let's add a comment to getQuery() saying it is lossy.

Copy link
Copy Markdown
Member

@jdcormie jdcormie Apr 21, 2026

Choose a reason for hiding this comment

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

Hmm, I'm not totally sure what you mean. I have two types of javadoc NBs io.grpc.Uri:

  1. Warnings about ambiguous percent decoding. Case 1 is when getPath() returns "foo/bar", you can't know whether this is a single path segment "foo/bar" or two path segments ["foo", "bar"]. Case 2 is getAuthority() where you can't know whether a return value of "x@y@z" means a userinfo of "x" or "x@y".
  2. Warnings about our baked-in assumption of that percent-encoded octets should be turned into characters using UTF-8.

The (1) methods have unambiguous alternatives so yes we could remove them. They exist because I didn't want to make handling these rare edge cases a prerequisite to migrating from java.net.URI. Many callers (like our NRPs) just don't care or somehow know externally that these edge cases can't happen. I agree that we should at least mark them as deprecated or something to raise awareness.

But getQuery()/setQuery only "suffers" from (2). That's because '=' and '&' don't actually have any special meaning in RFC 3986's definition of the query component. They can appear in the query component without percent encoding. Perhaps you are thinking of an application that's using the query component to encode (key,value) pairs like ?k1=v1&k2=v2. And to allow a key or value itself to contain a = or & character, such an app could percent-encode it (unnecessarily) and rely on the fact that io.grpc.Uri's parse/toString() roundtrip is transparent. The app could do a split(=) operation on getRawQuery() to access keys and values as they appeared in the original string version of the URI.

Anyway, all this is is fine but it exists at a layer above io.grpc.Uri -- I think setQuery() is fine just the way it is. I don't yet understand why this setter needs to become public but let me keep reading how it is used.

Copy link
Copy Markdown
Member

@ejona86 ejona86 Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I was just talking about the ambiguous decoding. getAuthority() has "NB: This method's decoding is lossy -- It only exists for compatibility with {@ link java.net.URI}." getPath() has "NB: Prefer {@ link #getPathSegments()} because this method's decoding is lossy." That's what I meant by "decoding is lossy."

The problems with all lossy cases is the parsing isn't complete when the percent decoding is performed. For query strings, even though it isn't part of the RFC, everyone uses = and & and even if not that, it will certainly be in some encoded form. I don't think it is safe to automatically encode/decode query, as essentially nobody should be consuming it that way. You must finish parsing the query string before percent decoding. And conversely, you must percent-encode while constructing the query string and setQuery() will mangle such a string.

They exist because I didn't want to make handling these rare edge cases a prerequisite to migrating from java.net.URI.

That's fair, but it doesn't apply to setQuery/getQuery. Although I do see from the URI RFC syntax perspective how that would be true, that's just because all the URI RFCs are bad. I'm auditing some pre-existing cases now, and especially for getAuthority() it seems many of them should have actually been using the raw version already. /sigh

Copy link
Copy Markdown
Member

@jdcormie jdcormie Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

OK I see now why you need Uri.Builder#setRawQuery(). We should make it public as y'all propose here. But I do suggest keeping io.grpc.Uri limited to concepts in RFC 3986. Query parameters can be implemented as a layer on top of io.grpc.Uri. Please have a look at #12772 to see what I mean.

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.

@jdcormie, seems okay for query to be made @Nullable, right? It just didn't need it previously because it wasn't exposed?

Copy link
Copy Markdown
Member

@jdcormie jdcormie Apr 21, 2026

Choose a reason for hiding this comment

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

If we truly need setRawQuery() to be public, then yes, query should be @Nullable and null means "clear this component," just like how setRawAuthority works.

Copy link
Copy Markdown
Member

@jdcormie jdcormie Apr 21, 2026

Choose a reason for hiding this comment

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

As a public method it would also need javadoc.

Copy link
Copy Markdown
Member

@ejona86 ejona86 Apr 22, 2026

Choose a reason for hiding this comment

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

@shivaspeaks, go ahead and make it nullable in this PR, which then lets you stop calling setQuery().

checkPercentEncodedArg(query, "query", queryChars);
this.query = query;
return this;
Expand Down
134 changes: 99 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 @@ -20,6 +20,7 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.CharStreams;
Expand Down Expand Up @@ -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();
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.

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.setQuery(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();
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.setQuery(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()) {
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,36 @@ 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;
for (String part : Splitter.on('&').split(query)) {
if (part.equals("force-xds")) {
return true;
}
if (part.startsWith("force-xds=")) {
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.

There is no value defined for this. Don't go making up your own, especially giving multiple aliases (false vs 0), as we need it to behave the same across languages. If it were necessary, we'd need to update the design so that all languages behaved the same. Since it doesn't seem necessary, delete this.

String value = part.substring("force-xds=".length());
return !value.equalsIgnoreCase("false") && !value.equals("0");
}
}
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;
}
StringBuilder sb = new StringBuilder();
for (String part : Splitter.on('&').split(query)) {
if (!part.equals("force-xds") && !part.startsWith("force-xds=")) {
if (sb.length() > 0) {
sb.append("&");
}
sb.append(part);
}
}
return res;
return sb.length() == 0 ? null : sb.toString();
}

private enum HttpConnectionFactory implements HttpConnectionProvider {
Expand Down
Loading
Loading