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.

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

refactor: add dex-issuer-url and remove public-url config options #209

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion charmcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
16 changes: 12 additions & 4 deletions config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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)://<publicly-accessible-dns-name>/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://<dex-auth-app-name>.<namespace>.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: ''
Expand Down
2 changes: 2 additions & 0 deletions metadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 15 additions & 5 deletions src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -33,11 +34,15 @@ def __init__(self, *args):
super().__init__(*args)

self.logger: logging.Logger = logging.getLogger(__name__)
self._namespace = self.model.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],
Expand All @@ -58,7 +63,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"
Expand Down Expand Up @@ -95,6 +99,15 @@ 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.model.app.name}.{self._namespace}.svc:{self.model.config['port']}/dex"
)

def _update_layer(self) -> None:
"""Updates the Pebble configuration layer if changed."""
self._check_container_connection()
Expand Down Expand Up @@ -192,9 +205,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 = {
Expand Down Expand Up @@ -227,7 +237,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"},
Expand Down
17 changes: 7 additions & 10 deletions tests/integration/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,16 +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}")
await ops_test.model.applications[DEX_AUTH_APP_NAME].set_config({"public-url": 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"
await ops_test.model.applications[OIDC_GATEKEEPER].set_config({"public-url": public_url})

await ops_test.model.wait_for_idle(
Expand Down
52 changes: 47 additions & 5 deletions tests/unit/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

from charm import Operator

DEX_OIDC_CONFIG_RELATION = "dex-oidc-config"


@pytest.fixture
def harness():
Expand Down Expand Up @@ -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)
Expand All @@ -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",
},
Expand Down Expand Up @@ -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)
Expand All @@ -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",
Expand Down Expand Up @@ -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 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.
Comment on lines +244 to +246
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember this discussion but do we have an issue for this? I mean if at some point there is a fix, we could link this.

Copy link
Contributor Author

@DnPlas DnPlas Jul 22, 2024

Choose a reason for hiding this comment

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

We do not have an issue, but also I'm not sure if there is a fix as this would be harness' behaviour. I can create one in our tracker just so we remember what it is and what are the things we have to do as workarounds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's if there is an issue in Harness. Otherwise, it's fine as is.

harness.update_config({"issuer-url": "http://my-dex.io/dex"})
harness.begin()

# 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())

# 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"]
Loading