feat(storage): add support for DirectPath over Interconnect - #13904
feat(storage): add support for DirectPath over Interconnect#13904nidhiii-27 wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for DirectPath xDS over Interconnect (on-premise xDS name resolution) across google-cloud-storage and gax-grpc. It adds the attemptDirectPathXdsOverInterconnect configuration, bypasses GCE environment checks when enabled, appends ?force-xds to the target URI, and updates endpoint validation to support custom URI schemes. Feedback on the changes includes restoring the accidentally removed getExecutor() method in InstantiatingGrpcChannelProvider, replacing fragile string equality checks for endpoint translation with a more robust replacement approach in GrpcStorageOptions, and simplifying redundant String.format calls in endpoint validation.
I am having trouble creating individual review comments. Click here to see my feedback.
sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java (268)
The getExecutor() method was accidentally removed from InstantiatingGrpcChannelProvider. Since this class implements TransportChannelProvider, removing this method will cause it to fallback to the interface's default implementation (which likely returns null), breaking the ability to retrieve the configured executor. Please restore this method.
@Nullable
@Override
public Executor getExecutor() {
return executor;
}java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java (235-241)
The current string equality checks for endpoint are fragile. If the host is configured with a port (e.g., storage.googleapis.com:443), a trailing slash, or other variations, the translation to storage.direct.googleapis.com will be bypassed. Using String.replace is much more robust, simpler, and handles all these cases seamlessly.
if (attemptDirectPathXdsOverInterconnect && endpoint != null) {
endpoint = endpoint.replace("storage.googleapis.com", "storage.direct.googleapis.com");
}sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java (1494-1512)
The String.format calls with no format arguments are redundant and can be simplified to direct string literals for better performance and readability. Avoid adding defensive null checks for endpoint as it is guaranteed to be non-null by design.
if (endpoint.contains(":///")) {
try {
java.net.URI.create(endpoint);
return;
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("invalid endpoint URI: " + endpoint, e);
}
}
int colon = endpoint.lastIndexOf(':');
if (colon < 0) {
throw new IllegalArgumentException("invalid endpoint, expecting \"<host>:<port>\"");
}
try {
Integer.parseInt(endpoint.substring(colon + 1));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("invalid endpoint, expecting \"<host>:<port>\"", e);
}References
- Avoid adding defensive null checks for values that are guaranteed to be non-null by design, as this can hide invariant breaks.
…erInterconnect is true [Generated-by: AI]
|
/gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for DirectPath over Interconnect (on-premise xDS name resolution) across the Java Storage and GAX gRPC libraries. It adds configuration options, endpoint rewriting logic, and bypasses Google Compute Engine (GCE) environment checks when this feature is enabled. Feedback on the changes includes making the host rewriting logic more robust to handle explicit ports or trailing slashes, and reusing a newly introduced helper method to construct default channel credentials to reduce code duplication.
| if (attemptDirectPathXdsOverInterconnect) { | ||
| if ("https://storage.googleapis.com".equals(endpoint)) { | ||
| endpoint = "https://storage.direct.googleapis.com"; | ||
| } else if ("storage.googleapis.com".equals(endpoint)) { | ||
| endpoint = "storage.direct.googleapis.com"; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current host rewriting logic only performs exact string equality checks. If a user configures the host with an explicit port (e.g., https://storage.googleapis.com:443) or a trailing slash, the endpoint will not be rewritten to the DirectPath endpoint, causing the feature to be silently bypassed. We should make the prefix check more robust to handle ports and trailing slashes while avoiding false positives (like storage.googleapis.com.example.com).
| if (attemptDirectPathXdsOverInterconnect) { | |
| if ("https://storage.googleapis.com".equals(endpoint)) { | |
| endpoint = "https://storage.direct.googleapis.com"; | |
| } else if ("storage.googleapis.com".equals(endpoint)) { | |
| endpoint = "storage.direct.googleapis.com"; | |
| } | |
| } | |
| if (attemptDirectPathXdsOverInterconnect) { | |
| if (endpoint.startsWith("https://storage.googleapis.com") | |
| && (endpoint.length() == 30 || endpoint.charAt(30) == ':' || endpoint.charAt(30) == '/')) { | |
| endpoint = "https://storage.direct.googleapis.com" + endpoint.substring(30); | |
| } else if (endpoint.startsWith("storage.googleapis.com") | |
| && (endpoint.length() == 22 || endpoint.charAt(22) == ':')) { | |
| endpoint = "storage.direct.googleapis.com" + endpoint.substring(22); | |
| } | |
| } |
| GoogleDefaultChannelCredentials.Builder channelCredsBuilder = | ||
| GoogleDefaultChannelCredentials.newBuilder().altsCallCredentials(altsCallCredentials); | ||
| if (credentials != null) { | ||
| channelCredsBuilder.callCredentials(io.grpc.auth.MoreCallCredentials.from(credentials)); | ||
| } | ||
| ChannelCredentials channelCreds = channelCredsBuilder.build(); |
There was a problem hiding this comment.
You have introduced the helper method getGoogleDefaultChannelCredentials() in this PR to construct the default channel credentials. We should reuse this helper method here to eliminate duplicate code and improve maintainability.
| GoogleDefaultChannelCredentials.Builder channelCredsBuilder = | |
| GoogleDefaultChannelCredentials.newBuilder().altsCallCredentials(altsCallCredentials); | |
| if (credentials != null) { | |
| channelCredsBuilder.callCredentials(io.grpc.auth.MoreCallCredentials.from(credentials)); | |
| } | |
| ChannelCredentials channelCreds = channelCredsBuilder.build(); | |
| ChannelCredentials channelCreds = getGoogleDefaultChannelCredentials(); |
|
|


No description provided.