Skip to content

Commit

Permalink
Fix per-app middleware injection (#114)
Browse files Browse the repository at this point in the history 
* Fix per-app middleware injection

* Fix the unit tests. Fix URL refreshing WITHOUT WAITING FOR AN EVENT, too.

* Extract config generation
  • Loading branch information
@rbarry82
rbarry82 authored Dec 12, 2022
1 parent 4e24e0a commit 6fddc92
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 64 deletions.
4 changes: 2 additions & 2 deletions lib/charms/traefik_k8s/v1/ingress.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def _on_ingress_revoked(self, event: IngressPerAppRevokedEvent):

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 7
LIBPATCH = 8

DEFAULT_RELATION_NAME = "ingress"
RELATION_INTERFACE = "ingress"
Expand Down Expand Up @@ -559,6 +559,6 @@ def url(self) -> Optional[str]:
Returns None if the URL isn't available yet.
"""
data = self._stored.current_url or None # type: ignore
data = self._stored.current_url or self._get_url_from_relation_data() # type: ignore
assert isinstance(data, (str, type(None))) # for static checker
return data
85 changes: 38 additions & 47 deletions src/charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import json
import logging
import typing
from typing import Tuple, Union
from typing import Any, Dict, List, Tuple, Union
from urllib.parse import urlparse

import yaml
Expand Down Expand Up @@ -487,12 +487,21 @@ def _generate_middleware_config(
}
}

return {}

def _generate_per_unit_config(self, data: "RequirerData_IPU") -> Tuple[dict, str]:
"""Generate a config dict for a given unit for IngressPerUnit."""
config = {"http": {"routers": {}, "services": {}}}
config = {"http": {"routers": {}, "services": {}}} # type: Dict[str, Any]
prefix = self._get_prefix(data)
host = self.external_host
if data["mode"] == "tcp":
# TODO: is there a reason why SNI-based routing (from TLS certs) is per-unit only?
# This is not a technical limitation in any way. It's meaningful/useful for
# authenticating to individual TLS-based servers where it may be desirable to reach
# one or more servers in a cluster (let's say Kafka), but limiting it to per-unit only
# actively impedes the architectural design of any distributed/ring-buffered TLS-based
# scale-out services which may only have frontends dedicated, but which do not "speak"
# HTTP(S). Such as any of the "cloud-native" SQL implementations (TiDB, Cockroach, etc)
port = data["port"]
unit_url = f"{host}:{port}"
config = {
Expand All @@ -515,45 +524,26 @@ def _generate_per_unit_config(self, data: "RequirerData_IPU") -> Tuple[dict, str
return config, unit_url

else:
if self._routing_mode is _RoutingMode.path:
route_rule = f"PathPrefix(`/{prefix}`)"
unit_url = f"http://{host}:{self._port}/{prefix}"
else: # _RoutingMode.subdomain
route_rule = f"Host(`{prefix}.{host}`)"
unit_url = f"http://{prefix}.{host}:{self._port}/"

traefik_router_name = f"juju-{prefix}-router"
traefik_service_name = f"juju-{prefix}-service"
lb_servers = [{"url": f"http://{data['host']}:{data['port']}"}]
return self._generate_config_block(prefix, lb_servers, data)

router_cfg = {
"rule": route_rule,
"service": traefik_service_name,
"entryPoints": ["web"],
}
def _generate_config_block(
self, prefix: str, lb_servers: List[Dict[str, str]], data: Dict[str, Any]
) -> Tuple[Dict[str, Any], str]:
"""Generate a configuration segment.
middlewares = self._generate_middleware_config(data, prefix)
if middlewares:
router_cfg["middlewares"] = list(middlewares.keys())

config["http"]["middlewares"] = middlewares
config["http"]["routers"][traefik_router_name] = router_cfg

config["http"]["services"][traefik_service_name] = {
"loadBalancer": {"servers": [{"url": f"http://{data['host']}:{data['port']}"}]}
}
return config, unit_url

def _generate_per_app_config(self, data: "RequirerData_IPA") -> Tuple[dict, str]:
Per-unit and per-app configuration blocks are mostly similar, with the principal
difference being the list of servers to load balance across (where IPU is one server per
unit and IPA may be more than one.
"""
host = self.external_host
port = self._port
prefix = self._get_prefix(data)

if self._routing_mode == _RoutingMode.path:
if self._routing_mode is _RoutingMode.path:
route_rule = f"PathPrefix(`/{prefix}`)"
app_url = f"http://{host}:{port}/{prefix}"
url = f"http://{host}:{self._port}/{prefix}"
else: # _RoutingMode.subdomain
route_rule = f"Host(`{prefix}.{host}`)"
app_url = f"http://{prefix}.{host}:{port}/"
url = f"http://{prefix}.{host}:{self._port}/"

traefik_router_name = f"juju-{prefix}-router"
traefik_service_name = f"juju-{prefix}-service"
Expand All @@ -566,25 +556,26 @@ def _generate_per_app_config(self, data: "RequirerData_IPA") -> Tuple[dict, str]
}
}

middlewares = self._generate_middleware_config(data, prefix)
if middlewares:
router_cfg["middlewares"] = list(middlewares.keys())

config = {
"http": {
"middlewares": middlewares,
"routers": router_cfg,
"services": {
traefik_service_name: {
"loadBalancer": {
"servers": [{"url": f"http://{data['host']}:{data['port']}"}]
}
}
},
"services": {traefik_service_name: {"loadBalancer": {"servers": lb_servers}}},
}
}

return config, app_url
middlewares = self._generate_middleware_config(data, prefix)

if middlewares:
config["http"]["middlewares"] = middlewares
router_cfg[traefik_router_name]["middlewares"] = list(middlewares.keys())

return config, url

def _generate_per_app_config(self, data: "RequirerData_IPA") -> Tuple[dict, str]:
prefix = self._get_prefix(data)

lb_servers = [{"url": f"http://{data['host']}:{data['port']}"}]
return self._generate_config_block(prefix, lb_servers, data)

def _wipe_ingress_for_all_relations(self):
for relation in self.model.relations["ingress"] + self.model.relations["ingress-per-unit"]:
Expand Down
35 changes: 20 additions & 15 deletions tests/unit/test_charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@
ops.testing.SIMULATE_CAN_CONNECT = True


def relate(harness: Harness):
relation_id = harness.add_relation("ingress-per-unit", "remote")
def relate(harness: Harness, per_app_relation: bool = False) -> Relation:
interface_name = "ingress" if per_app_relation else "ingress-per-unit"
relation_id = harness.add_relation(interface_name, "remote")
harness.add_relation_unit(relation_id, "remote/0")
relation = harness.model.get_relation("ingress-per-unit", relation_id)
relation = harness.model.get_relation(interface_name, relation_id)
requirer.relation = relation
requirer.local_app = harness.charm.app
return relation
Expand All @@ -33,6 +34,7 @@ def _requirer_provide_ingress_requirements(
host=socket.getfqdn(),
mode="http",
strip_prefix: bool = False,
per_app_relation: bool = False,
):
# same as requirer.provide_ingress_requirements(port=port, host=host)s
data = {
Expand All @@ -48,7 +50,7 @@ def _requirer_provide_ingress_requirements(

harness.update_relation_data(
relation.id,
"remote/0",
"remote" if per_app_relation else "remote/0",
data,
)
return data
Expand All @@ -71,7 +73,7 @@ def ingress(self):
@property
def url(self):
try:
return self.ingress["remote/0"]["url"]
return self.ingress.get("url", "") or self.ingress["remote/0"]["url"]
except: # noqa
return None

Expand Down Expand Up @@ -148,7 +150,6 @@ def test_pebble_ready_with_gateway_address_from_config_and_path_routing_mode(sel

expected = {
"http": {
"middlewares": None,
"routers": {
"juju-test-model-remote-0-router": {
"entryPoints": ["web"],
Expand All @@ -163,6 +164,7 @@ def test_pebble_ready_with_gateway_address_from_config_and_path_routing_mode(sel
},
}
}

if strip_prefix:
expected["http"].update(middlewares)
expected["http"]["routers"]["juju-test-model-remote-0-router"].update(
Expand All @@ -187,7 +189,7 @@ def test_pebble_ready_with_gateway_address_from_config_and_path_routing_mode_per

self.assertEqual(self.harness.charm.unit.status, ActiveStatus())

relation = relate(self.harness)
relation = relate(self.harness, per_app_relation=True)
for strip_prefix in (False, True):
with self.subTest():
_requirer_provide_ingress_requirements(
Expand All @@ -196,11 +198,13 @@ def test_pebble_ready_with_gateway_address_from_config_and_path_routing_mode_per
host="foo.bar",
port=3000,
strip_prefix=strip_prefix,
per_app_relation=True,
)

traefik_container = self.harness.charm.unit.get_container("traefik")
file = f"/opt/traefik/juju/juju_ingress_{relation.name}_{relation.id}_{relation.app.name}.yaml"
conf = yaml.safe_load(traefik_container.pull(file).read())

middlewares = {
"middlewares": {
"juju-sidecar-noprefix-test-model-remote-0": {
Expand All @@ -214,7 +218,6 @@ def test_pebble_ready_with_gateway_address_from_config_and_path_routing_mode_per

expected = {
"http": {
"middlewares": None,
"routers": {
"juju-test-model-remote-0-router": {
"entryPoints": ["web"],
Expand Down Expand Up @@ -257,7 +260,7 @@ def test_pebble_ready_with_gateway_address_from_config_and_subdomain_routing_mod

self.assertEqual(self.harness.charm.unit.status, ActiveStatus())

relation = relate(self.harness)
relation = relate(self.harness, per_app_relation=True)
for strip_prefix in (False, True):
# in subdomain routing mode this should not have any effect
with self.subTest():
Expand All @@ -267,6 +270,7 @@ def test_pebble_ready_with_gateway_address_from_config_and_subdomain_routing_mod
host="foo.bar",
port=3000,
strip_prefix=strip_prefix,
per_app_relation=True,
)

traefik_container = self.harness.charm.unit.get_container("traefik")
Expand All @@ -275,7 +279,6 @@ def test_pebble_ready_with_gateway_address_from_config_and_subdomain_routing_mod

expected = {
"http": {
"middlewares": None,
"routers": {
"juju-test-model-remote-0-router": {
"entryPoints": ["web"],
Expand Down Expand Up @@ -334,7 +337,6 @@ def test_pebble_ready_with_gateway_address_from_config_and_subdomain_routing_mod

expected = {
"http": {
"middlewares": None,
"routers": {
"juju-test-model-remote-0-router": {
"entryPoints": ["web"],
Expand Down Expand Up @@ -397,7 +399,6 @@ def test_pebble_ready_no_leader_with_gateway_address_from_config_and_subdomain_r

expected = {
"http": {
"middlewares": None,
"routers": {
"juju-test-model-remote-0-router": {
"entryPoints": ["web"],
Expand Down Expand Up @@ -582,9 +583,13 @@ def test_show_proxied_endpoints_action_only_ingress_per_app_relations(self):
self.harness.update_config({"external_hostname": "testhostname"})
self.harness.begin_with_initial_hooks()

relation = relate(self.harness)
relation = relate(self.harness, per_app_relation=True)
_requirer_provide_ingress_requirements(
harness=self.harness, relation=relation, host="10.0.0.1", port=3000
harness=self.harness,
relation=relation,
host="10.0.0.1",
port=3000,
per_app_relation=True,
)

self.harness.container_pebble_ready("traefik")
Expand All @@ -594,7 +599,7 @@ def test_show_proxied_endpoints_action_only_ingress_per_app_relations(self):
action_event.set_results.assert_called_once_with(
{
"proxied-endpoints": json.dumps(
{"remote/0": {"url": "http://testhostname:80/test-model-remote-0"}}
{"remote": {"url": "http://testhostname:80/test-model-remote-0"}}
)
}
)
Expand Down

0 comments on commit 6fddc92

Please sign in to comment.