feat(storage): add support for DirectPath over Interconnect - #14006
feat(storage): add support for DirectPath over Interconnect#14006nidhiii-27 wants to merge 5 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 ."
There was a problem hiding this comment.
Done. Updated isCredentialDirectPathCompatible() to check isAttemptDirectPathXdsOverInterconnect() prior to needsCredentials(), preventing misleading credential misconfiguration warnings when using DirectPath over Interconnect.
Co-authored by AI Agent
| 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().
There was a problem hiding this comment.
Done. Replaced hardcoded string literals in rewriteHost() and overrideAuthority() with DEFAULT_HOST_DIRECT_PATH and DEFAULT_HOST_NO_SCHEME constants.
Co-authored by AI Agent
| * Option for whether this client should attempt to use DirectPath over Interconnect (on-premise | ||
| * xDS name resolution). | ||
| * | ||
| * @since 2.45.0 |
There was a problem hiding this comment.
Done. Updated the @SInCE tag to 2.72.0 to reflect the current release version of google-cloud-storage.
Co-authored by AI Agent
| } 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?
There was a problem hiding this comment.
Done. Updated isDirectPathXdsEnabled() to also account for isAttemptDirectPathXdsOverInterconnect(), ensuring that enabling DirectPath over Interconnect is correctly recognized as xDS-enabled and does not trigger misconfiguration warnings.
Co-authored by AI Agent
| (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.
There was a problem hiding this comment.
Done. Exposed @internalapi public boolean isAttemptDirectPathXdsOverInterconnect() on InstantiatingGrpcChannelProvider following the pattern of isDirectPathXdsEnabled(), and updated StorageOptionsBuilderTest to call it directly instead of using reflection.
Co-authored by AI Agent
|
|
| @@ -134,6 +134,9 @@ public final class GrpcStorageOptions extends StorageOptions | |||
| private static final String GCS_SCOPE = "https://www.googleapis.com/auth/devstorage.full_control"; | |||
There was a problem hiding this comment.
Can you look into the feedback related to overrideAuthority here https://docs.google.com/document/d/1d9OS84flOtAWD9Bx4HGdzKFV2t1_XlB5l8YnXWUto7Q/edit?tab=t.0
| * | ||
| * @since 2.72.0 | ||
| */ | ||
| public GrpcStorageOptions.Builder setAttemptDirectPathXdsOverInterconnect( |
There was a problem hiding this comment.
Add Beta API annotation, till the feature is GA
3 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.