feat(storage): add support for DirectPath over Interconnect - #14006
feat(storage): add support for DirectPath over Interconnect#14006nidhiii-27 wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for DirectPath xDS over Interconnect (on-premise xDS name resolution) in GrpcStorageOptions and InstantiatingGrpcChannelProvider, allowing GCE environment checks to be bypassed when enabled. It also updates endpoint validation to support custom URI schemes like google-c2p:/// and adds corresponding tests. The reviewer suggests dynamically adjusting the log level for the DirectPath fallback warning to avoid excessive warning spam in non-GCE environments, recommending Level.WARNING on GCE and Level.FINE elsewhere.
| if (needsCredentials()) { | ||
| return false; | ||
| } | ||
| // xDS over Interconnect is designed to work on-premise using arbitrary service credentials. |
There was a problem hiding this comment.
Should we check isAttemptDirectPathXdsOverInterconnect() before needsCredentials()?
If needsCredentials (above) evaluates to true, isCredentialDirectPathCompatible returns false. Then validateDirectPathState logs: "DirectPath is misconfigured. DirectPath is only compatible with com.google.auth.oauth2.ComputeEngineCredentials ."
| private static final Set<String> SCOPES = ImmutableSet.of(GCS_SCOPE); | ||
| private static final String DEFAULT_HOST = "https://storage.googleapis.com"; | ||
| private static final String DEFAULT_HOST_DIRECT_PATH = "https://storage-direct.googleapis.com"; | ||
| private static final String DEFAULT_HOST_NO_SCHEME = "storage.googleapis.com"; |
There was a problem hiding this comment.
These are unused and the string literals are hardcoded in rewriteHost() and overrideAuthority().
| * Option for whether this client should attempt to use DirectPath over Interconnect (on-premise | ||
| * xDS name resolution). | ||
| * | ||
| * @since 2.45.0 |
| } else { | ||
| // Case 3: credential is not correctly set | ||
| // Case 3: DirectPath is enabled, but xDS is not. | ||
| if (!isDirectPathXdsEnabled()) { |
There was a problem hiding this comment.
If a customer configures the new option setAttemptDirectPathXdsOverInterconnect(true) without explicitly setting the attemptDirectPathXds option:
isDirectPathEnabled is true but isDirectPathXdsEnabled evaluates to false because it only checks attemptDirectPathXds and the environment variable.
As a result, GAX logs a warning stating that xDS is not enabled. Is this intended behavior?
| (com.google.api.gax.grpc.InstantiatingGrpcChannelProvider) tcp; | ||
|
|
||
| // Verify attemptDirectPathXdsOverInterconnect is set to true on the provider using reflection | ||
| java.lang.reflect.Field field = |
There was a problem hiding this comment.
IIUC, using reflection in unit tests is fragile.
Can we instead expose @InternalApi public boolean isAttemptDirectPathXdsOverInterconnect() on
InstantiatingGrpcChannelProvider? This follows the established pattern of isDirectPathXdsEnabled and eliminates the reflective test hack.
|
|
2 participants
google-c2p:///<service>?force-xdstarget scheme.:///syntax (e.g.google-c2p:///).storage-direct.googleapis.comand override the request authority to storage.googleapis.com for secure TLS handshakes.