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.

Sign up for GitHub

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

Fix per-app middleware injection #114

Merged
merged 3 commits into from
Dec 12, 2022
Merged

Fix per-app middleware injection #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7f6cc9
Fix per-app middleware injection
rbarry82 Dec 10, 2022
a94a35b
Fix the unit tests. Fix URL refreshing WITHOUT WAITING FOR AN EVENT, …
rbarry82 Dec 10, 2022
a46c1a8
Extract config generation
rbarry82 Dec 10, 2022
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
4 changes: 2 additions & 2 deletions lib/charms/traefik_k8s/v1/ingress.py
View file
Open in desktop
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
Open in desktop
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())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference here is pretty marginal, but if you look below, the additional selector of traefik_router_name is necessary for this to actually work.


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
Open in desktop
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