IAM-286-Create-GLAuth-Operator #2

bencekov · 2023-06-27T03:53:23Z

Charmed operator for GLAuth

Implemented:

Base charm
config file template
COS Integration
Base charm unit test
Base charm integration test

Missing:

ldap_public lib charm
rockcraft.yaml
Actions
GLAuth users and groups insertion into the postgresql backend
Unit tests
Integration tests

…dap_public lib fix: unit test fixes, charm fixes

feat: unit tests verifying that ldap_public provider and users.py

natalian98

I think the repo should be renamed to glauth-k8s -operator (see this guideline).

Would it be possible to split this PR and open a separate one for the COS integration? I think it will ease the review process.

natalian98 · 2023-07-07T11:10:50Z

LICENSE

@@ -187,7 +187,7 @@
      same "printed page" as the copyright notice for easier
      identification within third-party archives.

-   Copyright 2023 identity-team
+   Copyright 2022 Canonical Ltd.


You should add the year in which the repo was first published.

Suggested change

Copyright 2022 Canonical Ltd.

Copyright 2023 Canonical Ltd.

natalian98 · 2023-07-07T11:12:20Z

README.md

+
+## Canonical Observability Stack
+
+This charm offers integration with observability tools in the [Canonical Observability Stack](https://charmhub.io/topics/canonical-observability-stack).


tbh I would split this pr and have the COS integration added in a separate one.

natalian98 · 2023-07-07T11:16:07Z

config.yaml

+  ldap_port:
+    description: |
+      Port on which LDAP service is accessible
+    type: string
+    default: "3893"
+  http_port:
+    description: |
+      Port on which HTTP services are accessible
+    type: string
+    default: "5555"


I'm not sure that we need to make the ports configurable, as we want to maintain only the necessary config options. We don't have that configurable in login ui, for example.

natalian98 · 2023-07-07T11:18:19Z

metadata.yaml

-    # is also used by the 'canonical/charming-actions' Github action for automated releasing.
-    upstream-source: kennethreitz/httpbin
+    description: GLAuth oci-image
+    upstream-source: glauth_postgres


There is no such image, which is why the CI is failing

natalian98 · 2023-07-07T11:19:05Z

src/utils.py

+# Copyright 2023 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+"""Utility functions for the Kratos charm."""


Suggested change

"""Utility functions for the Kratos charm."""

"""Utility functions for the glauth-k8s charm."""

config.yaml

natalian98 · 2023-07-07T11:27:39Z

tests/integration/test_charm.py

+        status="active",
+        raise_on_blocked=True,
+        timeout=1000,
+        wait_for_exact_units=3,


This is not needed because we are waiting for the "full" charm to become active anyways.

Suggested change

wait_for_exact_units=3,

natalian98 · 2023-07-07T11:28:13Z

tests/unit/test_charm.py

@@ -1,68 +1,216 @@
-# Copyright 2023 identity-team
+# Copyright 2022 Canonical Ltd.


Suggested change

# Copyright 2022 Canonical Ltd.

# Copyright 2023 Canonical Ltd.

natalian98 · 2023-07-07T11:42:39Z

src/charm.py

+            self,
+            relation_name=self._db_relation_name,
+            database_name=self._db_name,
+            extra_user_roles="SUPERUSER",


Could you put the extra user roles in a global variable?

natalian98 · 2023-07-07T11:43:18Z

src/charm.py


+class GlauthK8SCharm(CharmBase):
+    """Charmed GLAuth."""


maybe

Suggested change

"""Charmed GLAuth."""

"""Charmed GLAuth Operator for Kubernetes."""

?

natalian98 · 2023-07-07T11:48:41Z

src/charm.py

+        if not self._container.isdir(str(self._log_dir)):
+            self._container.make_dir(path=str(self._log_dir), make_parents=True)
+            logger.info(f"Created directory {self._log_dir}")


Could you move the comment to this block?

Suggested change

if not self._container.isdir(str(self._log_dir)):

self._container.make_dir(path=str(self._log_dir), make_parents=True)

logger.info(f"Created directory {self._log_dir}")

if not self._container.isdir(str(self._log_dir)):

# Create a directory for log forwarding

self._container.make_dir(path=str(self._log_dir), make_parents=True)

logger.info(f"Created directory {self._log_dir}")

natalian98 · 2023-07-07T11:51:26Z

src/charm.py

+    @property
+    def _glauth_service_is_running(self) -> bool:
+        if not self._container.can_connect():
+            return False
+
+        try:
+            service = self._container.get_service(self._container_name)
+        except (ModelError, RuntimeError):
+            return False
+        return service.is_running()


this property is not used anywhere

nsklikas · 2023-07-10T13:31:25Z

src/charm.py

            "services": {
-                "httpbin": {
+                self._container_name: {


Let's change this variable name to service_name (or something along those line) even it has the same value.

nsklikas · 2023-07-10T13:38:57Z

src/charm.py

+    def _on_database_created(self, event: DatabaseCreatedEvent) -> None:
+        """Event Handler for database created event."""
+        if not self._container.can_connect():
+            event.defer()
+            logger.info("Cannot connect to GLAuth container. Deferring event.")
+            self.unit.status = WaitingStatus("Waiting to connect to glauth container")
+            return
+
+        self.unit.status = MaintenanceStatus(
+            "Configuring container and resources for database connection"
+        )
+
+        try:
+            self._container.get_service(self._container_name)
+        except (ModelError, RuntimeError):
+            event.defer()
+            self.unit.status = WaitingStatus("Waiting for glauth service")
+            logger.info("GLAuth service is absent. Deferring database created event.")
+            return
+
+        logger.info("Updating GLAuth config and restarting service")
+        self._container.add_layer(self._container_name, self._pebble_layer, combine=True)
+        self._container.push(self._config_file_path, self._render_conf_file(), make_dirs=True)
+
+        self._container.start(self._container_name)
+        self.unit.status = ActiveStatus()


Why is this needed?

when the db is first connected, I use the container's start method. On other db changes and config changes I use the restart method. Not sure what happens when calling restart method on unstarted container. Let me know if you rather I invoked the _handle_status_update_config as response to DataBaseCreatedEvent.

As far as I can tell this is the same as the logic in _handle_status_update_config, let's not duplicate code unless we have to.

nsklikas · 2023-07-10T13:39:51Z

src/charm.py

+    def _metrics_external_url(self) -> str:
+        return normalise_url(self.ingress.url) if self.ingress.is_ready() else ""


Why is this needed?

The prometheus charm client has the option to use an external url to access scraping features. If there's an ingress relation, then we're passing the ingress url to prometheus, otherwise we pass the empty string instead of None, which is the default value for the external_url parameter.

Sorry I wasn't clear enough, we are exposing this URL to prometheus, why do we need to "normalise" it?

I assumed normalising the urls was like a new standard of how we share urls. But you're right, it's not necessary here.

nsklikas · 2023-07-10T13:40:04Z

src/utils.py

+def normalise_url(url: str) -> str:
+    """Convert a URL to a more user friendly HTTPS URL.
+
+    The user will be redirected to this URL, we need to use the https prefix
+    in order to be able to set cookies (secure attribute is set). Also we remove
+    the port from the URL to make it more user-friendly.
+
+    For example:
+        http://ingress:80 -> https://ingress
+        http://ingress:80/ -> https://ingress/
+        http://ingress:80/path/subpath -> https://ingress/path/subpath
+
+    This conversion works under the following assumptions:
+    1) The ingress will serve https under the 443 port, the user-agent will
+       implicitly make the request on that port
+    2) The provided URL is not a relative path
+    3) No user/password is provided in the netloc
+
+    This is a hack and should be removed once traefik provides a way for us to
+    request the https URL.
+    """
+    p = urlparse(url)
+
+    p = p._replace(scheme="https")
+    p = p._replace(netloc=p.netloc.rsplit(":", 1)[0])
+
+    return urlunparse(p)


Why is this needed?

I assumed this was how we were supposed to format ingress urls based on the hydra and kratos charms.

in Hydra and Kratos we do that because we expose those URLS to the user so we want to prettify them. Is this needed here?

I removed it.

nsklikas · 2023-07-10T13:45:43Z

src/charm.py

+    def _basedn(self) -> str:
+        # baseDN example: "dc=glauth,dc=com"
+        dn_input = self.config["base_dn"]
+        list_dns = dn_input.split(",")
+        list_dc = [f"dc={dn}" for dn in list_dns]
+        return ",".join(list_dc)


Any reason we don't expect the user to define the config with the proper syntax, ie: dc=example,dc=com. Since it is a pretty common config for LDAP I don't think we need to be doing this transformation.

I'm somewhat new to LDAP, but I've seen a bunch of docs where people use space instead of a coma, or use all caps for for dc, and ou and such. It would be probably better though to change it, and then normalize the input.

nsklikas · 2023-07-10T13:49:57Z

templates/glauth.cfg.j2

+  IgnoreCapabilities = {{ ignore_capabilities  }}
+  LimitFailedBinds = {{ limited_failed_binds }}
+  NumberOfFailedBinds = {{ number_of_failed_binds }}
+  PeriodOfFailedBinds = {{  period_of_failed_binds }}
+  BlockFailedBindsFor = {{ block_failed_binds_for }}
+  PruneSourceTableEvery = {{ prune_source_tables_every }}
+  PruneSourcesOlderThan = {{ prune_sources_older_than }}


Are we sure we want to expose these as config? Isn't it our job as charmers to configure these to make the user's life easier? (cc @massigori)

On one hand yes, however: there are default values for all these, so users don't have to interact with these configs if they don't want to. If we don't make these configs exportable, then it's really hard to change them for the user.

nsklikas · 2023-07-10T13:58:08Z

src/charm.py

+        self._ingress_relation_name = "ingress"
+
+        self.service_patcher = KubernetesServicePatch(
+            self, [(self.app.name, int(self._ldap_port))]


Shouldn't we patch the api port as well?

So the API port is something we only use for prometheus metrics. I'm under the impression that the http server is something we only want to use if it's behind a proxy. With that being said I can patch that port too.

src/charm.py

nsklikas · 2023-10-12T10:58:07Z

Shall we close this in favor of #15?

bencekov added 7 commits June 26, 2023 21:49

feat: first draft for base charm

abef3a3

fix: fixed tox settings and linting errors

5da5ac1

feat: ldap_public library implements updated spec, charm implements l…

1291ea3

…dap_public lib fix: unit test fixes, charm fixes

fix: bugs in charm.py and ldap_public.py

44904ed

feat: unit tests verifying that ldap_public provider and users.py

fix: Working base charm for GLAuth without rockcraft.yaml

e103859

fix: fixed linting

118ede6

feat: Added README.md

c12e2be

bencekov marked this pull request as ready for review July 7, 2023 01:52

fix: fix linting in README

30bd68a

natalian98 reviewed Jul 7, 2023

View reviewed changes

fix: Added code owners

2e330f2

natalian98 requested a review from a team July 10, 2023 13:26

nsklikas requested changes Jul 10, 2023

View reviewed changes

bencekov added 2 commits July 10, 2023 12:34

fix: fixed comments in PR #2

f6ec509

fix: Added Kubernetes service patch to http port

b682fc4

natalian98 requested a review from a team July 13, 2023 15:58

natalian98 reviewed Jul 13, 2023

View reviewed changes

src/charm.py Outdated Show resolved Hide resolved

natalian98 requested a review from a team July 13, 2023 16:04

fix: minor fixes based on comments

eacaede

shipperizer closed this Nov 8, 2023

shipperizer deleted the IAM-286-Create-Glauth-Operator branch November 8, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IAM-286-Create-GLAuth-Operator #2

IAM-286-Create-GLAuth-Operator #2

bencekov commented Jun 27, 2023 •

edited

Loading

natalian98 left a comment

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023 •

edited

Loading

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

natalian98 Jul 7, 2023

nsklikas Jul 10, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas Jul 11, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas Jul 14, 2023

bencekov Jul 17, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas Jul 11, 2023

bencekov Jul 17, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas Jul 10, 2023

bencekov Jul 10, 2023

nsklikas commented Oct 12, 2023


		## Canonical Observability Stack

		This charm offers integration with observability tools in the [Canonical Observability Stack](https://charmhub.io/topics/canonical-observability-stack).

	"""Utility functions for the Kratos charm."""
	"""Utility functions for the glauth-k8s charm."""

		@@ -1,68 +1,216 @@
		# Copyright 2023 identity-team
		# Copyright 2022 Canonical Ltd.

	# Copyright 2022 Canonical Ltd.
	# Copyright 2023 Canonical Ltd.

	"""Charmed GLAuth."""
	"""Charmed GLAuth Operator for Kubernetes."""

		def _metrics_external_url(self) -> str:
		return normalise_url(self.ingress.url) if self.ingress.is_ready() else ""

IAM-286-Create-GLAuth-Operator #2

IAM-286-Create-GLAuth-Operator #2

Conversation

bencekov commented Jun 27, 2023 • edited Loading

natalian98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalian98 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsklikas commented Oct 12, 2023

bencekov commented Jun 27, 2023 •

edited

Loading

natalian98 Jul 7, 2023 •

edited

Loading