Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add attempt_direct_path argument to create_channel #583

Merged
merged 19 commits into from
Feb 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 73 additions & 6 deletions google/api_core/grpc_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

"""Helpers for :mod:`grpc`."""
from typing import Generic, TypeVar, Iterator
from typing import Generic, Iterator, Optional, TypeVar

import collections
import functools
Expand Down Expand Up @@ -271,11 +271,23 @@ def _create_composite_credentials(
# Create a set of grpc.CallCredentials using the metadata plugin.
google_auth_credentials = grpc.metadata_call_credentials(metadata_plugin)

if ssl_credentials is None:
ssl_credentials = grpc.ssl_channel_credentials()

# Combine the ssl credentials and the authorization credentials.
return grpc.composite_channel_credentials(ssl_credentials, google_auth_credentials)
# if `ssl_credentials` is set, use `grpc.composite_channel_credentials` instead of
# `grpc.compute_engine_channel_credentials` as the former supports passing
# `ssl_credentials` via `channel_credentials` which is needed for mTLS.
if ssl_credentials:
# Combine the ssl credentials and the authorization credentials.
# See https://grpc.github.io/grpc/python/grpc.html#grpc.composite_channel_credentials
return grpc.composite_channel_credentials(
ssl_credentials, google_auth_credentials
)
else:
# Use grpc.compute_engine_channel_credentials in order to support Direct Path.
# See https://grpc.github.io/grpc/python/grpc.html#grpc.compute_engine_channel_credentials
# TODO(b/323073050): Although `grpc.compute_engine_channel_credentials`
Copy link
Contributor

@vchudnov-g vchudnov-g Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is public code, I would prefer to a public (GitHub) issue in this TODO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e9199a0

# returns channel credentials outside of GCE, we should determine if there is a way to
# reliably detect when the client is in a GCE environment so that
# `grpc.compute_engine_channel_credentials` is not called outside of GCE.
return grpc.compute_engine_channel_credentials(google_auth_credentials)


def create_channel(
Expand All @@ -288,6 +300,7 @@ def create_channel(
default_scopes=None,
default_host=None,
compression=None,
attempt_direct_path: Optional[bool] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attempt_direct_path: Optional[bool] = None,
attempt_direct_path: Optional[bool] = False,

Since it's meant to be a Boolean, might as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0f3a0e4

**kwargs,
):
"""Create a secure channel with credentials.
Expand All @@ -311,6 +324,16 @@ def create_channel(
default_host (str): The default endpoint. e.g., "pubsub.googleapis.com".
compression (grpc.Compression): An optional value indicating the
compression method to be used over the lifetime of the channel.
attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
the request is made. Direct Path provides a proxyless connection which
increases the available throughput, reduces latency, and increases
reliability. Outside of GCE, the direct path request may fallback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to spell out what GCE stands for in public docs? Not everyone looking at this file may be using GCE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e7e3cf7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reliability. Outside of GCE, the direct path request may fallback
reliability. Outside of GCE, the direct path request may fall back

but see comment below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e7e3cf7

to DNS if this is configured by the Service. This argument should only
be set in a GCE environment and for Services that are known to support Direct Path.
If a `ServiceUnavailable` response is received when the request is sent, it is
recommended that the client repeat the request with `attempt_direct_path` set to `False`
as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
set to `True` will result in `ValueError` as it is not yet supported.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set to `True` will result in `ValueError` as it is not yet supported.
set to `True` will result in `ValueError` as this combination is not yet supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e7e3cf7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Should only be set in a GCE environment" is confusing in light of "Outside of GCE, the request may fall back". Clarify. I suspect you mean something like this:

Suggested change
attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
the request is made. Direct Path provides a proxyless connection which
increases the available throughput, reduces latency, and increases
reliability. Outside of GCE, the direct path request may fallback
to DNS if this is configured by the Service. This argument should only
be set in a GCE environment and for Services that are known to support Direct Path.
If a `ServiceUnavailable` response is received when the request is sent, it is
recommended that the client repeat the request with `attempt_direct_path` set to `False`
as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
set to `True` will result in `ValueError` as it is not yet supported.
attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
the request is made. Direct Path is only available within a Google Compute
Engine environment and provides a proxyless connection which increases the
available throughput, reduces latency, and increases reliability.
- This argument should only
be set in a GCE environment and for Services that are known to support Direct
Path.
- If this argument is set outside of GCE, then this request will fail
unless the back-end service happens to have configured fall-back to DNS.
- If the request causes a `ServiceUnavailable` response, we
recommend that the client repeat the request with `attempt_direct_path` set to
`False` as the Service may not support Direct Path.
- Using `ssl_credentials` with `attempt_direct_path`
set to `True` will result in `ValueError` as this combination is not yet
supported.

(And similarly for the async version)

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e7e3cf7

kwargs: Additional key-word args passed to
:func:`grpc_gcp.secure_channel` or :func:`grpc.secure_channel`.
Note: `grpc_gcp` is only supported in environments with protobuf < 4.0.0.
Expand All @@ -320,8 +343,15 @@ def create_channel(

Raises:
google.api_core.DuplicateCredentialArgs: If both a credentials object and credentials_file are passed.
ValueError: If `ssl_credentials` is set and `attempt_direct_path` is set to `True`.
"""

# If `ssl_credentials` is set and `attempt_direct_path` is set to `True`,
# raise ValueError as this is not yet supported.
# See https://github.com/googleapis/python-api-core/issues/590
if ssl_credentials is not None and attempt_direct_path:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify to just this?:

Suggested change
if ssl_credentials is not None and attempt_direct_path:
if ssl_credentials and attempt_direct_path:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 60b8acb

raise ValueError("Using ssl_credentials with Direct Path is not supported")

composite_credentials = _create_composite_credentials(
credentials=credentials,
credentials_file=credentials_file,
Expand All @@ -332,17 +362,54 @@ def create_channel(
default_host=default_host,
)

# Note that grpcio-gcp is deprecated
if HAS_GRPC_GCP: # pragma: NO COVER
if compression is not None and compression != grpc.Compression.NoCompression:
_LOGGER.debug(
"Compression argument is being ignored for grpc_gcp.secure_channel creation."
)
if attempt_direct_path:
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, the _LOGGER.debug in the previous lines should probably also become a warnings.warn

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1a7578d and d96499f

"""The `attempt_direct_path` argument is ignored for grpc_gcp.secure_channel creation.""",
DeprecationWarning,
)
return grpc_gcp.secure_channel(target, composite_credentials, **kwargs)

if attempt_direct_path:
target = _modify_target_for_direct_path(target)

return grpc.secure_channel(
target, composite_credentials, compression=compression, **kwargs
)


def _modify_target_for_direct_path(target: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: In general, an API endpoint could depend on the URL path and not just the host+port. Is this true of Google APIs? If so, where do we deal with the path part of the URI? (I realize these functions specify only host+port as inputs, so it's clear what they expect and they're doing the right thing, but I was wondering about this more general question.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, where do we deal with the path part of the URI?

I believe this is part of the transcode method

Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,

def transcode(http_options, message=None, **request_kwargs):
"""Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,
https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312
Args:
http_options (list(dict)): A list of dicts which consist of these keys,
'method' (str): The http method
'uri' (str): The path template
'body' (str): The body field name (optional)
(This is a simplified representation of the proto option `google.api.http`)

https://github.com/googleapis/gapic-generator-python/blob/b4e6cfc0be8594e2575910699389600f5e92e552/gapic/templates/%25namespace/%25name_%25version/%25sub/services/%25service/transports/rest.py.j2#L372

"""
Given a target, return a modified version which is compatible with Direct Path.

Args:
target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other
target (str): The target service address in the format 'hostname[:port]', 'dns://hostname[:port]' or other

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af3a21b

compatible format.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"other compatible format": link to a place that lists compatible formats, or remove this phrase which I think is puzzling by itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in af3a21b


Returns:
target (str): The target service address which is converted into a format compatible with Direct Path.
If the target contains `dns:///` or does not have contain `:///`, the target will be converted in
a format compatible with Direct Path, otherwise the original target will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
target (str): The target service address which is converted into a format compatible with Direct Path.
If the target contains `dns:///` or does not have contain `:///`, the target will be converted in
a format compatible with Direct Path, otherwise the original target will be returned.
target (str): The target service address which is converted into a format compatible with Direct Path.
If the target contains `dns:///` or does not contain `:///`, the target will be converted in
a format compatible with Direct Path; otherwise the original target will be returned.

So the idea is that a :/// (except for dns:///) already denotes Direct Path, so no action is needed, correct? It might be good to be explicit about this assumption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 44d5845

"""

dns_prefix = "dns:///"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to clarify in a comment what the dns_prefix means, which I take it is to be explicit about an endpoint living in the Internet (ie outside GCP).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e6ea7ec

# Remove "dns:///" if `attempt_direct_path` is set to True as
# the Direct Path prefix `google-c2p:///` will be used instead.
target = target.replace(dns_prefix, "")

direct_path_prefix = ":///"
if direct_path_prefix not in target:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
direct_path_prefix = ":///"
if direct_path_prefix not in target:
direct_path_separator = ":///"
if direct_path_separator not in target:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c0052e6

target_without_port = target.split(":")[0]
# Modify the target to use Direct Path by adding the `google-c2p:///` prefix
target = f"google-c2p{direct_path_prefix}{target_without_port}"
return target


_MethodCall = collections.namedtuple(
"_MethodCall", ("request", "timeout", "metadata", "credentials", "compression")
)
Expand Down
23 changes: 22 additions & 1 deletion google/api_core/grpc_helpers_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import asyncio
import functools

from typing import Generic, Iterator, AsyncGenerator, TypeVar
from typing import AsyncGenerator, Generic, Iterator, Optional, TypeVar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General notes:

  1. The comments from the sycn version apply here too
  2. Just a note for the future (not blocking this PR): a lot of the code seems identical or very similar to the sync code. We should consolidate sensibly to eliminate duplication. (I just filed consolidate duplicated code #593 to track more such instances)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening that issue! I also made changes to the async code based on the sync code feedback.


import grpc
from grpc import aio
Expand Down Expand Up @@ -223,6 +223,7 @@ def create_channel(
default_scopes=None,
default_host=None,
compression=None,
attempt_direct_path: Optional[bool] = None,
**kwargs
):
"""Create an AsyncIO secure channel with credentials.
Expand All @@ -246,15 +247,32 @@ def create_channel(
default_host (str): The default endpoint. e.g., "pubsub.googleapis.com".
compression (grpc.Compression): An optional value indicating the
compression method to be used over the lifetime of the channel.
attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
the request is made. Direct Path provides a proxyless connection which
increases the available throughput, reduces latency, and increases
reliability. Outside of GCE, the direct path request may fallback
to DNS if this is configured by the Service. This argument should only
be set in a GCE environment and for Services that are known to support Direct Path.
If a `ServiceUnavailable` response is received when the request is sent, it is
recommended that the client repeat the request with `attempt_direct_path` set to `False`
as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
set to `True` will result in `ValueError` as it is not yet supported.
kwargs: Additional key-word args passed to :func:`aio.secure_channel`.

Returns:
aio.Channel: The created channel.

Raises:
google.api_core.DuplicateCredentialArgs: If both a credentials object and credentials_file are passed.
ValueError: If `ssl_credentials` is set and `attempt_direct_path` is set to `True`.
"""

# If `ssl_credentials` is set and `attempt_direct_path` is set to `True`,
# raise ValueError as this is not yet supported.
# See https://github.com/googleapis/python-api-core/issues/590
if ssl_credentials is not None and attempt_direct_path:
raise ValueError("Using ssl_credentials with Direct Path is not supported")

composite_credentials = grpc_helpers._create_composite_credentials(
credentials=credentials,
credentials_file=credentials_file,
Expand All @@ -265,6 +283,9 @@ def create_channel(
default_host=default_host,
)

if attempt_direct_path:
target = grpc_helpers._modify_target_for_direct_path(target)

return aio.secure_channel(
target, composite_credentials, compression=compression, **kwargs
)
Expand Down
7 changes: 3 additions & 4 deletions pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ filterwarnings =
# Remove once support for grpcio-gcp is deprecated
# See https://github.com/googleapis/python-api-core/blob/42e8b6e6f426cab749b34906529e8aaf3f133d75/google/api_core/grpc_helpers.py#L39-L45
ignore:.*Support for grpcio-gcp is deprecated:DeprecationWarning
# Remove once https://github.com/googleapis/python-api-common-protos/pull/187/files is merged
ignore:The `attempt_direct_path` argument is ignored for grpc_gcp.secure_channel creation:DeprecationWarning
# Remove once the minimum supported version of googleapis-common-protos is 1.62.0
ignore:.*pkg_resources.declare_namespace:DeprecationWarning
ignore:.*pkg_resources is deprecated as an API:DeprecationWarning
# Remove once release PR https://github.com/googleapis/proto-plus-python/pull/391 is merged
ignore:datetime.datetime.utcfromtimestamp\(\) is deprecated:DeprecationWarning:proto.datetime_helpers
# Remove once https://github.com/grpc/grpc/issues/35086 is fixed
# Remove once https://github.com/grpc/grpc/issues/35086 is fixed (and version newer than 1.60.0 is published)
ignore:There is no current event loop:DeprecationWarning
Loading
Loading