From d20f197230bba01aee471847f9d661cd07b7fd38 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Thu, 11 Jul 2024 19:15:22 +0200 Subject: [PATCH 1/5] refactor: add dex-issuer-url and remove public-url config options This commit removes the public-url configuration option in favour of the dex-issuer-url one. The way to configure the issuer value for dex-auth is now by getting it from the aforementioned configuration option or by constructing it from dex-auths Kubernetes Service DNS name: "http://..svc:5556/dex" Closes #204 --- config.yaml | 16 +++++++--- metadata.yaml | 2 ++ src/charm.py | 19 ++++++++---- tests/integration/test_charm.py | 1 - tests/unit/test_charm.py | 52 +++++++++++++++++++++++++++++---- 5 files changed, 75 insertions(+), 15 deletions(-) diff --git a/config.yaml b/config.yaml index c4804304..f6ca5201 100644 --- a/config.yaml +++ b/config.yaml @@ -6,14 +6,22 @@ options: type: boolean default: true description: Allows dex to keep a list of passwords which can be used to login to dex + issuer-url: + type: string + default: '' + description: | + Format http(s):///dex + (Also referred to as issuer or OIDC provider ) This is the canonical URL that OIDC clients + MUST use to refer to dex. If not specified, it defaults to dex-auth's local + endpoint constructed from dex-auth's Kubernetes Service DNS name, the + Service port and Dex's endpoint, that is http://..svc:5556/dex. + The default is set by the charm code, not the configuration option. + This configuration must be set when using a Dex connector that will try to reach Dex from outside + the cluster, thus it should be a publicly accessible endpoint, for example https://my-instance.in-my-cloud.some-cloud.com/dex port: type: int default: 5556 description: Listening port - public-url: - type: string - default: '' - description: Publicly-accessible endpoint for cluster connectors: type: string default: '' diff --git a/metadata.yaml b/metadata.yaml index 1290c272..96280bd5 100755 --- a/metadata.yaml +++ b/metadata.yaml @@ -81,6 +81,8 @@ provides: interface: prometheus_scrape grafana-dashboard: interface: grafana_dashboard + dex-oidc-config: + interface: dex-oidc-config containers: dex: resource: oci-image diff --git a/src/charm.py b/src/charm.py index d8028970..44a31265 100755 --- a/src/charm.py +++ b/src/charm.py @@ -10,6 +10,7 @@ import bcrypt import yaml from charmed_kubeflow_chisme.exceptions import ErrorWithStatus +from charms.dex_auth.v0.dex_oidc_config import DexOidcConfigProvider from charms.grafana_k8s.v0.grafana_dashboard import GrafanaDashboardProvider from charms.loki_k8s.v1.loki_push_api import LogForwarder from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch @@ -33,11 +34,16 @@ def __init__(self, *args): super().__init__(*args) self.logger: logging.Logger = logging.getLogger(__name__) + self._namespace = self.model.name + self._app_name = self.model.app.name # Patch the service to correctly expose the ports to be used dex_port = ServicePort(int(self.model.config["port"]), name="dex") metrics_port = ServicePort(int(METRICS_PORT), name="metrics-port") + # Instantiate a DexOidcConfigProvider to share this app's issuer_url + self.dex_oidc_config_provider = DexOidcConfigProvider(self, issuer_url=self._issuer_url) + self.service_patcher = KubernetesServicePatch( self, [dex_port, metrics_port], @@ -58,7 +64,6 @@ def __init__(self, *args): self._logging = LogForwarder(charm=self) self._container_name = "dex" - self._namespace = self.model.name self._container = self.unit.get_container(self._container_name) self._entrypoint = "/usr/local/bin/docker-entrypoint" self._dex_config_path = "/etc/dex/config.docker.yaml" @@ -95,6 +100,13 @@ def _dex_auth_layer(self) -> Layer: } return Layer(layer_config) + @property + def _issuer_url(self) -> str: + """Return issuer-url value if config option exists; otherwise default Dex's endpoint.""" + if issuer_url := self.model.config["issuer-url"]: + return issuer_url + return f"http://{self._app_name}.{self._namespace}.svc:5556/dex" + def _update_layer(self) -> None: """Updates the Pebble configuration layer if changed.""" self._check_container_connection() @@ -192,9 +204,6 @@ def _generate_dex_auth_config(self) -> str: # Load config values as convenient variables connectors = yaml.safe_load(self.model.config["connectors"]) port = self.model.config["port"] - public_url = self.model.config["public-url"].lower() - if not public_url.startswith(("http://", "https://")): - public_url = f"http://{public_url}" enable_password_db = self.model.config["enable-password-db"] static_config = { @@ -227,7 +236,7 @@ def _generate_dex_auth_config(self) -> str: config = yaml.dump( { - "issuer": f"{public_url}/dex", + "issuer": self._issuer_url, "storage": {"type": "kubernetes", "config": {"inCluster": True}}, "web": {"http": f"0.0.0.0:{port}"}, "telemetry": {"http": "0.0.0.0:5558"}, diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 46850e7d..207db94c 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -164,7 +164,6 @@ async def test_relations(ops_test: OpsTest): namespace=ops_test.model_name, ) log.info(f"got public_url of {public_url}") - await ops_test.model.applications[DEX_AUTH_APP_NAME].set_config({"public-url": public_url}) await ops_test.model.applications[OIDC_GATEKEEPER].set_config({"public-url": public_url}) await ops_test.model.wait_for_idle( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 624003b8..0813dc23 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -11,6 +11,8 @@ from charm import Operator +DEX_OIDC_CONFIG_RELATION = "dex-oidc-config" + @pytest.fixture def harness(): @@ -69,7 +71,6 @@ def test_generate_dex_auth_config_raises(harness): config_updates = { "enable-password-db": False, "port": 5555, - "public-url": "dummy.url", } harness.update_config(config_updates) @@ -89,13 +90,11 @@ def test_generate_dex_auth_config_raises(harness): { "enable-password-db": False, "port": 5555, - "public-url": "dummy.url", "connectors": "test-connector", }, { "enable-password-db": True, "port": 5555, - "public-url": "dummy.url", "static-username": "new-user", "static-password": "new-pass", }, @@ -141,7 +140,6 @@ def test_disable_static_login_no_connector_blocked_status(harness): config_updates = { "enable-password-db": False, "port": 5555, - "public-url": "dummy.url", } harness.update_config(config_updates) @@ -156,8 +154,8 @@ def test_config_changed(update, harness): config_updates = { "enable-password-db": False, + "issuer-url": "http://my-dex.io/dex", "port": 5555, - "public-url": "dummy.url", "connectors": "connector01", "static-username": "new-user", "static-password": "new-pass", @@ -219,3 +217,47 @@ def test_main_ingress(update, harness): } assert data == yaml.safe_load(relation_data["data"]) + + +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) +def test_dex_oidc_config_with_data(harness): + """Test the relation data has values by default as the charm is broadcasting them.""" + harness.set_leader(True) + harness.begin() + + rel_id = harness.add_relation(DEX_OIDC_CONFIG_RELATION, "app") + rel_data = harness.model.get_relation(DEX_OIDC_CONFIG_RELATION, rel_id).data[harness.model.app] + + # Default values are expected + expected = {"issuer-url": f"http://{harness.model.app.name}.{harness.model.name}.svc:5556/dex"} + assert rel_data["issuer-url"] == expected["issuer-url"] + + +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) +def test_grpc_relation_with_data_when_data_changes( + harness, +): + """Test the relation data on config changed events.""" + harness.set_leader(True) + # Change the configuration option before starting harness so + # the correct values are passed to the K8sServiceInfo lib + # FIXME: the correct behaviour should be to change the config + # at any point in time to trigger config events and check the + # value gets passed correctly when it changes. + harness.update_config({"issuer-url": "http://my-dex.io/dex"}) + harness.begin() + + # Initialise a k8s-service requirer charm + # harness.charm.leadership_gate.get_status = MagicMock(return_value=ActiveStatus()) + # harness.charm.kubernetes_resources.get_status = MagicMock(return_value=ActiveStatus()) + + # Add relation between the requirer charm and this charm (dex-auth) + provider_rel_id = harness.add_relation( + relation_name=DEX_OIDC_CONFIG_RELATION, remote_app="other-app" + ) + provider_rel_data = harness.get_relation_data( + relation_id=provider_rel_id, app_or_unit=harness.charm.app.name + ) + + # Change the port of the service and check the value changes + assert provider_rel_data["issuer-url"] == harness.model.config["issuer-url"] From 2308669c6923994d8d090a84dbf4590814018730 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Mon, 22 Jul 2024 11:23:59 +0200 Subject: [PATCH 2/5] skip: correct name --- src/charm.py | 5 +++-- tests/unit/test_charm.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/charm.py b/src/charm.py index 44a31265..5c843180 100755 --- a/src/charm.py +++ b/src/charm.py @@ -35,7 +35,6 @@ def __init__(self, *args): self.logger: logging.Logger = logging.getLogger(__name__) self._namespace = self.model.name - self._app_name = self.model.app.name # Patch the service to correctly expose the ports to be used dex_port = ServicePort(int(self.model.config["port"]), name="dex") @@ -105,7 +104,9 @@ def _issuer_url(self) -> str: """Return issuer-url value if config option exists; otherwise default Dex's endpoint.""" if issuer_url := self.model.config["issuer-url"]: return issuer_url - return f"http://{self._app_name}.{self._namespace}.svc:5556/dex" + return ( + f"http://{self.model.app.name}.{self._namespace}.svc:{self.model.config['port']}/dex" + ) def _update_layer(self) -> None: """Updates the Pebble configuration layer if changed.""" diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 0813dc23..93735619 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -240,14 +240,14 @@ def test_grpc_relation_with_data_when_data_changes( """Test the relation data on config changed events.""" harness.set_leader(True) # Change the configuration option before starting harness so - # the correct values are passed to the K8sServiceInfo lib + # the correct values are passed to the DexOidcConfig lib # FIXME: the correct behaviour should be to change the config # at any point in time to trigger config events and check the # value gets passed correctly when it changes. harness.update_config({"issuer-url": "http://my-dex.io/dex"}) harness.begin() - # Initialise a k8s-service requirer charm + # Initialise a dex-oidc-config requirer charm # harness.charm.leadership_gate.get_status = MagicMock(return_value=ActiveStatus()) # harness.charm.kubernetes_resources.get_status = MagicMock(return_value=ActiveStatus()) From 06de9d025b329d89bc4adc34b9f12dc45d9f3c62 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Mon, 22 Jul 2024 15:09:00 +0200 Subject: [PATCH 3/5] skip: add build-packages --- charmcraft.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 855e28b8..0c2ab583 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -14,4 +14,4 @@ parts: charm-python-packages: [setuptools, pip] # Install rustc and cargo as build packages because some charm's # dependencies need this to be built and installed from source. - build-packages: [rustc, cargo] + build-packages: [cargo, rustc, pkg-config, libffi-dev, libssl-dev] From 6279209ac5c39ec9d48230d384e93efbea7db53e Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Tue, 23 Jul 2024 10:06:33 +0200 Subject: [PATCH 4/5] skip: add workaround for oidc config --- tests/integration/test_charm.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 207db94c..b4e75d4b 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -155,15 +155,13 @@ async def test_relations(ops_test: OpsTest): await ops_test.model.add_relation(KUBEFLOW_PROFILES, KUBEFLOW_DASHBOARD) await ops_test.model.add_relation(f"{ISTIO_PILOT}:ingress", f"{KUBEFLOW_DASHBOARD}:ingress") - # Set public-url for dex and oidc - # Note: This could be affected by a race condition (if service has not received - # an IP yet, this could fail) but probably this won't happen in practice - # because that IP is delivered quickly and we wait_for_idle on the istio_gateway - public_url = get_public_url( - service_name="istio-ingressgateway-workload", - namespace=ops_test.model_name, - ) - log.info(f"got public_url of {public_url}") + # Set public-url for oidc + # Leaving the default value of Dex's Kubernetes Service temporarily + # FIXME: remove this configuration option after canonical/oidc-gatekeeper-operator/pull/164 and + # canonical/oidc-gatekeeper-operator/pull/163 get merged + # After that, we should integrate dex-auth and oidc-gatekeeper. + # See canonical/oidc-gatekeeper-operator#157 for more details + public_url = f"http://{DEX_AUTH_APP_NAME}.{ops_test.model_name}.svc:5556/dex" await ops_test.model.applications[OIDC_GATEKEEPER].set_config({"public-url": public_url}) await ops_test.model.wait_for_idle( From 63eb4f41054ca39049761986ce656a44c8242595 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Tue, 23 Jul 2024 11:23:41 +0200 Subject: [PATCH 5/5] skip: fix typo --- tests/integration/test_charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index b4e75d4b..7e950ccb 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -161,7 +161,7 @@ async def test_relations(ops_test: OpsTest): # canonical/oidc-gatekeeper-operator/pull/163 get merged # After that, we should integrate dex-auth and oidc-gatekeeper. # See canonical/oidc-gatekeeper-operator#157 for more details - public_url = f"http://{DEX_AUTH_APP_NAME}.{ops_test.model_name}.svc:5556/dex" + public_url = f"http://{DEX_AUTH_APP_NAME}.{ops_test.model_name}.svc:5556" await ops_test.model.applications[OIDC_GATEKEEPER].set_config({"public-url": public_url}) await ops_test.model.wait_for_idle(