Don't allow a default port in URLs by default (#85)

We allow client implementors to set a default port for gRPC URLs again,
but the `parse_grpc_uri` function and `BaseApiClient` class still don't
provide a default port unless explicitly set.

Author: llucax

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
 

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}"
     )

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,25 +34,40 @@ 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,
                     private_key=None,
                     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)

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)