feat!: add ingress_per_unit integration to glauth-k8s-operator #78

NucciTheBoss · 2024-12-17T03:31:12Z

This PR adds the ingress_per_unit integration to glauth-k8s so that the glauth TCP service can be addressable from machine charms outside of the Kubernetes cluster. Now, when a machine charm wants to connect to glauth for LDAP goodness, all you need to do is integrate glauth-k8s with traefik:

# Enable LoadBalancer implementation for your k8s cloud.
# In testing I used MetalLB with an IPADDR range partitioned off of my bridged LXD network.
microk8s enable metallb:<IPADDR range>  # Can also use Canonical k8s.

# Do the necessary setup by integrating with `postgresql-k8s` and `self-signed-certificates`...
juju deploy glauth-k8s --trust
juju deploy traefik-k8s --trust
juju integrate glauth-k8s:ingress traefik-k8s
# glauth TCP endpoint will be proxied by traefik and reachable from machine charms such as SSSD.

Breaking changes

One thing to note is that I updated the function signature of the ldap_url to return List[str] instead of str. I don't think it's that much of an issue as ldap_url is only used two times within the operator, and its returned value was casted to List[str] to satisfy the type requirements for the urls field in the ldap interface.

Related issues

Fixes Ingress enablement for GLAuth charm #76

Signed-off-by: Jason C. Nucciarone <[email protected]>

BREAKING CHANGES: Updates the return type of the `ldap_url` property from `LdapIntegration` to `List[str]` from `str` to support retrieving multiple ingress urls for `glauth-k8s`. Ingress is required for glauth-k8s to be publicly addressable from outside Kubernetes. Does not change the `ldap` interface implementation as `url` was already provided as a list object within the `ldap` charm library, so it just moves the `List[str]` type casting in `provider_data`. Signed-off-by: Jason C. Nucciarone <[email protected]>

wood-push-melon · 2024-12-17T20:27:09Z

src/integrations.py

+    def ldap_url(self) -> List[str]:
+        if ingress := self._charm.ingress_per_unit.urls:
+            return [f"ldap://{url}" for url in ingress.values()]
+        else:


Just a minor nitpick: We don't need the else.

wood-push-melon · 2024-12-17T20:27:11Z

src/integrations.py

-    def ldap_url(self) -> str:
-        hostname = self._charm.config.get("hostname") or socket.getfqdn()
-        return f"ldap://{hostname}:{GLAUTH_LDAP_PORT}"
+    def ldap_url(self) -> List[str]:


Just a minor nitpick on naming: probably then we could rename it to ldap_urls?

(I could be wrong) Another minor thing is about the reason that ldap_url was changed to a container type initially. @nsklikas has more authority to clarify that. I remember it's somehow related to glauth proxy usage.

What I worry here is that this data container will represent inconsistent meanings when integrating with traefik or not (in a proxy mode).

Let me know if I am wrong :)

Yes, it was related to https://github.com/canonical/ldap-integrator/.

The reason is that glauth accepts a list of URLs when configuring it to acts as an ldap proxy (see https://glauth.github.io/docs/backends.html). When making an ldap request to the external server, tries to call them one by one, until one the requests succeeds.

I agree with the name change

Renaming the property to ldap_urls sounds good to me. Getting a list of possible endpoints for the LDAP server is also ideal for SSSD as you can configure to switch server endpoints if one endpoint fails: https://manpages.ubuntu.com/manpages/oracular/en/man5/sssd-ldap.5.html#failover.

Glauth high-availability is also achieved by having multiple running instances of Glauth functioning as their own individual endpoint, but share the same database backend.

wood-push-melon · 2024-12-17T20:27:15Z

src/charm.py

@@ -97,6 +98,13 @@ def __init__(self, *args: Any):
            extra_user_roles="SUPERUSER",
        )

+        self.ingress_per_unit = IngressPerUnitRequirer(
+            self,
+            "ingress",


Just a minor nitpick:

We usually put constant integration names in the constants.py. It would be nice to follow that. Thank you.

wood-push-melon · 2024-12-17T20:32:43Z

LGTM overall. Thank you for the contributions.

Just have a small concern about that ldap_url container. I'll let other team members review.

wood-push-melon · 2024-12-17T21:21:47Z

src/charm.py

@@ -97,6 +98,13 @@ def __init__(self, *args: Any):
            extra_user_roles="SUPERUSER",
        )

+        self.ingress_per_unit = IngressPerUnitRequirer(


Just have a question:

Do we need to subscribe the ready and revoked events to update the ldap integration databag if there are any existing ldap integrations?

Hmm... we likely should handle those events as this will change the address requirers utilize to contact the Glauth server. Simple netcat scan indicates that other pods can communicate with the open port on the on the load balancer:

# 10.0.0.135 is the address that traefik-k8s grabbed from MetalLB. root@loki-0:/$ nc -zv 10.0.0.135 3893 Connection to 10.0.0.135 3893 port [tcp/*] succeeded!

We'll need to check if any requirers have joined the ldap relation and update the databag.

wood-push-melon · 2024-12-17T22:44:04Z

Followed the steps, and encountered an error from traefik when I scaled up glauth. The health check of traefik also fails. scaling down to 1 unit turns traefik back to healthy.

$ k -n testing exec traefik-k8s-0 -c traefik -- /bin/traefik healthcheck

time="2024-12-17T22:36:58Z" level=info msg="Configuration loaded from file: /etc/traefik/traefik.yaml"
Error calling healthcheck: Head "http://:8082/ping": dial tcp :8082: connect: connection refused
command terminated with exit code 1

Not sure if this only happens to me. Already let Jason know.

wood-push-melon · 2024-12-18T15:33:16Z

Followed the steps, and encountered an error from traefik when I scaled up glauth. The health check of traefik also fails. scaling down to 1 unit turns traefik back to healthy.
$ k -n testing exec traefik-k8s-0 -c traefik -- /bin/traefik healthcheck

time="2024-12-17T22:36:58Z" level=info msg="Configuration loaded from file: /etc/traefik/traefik.yaml"
Error calling healthcheck: Head "http://:8082/ping": dial tcp :8082: connect: connection refused
command terminated with exit code 1
Not sure if this only happens to me. Already let Jason know.

Someone also hit this problem: canonical/traefik-k8s-operator#406

This is due to the fact that traefik ingress-per-unit TCP model in current implementation will use the port (passed from the requirer charm) to open the listening port for exposing the unit. If more units come up, traefik will complain because it tries to open the same port for other units.

NucciTheBoss · 2024-12-20T20:18:24Z

This is due to the fact that traefik ingress-per-unit TCP model in current implementation will use the port (passed from the requirer charm) to open the listening port for exposing the unit. If more units come up, traefik will complain because it tries to open the same port for other units.

Easiest fix here is just not scaling glauth-k8s until canonical/traefik-k8s-operator#406 is fixed 😅

This shouldn't be a blocking requirement for merging this PR, but it should be fixed in traefik-k8s since other charms are running into this issue. Given that the ingress urls already have the port provided in the returned urls, might just make sense for traefik-k8s to just grab random port numbers and provide them to requirers rather than have the requirers specify which port to use themselves.

NucciTheBoss added 2 commits December 16, 2024 22:12

feat: add ingress_per_unit charm library

0d7c063

Signed-off-by: Jason C. Nucciarone <[email protected]>

NucciTheBoss requested a review from a team as a code owner December 17, 2024 03:31

NucciTheBoss mentioned this pull request Dec 17, 2024

feat: add anonymousdse_enabled config option #79

Merged

wood-push-melon reviewed Dec 17, 2024

View reviewed changes

wood-push-melon mentioned this pull request Dec 17, 2024

Add integration test for ingress feature #80

Open

Merge branch 'canonical:main' into nuccitheboss/feat/ingress-per-unit

0cea2b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add ingress_per_unit integration to glauth-k8s-operator #78

feat!: add ingress_per_unit integration to glauth-k8s-operator #78

NucciTheBoss commented Dec 17, 2024 •

edited

Loading

wood-push-melon Dec 17, 2024

wood-push-melon Dec 17, 2024

nsklikas Dec 18, 2024

NucciTheBoss Dec 20, 2024

wood-push-melon Dec 17, 2024

wood-push-melon commented Dec 17, 2024

wood-push-melon Dec 17, 2024

NucciTheBoss Dec 20, 2024 •

edited

Loading

wood-push-melon commented Dec 17, 2024

wood-push-melon commented Dec 18, 2024 •

edited

Loading

NucciTheBoss commented Dec 20, 2024 •

edited

Loading

feat!: add ingress_per_unit integration to glauth-k8s-operator #78

Are you sure you want to change the base?

feat!: add ingress_per_unit integration to glauth-k8s-operator #78

Conversation

NucciTheBoss commented Dec 17, 2024 • edited Loading

Breaking changes

Related issues

wood-push-melon Dec 17, 2024

Choose a reason for hiding this comment

wood-push-melon Dec 17, 2024

Choose a reason for hiding this comment

nsklikas Dec 18, 2024

Choose a reason for hiding this comment

NucciTheBoss Dec 20, 2024

Choose a reason for hiding this comment

wood-push-melon Dec 17, 2024

Choose a reason for hiding this comment

wood-push-melon commented Dec 17, 2024

wood-push-melon Dec 17, 2024

Choose a reason for hiding this comment

NucciTheBoss Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

wood-push-melon commented Dec 17, 2024

wood-push-melon commented Dec 18, 2024 • edited Loading

NucciTheBoss commented Dec 20, 2024 • edited Loading

NucciTheBoss commented Dec 17, 2024 •

edited

Loading

NucciTheBoss Dec 20, 2024 •

edited

Loading

wood-push-melon commented Dec 18, 2024 •

edited

Loading

NucciTheBoss commented Dec 20, 2024 •

edited

Loading