fix: handle invalid combination of external_hostname and routing_path

config.yaml

@@ -51,6 +51,9 @@ options:
       will provide to the unit `my-unit/2` in the `my-model` model the following URL:
 
       `http://my-model-my-unit-2.foo:8080`
+
+      Note that, for 'subdomain' routing mode, the external_hostname must be set and not be set to an IP address.  This
+      is because subdomains are not supported for IP addresses.
     type: string
     default: path
 

lib/charms/traefik_k8s/v2/ingress.py

@@ -57,7 +57,18 @@ def _on_ingress_revoked(self, event: IngressPerAppRevokedEvent):
 import typing
 from dataclasses import dataclass
 from functools import partial
-from typing import Any, Callable, Dict, List, MutableMapping, Optional, Sequence, Tuple, Union
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    MutableMapping,
+    Optional,
+    Sequence,
+    Tuple,
+    Union,
+    cast,
+)
 
 import pydantic
 from ops.charm import CharmBase, RelationBrokenEvent, RelationEvent
@@ -226,7 +237,7 @@ class IngressUrl(BaseModel):
 class IngressProviderAppData(DatabagModel):
     """Ingress application databag schema."""
 
-    ingress: IngressUrl
+    ingress: Optional[IngressUrl] = None
 
 
 class ProviderSchema(BaseModel):
@@ -558,7 +569,16 @@ def _published_url(self, relation: Relation) -> Optional["IngressProviderAppData
     def publish_url(self, relation: Relation, url: str):
         """Publish to the app databag the ingress url."""
         ingress_url = {"url": url}
-        IngressProviderAppData(ingress=ingress_url).dump(relation.data[self.app])  # type: ignore
+        try:
+            IngressProviderAppData(ingress=ingress_url).dump(relation.data[self.app])  # type: ignore
+        except pydantic.ValidationError as e:
+            # If we cannot validate the url as valid, publish an empty databag and log the error.
+            log.error(f"Failed to validate ingress url '{url}' - got ValidationError {e}")
+            log.error(
+                "url was not published to ingress relation for {relation.app}.  This error is likely due to an"
+                " error or misconfiguration of the charm calling this library."
+            )
+            IngressProviderAppData(ingress=None).dump(relation.data[self.app])  # type: ignore
 
     @property
     def proxied_endpoints(self) -> Dict[str, Dict[str, str]]:
@@ -596,10 +616,14 @@ def proxied_endpoints(self) -> Dict[str, Dict[str, str]]:
             if not ingress_data:
                 log.warning(f"relation {ingress_relation} not ready yet: try again in some time.")
                 continue
+
+            # Validation above means ingress cannot be None, but type checker doesn't know that.
+            ingress = ingress_data.ingress
+            ingress = cast(IngressProviderAppData, ingress)
             if PYDANTIC_IS_V1:
-                results[ingress_relation.app.name] = ingress_data.ingress.dict()
+                results[ingress_relation.app.name] = ingress.dict()
             else:
-                results[ingress_relation.app.name] = ingress_data.ingress.model_dump(mode="json")
+                results[ingress_relation.app.name] = ingress.model_dump(mode="json")
         return results
 
 
@@ -834,7 +858,11 @@ def _get_url_from_relation_data(self) -> Optional[str]:
         if not databag:  # not ready yet
             return None
 
-        return str(IngressProviderAppData.load(databag).ingress.url)
+        ingress = IngressProviderAppData.load(databag).ingress
+        if ingress is None:
+            return None
+
+        return str(ingress.url)
 
     @property
     def url(self) -> Optional[str]:

src/charm.py

@@ -9,6 +9,7 @@
 import itertools
 import json
 import logging
+import re
 import socket
 from typing import Any, Dict, List, Optional, Tuple, Union, cast
 from urllib.parse import urlparse
@@ -126,6 +127,20 @@ class TraefikIngressCharm(CharmBase):
     def __init__(self, *args):
         super().__init__(*args)
 
+        # Before doing anything, validate the charm config.  If configuration is invalid, warn the user.
+        # FIXME: Invalid configuration here SHOULD halt the charm's operation until resolution, or at least make a
+        #  persistent BlockedStatus, but this charm handles events atomically rather than holistically.
+        #  This means that skipping events will result in unexpected issues, so if we halt the charm here we must
+        #  ensure the charm processes all backlogged events on resumption in the original order.  Rather than do that
+        #  and risk losing an event, we simply warn the user and continue functioning as best as possible.  The charm
+        #  will operate correctly except that it will not publish ingress urls to related applications, instead
+        #  leaving the ingress relation data empty and logging an error.
+        #  If we refactor this charm to handle events holistically (and thus we can skip events without issue), we
+        #  should refactor this validation truly halt the charm.
+        #  If we refactor this charm to use collect_unit_status, we could raise a persistent BlockedStatus message when
+        #  this configuration is invalid.
+        self._validate_config()
+
         self._stored.set_default(
             config_hash=None,
         )
@@ -1186,13 +1201,60 @@ def server_cert_sans_dns(self) -> List[str]:
         # If all else fails, we'd rather use the bare IP
         return [target] if target else []
 
+    def _validate_config(self):
+        """Validate the charm configuration, emitting warning messages on misconfigurations.
+
+        In scope for this validation is:
+        * validating the combination of external_hostname and routing_mode
+        """
+        # FIXME: This will false positive in cases where the LoadBalancer provides an external host rather than an IP.
+        #  The warning will occur, but the charm will function normally.  We could better validate the LoadBalancer if
+        #  we want to avoid this, but it probably isn't worth the effort until someone notices.
+        invalid_hostname_and_routing_mode_message = (
+            "Likely configuration error: When using routing_mode=='subdomain', external_hostname should be "
+            "set.  This is because when external_hostname is unset, Traefik uses the LoadBalancer's address as the "
+            "hostname for all provided URLS and that hostname is typically an IP address.  This leads to invalid urls "
+            "like `model-app.1.2.3.4`.  The charm will continue to operate as currently set, but will not provide urls"
+            " to any related applications if they would be invalid."
+        )
+
+        if self.config.get("routing_mode", "") == "subdomain":
+            # subdomain mode can only be used if an external_hostname is set and is not an IP address
+            external_hostname = self.config.get("external_hostname", "")
+            if not isinstance(external_hostname, str) or not is_valid_hostname(external_hostname):
+                logger.warning(invalid_hostname_and_routing_mode_message)
+
+
+def is_valid_hostname(hostname: str) -> bool:
+    """Check if a hostname is valid.
+
+    Modified from https://stackoverflow.com/a/33214423
+    """
+    if len(hostname) == 0:
+        return False
+    if hostname[-1] == ".":
+        # strip exactly one dot from the right, if present
+        hostname = hostname[:-1]
+    if len(hostname) > 253:
+        return False
+
+    labels = hostname.split(".")
+
+    # the TLD must be not all-numeric
+    if re.match(r"[0-9]+$", labels[-1]):
+        return False
+
+    allowed = re.compile(r"(?!-)[a-z0-9-]{1,63}(?<!-)$", re.IGNORECASE)
+    return all(allowed.match(label) for label in labels)
+
 
 @functools.lru_cache
 def _get_loadbalancer_status(namespace: str, service_name: str) -> Optional[str]:
     client = Client()  # type: ignore
     try:
         traefik_service = client.get(Service, name=service_name, namespace=namespace)
-    except ApiError:
+    except ApiError as e:
+        logger.warning(f"Got ApiError when trying to get Loadbalancer status: {e}")
         return None
 
     if not (status := traefik_service.status):  # type: ignore

tests/scenario/conftest.py

@@ -1,21 +1,21 @@
-from unittest.mock import PropertyMock, patch
+from unittest.mock import patch
 
 import pytest
 from ops import pebble
 from scenario import Container, Context, ExecOutput, Model, Mount
 
 from charm import TraefikIngressCharm
 
-MOCK_EXTERNAL_HOSTNAME = "testhostname"
+MOCK_LB_ADDRESS = "1.2.3.4"
 
 
 @pytest.fixture
 def traefik_charm():
     with patch("charm.KubernetesServicePatch"):
         with patch("lightkube.core.client.GenericSyncClient"):
             with patch(
-                "charm.TraefikIngressCharm._external_host",
-                PropertyMock(return_value=MOCK_EXTERNAL_HOSTNAME),
+                "charm._get_loadbalancer_status",
+                return_value=MOCK_LB_ADDRESS,
             ):
                 yield TraefikIngressCharm
 

tests/scenario/test_ingress_per_app.py

@@ -15,9 +15,10 @@
     IngressRequirerUnitData,
 )
 from ops import CharmBase, Framework
-from scenario import Context, Mount, Relation, State
+from scenario import Context, Model, Mount, Relation, State
 
 from tests.scenario._utils import create_ingress_relation
+from tests.scenario.conftest import MOCK_LB_ADDRESS
 
 
 @pytest.mark.parametrize(
@@ -301,3 +302,53 @@ def test_proxied_endpoints(
         assert charm.ingress_per_appv1.proxied_endpoints["remote"]["url"]
         assert charm.ingress_per_appv2.proxied_endpoints["remote"]["url"]
         assert charm.ingress_per_unit.proxied_endpoints["remote/0"]["url"]
+
+
+MODEL_NAME = "test-model"
+UNIT_NAME = "nms"
+
+
+@pytest.mark.parametrize(
+    "external_hostname, routing_mode, expected_local_app_data",
+    [
+        # Valid configurations
+        (
+            "foo.com",
+            "path",
+            {"ingress": json.dumps({"url": f"http://foo.com/{MODEL_NAME}-{UNIT_NAME}"})},
+        ),
+        (
+            "foo.com",
+            "subdomain",
+            {"ingress": json.dumps({"url": f"http://{MODEL_NAME}-{UNIT_NAME}.foo.com/"})},
+        ),
+        (
+            "",
+            "path",
+            {"ingress": json.dumps({"url": f"http://{MOCK_LB_ADDRESS}/{MODEL_NAME}-{UNIT_NAME}"})},
+        ),
+        # Invalid configuration, resulting in empty local_app_data
+        ("", "subdomain", {}),
+    ],
+)
+def test_ingress_with_hostname_and_routing_mode(
+    external_hostname,
+    routing_mode,
+    expected_local_app_data,
+    traefik_ctx,
+    traefik_container,
+    tmp_path,
+):
+    """Tests that the ingress relation provides a URL for valid external hostname and routing mode combinations."""
+    ipa = create_ingress_relation(strip_prefix=True, unit_name=UNIT_NAME)
+    state = State(
+        model=Model(name=MODEL_NAME),
+        config={"routing_mode": routing_mode, "external_hostname": external_hostname},
+        containers=[traefik_container],
+        relations=[ipa],
+        leader=True,
+    )
+
+    # event = getattr(ipa, f"changed_event")
+    state_out = traefik_ctx.run("config-changed", state)
+    assert state_out.relations[0].local_app_data == expected_local_app_data

tests/scenario/test_ingress_per_unit.py

@@ -1,7 +1,7 @@
 import pytest
 from scenario import Relation, State
 
-from tests.scenario.conftest import MOCK_EXTERNAL_HOSTNAME
+from tests.scenario.conftest import MOCK_LB_ADDRESS
 
 
 @pytest.mark.parametrize("leader", (True, False))
@@ -48,12 +48,12 @@ def test_ingress_unit_provider_request_response(
         assert not local_app_data
     else:
         if mode == "tcp":
-            expected_url = f"{MOCK_EXTERNAL_HOSTNAME}:{port}"
+            expected_url = f"{MOCK_LB_ADDRESS}:{port}"
         else:
             prefix = f"{model}-{remote_unit_name.replace('/', '-')}"
             if routing_mode == "path":
-                expected_url = f"http://{MOCK_EXTERNAL_HOSTNAME}/{prefix}"
+                expected_url = f"http://{MOCK_LB_ADDRESS}/{prefix}"
             else:
-                expected_url = f"http://{prefix}.{MOCK_EXTERNAL_HOSTNAME}/"
+                expected_url = f"http://{prefix}.{MOCK_LB_ADDRESS}/"
 
         assert local_app_data == {"ingress": f"{remote_unit_name}:\n  url: {expected_url}\n"}