feat(storage): support DirectPath over Interconnect in GCS gRPC - #18431
nidhiii-27 wants to merge 2 commits into
Conversation
Add support for DirectPath over Cloud Interconnect (DP over GCI) across google.api_core.grpc_helpers, google.api_core.grpc_helpers_async, google.cloud.storage.grpc_client.GrpcClient, and google.cloud.storage.asyncio.async_grpc_client.AsyncGrpcClient. - Add attempt_direct_path_xds_over_interconnect option and GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS_OVER_INTERCONNECT env override. - Synthesize standard TLS composite credentials instead of GCE ALTS when DirectPath over Interconnect is enabled. - Rewrite storage.googleapis.com to storage-direct.googleapis.com and append ?force-xds to google-c2p:/// target URIs. [Generated-by: AI]
![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 adds support for DirectPath over Cloud Interconnect using xDS and standard TLS credentials across both synchronous and asynchronous gRPC helpers in google-api-core and the gRPC client in google-cloud-storage. Key feedback includes fixing a target rewriting bug where dns:/// prefixes cause conversion to be skipped, restricting the unsafe replacement of -direct. to only target -direct.googleapis.com to avoid corrupting custom user endpoints, and improving environment variable parsing to handle case-insensitivity and raise errors on invalid configuration values.
| direct_path_separator = ":///" | ||
| if direct_path_separator not in target: |
There was a problem hiding this comment.
The current implementation of _modify_target_for_direct_path fails to correctly parse targets prefixed with dns:/// (such as dns:///storage-direct.googleapis.com:443). Because :/// is present in the target, the function skips the conversion to google-c2p:/// entirely, returning the original dns:/// target. This causes the newly added test case test__modify_target_for_direct_path_interconnect to fail.
We should strip the dns:/// prefix at the beginning of the function if it is present, allowing the host and port to be correctly extracted and converted to a google-c2p:/// target.
if target.startswith("dns:///"):
target = target[7:]
direct_path_separator = ":///"
if direct_path_separator not in target:| elif "-direct." in target and not target.startswith("google-c2p:///"): | ||
| target = target.replace("-direct.", ".") |
There was a problem hiding this comment.
Replacing any occurrence of "-direct." with "." is unsafe because it can corrupt custom user endpoints that happen to contain "-direct." (e.g., my-direct.example.com would be incorrectly rewritten to my.example.com).
To prevent unintended routing issues, restrict this replacement to Google API endpoints by matching "-direct.googleapis.com" instead.
| elif "-direct." in target and not target.startswith("google-c2p:///"): | |
| target = target.replace("-direct.", ".") | |
| elif "-direct.googleapis.com" in target and not target.startswith("google-c2p:///"): | |
| target = target.replace("-direct.googleapis.com", ".googleapis.com") |
| elif "-direct." in target and not target.startswith("google-c2p:///"): | ||
| target = target.replace("-direct.", ".") |
There was a problem hiding this comment.
Replacing any occurrence of "-direct." with "." is unsafe because it can corrupt custom user endpoints that happen to contain "-direct." (e.g., my-direct.example.com would be incorrectly rewritten to my.example.com).
To prevent unintended routing issues, restrict this replacement to Google API endpoints by matching "-direct.googleapis.com" instead.
| elif "-direct." in target and not target.startswith("google-c2p:///"): | |
| target = target.replace("-direct.", ".") | |
| elif "-direct.googleapis.com" in target and not target.startswith("google-c2p:///"): | |
| target = target.replace("-direct.googleapis.com", ".googleapis.com") |
| env_val = os.environ.get(_DIRECT_PATH_INTERCONNECT_ENV) | ||
| if env_val == "true": | ||
| return True | ||
| if env_val == "false": | ||
| return False |
There was a problem hiding this comment.
Environment variables are strings and can be set with different casings (e.g., True or TRUE). It is safer to convert the environment variable value to lowercase before comparing it to "true" or "false". Additionally, if the user explicitly sets an invalid value (such as whitespace-only or an invalid configuration string), we should fail fast and raise a ValueError to notify them of the invalid configuration rather than silently falling back.
| env_val = os.environ.get(_DIRECT_PATH_INTERCONNECT_ENV) | |
| if env_val == "true": | |
| return True | |
| if env_val == "false": | |
| return False | |
| env_val = os.environ.get(_DIRECT_PATH_INTERCONNECT_ENV) | |
| if env_val is not None: | |
| env_val_clean = env_val.strip().lower() | |
| if env_val_clean == "true": | |
| return True | |
| elif env_val_clean == "false": | |
| return False | |
| else: | |
| raise ValueError(f"Invalid value for {_DIRECT_PATH_INTERCONNECT_ENV}: {env_val}") |
References
- When parsing environment variables, if a user explicitly sets an invalid value (such as whitespace-only), fail fast and raise an error to notify them of the invalid configuration rather than silently falling back to a default value.
| env_val = os.environ.get(_DIRECT_PATH_INTERCONNECT_ENV) | ||
| if env_val == "true": | ||
| return True | ||
| if env_val == "false": | ||
| return False |
There was a problem hiding this comment.
Environment variables are strings and can be set with different casings (e.g., True or TRUE). It is safer to convert the environment variable value to lowercase before comparing it to "true" or "false". Additionally, if the user explicitly sets an invalid value (such as whitespace-only or an invalid configuration string), we should fail fast and raise a ValueError to notify them of the invalid configuration rather than silently falling back.
env_val = os.environ.get(_DIRECT_PATH_INTERCONNECT_ENV)
if env_val is not None:
env_val_clean = env_val.strip().lower()
if env_val_clean == "true":
return True
elif env_val_clean == "false":
return False
else:
raise ValueError(f"Invalid value for {_DIRECT_PATH_INTERCONNECT_ENV}: {env_val}")References
- When parsing environment variables, if a user explicitly sets an invalid value (such as whitespace-only), fail fast and raise an error to notify them of the invalid configuration rather than silently falling back to a default value.
Ensure 100% test coverage for _resolve_direct_path_interconnect and _create_composite_credentials in google.api_core.grpc_helpers. [Generated-by: AI]
|
/gcbrun |
1 participant
Summary
Adds support for DirectPath over Cloud Interconnect (DP over GCI) across
google.api_core.grpc_helpers,google.api_core.grpc_helpers_async,google.cloud.storage.grpc_client.GrpcClient, andgoogle.cloud.storage.asyncio.async_grpc_client.AsyncGrpcClient.google-api-core):attempt_direct_path_xds_over_interconnect: Optional[bool] = Falsetogrpc_helpers.create_channelandgrpc_helpers_async.create_channel.GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS_OVER_INTERCONNECT("true"/"false") environment variable override.grpc.composite_channel_credentials(ssl_credentials, google_auth_credentials)) instead of GCE-only ALTS (grpc.compute_engine_channel_credentials) when DirectPath over Interconnect is enabled._modify_target_for_direct_pathto append?force-xds(or&force-xds) togoogle-c2p:///targets and rewrite-direct.back to.on CloudPath fallback.google-cloud-storage):attempt_direct_path_xds_over_interconnect=FalsetoGrpcClientandAsyncGrpcClient.storage.googleapis.comtostorage-direct.googleapis.com(with delimiter verification) when DirectPath over Interconnect is enabled.Test Plan
PYTHONPATH=packages/google-api-core pytest packages/google-api-core/tests/unit/test_grpc_helpers.py packages/google-api-core/tests/asyncio/test_grpc_helpers_async.py(143 passed)PYTHONPATH=packages/google-api-core:packages/google-cloud-storage pytest packages/google-cloud-storage/tests/unit/test_grpc_client.py packages/google-cloud-storage/tests/unit/asyncio/test_async_grpc_client.py(33 passed)[Generated-by: AI]