From 81405db11d8b81f3c254443775c481e71a573592 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Tue, 3 Sep 2024 10:56:56 +0200 Subject: [PATCH] Remove default port Allow client implementors to set a default port for gRPC URLs via the `defaults` argument but make the `parse_grpc_uri` function and `BaseApiClient` class don't provide a default port unless explicitly set by the implementors, as using a default port really depends on the specific API being implemented. Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 1 + src/frequenz/client/base/channel.py | 10 ++++-- tests/test_channel.py | 50 ++++++++++++++++++++--------- tests/test_client.py | 2 +- 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 2e8f153..f70d38b 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,7 @@ ## Upgrading - `GrpcStreamBroadcaster` now takes a `AsyncIterable` instead of a `AsyncIterator` as the `stream_method`. This is to match the type of streaming methods generated by `grpc`, so no conversion to an `AsyncIterator` is needed. +- gRPC URLs don't have a default port anymore, unless a default is set via `ChannelOptions`. If you want to set a default port for URLs, please pass custom `ChannelOptions` as `defaults` to `parse_grpc_uri` or as `channel_defaults` to `BaseApiClient`. ## New Features diff --git a/src/frequenz/client/base/channel.py b/src/frequenz/client/base/channel.py index 5a930e9..cfbd9f8 100644 --- a/src/frequenz/client/base/channel.py +++ b/src/frequenz/client/base/channel.py @@ -45,7 +45,7 @@ class SslOptions: class ChannelOptions: """Options for a gRPC channel.""" - port: int = 9090 + port: int | None = None """The port number to connect to.""" ssl: SslOptions = SslOptions() @@ -68,7 +68,8 @@ def parse_grpc_uri( A few things to consider about URI components: - If any other components are present in the URI, a [`ValueError`][] is raised. - - If the port is omitted, the `default_port` is used. + - If the port is omitted, the `default_port` is used unless it is `None`, in which + case a `ValueError` is raised - If a query parameter is passed many times, the last value is used. - Boolean query parameters can be specified with the following values (case-insensitive): `true`, `1`, `on`, `false`, `0`, `off`. @@ -110,6 +111,11 @@ def parse_grpc_uri( options = _parse_query_params(uri, parsed_uri.query) + if parsed_uri.port is None and defaults.port is None: + raise ValueError( + f"The gRPC URI '{uri}' doesn't specify a port and there is no default." + ) + target = ( parsed_uri.netloc if parsed_uri.port else f"{parsed_uri.netloc}:{defaults.port}" ) diff --git a/tests/test_channel.py b/tests/test_channel.py index f5ebe3c..e89128e 100644 --- a/tests/test_channel.py +++ b/tests/test_channel.py @@ -24,6 +24,7 @@ class _ValidUrlTestCase: title: str uri: str expected_host: str + expected_port: int | None expected_options: ChannelOptions defaults: ChannelOptions = ChannelOptions() @@ -33,10 +34,24 @@ class _ValidUrlTestCase: [ _ValidUrlTestCase( title="default", + uri="grpc://localhost:9090", + expected_host="localhost", + expected_port=9090, + expected_options=ChannelOptions( + ssl=SslOptions( + enabled=True, + root_certificates=None, + private_key=None, + certificate_chain=None, + ), + ), + ), + _ValidUrlTestCase( + title="default with default port", uri="grpc://localhost", expected_host="localhost", + expected_port=9090, expected_options=ChannelOptions( - port=9090, ssl=SslOptions( enabled=True, root_certificates=None, @@ -44,14 +59,15 @@ class _ValidUrlTestCase: certificate_chain=None, ), ), + defaults=ChannelOptions(port=9090), ), _ValidUrlTestCase( title="default no SSL defaults", - uri="grpc://localhost", - defaults=ChannelOptions(port=2355, ssl=SslOptions(enabled=False)), + uri="grpc://localhost:2355", + defaults=ChannelOptions(ssl=SslOptions(enabled=False)), expected_host="localhost", + expected_port=2355, expected_options=ChannelOptions( - port=2355, ssl=SslOptions( enabled=False, root_certificates=None, @@ -62,9 +78,8 @@ class _ValidUrlTestCase: ), _ValidUrlTestCase( title="default with SSL defaults", - uri="grpc://localhost", + uri="grpc://localhost:2355", defaults=ChannelOptions( - port=2355, ssl=SslOptions( enabled=True, root_certificates=None, @@ -73,8 +88,8 @@ class _ValidUrlTestCase: ), ), expected_host="localhost", + expected_port=2355, expected_options=ChannelOptions( - port=2355, ssl=SslOptions( enabled=True, root_certificates=None, @@ -88,8 +103,8 @@ class _ValidUrlTestCase: uri="grpc://localhost:1234?ssl=1&ssl_root_certificates_path=/root_cert" "&ssl_private_key_path=/key&ssl_certificate_chain_path=/chain", expected_host="localhost", + expected_port=1234, expected_options=ChannelOptions( - port=1234, ssl=SslOptions( enabled=True, root_certificates=pathlib.Path("/root_cert"), @@ -103,7 +118,6 @@ class _ValidUrlTestCase: uri="grpc://localhost:1234?ssl=1&ssl_root_certificates_path=/root_cert" "&ssl_private_key_path=/key&ssl_certificate_chain_path=/chain", defaults=ChannelOptions( - port=4444, ssl=SslOptions( enabled=True, root_certificates=pathlib.Path("/root_cert_def"), @@ -112,8 +126,8 @@ class _ValidUrlTestCase: ), ), expected_host="localhost", + expected_port=1234, expected_options=ChannelOptions( - port=1234, ssl=SslOptions( enabled=True, root_certificates=pathlib.Path("/root_cert"), @@ -138,11 +152,7 @@ def test_parse_uri_ok( # pylint: disable=too-many-locals expected_credentials = mock.MagicMock( name="mock_credentials", spec=ssl_channel_credentials ) - expected_port = ( - expected_options.port - if f":{expected_options.port}" in uri or defaults.port is None - else defaults.port - ) + expected_port = case.expected_port expected_ssl = ( expected_options.ssl.enabled if "ssl=" in uri or defaults.ssl.enabled is None @@ -270,3 +280,13 @@ def test_parse_uri_error( """Test parsing of invalid gRPC URIs for grpclib.""" with pytest.raises(ValueError, match=error_msg): parse_grpc_uri(uri) + + +def test_invalid_url_no_default_port() -> None: + """Test parsing of invalid gRPC URIs for grpclib.""" + uri = "grpc://localhost" + with pytest.raises( + ValueError, + match=r"The gRPC URI 'grpc://localhost' doesn't specify a port and there is no default.", + ): + parse_grpc_uri(uri) diff --git a/tests/test_client.py b/tests/test_client.py index c59e240..2cbdcbc 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -105,7 +105,7 @@ def test_base_api_client_init_with_channel_defaults( mocker: pytest_mock.MockFixture, ) -> None: """Test initializing the BaseApiClient with channel defaults.""" - channel_defaults = ChannelOptions(port=1234, ssl=SslOptions(enabled=False)) + channel_defaults = ChannelOptions(ssl=SslOptions(enabled=False)) client, mocks = create_client_with_mocks(mocker, channel_defaults=channel_defaults) assert client.server_url == _DEFAULT_SERVER_URL mocks.parse_grpc_uri.assert_called_once_with(client.server_url, channel_defaults)