Add juju config and logic for creating traefik routes #3

natalian98 · 2023-08-24T12:05:27Z

This PR adds:

oathkeeper-related config options (access rules, headers)
traefik-related config options (root_url, rule)
config validation
logic for generating routing rules with forwardauth middleware for charms related via ingress-per-unit interface

The further work will be to support generating rules for non-charmed workloads.

Testing Instructions

charmcraft pack
juju deploy ./oathkeeper-configurator*.charm
juju config oathkeeper-configurator access_rules=@access_rules.json root_url=http://foo.bar/{{juju_model}}-{{juju_unit}}

where access_rules.json is a file that contains the rules.

The charm will get blocked unless you deploy any charm that supports ingress-per-unit relation, for example prometheus-k8s:

juju deploy prometheus-k8s
juju relate prometheus-k8s oathkeeper-configurator

This is because the configurator charm requires some configs which come from that relation to set up the routes (model, unit names, host, port).
When the charm status becomes active, you should see the rendered rule in logs. If you scale up prometheus it should also get updated to include multiple units:

Generated unit configs: {'http': {'routers': {'juju-prometheus-k8s-0-test1-router': {'rule': 'Host(`foo.bar`)', 'service': 'juju-prometheus-k8s-0-test1-service', 'entryPoints': ['web'], 'middlewares': ['oathkeeper']}, 'juju-prometheus-k8s-1-test1-router': {'rule': 'Host(`foo.bar`)', 'service': 'juju-prometheus-k8s-1-test1-service', 'entryPoints': ['web'], 'middlewares': ['oathkeeper']}}, 'services': {'juju-prometheus-k8s-0-test1-service': {'loadBalancer': {'servers': [{'url': 'http://foo.bar/test1-prometheus-k8s-0'}]}}, 'juju-prometheus-k8s-1-test1-service': {'loadBalancer': {'servers': [{'url': 'http://foo.bar/test1-prometheus-k8s-1'}]}}}}}

src/charm.py

wood-push-melon · 2023-08-28T21:18:51Z

src/charm.py

+    rule: Optional[str] = None
+
+    @property
+    def is_valid(self) -> bool:


Have a question about the attribute validation logic: Do you think it is better if we implement it via a descriptor class?

The attribute validation logic is based on the traefik-route charm.
For simplicity of maintenance (and solving possible bugs), I would keep this part as in the "upstream" charm.

oh, I see that you are referring to this part: https://github.com/canonical/traefik-route-k8s-operator/blob/main/src/charm.py#L69-L108, right? Probably I don't understand the biz logic. But yeah, go ahead to keep this part if it is easy to maintain.

wood-push-melon · 2023-08-28T21:18:54Z

src/charm.py

+                    f"{name}=<{name.upper()}>"
+                )
+
+            elif obj != (stripped := obj.strip()):


Maybe str.startswith and str.endswith?

Another thing is that:
Is this rule very important to us in the biz logic? If not, probably it would be better if we could skip verifying this by normalizing the attribute based on some rules (e.g. do not start or end with whitespaces) for users. We can normalize it when we set the attribute using the descriptor.

src/charm.py

wood-push-melon · 2023-08-28T21:19:07Z

src/charm.py

+        return f"Host(`{url_.hostname}`)"
+
+
+def _is_not_empty(config: str) -> bool:


Nitpick:

Probably we can name it with a better function name, e.g. _is_access_rule_xxx?

I'm using it for both access rules and root_url configs. Does _is_config_not_empty sound better?

yeah, sure.

src/charm.py

wood-push-melon · 2023-08-28T21:19:15Z

src/charm.py

+        return self._config.render(model_name=model_name, unit_name=unit_name, app_name=app_name)
+
+    @staticmethod
+    def _generate_traefik_unit_config(config: RouteConfig) -> UnitConfig:


It looks like the logic about traefik unit config generation, merging, checking if it is ready, etc. is nothing to do with the charm itself. Do you think it is better we extract them into an individual class?

I could extract some of that logic into an individual class, but most of it is coupled with the charm, e.g. when it relies on charm relations, config or unit status, or uses charm's stored state to store info on config's validity.

src/charm.py

nsklikas

I left some comments, most of them are related to the code style and you can ignore them if you disagree.

What I fail to understand is what is this charm's relation to the traefik-route-k8s-operator, it looks like it tries to serve a superset of its functionality. I also fail to see how the config is supposed to work.

src/charm.py

nsklikas · 2023-08-29T10:21:54Z

config.yaml

+  rule:
+    default:
+    description: |
+      A Traefik routing rule, see https://doc.traefik.io/traefik/routing/routers/ for
+      an overview.
+
+      The value of the field is going to be processed as a Jinja2 template, with the
+      following globals available:
+       - {{juju_model}} resolves to the model name of the downstream proxied 
+         application.
+       - {{juju_application}} resolves to the application name of the downstream 
+         proxied application.
+       - {{juju_unit}} resolves to the unit name of the downstream proxied unit;
+         to avoid issues when used together with the Host directive or similar, 
+         the slash character between application name and unit index is replaced by a dash.
+
+      For example, given a downstream unit called `prometheus/0` in the `cos` model, the following:
+
+        Host(`foo.bar/{{juju_unit}}-{{juju_model}}`)
+
+      would evaluate to:
+
+        Host(`foo.bar/cos-prometheus-0`)
+
+      If the host is omitted, but the root_url is provided, the charm will 
+      extract the hostname from the url and generate a Host rule for you.
+        root_url=`http://{{juju_unit}}.bar.baz:80/qux`
+        --> rule=Host(`{{juju_unit}}.bar.baz`)
+    type: string


Not sure why those are needed, shouldn't we get those from a relation??

The rule will be generated from root_url if the first is not provided (with the globals coming from the relation), although if an admin wants to configure the rules differently, this config option provides that possibility. I'm trying to follow the logic of traefik-route-k8s here as the charm is tested against edge cases.
It's also a ground for non-charmed workloads as I imagine the rules will have to be supplied via config.

src/charm.py

src/types_.py

natalian98 · 2023-08-30T10:38:40Z

What I fail to understand is what is this charm's relation to the traefik-route-k8s-operator, it looks like it tries to serve a superset of its functionality. I also fail to see how the config is supposed to work.

This charm is in a way a superset of traefik-route-k8s charm: following the discussion on ID006 spec, we decided to use traefik_route interface as the base for "ingress_auth" (see this diagram)
Using traefik_route entails that the configurator charm will have to be responsible for creating the routes and configuring the middlewares for traefik. The routes will then be sent to traefik-k8s via the ingress_auth interface.
For this reason, this charm includes the traefik-route's logic for rendering traefik rules, with an addition of the forwardauth middleware and oathkeeper-related config.
The headers and access rules will also be sent to traefik and then to oathkeeper (more info in the spec).

natalian98 · 2023-09-13T08:25:47Z

Closing as the repo will be archived due to IAP redesign.

natalian98 added 8 commits August 23, 2023 18:27

feat: add config options and routes creation logic

e8e4888

refactor: modify or delete log messages

42e3626

test: minor unit tests changes

2cc3c17

refactor: minor updates

52f006b

docs: update readme

47f4493

fix: install jsonschema from binary

b42dbd5

refactor: remove unused methods

2c8942a

test: add unit test for rule rendering

f78b3cf

natalian98 marked this pull request as ready for review August 24, 2023 14:13

natalian98 requested a review from a team as a code owner August 24, 2023 14:13