Skip to content

googleapis: DirectPath over Interconnect#12760

Open
shivaspeaks wants to merge 2 commits intogrpc:masterfrom
shivaspeaks:directpath-over-interconnect
Open

googleapis: DirectPath over Interconnect#12760
shivaspeaks wants to merge 2 commits intogrpc:masterfrom
shivaspeaks:directpath-over-interconnect

Conversation

@shivaspeaks
Copy link
Copy Markdown
Member

@shivaspeaks shivaspeaks commented Apr 15, 2026

Implements go/directpath-interconnect-client

CC: @anicr7

@shivaspeaks
Copy link
Copy Markdown
Member Author

shivaspeaks commented Apr 15, 2026

The following changes are made outside of the design doc specifications or assuming few things:

  • Node ID Suffix and UUID Size: While the design document suggests a 64-bit integer for the UUID, this PR maintains the existing 32-bit int (generated via Random().nextInt()) to remain consistent with the current implementation's ID generation logic.

  • Ampersand Decoding (%26): The resolver now utilizes getRawQuery() instead of getQuery() for detecting and stripping the force-xds flag. This approach prevents a "splitting bug" where an encoded ampersand (%26) in a parameter value would be decoded to a literal & and incorrectly interpreted as a parameter separator.

  • Separate Locks for Bootstrap Caching: I have introduced separate locks (BOOTSTRAP_LOCK and FORCE_XDS_BOOTSTRAP_LOCK) for managing the standard and forced xDS bootstrap configurations. This provides logical isolation and ensures that Interconnect clients using force-xds can initialize immediately without being blocked by threads waiting on potentially slow metadata server calls in the standard GCP detection path.

  • URI Normalization via io.grpc.Uri: Even in the legacy java.net.URI constructor, we now use io.grpc.Uri to perform the transformation. This ensures consistent behavior across both constructors, particularly regarding "empty" vs "absent" authorities. Using the gRPC-specific Uri class ensures that hierarchical URIs like google-c2p:///svc (empty authority) are correctly reconstructed as dns:///svc or xds://.../svc rather than being mangled into invalid formats.

  • Flexible Query Param Value Handling and Case Sensitivity for force-xds: The implementation treats the force-xds parameter key as case-sensitive to align with standard gRPC naming conventions. However, it provides flexible support for different value formats: the resolver recognizes both the standalone flag (?force-xds) and explicit key-value pairs like ?force-xds=true or ?force-xds=1. To avoid misinterpretation, the logic explicitly returns false only when the value is a case-insensitive match for "false" or is exactly "0", ensuring that common truthy values like true or 1 are correctly handled as enabled

@shivaspeaks shivaspeaks marked this pull request as ready for review April 15, 2026 09:39
@ejona86 ejona86 requested a review from jdcormie April 21, 2026 20:15

@CanIgnoreReturnValue
Builder setRawQuery(String query) {
public Builder setRawQuery(String query) {
Copy link
Copy Markdown
Member

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

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

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

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.

private static int c2pId = new Random().nextInt();

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

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.

if (part.equals("force-xds")) {
return true;
}
if (part.startsWith("force-xds=")) {
Copy link
Copy Markdown
Member

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.


@CanIgnoreReturnValue
Builder setRawQuery(String query) {
public Builder setRawQuery(String query) {
Copy link
Copy Markdown
Member

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

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

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

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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants