From 2d471a136c8496806eff90cc66ed5dc1c3c64988 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Mon, 15 Jun 2026 20:42:28 +0700 Subject: [PATCH 01/21] chore: add garm_configurator_v0 relation endpoints to both charms Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm-configurator/charmcraft.yaml | 4 ++++ charms/garm/charmcraft.yaml | 2 ++ 2 files changed, 6 insertions(+) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 8e7b09e7..77e02ace 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -108,6 +108,10 @@ config: A Python dict-like string containing key-value pairs of script name to bash script to be run prior to runner installation. +provides: + garm-configurator: + interface: garm_configurator_v0 + requires: image: interface: github_runner_image_v0 diff --git a/charms/garm/charmcraft.yaml b/charms/garm/charmcraft.yaml index 2e2dc4e4..2c0da442 100644 --- a/charms/garm/charmcraft.yaml +++ b/charms/garm/charmcraft.yaml @@ -28,4 +28,6 @@ requires: interface: postgresql_client optional: false limit: 1 + garm-configurator: + interface: garm_configurator_v0 From 37b616a0ab3bc313ddc966bc2098a06d2e84bbba Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:00:32 +0700 Subject: [PATCH 02/21] feat(garm): add ScalesetReconciler with diff-and-apply logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm/src/scaleset_reconciler.py | 140 ++++++++++++ .../tests/unit/test_scaleset_reconciler.py | 199 ++++++++++++++++++ 2 files changed, 339 insertions(+) create mode 100644 charms/garm/src/scaleset_reconciler.py create mode 100644 charms/garm/tests/unit/test_scaleset_reconciler.py diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py new file mode 100644 index 00000000..5e6b4067 --- /dev/null +++ b/charms/garm/src/scaleset_reconciler.py @@ -0,0 +1,140 @@ +#!/usr/bin/env python3 +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Scaleset reconciler: diffs desired vs observed GARM scalesets and applies changes.""" + +import logging +from dataclasses import dataclass, field + +from garm_api import GarmClient + +logger = logging.getLogger(__name__) + + +@dataclass +class ScalesetSpec: + """Desired state for one GARM scaleset.""" + + name: str + provider_name: str + credentials_name: str + image_id: str + flavor: str + os_arch: str + min_idle_runners: int + max_runners: int + labels: list[str] = field(default_factory=list) + runner_group: str = "default" + pre_install_scripts: dict[str, str] = field(default_factory=dict) + + +class ScalesetReconciler: + """Reconciles GARM scalesets against a desired spec list.""" + + def __init__(self, client: GarmClient) -> None: + """Initialise the reconciler. + + Args: + client: Authenticated GarmClient instance. + """ + self._client = client + + def reconcile(self, desired: list[ScalesetSpec]) -> None: + """Sync GARM scalesets to match *desired*. + + Performs the minimum set of CREATE / UPDATE / DELETE operations. + If a referenced provider or credential is missing for a spec, that + spec is skipped silently (deferred creation) — no error state is set. + + Args: + desired: The full desired set of scalesets. + """ + providers = {provider["name"] for provider in self._client.list_providers()} + credentials = {credential["name"] for credential in self._client.list_credentials()} + observed = {scaleset["name"]: scaleset for scaleset in self._client.list_scalesets()} + + desired_names: set[str] = set() + + for spec in desired: + if spec.provider_name not in providers: + logger.warning( + "Skipping scaleset %s: provider %s not registered yet", + spec.name, + spec.provider_name, + ) + continue + if spec.credentials_name not in credentials: + logger.warning( + "Skipping scaleset %s: credential %s not registered yet", + spec.name, + spec.credentials_name, + ) + continue + + desired_names.add(spec.name) + + if spec.name in observed: + self._maybe_update(observed[spec.name], spec) + else: + self._create(spec) + + for name, scaleset in observed.items(): + if name not in desired_names: + logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset["id"]) + self._client.delete_scaleset(scaleset["id"]) + + def _create(self, spec: ScalesetSpec) -> None: + extra_specs: dict[str, object] = {} + if spec.pre_install_scripts: + extra_specs["pre_install_scripts"] = spec.pre_install_scripts + + payload = { + "name": spec.name, + "provider_name": spec.provider_name, + "credentials_name": spec.credentials_name, + "image_id": spec.image_id, + "flavor": spec.flavor, + "os_arch": spec.os_arch, + "min_idle_runners": spec.min_idle_runners, + "max_runners": spec.max_runners, + "tags": sorted(spec.labels), + "runner_group": spec.runner_group, + "extra_specs": extra_specs, + } + logger.info("Creating scaleset %s", spec.name) + self._client.create_scaleset(payload) + + def _maybe_update(self, observed: dict, spec: ScalesetSpec) -> None: + if not self._needs_update(observed, spec): + logger.debug("Scaleset %s is up to date", spec.name) + return + + extra_specs: dict[str, object] = {} + if spec.pre_install_scripts: + extra_specs["pre_install_scripts"] = spec.pre_install_scripts + + payload = { + "image_id": spec.image_id, + "flavor": spec.flavor, + "min_idle_runners": spec.min_idle_runners, + "max_runners": spec.max_runners, + "tags": sorted(spec.labels), + "runner_group": spec.runner_group, + "extra_specs": extra_specs, + } + logger.info("Updating scaleset %s (id=%s)", spec.name, observed["id"]) + self._client.update_scaleset(observed["id"], payload) + + @staticmethod + def _needs_update(observed: dict, spec: ScalesetSpec) -> bool: + observed_scripts = (observed.get("extra_specs") or {}).get("pre_install_scripts", {}) + return ( + observed.get("image_id") != spec.image_id + or observed.get("flavor") != spec.flavor + or observed.get("max_runners") != spec.max_runners + or observed.get("min_idle_runners") != spec.min_idle_runners + or sorted(observed.get("tags") or []) != sorted(spec.labels) + or observed.get("runner_group") != spec.runner_group + or observed_scripts != spec.pre_install_scripts + ) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py new file mode 100644 index 00000000..62bf8010 --- /dev/null +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -0,0 +1,199 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the scaleset reconciler.""" + +from unittest.mock import MagicMock + +from scaleset_reconciler import ScalesetReconciler, ScalesetSpec + + +def _mock_client(providers=None, credentials=None, scalesets=None): + client = MagicMock() + client.list_providers.return_value = providers or [] + client.list_credentials.return_value = credentials or [] + client.list_scalesets.return_value = scalesets or [] + client.create_scaleset.return_value = {"id": "new-uuid", "name": "test"} + client.update_scaleset.return_value = {"id": "uuid-1", "name": "test"} + return client + + +def _spec( + name="my-scaleset", + provider_name="openstack-demo", + credentials_name="github-app-12345", + image_id="ubuntu-22.04", + flavor="m1.small", + os_arch="x64", + min_idle=0, + max_runners=5, + labels=None, + runner_group="default", + pre_install_scripts=None, +): + return ScalesetSpec( + name=name, + provider_name=provider_name, + credentials_name=credentials_name, + image_id=image_id, + flavor=flavor, + os_arch=os_arch, + min_idle_runners=min_idle, + max_runners=max_runners, + labels=labels or [], + runner_group=runner_group, + pre_install_scripts=pre_install_scripts or {}, + ) + + +def test_create_scaleset_when_not_exists(): + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + client.create_scaleset.assert_called_once() + call_payload = client.create_scaleset.call_args[0][0] + assert call_payload["name"] == "my-scaleset" + assert call_payload["image_id"] == "ubuntu-22.04" + assert call_payload["flavor"] == "m1.small" + + +def test_skip_creation_when_provider_missing(): + client = _mock_client( + providers=[], + credentials=[{"name": "github-app-12345"}], + scalesets=[], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + client.create_scaleset.assert_not_called() + + +def test_skip_creation_when_credential_missing(): + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[], + scalesets=[], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + client.create_scaleset.assert_not_called() + + +def test_update_scaleset_when_image_changed(): + existing = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-20.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + } + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec(image_id="ubuntu-22.04")]) + client.update_scaleset.assert_called_once() + args = client.update_scaleset.call_args + assert args[0][0] == "uuid-1" + assert args[0][1]["image_id"] == "ubuntu-22.04" + + +def test_no_update_when_scaleset_unchanged(): + existing = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + } + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + client.update_scaleset.assert_not_called() + client.create_scaleset.assert_not_called() + + +def test_delete_scaleset_not_in_desired(): + existing = { + "id": "uuid-stale", + "name": "stale-scaleset", + "image_id": "ubuntu-20.04", + "flavor": "m1.small", + "max_runners": 2, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + } + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec(name="new-scaleset")]) + client.delete_scaleset.assert_called_once_with("uuid-stale") + + +def test_no_delete_when_desired_is_empty(): + client = _mock_client( + providers=[], + credentials=[], + scalesets=[], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([]) + client.delete_scaleset.assert_not_called() + + +def test_pre_install_scripts_included_in_create(): + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[], + ) + reconciler = ScalesetReconciler(client) + scripts = {"setup.sh": "#!/bin/bash\napt-get update"} + reconciler.reconcile([_spec(pre_install_scripts=scripts)]) + payload = client.create_scaleset.call_args[0][0] + assert payload["extra_specs"]["pre_install_scripts"] == scripts + + +def test_labels_sorted_for_comparison(): + existing = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": ["z-tag", "a-tag"], + "runner_group": "default", + "extra_specs": {}, + } + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec(labels=["a-tag", "z-tag"])]) + client.update_scaleset.assert_not_called() From 6494b1997c08e3c1f74cae4fc21b5c26390a1f31 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:01:43 +0700 Subject: [PATCH 03/21] feat(garm): add GarmClient REST API wrapper (garm_api.py) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm/src/garm_api.py | 233 ++++++++++++++++++++++++ charms/garm/tests/unit/test_garm_api.py | 184 +++++++++++++++++++ 2 files changed, 417 insertions(+) create mode 100644 charms/garm/src/garm_api.py create mode 100644 charms/garm/tests/unit/test_garm_api.py diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py new file mode 100644 index 00000000..d6f41a49 --- /dev/null +++ b/charms/garm/src/garm_api.py @@ -0,0 +1,233 @@ +#!/usr/bin/env python3 +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""GARM REST API client.""" + +import json +import logging +import urllib.error +import urllib.request +from typing import Any + +logger = logging.getLogger(__name__) + + +class GarmApiError(Exception): + """Raised when the GARM REST API returns an unexpected response.""" + + +class GarmClient: + """Thin HTTP client for the GARM REST API.""" + + def __init__(self, base_url: str) -> None: + """Initialise the client. + + Args: + base_url: GARM API base URL, e.g. 'http://localhost:9997/api/v1'. + """ + self.base_url = base_url.rstrip("/") + self.token: str = "" + + def _request( + self, + method: str, + path: str, + payload: dict[str, Any] | None = None, + *, + auth: bool = True, + ) -> Any: + """Make an HTTP request to the GARM API. + + Args: + method: HTTP method (GET, POST, PUT, DELETE). + path: Path relative to base_url (must start with /). + payload: Optional JSON request body. + auth: Whether to include the Bearer token header. + + Raises: + GarmApiError: If the response status is not 2xx or a network error occurs. + + Returns: + Parsed JSON response body, or None for empty responses. + """ + url = f"{self.base_url}{path}" + data = json.dumps(payload).encode() if payload is not None else None + headers: dict[str, str] = { + "Content-Type": "application/json", + "Accept": "application/json", + } + if auth and self.token: + headers["Authorization"] = f"Bearer {self.token}" + req = urllib.request.Request(url, data=data, headers=headers, method=method) + try: + with urllib.request.urlopen(req, timeout=30) as resp: + body = resp.read() + if not body: + return None + return json.loads(body) + except urllib.error.HTTPError as exc: + raise GarmApiError( + f"{method} {url} failed with status {exc.code}: {exc.read().decode()[:200]}" + ) from exc + except urllib.error.URLError as exc: + raise GarmApiError(f"{method} {url} failed: {exc.reason}") from exc + + def first_run( + self, + username: str, + password: str, + email: str, + full_name: str, + ) -> None: + """Complete GARM first-run initialisation (idempotent — HTTP 409 is silently ignored). + + Args: + username: Admin username. + password: Admin password. + email: Admin email address. + full_name: Admin display name. + + Raises: + GarmApiError: On unexpected API errors (not 409). + """ + try: + self._request( + "POST", + "/first-run", + { + "username": username, + "password": password, + "email": email, + "full_name": full_name, + }, + auth=False, + ) + except GarmApiError as exc: + if "409" in str(exc): + logger.debug("GARM first-run already completed (409)") + return + raise + + def configure_controller( + self, + metadata_url: str, + callback_url: str, + webhook_url: str, + ) -> None: + """Configure GARM controller URLs (idempotent). + + Args: + metadata_url: URL for runner metadata endpoint. + callback_url: URL for runner callback endpoint. + webhook_url: URL for GitHub webhooks endpoint. + + Raises: + GarmApiError: On API error. + """ + self._request( + "PUT", + "/controller", + { + "metadata_url": metadata_url, + "callback_url": callback_url, + "webhook_url": webhook_url, + }, + ) + + def login(self, username: str, password: str) -> str: + """Log in to the GARM API and return a JWT token. + + Args: + username: Admin username. + password: Admin password. + + Raises: + GarmApiError: If login fails or response has no token. + + Returns: + JWT token string. + """ + result = self._request( + "POST", + "/auth/login", + {"username": username, "password": password}, + auth=False, + ) + token = (result or {}).get("token", "") + if not token: + raise GarmApiError("login response did not contain a token") + return token + + def list_providers(self) -> list[dict[str, Any]]: + """List all registered GARM providers. + + Raises: + GarmApiError: On API error. + + Returns: + List of provider dicts (each has at minimum a 'name' key). + """ + return self._request("GET", "/providers") or [] + + def list_credentials(self) -> list[dict[str, Any]]: + """List all registered GARM credentials. + + Raises: + GarmApiError: On API error. + + Returns: + List of credential dicts (each has at minimum a 'name' key). + """ + return self._request("GET", "/credentials") or [] + + def list_scalesets(self) -> list[dict[str, Any]]: + """List all scalesets. + + Raises: + GarmApiError: On API error. + + Returns: + List of scaleset dicts (each has at minimum 'id' and 'name' keys). + """ + return self._request("GET", "/scalesets") or [] + + def create_scaleset(self, payload: dict[str, Any]) -> dict[str, Any]: + """Create a new scaleset. + + Args: + payload: CreateScaleSetParams dict. + + Raises: + GarmApiError: On API error. + + Returns: + Created scaleset dict. + """ + return self._request("POST", "/scalesets", payload) + + def update_scaleset(self, scaleset_id: str, payload: dict[str, Any]) -> dict[str, Any]: + """Update an existing scaleset. + + Args: + scaleset_id: Scaleset UUID. + payload: UpdateScaleSetParams dict (only changed fields needed). + + Raises: + GarmApiError: On API error. + + Returns: + Updated scaleset dict. + """ + return self._request("PUT", f"/scalesets/{scaleset_id}", payload) + + def delete_scaleset(self, scaleset_id: str) -> None: + """Delete a scaleset. + + Args: + scaleset_id: Scaleset UUID. + + Raises: + GarmApiError: On API error. + """ + self._request("DELETE", f"/scalesets/{scaleset_id}") diff --git a/charms/garm/tests/unit/test_garm_api.py b/charms/garm/tests/unit/test_garm_api.py new file mode 100644 index 00000000..a6e0a971 --- /dev/null +++ b/charms/garm/tests/unit/test_garm_api.py @@ -0,0 +1,184 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the GARM REST API client.""" + +import json +from io import BytesIO +from unittest.mock import MagicMock, patch +from urllib.error import HTTPError, URLError + +import pytest + +from garm_api import GarmApiError, GarmClient + + +def _make_response(body): + """Build a mock urllib response context manager.""" + mock_resp = MagicMock() + mock_resp.__enter__ = lambda s: s + mock_resp.__exit__ = MagicMock(return_value=False) + mock_resp.read.return_value = json.dumps(body).encode() + return mock_resp + + +def _make_http_error(status: int, body: str = "error"): + return HTTPError( + url="http://localhost:9997/api/v1/test", + code=status, + msg=body, + hdrs=None, + fp=BytesIO(body.encode()), + ) + + +def test_list_providers_returns_parsed_list(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([{"name": "openstack", "id": "abc"}]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_providers() + assert result == [{"name": "openstack", "id": "abc"}] + + +def test_list_providers_returns_empty_on_null_body(): + with patch("urllib.request.urlopen") as mock_open: + mock_resp = MagicMock() + mock_resp.__enter__ = lambda s: s + mock_resp.__exit__ = MagicMock(return_value=False) + mock_resp.read.return_value = b"" + mock_open.return_value = mock_resp + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_providers() + assert result == [] + + +def test_list_credentials_returns_parsed_list(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([{"name": "github-app-12345"}]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_credentials() + assert result == [{"name": "github-app-12345"}] + + +def test_list_scalesets_returns_parsed_list(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([{"id": "uuid-1", "name": "my-ss"}]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_scalesets() + assert result == [{"id": "uuid-1", "name": "my-ss"}] + + +def test_create_scaleset_posts_and_returns_dict(): + expected = {"id": "new-uuid", "name": "my-ss"} + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(expected) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.create_scaleset({"name": "my-ss"}) + assert result == expected + + +def test_update_scaleset_puts_and_returns_dict(): + expected = {"id": "uuid-1", "name": "my-ss", "flavor": "m1.xlarge"} + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(expected) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.update_scaleset("uuid-1", {"flavor": "m1.xlarge"}) + assert result == expected + + +def test_delete_scaleset_makes_delete_request(): + with patch("urllib.request.urlopen") as mock_open: + mock_resp = MagicMock() + mock_resp.__enter__ = lambda s: s + mock_resp.__exit__ = MagicMock(return_value=False) + mock_resp.read.return_value = b"" + mock_open.return_value = mock_resp + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.delete_scaleset("uuid-1") + call_args = mock_open.call_args + req = call_args[0][0] + assert req.get_method() == "DELETE" + assert "uuid-1" in req.full_url + + +def test_request_raises_garm_api_error_on_http_error(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.side_effect = _make_http_error(500, "internal server error") + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + with pytest.raises(GarmApiError, match="500"): + client.list_providers() + + +def test_request_raises_garm_api_error_on_url_error(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.side_effect = URLError("connection refused") + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + with pytest.raises(GarmApiError, match="connection refused"): + client.list_providers() + + +def test_first_run_ignores_409(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.side_effect = _make_http_error(409, "already initialized") + client = GarmClient("http://localhost:9997/api/v1") + # Should not raise + client.first_run("admin", "password", "admin@test.local", "Admin") + + +def test_first_run_raises_on_other_errors(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.side_effect = _make_http_error(500, "server error") + client = GarmClient("http://localhost:9997/api/v1") + with pytest.raises(GarmApiError): + client.first_run("admin", "password", "admin@test.local", "Admin") + + +def test_login_returns_token(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response({"token": "eyJhbGci.payload.sig"}) + client = GarmClient("http://localhost:9997/api/v1") + token = client.login("admin", "password") + assert token == "eyJhbGci.payload.sig" + + +def test_login_raises_when_token_missing(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response({"token": ""}) + client = GarmClient("http://localhost:9997/api/v1") + with pytest.raises(GarmApiError, match="token"): + client.login("admin", "password") + + +def test_bearer_token_sent_in_auth_header(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "my-jwt" + client.list_scalesets() + req = mock_open.call_args[0][0] + assert req.get_header("Authorization") == "Bearer my-jwt" + + +def test_configure_controller_sends_put(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response({}) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.configure_controller( + metadata_url="http://1.2.3.4:9997/api/v1/metadata", + callback_url="http://1.2.3.4:9997/api/v1/callbacks", + webhook_url="http://1.2.3.4:9997/webhooks", + ) + req = mock_open.call_args[0][0] + assert req.get_method() == "PUT" + body = json.loads(req.data) + assert body["metadata_url"] == "http://1.2.3.4:9997/api/v1/metadata" From 9671594da288a091d2d8a1192434d6f5a18564df Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:03:29 +0700 Subject: [PATCH 04/21] fix(garm): narrow 409 check in first_run to avoid false positives Use 'status 409' substring match to match the specific error format string rather than bare '409' which could appear in error bodies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm/src/garm_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index d6f41a49..62cf267a 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -104,7 +104,7 @@ def first_run( auth=False, ) except GarmApiError as exc: - if "409" in str(exc): + if "status 409" in str(exc): logger.debug("GARM first-run already completed (409)") return raise From b1e952eefd083cfab13b2018002afc223e38aef5 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:19:31 +0700 Subject: [PATCH 05/21] feat(garm-configurator): write scaleset data to garm-configurator relation databag Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm-configurator/src/charm.py | 23 ++++ charms/garm-configurator/src/charm_state.py | 9 ++ .../tests/unit/test_charm.py | 107 ++++++++++++++++++ 3 files changed, 139 insertions(+) diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 8f7f3c37..c1cb2360 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -10,6 +10,8 @@ from charm_state import IMAGE_RELATION_NAME, CharmConfigInvalidError, CharmState +GARM_CONFIGURATOR_RELATION_NAME = "garm-configurator" + class GarmConfiguratorCharm(ops.CharmBase): """GARM configurator charm.""" @@ -24,6 +26,7 @@ def __init__(self, *args: typing.Any) -> None: for event in [ self.on.config_changed, self.on.secret_changed, + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, self.on[IMAGE_RELATION_NAME].relation_joined, self.on[IMAGE_RELATION_NAME].relation_changed, self.on[IMAGE_RELATION_NAME].relation_broken, @@ -58,6 +61,26 @@ def _reconcile(self, event: ops.EventBase) -> None: } ) + relation_data = { + "name": state.scaleset_config.name, + "provider_name": state.provider_name, + "credentials_name": state.credentials_name, + "image_id": str(state.image_id), + "flavor": state.scaleset_config.flavor, + "os_arch": state.scaleset_config.os_arch, + "min_idle_runner": str(state.scaleset_config.min_idle_runner), + "max_runner": str(state.scaleset_config.max_runner), + "labels": ( + state.scaleset_config.labels + if isinstance(state.scaleset_config.labels, str) + else ",".join(state.scaleset_config.labels) + ), + "runner_group": state.scaleset_config.runner_group, + "pre_install_scripts": str(state.scaleset_config.pre_install_scripts), + } + for garm_relation in self.model.relations[GARM_CONFIGURATOR_RELATION_NAME]: + garm_relation.data[self.unit].update(relation_data) + if relation is None: self.unit.status = ops.WaitingStatus("Waiting for image builder relation") elif state.image_id is None: diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index b241772d..329c5fd9 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -347,6 +347,15 @@ def from_charm(cls, charm: ops.CharmBase) -> "CharmState": image_id=image_id, ) + @property + def provider_name(self) -> str: + """Return the provider name derived from the OpenStack project.""" + return f"openstack-{self.provider_config.project_name}" + + @property + def credentials_name(self) -> str: + """Return the credentials name derived from the GitHub App client ID.""" + return f"github-app-{self.github_app_config.client_id}" def _get_image_id_from_relation(charm: ops.CharmBase) -> str | None: """Return the OpenStack image UUID from the image builder relation, if available. diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index a4d35e59..d54eff07 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -18,6 +18,10 @@ def _make_private_key_secret(): return Secret(tracked_content={"value": "random-secret"}) +def _make_garm_configurator_relation(): + return Relation(endpoint="garm-configurator") + + def _valid_config(secret: Secret, private_key_secret: Secret) -> dict: return { "openstack-auth-url": "https://keystone.example.com:5000/v3", @@ -430,3 +434,106 @@ def test_status_waiting_on_relation_broken(): ) out = ctx.run(ctx.on.relation_broken(image_relation), state) assert out.unit_status == ops.WaitingStatus("Waiting for image builder relation") + + +def test_garm_configurator_relation_data_written_on_reconcile(): + """ + arrange: Valid config and an active garm-configurator relation. + act: Run config-changed. + assert: The charm writes all expected scaleset fields to local unit relation data. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["labels"] = "self-hosted,linux" + config["pre-install-scripts"] = "echo hello" + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + ) + + out = ctx.run(ctx.on.config_changed(), state) + + rel_out = out.get_relation(garm_relation.id) + expected_relation_data = { + "name": "my-scaleset", + "provider_name": "openstack-myproject", + "credentials_name": "github-app-12345", + "image_id": "None", + "flavor": "m1.large", + "os_arch": "amd64", + "min_idle_runner": "0", + "max_runner": "5", + "labels": "self-hosted,linux", + "runner_group": "default", + "pre_install_scripts": "echo hello", + } + for key, value in expected_relation_data.items(): + assert rel_out.local_unit_data[key] == value + + +def test_garm_configurator_relation_data_reflects_charm_state(): + """ + arrange: Valid config with project and client identifiers plus a garm-configurator relation. + act: Run config-changed. + assert: Derived provider and credentials names are written from CharmState. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["openstack-project-name"] = "demo-project" + config["github-app-client-id"] = "abc123" + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + ) + + out = ctx.run(ctx.on.config_changed(), state) + + rel_out = out.get_relation(garm_relation.id) + assert rel_out.local_unit_data["provider_name"] == "openstack-demo-project" + assert rel_out.local_unit_data["credentials_name"] == "github-app-abc123" + + +def test_garm_configurator_no_error_when_no_relation(): + """ + arrange: Valid config with no garm-configurator relation. + act: Run config-changed. + assert: Reconcile completes and preserves the existing waiting status behavior. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + state = State(config=_valid_config(secret, pk_secret), secrets=[secret, pk_secret]) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.unit_status == ops.WaitingStatus("Waiting for image builder relation") + + +def test_garm_configurator_relation_changed_triggers_reconcile(): + """ + arrange: Valid config and a garm-configurator relation with no existing local unit data. + act: Run garm-configurator relation-changed. + assert: Reconcile writes relation data for the local unit. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + garm_relation = _make_garm_configurator_relation() + state = State( + config=_valid_config(secret, pk_secret), + secrets=[secret, pk_secret], + relations=[garm_relation], + ) + + out = ctx.run(ctx.on.relation_changed(garm_relation), state) + + rel_out = out.get_relation(garm_relation.id) + assert rel_out.local_unit_data["name"] == "my-scaleset" From 371f5df719a9a361d13ec64a66c0300ec4f73f35 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:29:26 +0700 Subject: [PATCH 06/21] feat(garm-configurator): add provider_name and credentials_name to CharmState Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm-configurator/src/charm_state.py | 4 +- .../tests/unit/test_charm_state.py | 72 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 charms/garm-configurator/tests/unit/test_charm_state.py diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 329c5fd9..d180ff45 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -349,12 +349,12 @@ def from_charm(cls, charm: ops.CharmBase) -> "CharmState": @property def provider_name(self) -> str: - """Return the provider name derived from the OpenStack project.""" + """Derived GARM provider name for scalesets.""" return f"openstack-{self.provider_config.project_name}" @property def credentials_name(self) -> str: - """Return the credentials name derived from the GitHub App client ID.""" + """Derived GARM credentials name for scalesets.""" return f"github-app-{self.github_app_config.client_id}" def _get_image_id_from_relation(charm: ops.CharmBase) -> str | None: diff --git a/charms/garm-configurator/tests/unit/test_charm_state.py b/charms/garm-configurator/tests/unit/test_charm_state.py new file mode 100644 index 00000000..b453e2bc --- /dev/null +++ b/charms/garm-configurator/tests/unit/test_charm_state.py @@ -0,0 +1,72 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for charm_state.""" + +from charm_state import CharmState, GithubAppConfig, ProviderConfig, ScalesetConfig + + +def test_provider_name_derived_from_project_name(): + """CharmState.provider_name is openstack-{project_name}.""" + project_name = "myproject" + state = CharmState( + provider_config=ProviderConfig( + auth_url="https://keystone.example.com:5000/v3", + username="admin", + password="s3cr3t", + project_name=project_name, + user_domain_name="Default", + project_domain_name="Default", + region_name="RegionOne", + network="external-net", + ), + github_app_config=GithubAppConfig( + client_id="12345", + installation_id="67890", + private_key="private-key", + ), + scaleset_config=ScalesetConfig( + name="my-scaleset", + flavor="m1.large", + os_arch="amd64", + min_idle_runner=0, + max_runner=5, + repo="myorg/myrepo", + ), + image_id=None, + ) + + assert state.provider_name == f"openstack-{project_name}" + + +def test_credentials_name_derived_from_client_id(): + """CharmState.credentials_name is github-app-{client_id}.""" + client_id = "12345" + state = CharmState( + provider_config=ProviderConfig( + auth_url="https://keystone.example.com:5000/v3", + username="admin", + password="s3cr3t", + project_name="myproject", + user_domain_name="Default", + project_domain_name="Default", + region_name="RegionOne", + network="external-net", + ), + github_app_config=GithubAppConfig( + client_id=client_id, + installation_id="67890", + private_key="private-key", + ), + scaleset_config=ScalesetConfig( + name="my-scaleset", + flavor="m1.large", + os_arch="amd64", + min_idle_runner=0, + max_runner=5, + repo="myorg/myrepo", + ), + image_id=None, + ) + + assert state.credentials_name == f"github-app-{client_id}" From 88a6983aebf5f1d496dd1adf0e4517cb2e11477d Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 11:38:42 +0700 Subject: [PATCH 07/21] feat(garm): add admin secrets and scaleset reconcile via garm-configurator relation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm/src/charm.py | 129 ++++++++++++++++++++++++- charms/garm/tests/unit/test_charm.py | 136 ++++++++++++++++++++++++++- 2 files changed, 259 insertions(+), 6 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 8c9b49c1..91d616f6 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -6,7 +6,7 @@ import dataclasses import logging -import secrets +import secrets as _secrets import string import typing @@ -14,11 +14,14 @@ import paas_charm.go import tomli_w from paas_charm.app import WorkloadConfig +from garm_api import GarmApiError, GarmClient +from scaleset_reconciler import ScalesetReconciler, ScalesetSpec logger = logging.getLogger(__name__) GARM_CONFIG_PATH: typing.Final[str] = "/etc/garm/config.toml" GARM_SECRETS_LABEL: typing.Final[str] = "garm-secrets" +GARM_CONFIGURATOR_RELATION_NAME: typing.Final[str] = "garm-configurator" CONTAINER_NAME: typing.Final[str] = "app" PEBBLE_SERVICE_NAME: typing.Final[str] = "app" GARM_BINARY: typing.Final[str] = "/usr/local/bin/garm" @@ -38,7 +41,7 @@ def _generate_passphrase(length: int = _DB_PASSPHRASE_LENGTH) -> str: Random alphanumeric string of the given length. """ alphabet = string.ascii_letters + string.digits - return "".join(secrets.choice(alphabet) for _ in range(length)) + return "".join(_secrets.choice(alphabet) for _ in range(length)) def render_garm_toml( @@ -102,11 +105,26 @@ def _generate_garm_secrets() -> dict[str, str]: Dict with ``jwt-secret`` (64-char hex) and ``db-passphrase`` (32-char alnum). """ return { - "jwt-secret": secrets.token_hex(32), + "jwt-secret": _secrets.token_hex(32), "db-passphrase": _generate_passphrase(), + "admin-username": "admin", + "admin-password": f"Admin-{_secrets.token_hex(8)}-Gx1!", } +def _parse_pre_install_scripts(raw: str) -> dict[str, str]: + """Parse pre_install_scripts from relation data string.""" + if not raw: + return {} + try: + result = ast.literal_eval(raw) + if isinstance(result, dict): + return result + except (ValueError, SyntaxError): + pass + return {} + + class GarmCharm(paas_charm.go.Charm): """GARM charm — manages the GARM service via Pebble.""" @@ -118,6 +136,14 @@ def __init__(self, *args: typing.Any) -> None: """ super().__init__(*args) self.framework.observe(self.on.install, self._on_install) + self.framework.observe( + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, + self._on_configurator_changed, + ) + self.framework.observe( + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_departed, + self._on_configurator_changed, + ) def _on_install(self, _: ops.InstallEvent) -> None: """Ensure secrets exist on first install.""" @@ -136,6 +162,10 @@ def _workload_config(self) -> WorkloadConfig: """ return dataclasses.replace(super()._workload_config, port=GARM_PORT, metrics_target=None) + def _on_configurator_changed(self, _: typing.Any) -> None: + """Handle garm-configurator relation changes.""" + self._reconcile_scalesets() + def restart(self, rerun_migrations: bool = False) -> None: """Write GARM config then restart the workload. @@ -210,10 +240,26 @@ def _ensure_secrets(self) -> None: """Create the garm-secrets juju secret on first call (leader only).""" if not self.unit.is_leader(): return + secret = self._get_garm_secrets() + if secret is None: + self.app.add_secret(_generate_garm_secrets(), label=GARM_SECRETS_LABEL) + return + + secret_content = secret.get_content(refresh=True) + missing_secret_content = {} + if "admin-username" not in secret_content: + missing_secret_content["admin-username"] = "admin" + if "admin-password" not in secret_content: + missing_secret_content["admin-password"] = f"Admin-{_secrets.token_hex(8)}-Gx1!" + if missing_secret_content: + secret.set_content({**secret_content, **missing_secret_content}) + + def _get_garm_secrets(self) -> ops.Secret | None: + """Return the GARM secret object when available.""" try: - self.model.get_secret(label=GARM_SECRETS_LABEL) + return self.model.get_secret(label=GARM_SECRETS_LABEL) except ops.SecretNotFoundError: - self.app.add_secret(_generate_garm_secrets(), label=GARM_SECRETS_LABEL) + return None def _get_secrets(self) -> dict[str, str]: """Retrieve secrets from the juju secret store. @@ -227,6 +273,14 @@ def _get_secrets(self) -> dict[str, str]: secret = self.model.get_secret(label=GARM_SECRETS_LABEL) return secret.get_content() + def _get_garm_url(self) -> str: + """Build the local GARM API URL from charm configuration.""" + listen_address = str(self.config.get("garm-listen-address", "")) + listen_port = self.config.get("garm-listen-port", 9997) + if not listen_address: + return "" + return f"http://{listen_address}:{listen_port}" + def _get_postgresql_config(self) -> dict[str, typing.Any] | None: """Get PostgreSQL config from relation data, or None if not available. @@ -265,6 +319,71 @@ def _get_postgresql_config(self) -> dict[str, typing.Any] | None: return None + def _build_desired_scalesets(self) -> list[ScalesetSpec]: + """Build the desired scaleset list from all garm-configurator relation units.""" + specs = [] + for relation in self.model.relations.get(GARM_CONFIGURATOR_RELATION_NAME, []): + for unit in relation.units: + data = relation.data[unit] + name = data.get("name", "") + if not name: + continue + try: + min_idle = int(data.get("min_idle_runner", "0")) + max_runners = int(data.get("max_runner", "5")) + except ValueError: + continue + specs.append( + ScalesetSpec( + name=name, + provider_name=data.get("provider_name", ""), + credentials_name=data.get("credentials_name", ""), + image_id=data.get("image_id", ""), + flavor=data.get("flavor", ""), + os_arch=data.get("os_arch", "x64"), + min_idle_runners=min_idle, + max_runners=max_runners, + labels=[ + label.strip() + for label in data.get("labels", "").split(",") + if label.strip() + ], + runner_group=data.get("runner_group", "default"), + pre_install_scripts=_parse_pre_install_scripts( + data.get("pre_install_scripts", "") + ), + ) + ) + return specs + + def _reconcile_scalesets(self) -> None: + """Sync GARM scalesets against garm-configurator relation data.""" + secret = self._get_garm_secrets() + if not secret: + logger.warning("GARM secrets not yet available; deferring scaleset reconcile") + return + + secret_content = secret.get_content(refresh=True) + admin_username = secret_content.get("admin-username", "admin") + admin_password = secret_content.get("admin-password", "") + if not admin_password: + logger.warning("admin-password not in GARM secrets; deferring scaleset reconcile") + return + + garm_url = self._get_garm_url() + if not garm_url: + logger.warning("GARM URL not yet available; deferring scaleset reconcile") + return + + try: + client = GarmClient(f"{garm_url}/api/v1") + client.token = client.login(admin_username, admin_password) + desired = self._build_desired_scalesets() + reconciler = ScalesetReconciler(client) + reconciler.reconcile(desired) + except GarmApiError as exc: + logger.warning("GARM API error during scaleset reconcile: %s", exc) + def _push_garm_config(self, container: ops.Container) -> None: """Render and push the GARM TOML config into the Pebble container. diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index a7fcbdf5..9adff18f 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -4,6 +4,7 @@ """Unit tests for GarmCharm.""" import string +from unittest.mock import MagicMock, PropertyMock, patch import pytest @@ -12,7 +13,7 @@ except ImportError: import tomli as tomllib # type: ignore[no-redef] -from charm import _generate_garm_secrets, render_garm_toml +from charm import GARM_SECRETS_LABEL, GarmCharm, _generate_garm_secrets, render_garm_toml _DEFAULT_PG_CONFIG = { "username": "u", @@ -133,3 +134,136 @@ def test_generate_garm_secrets_produces_unique_values(): second = _generate_garm_secrets() assert first["jwt-secret"] != second["jwt-secret"] assert first["db-passphrase"] != second["db-passphrase"] + + +def test_generate_garm_secrets_includes_admin_username(): + """Generated secrets include the fixed admin username.""" + result = _generate_garm_secrets() + assert result["admin-username"] == "admin" + + +def test_generate_garm_secrets_includes_admin_password(): + """Generated secrets include a strong admin password with the expected wrapper.""" + result = _generate_garm_secrets() + password = result["admin-password"] + assert password.startswith("Admin-") + assert password.endswith("-Gx1!") + + +def test_admin_password_format_is_strong(): + """Generated admin passwords satisfy the expected strength constraints.""" + for _ in range(5): + password = _generate_garm_secrets()["admin-password"] + assert len(password) >= 12 + assert any(char.isupper() for char in password) + assert any(char.islower() for char in password) + assert any(char.isdigit() for char in password) + assert any(not char.isalnum() for char in password) + + +def test_admin_username_is_always_admin(): + """Generated admin username remains stable across calls.""" + for _ in range(5): + assert _generate_garm_secrets()["admin-username"] == "admin" + + +def test_generate_garm_secrets_does_not_overwrite_existing(): + """Existing garm-secrets are preserved when the charm ensures secrets.""" + charm = object.__new__(GarmCharm) + mock_unit = MagicMock() + mock_unit.is_leader.return_value = True + existing_secret = MagicMock() + existing_secret.get_content.return_value = { + "jwt-secret": "existing-jwt", + "db-passphrase": "existing-passphrase", + "admin-username": "admin", + "admin-password": "Admin-deadbeefcafebabe-Gx1!", + } + mock_model = MagicMock() + mock_model.get_secret.return_value = existing_secret + mock_app = MagicMock() + + with ( + patch.object(GarmCharm, "unit", new_callable=PropertyMock, return_value=mock_unit), + patch.object(GarmCharm, "model", new_callable=PropertyMock, return_value=mock_model), + patch.object(GarmCharm, "app", new_callable=PropertyMock, return_value=mock_app), + ): + charm._ensure_secrets() + + mock_model.get_secret.assert_called_once_with(label=GARM_SECRETS_LABEL) + mock_app.add_secret.assert_not_called() + existing_secret.set_content.assert_not_called() + + +def test_ensure_secrets_backfills_missing_admin_credentials(): + """Existing secrets gain admin credentials without changing current values.""" + charm = object.__new__(GarmCharm) + mock_unit = MagicMock() + mock_unit.is_leader.return_value = True + existing_secret = MagicMock() + existing_secret.get_content.return_value = { + "jwt-secret": "existing-jwt", + "db-passphrase": "existing-passphrase", + } + mock_model = MagicMock() + mock_model.get_secret.return_value = existing_secret + mock_app = MagicMock() + + with ( + patch.object(GarmCharm, "unit", new_callable=PropertyMock, return_value=mock_unit), + patch.object(GarmCharm, "model", new_callable=PropertyMock, return_value=mock_model), + patch.object(GarmCharm, "app", new_callable=PropertyMock, return_value=mock_app), + ): + charm._ensure_secrets() + + existing_secret.set_content.assert_called_once() + updated = existing_secret.set_content.call_args[0][0] + assert updated["jwt-secret"] == "existing-jwt" + assert updated["db-passphrase"] == "existing-passphrase" + assert updated["admin-username"] == "admin" + assert updated["admin-password"].startswith("Admin-") + assert updated["admin-password"].endswith("-Gx1!") + + +def test_admin_password_is_unique_across_calls(): + """Separate generated secret payloads use different admin passwords.""" + first = _generate_garm_secrets() + second = _generate_garm_secrets() + assert first["admin-password"] != second["admin-password"] + + +def test_reconcile_scalesets_skips_when_no_secrets(): + """Scaleset reconciliation exits early when GARM secrets are unavailable.""" + charm = object.__new__(GarmCharm) + charm._get_garm_secrets = MagicMock(return_value=None) + + with patch("charm.GarmClient") as mock_client: + charm._reconcile_scalesets() + + mock_client.assert_not_called() + + +def test_reconcile_scalesets_skips_restart(): + """Scaleset reconciliation must not restart the workload.""" + charm = object.__new__(GarmCharm) + secret = MagicMock() + secret.get_content.return_value = { + "admin-username": "admin", + "admin-password": "Admin-deadbeefcafebabe-Gx1!", + } + charm._get_garm_secrets = MagicMock(return_value=secret) + charm._get_garm_url = MagicMock(return_value="http://garm") + charm._build_desired_scalesets = MagicMock(return_value=[]) + charm.restart = MagicMock() + charm._restart_service = MagicMock() + + with patch("charm.GarmClient") as mock_client_cls, patch("charm.ScalesetReconciler") as mock_reconciler_cls: + mock_client = mock_client_cls.return_value + mock_client.login.return_value = "token" + + charm._reconcile_scalesets() + + mock_client.login.assert_called_once_with("admin", "Admin-deadbeefcafebabe-Gx1!") + mock_reconciler_cls.return_value.reconcile.assert_called_once_with([]) + charm.restart.assert_not_called() + charm._restart_service.assert_not_called() From 5d3df8e094e0b0710b4e0bd0707fa203d662f1ad Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 12:25:02 +0700 Subject: [PATCH 08/21] test(garm): add integration tests for scaleset create/update/delete/defer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/tests/integration/conftest.py | 110 +++++++ charms/tests/integration/test_garm.py | 404 ++++++++++++++++++++++++++ 2 files changed, 514 insertions(+) diff --git a/charms/tests/integration/conftest.py b/charms/tests/integration/conftest.py index 6230868f..db95d44a 100644 --- a/charms/tests/integration/conftest.py +++ b/charms/tests/integration/conftest.py @@ -19,6 +19,8 @@ ) logger = logging.getLogger(__name__) +GARM_API_PORT = 9997 +GARM_SECRETS_LABEL = "garm-secrets" @pytest.fixture(name="planner_charm_file", scope="module") @@ -332,6 +334,54 @@ def garm_app_image_fixture(pytestconfig: pytest.Config) -> str | None: return image +@pytest.fixture(name="garm_configurator_charm_file", scope="module") +def garm_configurator_charm_file_fixture(pytestconfig: pytest.Config) -> str: + """Return the path to the built garm-configurator charm file.""" + charm = pytestconfig.getoption(CHARM_FILE_PARAM) + if not charm: + pytest.skip( + f"missing required {CHARM_FILE_PARAM} option for garm-configurator integration tests" + ) + configurator_charms = [file for file in charm if "garm-configurator" in file] + if not configurator_charms: + pytest.skip( + "scaleset integration tests require a garm-configurator charm path in " + f"{CHARM_FILE_PARAM}" + ) + return configurator_charms[0] + + +def garm_login_from_secret(juju: jubilant.Juju, garm_app_name: str, garm_url: str) -> str: + """Log into the GARM API using admin credentials stored in Juju secrets.""" + secrets_json = juju.cli("secrets", "--format=json") + secrets = json.loads(secrets_json) + garm_secret_uri = next( + (uri for uri, info in secrets.items() if info.get("label") == GARM_SECRETS_LABEL), + None, + ) + assert garm_secret_uri, f"{GARM_SECRETS_LABEL} not found for {garm_app_name}" + + secret_json = juju.cli("show-secret", "--reveal", "--format=json", garm_secret_uri) + secret_data = json.loads(secret_json) + content = secret_data[garm_secret_uri]["content"]["Data"] + admin_username = content["admin-username"] + admin_password = content["admin-password"] + + base_url = garm_url.rstrip("/") + if not base_url.endswith("/api/v1"): + base_url = f"{base_url}/api/v1" + + resp = requests.post( + f"{base_url}/auth/login", + json={"username": admin_username, "password": admin_password}, + timeout=30, + ) + resp.raise_for_status() + token = resp.json().get("token", "") + assert token, "Expected non-empty JWT token from GARM login" + return token + + def _pre_pull_garm_image(image: str) -> None: """Pre-pull the GARM ROCK image into microk8s containerd. @@ -505,3 +555,63 @@ def _on_image_relation_joined(self, event): delay=10, ) return app_name + + +@pytest.fixture(scope="module", name="garm_configurator_for_scaleset_tests") +def garm_configurator_for_scaleset_tests_fixture( + juju: jubilant.Juju, + garm_configurator_charm_file: str, + any_charm_image_builder_app: str, +) -> str: + """Deploy garm-configurator for GARM scaleset-sync integration tests.""" + app_name = "garm-configurator-scaleset-test" + juju.deploy(charm=garm_configurator_charm_file, app=app_name) + juju.wait( + lambda status: jubilant.all_blocked(status, app_name), + timeout=5 * 60, + delay=10, + ) + + password_secret_uri = juju.add_secret(name="os-pw-scaleset", content={"value": "fake-password"}) + private_key_secret_uri = juju.add_secret( + name="gh-key-scaleset", content={"value": "fake-private-key"} + ) + juju.grant_secret(password_secret_uri, app_name) + juju.grant_secret(private_key_secret_uri, app_name) + + juju.config( + app_name, + values={ + "openstack-auth-url": "https://keystone.example.com:5000/v3", + "openstack-username": "admin", + "openstack-password": password_secret_uri, + "openstack-project-name": "myproject", + "openstack-user-domain-name": "Default", + "openstack-project-domain-name": "Default", + "openstack-region-name": "RegionOne", + "openstack-network": "external-net", + "github-app-client-id": "12345", + "github-app-installation-id": "67890", + "github-app-private-key": private_key_secret_uri, + "name": "test-scaleset", + "flavor": "m1.large", + "os-arch": "amd64", + "repo": "myorg/myrepo", + }, + ) + juju.wait( + lambda status: jubilant.all_waiting(status, app_name), + timeout=5 * 60, + delay=10, + ) + + juju.integrate( + f"{app_name}:image", + f"{any_charm_image_builder_app}:provide-github-runner-image-v0", + ) + juju.wait( + lambda status: jubilant.all_active(status, app_name), + timeout=5 * 60, + delay=10, + ) + return app_name diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index ef3a9edb..0293507d 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -3,13 +3,18 @@ """Integration tests for the GARM charm.""" +import base64 import json import logging import secrets +import shlex +import urllib.error +import urllib.request import jubilant import pytest import requests +from charms.tests.integration.conftest import garm_login_from_secret from requests.adapters import HTTPAdapter from tenacity import ( retry, @@ -32,6 +37,24 @@ # the same credentials. Format guarantees GARM's strong-password requirements: # uppercase (A), lowercase (dmin + hex), digit (1 + hex), symbols (-, !). _GARM_ADMIN_PASSWORD = f"Admin-{secrets.token_hex(8)}-X1!" +_SCALESET_TEST_NAME = "test-scaleset" +_SCALESET_TEST_CREDENTIAL_NAME = "github-app-12345" + +_TEST_RSA_PRIVATE_KEY = """-----BEGIN RSA PRIVATE KEY----- +MIICXwIBAAKBgQC2tCW5B18y5VnqqokOeamJgasI3H1405WWv7FmWl31I1Cgabhi +MFcHdNECXFUC3wtqo/bXyCQbANRBkpZudJfSGos3+1iOJK1fd+MU8ntHVtgpvb5j +whdFSVJ9EL4/2u0K0S+fIyilD9q7K5mhk0MYLYWumPIRLkbtwr9a7LgY5wIDAQAB +AoGBAKIQGCoRjPNjmCfdT6fEaYtstt8sXiwQWu+WaHDnFdL9mWZBgOmwAXK+vyt9 +5XafjMvyV2I+yTAewyjLM58U0xlslJu6Bk0Zw920sTmK9Qvvq/2mjsqw+PWr9rRx +qZFDCefAlB0Npo9tXHAf3ec5+vlm4QsEl6dty+Wx6aSHHMRpAkEA8e5IwkJZFcWO +aCc8Z+cnoidomlkvGlruncXMG1KhisQTleQVc1bM8tIZq2nNUG1zKJqHeCacQLiV +LKALnZDSCwJBAMFUIHd7ikYaAgTvrAKmzOZlMKVuGr2SHPODWoaWkEagEsrOw+H2 +PYonSYkbzPyXH6iKUOhWH+ZA1r6K1lhdWhUCQQCquaTOsVN8cbVU+ps+F3l4jKbc +hSMgThsla3flsCIfcs7/b71Tb2Wh1XIX7Mnef95MQQBoYZbSdW+P1kFcJ96RAkEA +oSyuqI4BGDJkjpL1l3xSBJ5F8RUbDAI9SrKujNgHTinzoMrCOabdZUkdoEXiHo8r +IIq3qwrqKz7RCSecTSz+hQJBAJDKODanbnrPxNDgmIp52BMtiYI4vv7gKp/MSW0N +PG8an+PHNVGDEj1cOOwp/YNQieRp/WPH6bpBtwwe0r6pQZQ= +-----END RSA PRIVATE KEY-----""" def _pebble_exec(juju: jubilant.Juju, unit: str, command: str) -> jubilant.Task: @@ -185,6 +208,231 @@ def _scrape_metrics_until_ready(metrics_url: str) -> requests.Response: raise _MetricsNotReady() return response +def _garm_api_base_url(address: str) -> str: + """Return the GARM v1 API base URL for a unit address.""" + return f"http://{address}:{GARM_API_PORT}/api/v1" + + +def _garm_auth_headers(token: str) -> dict[str, str]: + """Return authorization headers for GARM API requests.""" + return {"Authorization": f"Bearer {token}"} + + +def _list_scalesets(base_url: str, token: str) -> list[dict]: + """List GARM scalesets via the REST API.""" + resp = requests.get(f"{base_url}/scalesets", headers=_garm_auth_headers(token), timeout=30) + resp.raise_for_status() + scalesets = resp.json() + assert isinstance(scalesets, list), f"Expected list response, got: {type(scalesets)}" + return scalesets + + +def _list_providers(base_url: str, token: str) -> list[dict]: + """List GARM providers via the REST API.""" + resp = requests.get(f"{base_url}/providers", headers=_garm_auth_headers(token), timeout=30) + resp.raise_for_status() + providers = resp.json() + assert isinstance(providers, list), f"Expected list response, got: {type(providers)}" + return providers + + +def _find_scaleset(scalesets: list[dict], name: str) -> dict | None: + """Return the first scaleset with the requested name.""" + return next((scaleset for scaleset in scalesets if scaleset.get("name") == name), None) + + +def _get_relation_info(juju: jubilant.Juju, unit: str, endpoint: str, related_app: str) -> dict: + """Return show-unit relation info for a specific endpoint and related app.""" + unit_info = json.loads(juju.cli("show-unit", unit, "--format=json"))[unit] + for relation in unit_info["relation-info"]: + if relation["endpoint"] != endpoint: + continue + related_units = relation.get("related-units", {}) + if any(name.startswith(f"{related_app}/") for name in related_units): + return relation + raise AssertionError( + f"Relation {endpoint!r} between {unit!r} and app {related_app!r} not found" + ) + + +def _relation_exists(juju: jubilant.Juju, app_name: str, related_app: str) -> bool: + """Return whether the GARM app is related to the configurator app.""" + try: + _get_relation_info(juju, f"{app_name}/0", "garm-configurator", related_app) + except AssertionError: + return False + return True + + +def _get_scaleset_relation_data( + juju: jubilant.Juju, garm_app: str, garm_configurator_for_scaleset_tests: str +) -> dict[str, str]: + """Return the configurator unit relation data as seen from the GARM side.""" + relation = _get_relation_info( + juju, + f"{garm_app}/0", + "garm-configurator", + garm_configurator_for_scaleset_tests, + ) + related_unit = f"{garm_configurator_for_scaleset_tests}/0" + data = relation["related-units"][related_unit]["data"] + return {key: str(value) for key, value in data.items()} + + +def _ensure_garm_configurator_relation( + juju: jubilant.Juju, garm_app: str, garm_configurator_for_scaleset_tests: str +) -> None: + """Ensure the GARM app is integrated with the scaleset-test configurator.""" + if not _relation_exists(juju, garm_app, garm_configurator_for_scaleset_tests): + juju.integrate(garm_app, garm_configurator_for_scaleset_tests) + juju.wait( + lambda status: jubilant.all_active(status, garm_app), + timeout=3 * 60, + delay=10, + ) + + +def _set_scaleset_relation_data( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, + **overrides: str, +) -> None: + """Mutate configurator relation data to exercise GARM reconcile scenarios.""" + unit = f"{garm_configurator_for_scaleset_tests}/0" + relation = _get_relation_info(juju, unit, "garm-configurator", garm_app) + relation_id = relation["relation-id"] + relation_data = _get_scaleset_relation_data( + juju, garm_app, garm_configurator_for_scaleset_tests + ) + relation_data.update(overrides) + command = " ".join( + [ + "relation-set", + "-r", + str(relation_id), + *[shlex.quote(f"{key}={value}") for key, value in relation_data.items()], + ] + ) + juju.exec(command, unit=unit) + juju.wait( + lambda status: jubilant.all_active(status, garm_app), + timeout=3 * 60, + delay=10, + ) + + +def _create_test_credential(garm_url: str, token: str) -> str: + """Create a GitHub App credential in GARM for testing.""" + + def _request( + path: str, payload: dict, *, method: str = "POST", allow_conflict: bool = True + ) -> dict | None: + data = json.dumps(payload).encode() + req = urllib.request.Request( + f"{garm_url}{path}", + data=data, + headers={"Authorization": f"Bearer {token}", "Content-Type": "application/json"}, + method=method, + ) + try: + with urllib.request.urlopen(req, timeout=30) as resp: + body = resp.read() + return json.loads(body) if body else None + except urllib.error.HTTPError as exc: + if allow_conflict and exc.code == 409: + return None + raise + + try: + _request( + "/github/endpoints", + { + "name": "github.com", + "description": "GitHub.com test endpoint", + "base_url": "https://github.com", + "api_base_url": "https://api.github.com", + "upload_base_url": "https://uploads.github.com", + }, + ) + _request( + "/github/credentials", + { + "name": _SCALESET_TEST_CREDENTIAL_NAME, + "description": "Test credential for scaleset integration tests", + "endpoint": "github.com", + "auth_type": "app", + "app": { + "app_id": 12345, + "installation_id": 67890, + "private_key_bytes": base64.b64encode( + _TEST_RSA_PRIVATE_KEY.encode() + ).decode(), + }, + }, + ) + except urllib.error.HTTPError: + _request( + "/credentials", + { + "name": _SCALESET_TEST_CREDENTIAL_NAME, + "description": "Test credential for scaleset integration tests", + "base_url": "https://github.com", + "api_base_url": "https://api.github.com", + "upload_base_url": "https://uploads.github.com", + "ca_cert_bundle": "", + "credentials": { + "name": "test-creds", + "description": "test", + "type": "github_app", + "payload": { + "app_id": 12345, + "installation_id": 67890, + "private_key": _TEST_RSA_PRIVATE_KEY, + }, + }, + }, + allow_conflict=True, + ) + return _SCALESET_TEST_CREDENTIAL_NAME + + +@retry( + retry=retry_if_exception_type((AssertionError, requests.exceptions.RequestException)), + wait=wait_exponential(multiplier=1, min=1, max=10), + stop=stop_after_attempt(18), + reraise=True, +) +def _wait_for_scaleset( + base_url: str, + token: str, + name: str, + *, + image_id: str | None = None, +) -> dict: + """Wait until a named scaleset exists and optionally reflects a new image.""" + scaleset = _find_scaleset(_list_scalesets(base_url, token), name) + assert scaleset is not None, f"Expected scaleset {name!r} to exist" + if image_id is not None: + observed_image = scaleset.get("image_id") or scaleset.get("image") + assert observed_image == image_id, ( + f"Expected scaleset {name!r} image/image_id to be {image_id!r}, " + f"got {observed_image!r}" + ) + return scaleset + + +@retry( + retry=retry_if_exception_type((AssertionError, requests.exceptions.RequestException)), + wait=wait_exponential(multiplier=1, min=1, max=10), + stop=stop_after_attempt(18), + reraise=True, +) +def _wait_for_scaleset_absent(base_url: str, token: str, name: str) -> None: + """Wait until a named scaleset no longer exists.""" + scaleset = _find_scaleset(_list_scalesets(base_url, token), name) + assert scaleset is None, f"Expected scaleset {name!r} to be absent, got: {scaleset}" + def test_garm_blocks_without_postgresql( juju: jubilant.Juju, @@ -443,3 +691,159 @@ def test_garm_metrics_endpoint_no_auth( "Expected the garm_health metric in the /metrics response; " f"got first 500 chars: {resp.text[:500]}" ) + + +def test_scaleset_creation_deferred_when_provider_missing( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, +): + """ + arrange: GARM and the test configurator are deployed, and a matching credential exists. + act: Override the relation data with a provider name GARM does not know about. + assert: No scaleset is created and the GARM app remains healthy. + """ + _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) + address = _get_garm_address(juju, garm_app) + base_url = _garm_api_base_url(address) + token = garm_login_from_secret(juju, garm_app, base_url) + _create_test_credential(base_url, token) + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + provider_name="missing-provider", + ) + + _wait_for_scaleset_absent(base_url, token, _SCALESET_TEST_NAME) + + status = juju.status() + app_status = status.apps[garm_app].app_status.current + assert app_status in ("active", "waiting"), f"Expected active/waiting, got {app_status}" + + +def test_scalesets_created_from_relation_data( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, +): + """ + arrange: GARM is active and receives valid scaleset relation data plus a test credential. + act: Reconcile against relation data that points at the built-in openstack provider. + assert: A matching scaleset appears in the GARM API. + """ + _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) + address = _get_garm_address(juju, garm_app) + base_url = _garm_api_base_url(address) + token = garm_login_from_secret(juju, garm_app, base_url) + _create_test_credential(base_url, token) + relation_data = _get_scaleset_relation_data( + juju, garm_app, garm_configurator_for_scaleset_tests + ) + provider_names = {provider.get("name", "") for provider in _list_providers(base_url, token)} + provider_name = relation_data["provider_name"] + if provider_name not in provider_names: + pytest.skip( + f"requires GARM provider {provider_name!r}; available providers: {sorted(provider_names)}" + ) + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + min_idle_runner="1", + ) + + scaleset = _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + assert scaleset["name"] == _SCALESET_TEST_NAME + + +def test_scaleset_updated_on_relation_change( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, +): + """ + arrange: A scaleset already exists in GARM from valid configurator relation data. + act: Change the relation data image_id and wait for another reconcile. + assert: The scaleset reflects the updated image_id. + """ + _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) + address = _get_garm_address(juju, garm_app) + base_url = _garm_api_base_url(address) + token = garm_login_from_secret(juju, garm_app, base_url) + _create_test_credential(base_url, token) + relation_data = _get_scaleset_relation_data( + juju, garm_app, garm_configurator_for_scaleset_tests + ) + provider_names = {provider.get("name", "") for provider in _list_providers(base_url, token)} + provider_name = relation_data["provider_name"] + if provider_name not in provider_names: + pytest.skip( + f"requires GARM provider {provider_name!r}; available providers: {sorted(provider_names)}" + ) + original_image_id = relation_data["image_id"] + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + min_idle_runner="1", + ) + _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + image_id=f"{original_image_id}-updated", + ) + + scaleset = _wait_for_scaleset( + base_url, + token, + _SCALESET_TEST_NAME, + image_id=f"{original_image_id}-updated", + ) + observed_image = scaleset.get("image_id") or scaleset.get("image") + assert observed_image == f"{original_image_id}-updated" + + +def test_scaleset_deleted_when_relation_removed( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, +): + """ + arrange: A scaleset exists in GARM for the test configurator relation. + act: Remove the garm-configurator relation from GARM. + assert: The matching scaleset disappears from GARM. + """ + _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) + address = _get_garm_address(juju, garm_app) + base_url = _garm_api_base_url(address) + token = garm_login_from_secret(juju, garm_app, base_url) + _create_test_credential(base_url, token) + relation_data = _get_scaleset_relation_data( + juju, garm_app, garm_configurator_for_scaleset_tests + ) + provider_names = {provider.get("name", "") for provider in _list_providers(base_url, token)} + provider_name = relation_data["provider_name"] + if provider_name not in provider_names: + pytest.skip( + f"requires GARM provider {provider_name!r}; available providers: {sorted(provider_names)}" + ) + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + min_idle_runner="1", + ) + _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + + juju.remove_relation(garm_app, garm_configurator_for_scaleset_tests) + juju.wait( + lambda status: jubilant.all_active(status, garm_app), + timeout=3 * 60, + delay=10, + ) + + _wait_for_scaleset_absent(base_url, token, _SCALESET_TEST_NAME) From d6028e30c9505331656d2964327628ab4b1263c2 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 12:26:01 +0700 Subject: [PATCH 09/21] fix(garm): preserve scalesets on missing provider/credential; use JSON for pre_install_scripts - scaleset_reconciler: track all desired names (not just ready ones) so existing scalesets are NOT deleted when their provider or credential is temporarily unavailable - charm.py / configurator charm.py: switch pre_install_scripts wire format from str()/ast.literal_eval to json.dumps()/json.loads for a well-defined serialization; add regression test for the delete-on-skip bug Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm-configurator/src/charm.py | 7 +++++- .../tests/unit/test_charm.py | 2 +- charms/garm/src/charm.py | 7 +++--- charms/garm/src/scaleset_reconciler.py | 7 +++--- .../tests/unit/test_scaleset_reconciler.py | 23 ++++++++++++++++++- 5 files changed, 37 insertions(+), 9 deletions(-) diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index c1cb2360..3643604a 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -4,6 +4,7 @@ """Charm entrypoint for the GARM configurator charm.""" +import json import typing import ops @@ -76,7 +77,11 @@ def _reconcile(self, event: ops.EventBase) -> None: else ",".join(state.scaleset_config.labels) ), "runner_group": state.scaleset_config.runner_group, - "pre_install_scripts": str(state.scaleset_config.pre_install_scripts), + "pre_install_scripts": json.dumps( + {"pre_install.sh": state.scaleset_config.pre_install_scripts} + if state.scaleset_config.pre_install_scripts + else {} + ), } for garm_relation in self.model.relations[GARM_CONFIGURATOR_RELATION_NAME]: garm_relation.data[self.unit].update(relation_data) diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index d54eff07..afae0684 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -469,7 +469,7 @@ def test_garm_configurator_relation_data_written_on_reconcile(): "max_runner": "5", "labels": "self-hosted,linux", "runner_group": "default", - "pre_install_scripts": "echo hello", + "pre_install_scripts": '{"pre_install.sh": "echo hello"}', } for key, value in expected_relation_data.items(): assert rel_out.local_unit_data[key] == value diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 91d616f6..50fbb28f 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -5,6 +5,7 @@ """GARM charm entrypoint.""" import dataclasses +import json import logging import secrets as _secrets import string @@ -113,14 +114,14 @@ def _generate_garm_secrets() -> dict[str, str]: def _parse_pre_install_scripts(raw: str) -> dict[str, str]: - """Parse pre_install_scripts from relation data string.""" + """Parse pre_install_scripts from JSON relation data string.""" if not raw: return {} try: - result = ast.literal_eval(raw) + result = json.loads(raw) if isinstance(result, dict): return result - except (ValueError, SyntaxError): + except (ValueError, json.JSONDecodeError): pass return {} diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 5e6b4067..02a6604b 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -54,7 +54,8 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: credentials = {credential["name"] for credential in self._client.list_credentials()} observed = {scaleset["name"]: scaleset for scaleset in self._client.list_scalesets()} - desired_names: set[str] = set() + all_desired_names: set[str] = {spec.name for spec in desired} + ready_names: set[str] = set() for spec in desired: if spec.provider_name not in providers: @@ -72,7 +73,7 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: ) continue - desired_names.add(spec.name) + ready_names.add(spec.name) if spec.name in observed: self._maybe_update(observed[spec.name], spec) @@ -80,7 +81,7 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: self._create(spec) for name, scaleset in observed.items(): - if name not in desired_names: + if name not in all_desired_names: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset["id"]) self._client.delete_scaleset(scaleset["id"]) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 62bf8010..610fa8b3 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -177,7 +177,28 @@ def test_pre_install_scripts_included_in_create(): assert payload["extra_specs"]["pre_install_scripts"] == scripts -def test_labels_sorted_for_comparison(): +def test_existing_scaleset_not_deleted_when_provider_temporarily_missing(): + existing = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + } + client = _mock_client( + providers=[], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + client.delete_scaleset.assert_not_called() + client.create_scaleset.assert_not_called() + existing = { "id": "uuid-1", "name": "my-scaleset", From dad0f08ff1f042315e14b9f97ced52c84dcfa1f0 Mon Sep 17 00:00:00 2001 From: florentianayuwono Date: Tue, 16 Jun 2026 12:28:06 +0700 Subject: [PATCH 10/21] style: fix import sort and blank line lint issues Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- charms/garm-configurator/src/charm_state.py | 1 + charms/garm/src/charm.py | 1 + 2 files changed, 2 insertions(+) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index d180ff45..8a939894 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -357,6 +357,7 @@ def credentials_name(self) -> str: """Derived GARM credentials name for scalesets.""" return f"github-app-{self.github_app_config.client_id}" + def _get_image_id_from_relation(charm: ops.CharmBase) -> str | None: """Return the OpenStack image UUID from the image builder relation, if available. diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 50fbb28f..aa8bf578 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -15,6 +15,7 @@ import paas_charm.go import tomli_w from paas_charm.app import WorkloadConfig + from garm_api import GarmApiError, GarmClient from scaleset_reconciler import ScalesetReconciler, ScalesetSpec From 416738699d5d720f12366a73d88846f9a5e724a6 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:19:10 +0200 Subject: [PATCH 11/21] feat(garm): apply configurator runner options via per-scaleset GARM templates Expose six runner-behaviour options on the garm-configurator charm (dockerhub-mirror, runner-http-proxy, aproxy-exclude-addresses, aproxy-redirect-ports, otel-collector-endpoint, pre-job-script), validated by a new RunnerConfig model and shared over the garm-configurator relation. The GARM charm reads them into ScalesetSpec.runner_config and the scaleset reconciler copies GARM's system github_linux template, injects the rendered pre-install and pre-job hook blocks (runner_template.py), and creates/updates a per-scaleset template via the templates API, referencing it by template_id. Also enable the scaleset reconcile path on this branch: register the GARM admin via GarmClient.first_run (_maybe_first_run) and target the in-pod GARM URL, and align the integration test to authenticate with the charm-managed admin credentials. --- charms/garm-configurator/charmcraft.yaml | 24 ++ charms/garm-configurator/src/charm.py | 7 + charms/garm-configurator/src/charm_state.py | 145 ++++++++++ .../tests/unit/test_charm.py | 131 +++++++++ .../tests/unit/test_charm_state.py | 10 +- charms/garm/src/charm.py | 52 +++- charms/garm/src/garm_api.py | 129 +++++++++ charms/garm/src/runner_template.py | 266 ++++++++++++++++++ charms/garm/src/scaleset_reconciler.py | 106 ++++++- charms/garm/tests/unit/test_charm.py | 39 +++ charms/garm/tests/unit/test_garm_api.py | 155 ++++++++++ .../garm/tests/unit/test_runner_template.py | 111 ++++++++ .../tests/unit/test_scaleset_reconciler.py | 109 ++++++- charms/tests/integration/test_garm.py | 117 ++++++-- 14 files changed, 1363 insertions(+), 38 deletions(-) create mode 100644 charms/garm/src/runner_template.py create mode 100644 charms/garm/tests/unit/test_runner_template.py diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 77e02ace..ec87aa5c 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -107,6 +107,30 @@ config: description: | A Python dict-like string containing key-value pairs of script name to bash script to be run prior to runner installation. + dockerhub-mirror: + type: string + description: | + Optional Docker registry mirror URL for runners. Must be a valid http(s) address. + runner-http-proxy: + type: string + description: | + HTTP proxy that aproxy forwards to. + aproxy-exclude-addresses: + type: string + description: | + Comma-separated IP addresses/CIDRs to exclude from aproxy forwarding. + aproxy-redirect-ports: + type: string + description: | + Comma-separated ports or port ranges (e.g. 80,443,8000-9000) to forward to aproxy. + otel-collector-endpoint: + type: string + description: | + The OTEL exporter address for the otel-collector to connect to. Must be a valid address. + pre-job-script: + type: string + description: | + A bash script appended to the end of the runner pre-job script. provides: garm-configurator: diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 3643604a..ddc8df74 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -62,6 +62,7 @@ def _reconcile(self, event: ops.EventBase) -> None: } ) + rc = state.runner_config relation_data = { "name": state.scaleset_config.name, "provider_name": state.provider_name, @@ -82,6 +83,12 @@ def _reconcile(self, event: ops.EventBase) -> None: if state.scaleset_config.pre_install_scripts else {} ), + "dockerhub_mirror": rc.dockerhub_mirror or "", + "runner_http_proxy": rc.runner_http_proxy or "", + "aproxy_exclude_addresses": rc.aproxy_exclude_addresses or "", + "aproxy_redirect_ports": rc.aproxy_redirect_ports or "", + "otel_collector_endpoint": rc.otel_collector_endpoint or "", + "pre_job_script": rc.pre_job_script or "", } for garm_relation in self.model.relations[GARM_CONFIGURATOR_RELATION_NAME]: garm_relation.data[self.unit].update(relation_data) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 8a939894..a1b91a4f 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -3,6 +3,10 @@ """State of the GARM configurator charm.""" +import contextlib +import ipaddress +import urllib.parse + import ops from pydantic import BaseModel @@ -30,6 +34,13 @@ SCALESET_RUNNER_GROUP_CONFIG_NAME = "runner-group" SCALESET_PRE_INSTALL_SCRIPTS_CONFIG_NAME = "pre-install-scripts" +DOCKERHUB_MIRROR_CONFIG_NAME = "dockerhub-mirror" +RUNNER_HTTP_PROXY_CONFIG_NAME = "runner-http-proxy" +APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME = "aproxy-exclude-addresses" +APROXY_REDIRECT_PORTS_CONFIG_NAME = "aproxy-redirect-ports" +OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME = "otel-collector-endpoint" +PRE_JOB_SCRIPT_CONFIG_NAME = "pre-job-script" + IMAGE_RELATION_NAME = "image" @@ -292,6 +303,134 @@ def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": ) +def _validate_http_url(config_name: str, value: str) -> None: + """Raise CharmConfigInvalidError if value is not a valid http(s) URL. + + Args: + config_name: Config option name used in the error message. + value: URL string to validate. + + Raises: + CharmConfigInvalidError: When the URL scheme is not http/https or netloc is empty. + """ + parsed = urllib.parse.urlparse(value) + if parsed.scheme not in ("http", "https") or not parsed.netloc: + raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") + + +class RunnerConfig(BaseModel): + """Optional runner-level configuration forwarded to the GARM scaleset. + + Attributes: + dockerhub_mirror: Optional Docker registry mirror URL. + runner_http_proxy: HTTP proxy address for aproxy to forward to. + aproxy_exclude_addresses: Comma-separated IPs/CIDRs excluded from aproxy forwarding. + aproxy_redirect_ports: Comma-separated ports or N-M ranges forwarded to aproxy. + otel_collector_endpoint: OTEL exporter address for the otel-collector. + pre_job_script: Bash snippet appended to the runner pre-job script. + """ + + dockerhub_mirror: str | None = None + runner_http_proxy: str | None = None + aproxy_exclude_addresses: str | None = None + aproxy_redirect_ports: str | None = None + otel_collector_endpoint: str | None = None + pre_job_script: str | None = None + + @classmethod + def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": + """Initialize the runner config from charm, applying best-effort validation. + + All options are optional; unset or empty values result in None fields. + + Args: + charm: The charm instance. + + Raises: + CharmConfigInvalidError: If a set option fails validation. + + Returns: + The parsed runner configuration. + """ + url_options = ( + DOCKERHUB_MIRROR_CONFIG_NAME, + RUNNER_HTTP_PROXY_CONFIG_NAME, + OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME, + ) + url_values: dict[str, str | None] = {} + for config_name in url_options: + raw = charm.config.get(config_name) + value = str(raw).strip() if raw else None + if value: + _validate_http_url(config_name, value) + url_values[config_name] = value or None + + raw_exclude = charm.config.get(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME) + aproxy_exclude_addresses: str | None = None + if raw_exclude: + tokens = [t.strip() for t in str(raw_exclude).split(",")] + # Reject empty tokens (e.g. trailing comma) as they signal a misconfiguration. + for token in tokens: + if not token: + raise CharmConfigInvalidError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} contains an empty address token" + ) + with contextlib.suppress(ValueError): + # Non-IP tokens (hostnames) are accepted on a best-effort basis. + ipaddress.ip_network(token, strict=False) + aproxy_exclude_addresses = ",".join(tokens) + + raw_ports = charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME) + aproxy_redirect_ports: str | None = None + if raw_ports: + tokens_ports = [t.strip() for t in str(raw_ports).split(",")] + for token in tokens_ports: + _validate_port_token(token) + aproxy_redirect_ports = ",".join(tokens_ports) + + raw_script = charm.config.get(PRE_JOB_SCRIPT_CONFIG_NAME) + pre_job_script = str(raw_script).strip() if raw_script else None + + return cls( + dockerhub_mirror=url_values[DOCKERHUB_MIRROR_CONFIG_NAME], + runner_http_proxy=url_values[RUNNER_HTTP_PROXY_CONFIG_NAME], + aproxy_exclude_addresses=aproxy_exclude_addresses, + aproxy_redirect_ports=aproxy_redirect_ports, + otel_collector_endpoint=url_values[OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME], + pre_job_script=pre_job_script or None, + ) + + +def _validate_port_token(token: str) -> None: + """Raise CharmConfigInvalidError if token is not a valid port or N-M range. + + Args: + token: A single comma-split token from the aproxy-redirect-ports config. + + Raises: + CharmConfigInvalidError: When the token is not a valid port or N<=M range in 1..65535. + """ + error_msg = ( + f"{APROXY_REDIRECT_PORTS_CONFIG_NAME} must be a comma-separated list of " + "ports or N-M ranges in 1..65535" + ) + if "-" in token: + parts = token.split("-", 1) + try: + low, high = int(parts[0]), int(parts[1]) + except (ValueError, IndexError): + raise CharmConfigInvalidError(error_msg) + if not (1 <= low <= 65535 and 1 <= high <= 65535 and low <= high): + raise CharmConfigInvalidError(error_msg) + else: + try: + port = int(token) + except ValueError: + raise CharmConfigInvalidError(error_msg) + if not 1 <= port <= 65535: + raise CharmConfigInvalidError(error_msg) + + class CharmState: """The charm state. @@ -299,6 +438,7 @@ class CharmState: provider_config: OpenStack provider configuration. github_app_config: GitHub App configuration. scaleset_config: Scaleset configuration. + runner_config: Optional runner-level configuration. image_id: OpenStack image UUID received from the image builder relation, or None. """ @@ -308,6 +448,7 @@ def __init__( provider_config: ProviderConfig, github_app_config: GithubAppConfig, scaleset_config: ScalesetConfig, + runner_config: RunnerConfig, image_id: str | None, ) -> None: """Initialize the charm state. @@ -316,11 +457,13 @@ def __init__( provider_config: The OpenStack provider configuration. github_app_config: The GitHub App configuration. scaleset_config: The scaleset configuration. + runner_config: The optional runner configuration. image_id: The OpenStack image UUID from the image builder relation. """ self.provider_config = provider_config self.github_app_config = github_app_config self.scaleset_config = scaleset_config + self.runner_config = runner_config self.image_id = image_id @classmethod @@ -339,11 +482,13 @@ def from_charm(cls, charm: ops.CharmBase) -> "CharmState": provider_config = ProviderConfig.from_charm(charm) github_app_config = GithubAppConfig.from_charm(charm) scaleset_config = ScalesetConfig.from_charm(charm) + runner_config = RunnerConfig.from_charm(charm) image_id = _get_image_id_from_relation(charm) return cls( provider_config=provider_config, github_app_config=github_app_config, scaleset_config=scaleset_config, + runner_config=runner_config, image_id=image_id, ) diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index afae0684..babb1edb 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -537,3 +537,134 @@ def test_garm_configurator_relation_changed_triggers_reconcile(): rel_out = out.get_relation(garm_relation.id) assert rel_out.local_unit_data["name"] == "my-scaleset" + + +# --------------------------------------------------------------------------- +# Runner config — invalid value tests +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "config_key, bad_value, expected_fragment", + [ + pytest.param( + "dockerhub-mirror", + "ftp://registry.example.com", + "dockerhub-mirror must be a valid http(s) URL", + id="dockerhub-mirror-bad-scheme", + ), + pytest.param( + "runner-http-proxy", + "not-a-url", + "runner-http-proxy must be a valid http(s) URL", + id="runner-http-proxy-no-scheme", + ), + pytest.param( + "otel-collector-endpoint", + "://missing-host", + "otel-collector-endpoint must be a valid http(s) URL", + id="otel-collector-endpoint-missing-host", + ), + pytest.param( + "aproxy-redirect-ports", + "80,not-a-port", + "aproxy-redirect-ports must be a comma-separated list of ports or N-M ranges in 1..65535", + id="aproxy-redirect-ports-non-numeric", + ), + pytest.param( + "aproxy-redirect-ports", + "0", + "aproxy-redirect-ports must be a comma-separated list of ports or N-M ranges in 1..65535", + id="aproxy-redirect-ports-out-of-range", + ), + pytest.param( + "aproxy-redirect-ports", + "443-80", + "aproxy-redirect-ports must be a comma-separated list of ports or N-M ranges in 1..65535", + id="aproxy-redirect-ports-inverted-range", + ), + ], +) +def test_charm_blocked_invalid_runner_config( + config_key: str, bad_value: str, expected_fragment: str +): + """ + arrange: A single runner config option is set to an invalid value. + act: Run config-changed. + assert: Unit status is Blocked with a message matching the expected fragment. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config[config_key] = bad_value + state = State(config=config, secrets=[secret, pk_secret]) + out = ctx.run(ctx.on.config_changed(), state) + assert isinstance(out.unit_status, ops.BlockedStatus) + assert expected_fragment in out.unit_status.message + + +def test_runner_config_fields_written_to_garm_configurator_relation(): + """ + arrange: Valid config with all six runner config options set. + act: Run config-changed with a garm-configurator relation present. + assert: All six runner config keys appear in the relation databag with correct values. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["dockerhub-mirror"] = "https://mirror.example.com" + config["runner-http-proxy"] = "http://proxy.example.com:3128" + config["aproxy-exclude-addresses"] = "10.0.0.1,192.168.0.0/16" + config["aproxy-redirect-ports"] = "80,443,8000-9000" + config["otel-collector-endpoint"] = "http://otel.example.com:4317" + config["pre-job-script"] = " echo start " + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + ) + + out = ctx.run(ctx.on.config_changed(), state) + + rel_out = out.get_relation(garm_relation.id) + assert rel_out.local_unit_data["dockerhub_mirror"] == "https://mirror.example.com" + assert rel_out.local_unit_data["runner_http_proxy"] == "http://proxy.example.com:3128" + assert rel_out.local_unit_data["aproxy_exclude_addresses"] == "10.0.0.1,192.168.0.0/16" + assert rel_out.local_unit_data["aproxy_redirect_ports"] == "80,443,8000-9000" + assert rel_out.local_unit_data["otel_collector_endpoint"] == "http://otel.example.com:4317" + assert rel_out.local_unit_data["pre_job_script"] == "echo start" + + +def test_runner_config_fields_absent_when_unset(): + """ + arrange: Valid config with no runner config options set. + act: Run config-changed with a garm-configurator relation present. + assert: The six runner config keys are absent from the databag (empty strings are stripped by + the scenario harness, mirroring how Juju omits unset string keys from real databags). + Consumers should use .get(key, "") to treat absent keys as empty. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + garm_relation = _make_garm_configurator_relation() + state = State( + config=_valid_config(secret, pk_secret), + secrets=[secret, pk_secret], + relations=[garm_relation], + ) + + out = ctx.run(ctx.on.config_changed(), state) + + rel_out = out.get_relation(garm_relation.id) + for key in ( + "dockerhub_mirror", + "runner_http_proxy", + "aproxy_exclude_addresses", + "aproxy_redirect_ports", + "otel_collector_endpoint", + "pre_job_script", + ): + assert rel_out.local_unit_data.get(key, "") == "" diff --git a/charms/garm-configurator/tests/unit/test_charm_state.py b/charms/garm-configurator/tests/unit/test_charm_state.py index b453e2bc..855fd9ad 100644 --- a/charms/garm-configurator/tests/unit/test_charm_state.py +++ b/charms/garm-configurator/tests/unit/test_charm_state.py @@ -3,7 +3,13 @@ """Unit tests for charm_state.""" -from charm_state import CharmState, GithubAppConfig, ProviderConfig, ScalesetConfig +from charm_state import ( + CharmState, + GithubAppConfig, + ProviderConfig, + RunnerConfig, + ScalesetConfig, +) def test_provider_name_derived_from_project_name(): @@ -34,6 +40,7 @@ def test_provider_name_derived_from_project_name(): repo="myorg/myrepo", ), image_id=None, + runner_config=RunnerConfig(), ) assert state.provider_name == f"openstack-{project_name}" @@ -67,6 +74,7 @@ def test_credentials_name_derived_from_client_id(): repo="myorg/myrepo", ), image_id=None, + runner_config=RunnerConfig(), ) assert state.credentials_name == f"github-app-{client_id}" diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index aa8bf578..93d0eaa6 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -17,6 +17,7 @@ from paas_charm.app import WorkloadConfig from garm_api import GarmApiError, GarmClient +from runner_template import RunnerConfig from scaleset_reconciler import ScalesetReconciler, ScalesetSpec logger = logging.getLogger(__name__) @@ -29,6 +30,8 @@ GARM_BINARY: typing.Final[str] = "/usr/local/bin/garm" OPENSTACK_PROVIDER_BINARY: typing.Final[str] = "/usr/local/bin/garm-provider-openstack" GARM_PORT: typing.Final[int] = 8080 +GARM_ADMIN_EMAIL: typing.Final[str] = "admin@garm.local" +GARM_ADMIN_FULL_NAME: typing.Final[str] = "GARM Admin" _DB_PASSPHRASE_LENGTH: typing.Final[int] = 32 @@ -138,6 +141,7 @@ def __init__(self, *args: typing.Any) -> None: """ super().__init__(*args) self.framework.observe(self.on.install, self._on_install) + self.framework.observe(self.on.update_status, self._on_update_status) self.framework.observe( self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, self._on_configurator_changed, @@ -151,6 +155,10 @@ def _on_install(self, _: ops.InstallEvent) -> None: """Ensure secrets exist on first install.""" self._ensure_secrets() + def _on_update_status(self, _: ops.HookEvent) -> None: + """Retry GARM admin first-run until it succeeds.""" + self._maybe_first_run() + @property def _workload_config(self) -> WorkloadConfig: """Pin GARM to a fixed port and disable the default metrics scrape job. @@ -237,6 +245,35 @@ def restart(self, rerun_migrations: bool = False) -> None: combine=True, ) container.replan() + self._maybe_first_run() + + def _maybe_first_run(self) -> None: + """Register the GARM admin user if it does not exist yet (leader only). + + GARM needs an initial admin (POST /first-run) before its API accepts + authenticated calls. GarmClient.first_run is idempotent — a 409 + "already initialised" is ignored — so this is safe to call on every + event; a failure (e.g. GARM not yet accepting connections right after a + restart) is logged and retried on the next event (update-status, relation + change, or scaleset reconcile). + """ + if not self.unit.is_leader(): + return + secret = self._get_garm_secrets() + if secret is None: + return + content = secret.get_content(refresh=True) + admin_username = content.get("admin-username") + admin_password = content.get("admin-password") + if not admin_username or not admin_password: + return + try: + client = GarmClient(f"{self._get_garm_url()}/api/v1") + client.first_run( + admin_username, admin_password, GARM_ADMIN_EMAIL, GARM_ADMIN_FULL_NAME + ) + except GarmApiError as exc: + logger.info("GARM first-run not completed yet (will retry): %s", exc) def _ensure_secrets(self) -> None: """Create the garm-secrets juju secret on first call (leader only).""" @@ -276,12 +313,12 @@ def _get_secrets(self) -> dict[str, str]: return secret.get_content() def _get_garm_url(self) -> str: - """Build the local GARM API URL from charm configuration.""" - listen_address = str(self.config.get("garm-listen-address", "")) - listen_port = self.config.get("garm-listen-port", 9997) - if not listen_address: - return "" - return f"http://{listen_address}:{listen_port}" + """Return the local GARM API URL. + + GARM binds its API to the fixed port in-pod (see render_garm_toml), so + the charm always reaches it over loopback. + """ + return f"http://127.0.0.1:{GARM_PORT}" def _get_postgresql_config(self) -> dict[str, typing.Any] | None: """Get PostgreSQL config from relation data, or None if not available. @@ -354,12 +391,15 @@ def _build_desired_scalesets(self) -> list[ScalesetSpec]: pre_install_scripts=_parse_pre_install_scripts( data.get("pre_install_scripts", "") ), + runner_config=RunnerConfig.from_databag(data), ) ) return specs def _reconcile_scalesets(self) -> None: """Sync GARM scalesets against garm-configurator relation data.""" + # Ensure the GARM admin exists before we attempt to authenticate below. + self._maybe_first_run() secret = self._get_garm_secrets() if not secret: logger.warning("GARM secrets not yet available; deferring scaleset reconcile") diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index 62cf267a..525bf4f2 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -4,9 +4,11 @@ """GARM REST API client.""" +import base64 import json import logging import urllib.error +import urllib.parse import urllib.request from typing import Any @@ -231,3 +233,130 @@ def delete_scaleset(self, scaleset_id: str) -> None: GarmApiError: On API error. """ self._request("DELETE", f"/scalesets/{scaleset_id}") + + def list_templates( + self, + os_type: str | None = None, + forge_type: str | None = None, + partial_name: str | None = None, + ) -> list[dict[str, Any]]: + """List runner templates, optionally filtered by type or name. + + Args: + os_type: Optional OS type filter (e.g. "linux", "windows"). + forge_type: Optional forge type filter (e.g. "github", "gitea"). + partial_name: Optional substring to filter template names. + + Raises: + GarmApiError: On API error. + + Returns: + List of template dicts, each containing at minimum 'id' and 'name'. + """ + params = { + k: v + for k, v in { + "os_type": os_type, + "forge_type": forge_type, + "partial_name": partial_name, + }.items() + if v is not None + } + path = "/templates" + if params: + path = f"{path}?{urllib.parse.urlencode(params)}" + return self._request("GET", path) or [] + + def get_template(self, template_id: int) -> dict[str, Any]: + """Fetch a single template by ID. + + Args: + template_id: Numeric template ID. + + Raises: + GarmApiError: On API error. + + Returns: + Template dict with at minimum 'id', 'name', 'os_type', 'forge_type', and 'data'. + """ + return self._request("GET", f"/templates/{template_id}") + + def create_template( + self, + *, + name: str, + data: bytes, + os_type: str = "linux", + forge_type: str = "github", + description: str = "", + ) -> dict[str, Any]: + """Create a new runner template. + + The ``data`` bytes are base64-encoded before sending because GARM stores + template scripts as a Go ``[]byte``, which serialises to base64 in JSON. + + Args: + name: Template name. + data: Raw script bytes to store in the template. + os_type: OS type (default: "linux"). + forge_type: Forge type (default: "github"). + description: Optional human-readable description. + + Raises: + GarmApiError: On API error. + + Returns: + Created template dict. + """ + payload = { + "name": name, + "data": base64.b64encode(data).decode(), + "os_type": os_type, + "forge_type": forge_type, + "description": description, + } + return self._request("POST", "/templates", payload) + + def update_template( + self, + template_id: int, + *, + data: bytes, + name: str | None = None, + description: str | None = None, + ) -> dict[str, Any]: + """Update an existing template's data and optionally its name or description. + + Only ``data`` is always sent; ``name`` and ``description`` are included + only when explicitly provided, leaving unspecified fields unchanged on + the server. + + Args: + template_id: Numeric template ID. + data: New raw script bytes (required by the GARM API). + name: New template name, or None to leave unchanged. + description: New description, or None to leave unchanged. + + Raises: + GarmApiError: On API error. + + Returns: + Updated template dict. + """ + payload: dict[str, Any] = {"data": base64.b64encode(data).decode()} + if name is not None: + payload["name"] = name + if description is not None: + payload["description"] = description + return self._request("PUT", f"/templates/{template_id}", payload) + + def delete_template(self, template_id: int) -> None: + """Delete a template by ID. + + Args: + template_id: Numeric template ID. + + Raises: + GarmApiError: On API error. + """ + self._request("DELETE", f"/templates/{template_id}") diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py new file mode 100644 index 00000000..f7f5e156 --- /dev/null +++ b/charms/garm/src/runner_template.py @@ -0,0 +1,266 @@ +#!/usr/bin/env python3 +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Render runner-behaviour options into a GARM runner-install template. + +GARM 0.2 stores the runner-install script as a server-side template that a +scaleset references by ``template_id``. The six operator-facing runner options +(docker mirror, proxy/aproxy, telemetry, pre-job hook) are delivered by copying +GARM's system ``github_linux`` template and injecting two blocks right after the +shebang — before the base template's ``set -e`` — so a best-effort step can never +abort the whole bootstrap: + + * a *pre-install* block that runs as root before the runner is installed + (aproxy, docker registry mirror, static host prep), and + * a *runner job hooks* block that writes the GitHub job-start hook and the + runner ``env`` file under ``/home/runner/actions-runner`` (GARM hardcodes the + ``runner`` user and that path). + +The reconciler creates/updates the per-scaleset template with the bytes returned +by :func:`build_template_data` and only does so when :meth:`RunnerConfig.has_config` +is true. +""" + +import json +import shlex +from collections.abc import Mapping +from dataclasses import dataclass + +# GARM hardcodes the runner username and actions-runner directory; the env file +# read by the runner service therefore lives at the path below. +RUNNER_USER = "runner" +RUNNER_HOME = "/home/runner/actions-runner" +PRE_JOB_HOOK_PATH = f"{RUNNER_HOME}/pre-job.sh" +RUNNER_ENV_PATH = f"{RUNNER_HOME}/env" + +# Databag keys written by the garm-configurator charm. Kept here as the single +# source of truth for the configurator→garm relation contract for runner options. +DATABAG_KEYS = ( + "dockerhub_mirror", + "runner_http_proxy", + "aproxy_exclude_addresses", + "aproxy_redirect_ports", + "otel_collector_endpoint", + "pre_job_script", +) + + +@dataclass(frozen=True) +class RunnerConfig: + """Runner-behaviour options sourced from the garm-configurator relation. + + Every field is a plain string; an empty string means "unset". Values are + validated upstream by the configurator charm, so rendering here is purely + mechanical. + + Attributes: + dockerhub_mirror: Docker registry mirror URL, or "". + runner_http_proxy: Upstream HTTP proxy that aproxy forwards to, or "". + aproxy_exclude_addresses: Comma-separated addresses/CIDRs to bypass, or "". + aproxy_redirect_ports: Comma-separated ports / N-M ranges to redirect, or "". + otel_collector_endpoint: OTEL exporter endpoint, or "". + pre_job_script: Operator bash appended to the pre-job hook, or "". + """ + + dockerhub_mirror: str = "" + runner_http_proxy: str = "" + aproxy_exclude_addresses: str = "" + aproxy_redirect_ports: str = "" + otel_collector_endpoint: str = "" + pre_job_script: str = "" + + @classmethod + def from_databag(cls, data: Mapping[str, str]) -> "RunnerConfig": + """Build a config from a relation databag, ignoring missing keys. + + Args: + data: The relation unit databag (or any mapping). + + Returns: + A RunnerConfig with each field taken from its databag key, "" if absent. + """ + return cls(**{key: (data.get(key) or "").strip() for key in DATABAG_KEYS}) + + def has_config(self) -> bool: + """Whether any runner option is set (i.e. a custom template is needed). + + Returns: + True if at least one field is non-empty. + """ + return any(getattr(self, key) for key in DATABAG_KEYS) + + +def build_template_data(base: bytes, config: RunnerConfig) -> bytes: + """Inject the runner-option blocks into a base runner-install template. + + The blocks are inserted immediately after the shebang line (before the base + template's ``set -e``), mirroring GARM's documented "prepend after the + shebang" approach. + + Args: + base: The system ``github_linux`` template bytes to copy from. + config: The runner options to render. + + Returns: + The new template bytes, with the pre-install and job-hook blocks injected. + """ + text = base.decode("utf-8") + injection = render_pre_install(config) + render_pre_job_hooks(config) + if "\n" in text: + shebang, rest = text.split("\n", 1) + return f"{shebang}\n{injection}{rest}".encode("utf-8") + return f"{text}\n{injection}".encode("utf-8") + + +def render_pre_install(config: RunnerConfig) -> str: + """Render the root pre-install block (runs before the runner is installed). + + Args: + config: The runner options to render. + + Returns: + A bash snippet, terminated by a newline. + """ + sections = ["", "# ===== charm-injected pre-install setup (runs as root) ====="] + if config.runner_http_proxy: + sections.append(_render_aproxy(config)) + if config.dockerhub_mirror: + sections.append(_render_dockerhub_mirror(config.dockerhub_mirror)) + sections.append(_render_static_host_prep()) + sections.append("# ===== end charm-injected pre-install setup =====\n") + return "\n".join(sections) + + +def render_pre_job_hooks(config: RunnerConfig) -> str: + """Render the runner ``env`` file and GitHub job-start hook. + + The hook always carries a hardcoded proxy-IP-resolution step (pins the proxy + address for the job's lifetime) and appends the operator's ``pre-job-script``. + The OTEL endpoint, when set, is exported via the runner ``env`` file. + + Args: + config: The runner options to render. + + Returns: + A bash snippet, terminated by a newline. + """ + hook_body = _PROXY_RESOLVE_SNIPPET + if config.pre_job_script: + hook_body += "\n\n# --- operator-provided pre-job-script ---\n" + config.pre_job_script + + env_entries = [f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}"] + if config.otel_collector_endpoint: + env_entries.append(f"OTEL_EXPORTER_OTLP_ENDPOINT={config.otel_collector_endpoint}") + + return "\n".join( + [ + "", + "# ===== charm-injected runner job hooks =====", + f"mkdir -p {RUNNER_HOME}", + f"cat > {PRE_JOB_HOOK_PATH} <<'GARM_CHARM_PREJOB'", + hook_body, + "GARM_CHARM_PREJOB", + f"chmod 0755 {PRE_JOB_HOOK_PATH}", + f"cat >> {RUNNER_ENV_PATH} <<'GARM_CHARM_ENV'", + *env_entries, + "GARM_CHARM_ENV", + f"chown -R {RUNNER_USER}:{RUNNER_USER} {RUNNER_HOME} 2>/dev/null || true", + "# ===== end charm-injected runner job hooks =====\n", + ] + ) + + +# The production github-runner charm pins the proxy IP once per job so it cannot +# drift mid-run. The exact resolution logic is environment-specific; this is a +# faithful-but-minimal port. +# TODO(ISD-278): port the exact proxy-resolution script from +# canonical/github-runner-operator once the production source is available. +_PROXY_RESOLVE_SNIPPET = """\ +#!/bin/bash +# Pin the proxy address for the duration of this job. +set -uo pipefail +if [ -n "${HTTP_PROXY:-}" ]; then + proxy_host=$(echo "${HTTP_PROXY}" | sed -E 's#^https?://##; s#[:/].*$##') + proxy_ip=$(getent hosts "${proxy_host}" | awk '{print $1; exit}' || true) + if [ -n "${proxy_ip}" ]; then + echo "${proxy_ip} ${proxy_host}" | sudo tee -a /etc/hosts >/dev/null + fi +fi""" + + +def _render_aproxy(config: RunnerConfig) -> str: + """Render aproxy install + nftables redirect rules. + + Args: + config: The runner options (uses proxy, exclude addresses, redirect ports). + + Returns: + A bash snippet configuring aproxy as a transparent forward proxy. + """ + redirect_ports = config.aproxy_redirect_ports or "80,443" + nft_ports = ", ".join(part.strip() for part in redirect_ports.split(",") if part.strip()) + exclude_guard = "" + if config.aproxy_exclude_addresses: + excludes = ", ".join( + part.strip() for part in config.aproxy_exclude_addresses.split(",") if part.strip() + ) + exclude_guard = f"ip daddr != {{ {excludes} }} " + + # TODO(ISD-278): confirm the exact aproxy listen port / nft ruleset against + # the production github-runner charm cloud-init. + return "\n".join( + [ + "# Transparently forward runner egress through the configured HTTP proxy.", + "snap install aproxy --edge >/dev/null 2>&1 || true", + f"snap set aproxy proxy={shlex.quote(config.runner_http_proxy)} listen=:8443 || true", + "nft -f - <<'GARM_CHARM_NFT' || true", + "table ip aproxy {", + " chain output {", + " type nat hook output priority -100; policy accept;", + f" {exclude_guard}tcp dport {{ {nft_ports} }} counter redirect to :8443", + " }", + "}", + "GARM_CHARM_NFT", + ] + ) + + +def _render_dockerhub_mirror(mirror: str) -> str: + """Render Docker daemon config pointing at the registry mirror. + + Args: + mirror: The registry mirror URL. + + Returns: + A bash snippet writing /etc/docker/daemon.json and restarting docker. + """ + daemon_json = json.dumps({"registry-mirrors": [mirror]}) + return "\n".join( + [ + "# Point Docker at the configured registry mirror.", + "mkdir -p /etc/docker", + "cat > /etc/docker/daemon.json <<'GARM_CHARM_DOCKER'", + daemon_json, + "GARM_CHARM_DOCKER", + "systemctl restart docker >/dev/null 2>&1 || true", + ] + ) + + +def _render_static_host_prep() -> str: + """Render the always-on host-preparation steps ported from the old charm. + + Returns: + A bash snippet. Group membership is applied here; the remaining + production steps are stubbed pending a faithful port. + """ + # TODO(ISD-278): port the remaining production cloud-init steps from + # canonical/github-runner-operator: apt mirror sync self-test, tmate proxy + # setup, and the post-job metrics collector. + return "\n".join( + [ + "# Static runner host preparation (ported from the github-runner charm).", + "usermod -aG lxd,adm ubuntu >/dev/null 2>&1 || true", + ] + ) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 02a6604b..415266ee 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -4,13 +4,19 @@ """Scaleset reconciler: diffs desired vs observed GARM scalesets and applies changes.""" +import base64 import logging from dataclasses import dataclass, field from garm_api import GarmClient +from runner_template import RunnerConfig, build_template_data logger = logging.getLogger(__name__) +# GARM seeds a non-editable system template per forge/OS; we copy this one to +# build per-scaleset runner templates carrying the operator's runner options. +SYSTEM_TEMPLATE_NAME = "github_linux" + @dataclass class ScalesetSpec: @@ -27,6 +33,7 @@ class ScalesetSpec: labels: list[str] = field(default_factory=list) runner_group: str = "default" pre_install_scripts: dict[str, str] = field(default_factory=dict) + runner_config: RunnerConfig = field(default_factory=RunnerConfig) class ScalesetReconciler: @@ -43,9 +50,10 @@ def __init__(self, client: GarmClient) -> None: def reconcile(self, desired: list[ScalesetSpec]) -> None: """Sync GARM scalesets to match *desired*. - Performs the minimum set of CREATE / UPDATE / DELETE operations. - If a referenced provider or credential is missing for a spec, that - spec is skipped silently (deferred creation) — no error state is set. + Performs the minimum set of CREATE / UPDATE / DELETE operations, and + maintains a per-scaleset runner-install template carrying the runner + options. If a referenced provider or credential is missing for a spec, + that spec is skipped silently (deferred creation) — no error state is set. Args: desired: The full desired set of scalesets. @@ -53,9 +61,9 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: providers = {provider["name"] for provider in self._client.list_providers()} credentials = {credential["name"] for credential in self._client.list_credentials()} observed = {scaleset["name"]: scaleset for scaleset in self._client.list_scalesets()} + templates = {template["name"]: template for template in self._client.list_templates()} all_desired_names: set[str] = {spec.name for spec in desired} - ready_names: set[str] = set() for spec in desired: if spec.provider_name not in providers: @@ -73,19 +81,92 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: ) continue - ready_names.add(spec.name) + template_id = self._ensure_template(spec, templates) if spec.name in observed: - self._maybe_update(observed[spec.name], spec) + self._maybe_update(observed[spec.name], spec, template_id) else: - self._create(spec) + self._create(spec, template_id) for name, scaleset in observed.items(): if name not in all_desired_names: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset["id"]) self._client.delete_scaleset(scaleset["id"]) + self._delete_custom_template(name, templates) + + def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> int | None: + """Ensure the scaleset's runner template reflects its runner options. + + Copies the system ``github_linux`` template, injects the runner options, + and creates or updates the per-scaleset template. The template content is + refreshed in place (same id) on every reconcile, so an option change is + applied without touching the scaleset itself. + + Args: + spec: The desired scaleset. + templates: Observed templates keyed by name. + + Returns: + The template id to reference from the scaleset, or None to keep the + default template (no runner options set, or the system template is + unavailable). + """ + if not spec.runner_config.has_config(): + return None + + base = templates.get(SYSTEM_TEMPLATE_NAME) + if base is None: + logger.warning( + "System template %s not found; scaleset %s will use the default template", + SYSTEM_TEMPLATE_NAME, + spec.name, + ) + return None + + new_data = build_template_data(self._template_bytes(base), spec.runner_config) + custom_name = f"{SYSTEM_TEMPLATE_NAME}-{spec.name}" + existing = templates.get(custom_name) + if existing is not None: + if self._template_bytes(existing) != new_data: + logger.info("Updating runner template %s", custom_name) + self._client.update_template(existing["id"], data=new_data) + return existing["id"] + + logger.info("Creating runner template %s", custom_name) + created = self._client.create_template( + name=custom_name, + data=new_data, + description=f"Runner template for scaleset {spec.name}", + ) + return created["id"] + + def _template_bytes(self, template: dict) -> bytes: + """Return a template's raw bytes, fetching the full object if needed. + + Args: + template: A template dict, possibly without its ``data`` field populated. + + Returns: + The decoded template bytes. + """ + data = template.get("data") + if not data: + data = self._client.get_template(template["id"]).get("data", "") + return base64.b64decode(data) if data else b"" + + def _delete_custom_template(self, scaleset_name: str, templates: dict[str, dict]) -> None: + """Delete a scaleset's custom runner template if one exists. + + Args: + scaleset_name: The name of the scaleset being removed. + templates: Observed templates keyed by name. + """ + custom = templates.get(f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}") + if custom is not None: + logger.info("Deleting orphaned runner template %s", custom["name"]) + self._client.delete_template(custom["id"]) - def _create(self, spec: ScalesetSpec) -> None: + def _create(self, spec: ScalesetSpec, template_id: int | None) -> None: extra_specs: dict[str, object] = {} if spec.pre_install_scripts: extra_specs["pre_install_scripts"] = spec.pre_install_scripts @@ -103,11 +184,14 @@ def _create(self, spec: ScalesetSpec) -> None: "runner_group": spec.runner_group, "extra_specs": extra_specs, } + if template_id is not None: + payload["template_id"] = template_id logger.info("Creating scaleset %s", spec.name) self._client.create_scaleset(payload) - def _maybe_update(self, observed: dict, spec: ScalesetSpec) -> None: - if not self._needs_update(observed, spec): + def _maybe_update(self, observed: dict, spec: ScalesetSpec, template_id: int | None) -> None: + template_changed = template_id is not None and observed.get("template_id") != template_id + if not self._needs_update(observed, spec) and not template_changed: logger.debug("Scaleset %s is up to date", spec.name) return @@ -124,6 +208,8 @@ def _maybe_update(self, observed: dict, spec: ScalesetSpec) -> None: "runner_group": spec.runner_group, "extra_specs": extra_specs, } + if template_id is not None: + payload["template_id"] = template_id logger.info("Updating scaleset %s (id=%s)", spec.name, observed["id"]) self._client.update_scaleset(observed["id"], payload) diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 9adff18f..33ea0385 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -235,6 +235,7 @@ def test_admin_password_is_unique_across_calls(): def test_reconcile_scalesets_skips_when_no_secrets(): """Scaleset reconciliation exits early when GARM secrets are unavailable.""" charm = object.__new__(GarmCharm) + charm._maybe_first_run = MagicMock() charm._get_garm_secrets = MagicMock(return_value=None) with patch("charm.GarmClient") as mock_client: @@ -246,6 +247,7 @@ def test_reconcile_scalesets_skips_when_no_secrets(): def test_reconcile_scalesets_skips_restart(): """Scaleset reconciliation must not restart the workload.""" charm = object.__new__(GarmCharm) + charm._maybe_first_run = MagicMock() secret = MagicMock() secret.get_content.return_value = { "admin-username": "admin", @@ -267,3 +269,40 @@ def test_reconcile_scalesets_skips_restart(): mock_reconciler_cls.return_value.reconcile.assert_called_once_with([]) charm.restart.assert_not_called() charm._restart_service.assert_not_called() + + +def test_maybe_first_run_registers_admin_from_secret(): + """The leader registers GARM's admin using the garm-secrets credentials.""" + charm = object.__new__(GarmCharm) + secret = MagicMock() + secret.get_content.return_value = { + "admin-username": "admin", + "admin-password": "Admin-deadbeefcafebabe-Gx1!", + } + charm._get_garm_secrets = MagicMock(return_value=secret) + charm._get_garm_url = MagicMock(return_value="http://127.0.0.1:8080") + + with ( + patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, + patch("charm.GarmClient") as mock_client_cls, + ): + mock_unit.return_value.is_leader.return_value = True + charm._maybe_first_run() + + mock_client_cls.return_value.first_run.assert_called_once_with( + "admin", "Admin-deadbeefcafebabe-Gx1!", "admin@garm.local", "GARM Admin" + ) + + +def test_maybe_first_run_skips_when_not_leader(): + """Non-leader units do not attempt GARM first-run.""" + charm = object.__new__(GarmCharm) + + with ( + patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, + patch("charm.GarmClient") as mock_client_cls, + ): + mock_unit.return_value.is_leader.return_value = False + charm._maybe_first_run() + + mock_client_cls.assert_not_called() diff --git a/charms/garm/tests/unit/test_garm_api.py b/charms/garm/tests/unit/test_garm_api.py index a6e0a971..db3feb9c 100644 --- a/charms/garm/tests/unit/test_garm_api.py +++ b/charms/garm/tests/unit/test_garm_api.py @@ -3,6 +3,7 @@ """Unit tests for the GARM REST API client.""" +import base64 import json from io import BytesIO from unittest.mock import MagicMock, patch @@ -182,3 +183,157 @@ def test_configure_controller_sends_put(): assert req.get_method() == "PUT" body = json.loads(req.data) assert body["metadata_url"] == "http://1.2.3.4:9997/api/v1/metadata" + + +# --------------------------------------------------------------------------- +# Template API tests +# --------------------------------------------------------------------------- + + +def test_list_templates_returns_parsed_list(): + expected = [{"id": 1, "name": "my-template", "os_type": "linux", "forge_type": "github"}] + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(expected) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_templates() + assert result == expected + req = mock_open.call_args[0][0] + assert req.get_method() == "GET" + assert req.full_url == "http://localhost:9997/api/v1/templates" + + +def test_list_templates_returns_empty_on_null_body(): + with patch("urllib.request.urlopen") as mock_open: + mock_resp = MagicMock() + mock_resp.__enter__ = lambda s: s + mock_resp.__exit__ = MagicMock(return_value=False) + mock_resp.read.return_value = b"" + mock_open.return_value = mock_resp + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.list_templates() + assert result == [] + + +def test_list_templates_builds_query_params(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.list_templates(os_type="linux", forge_type="github", partial_name="runner") + req = mock_open.call_args[0][0] + assert "os_type=linux" in req.full_url + assert "forge_type=github" in req.full_url + assert "partial_name=runner" in req.full_url + + +def test_list_templates_omits_none_params(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response([]) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.list_templates(os_type="linux") + req = mock_open.call_args[0][0] + assert "os_type=linux" in req.full_url + assert "forge_type" not in req.full_url + assert "partial_name" not in req.full_url + + +def test_get_template_returns_parsed_dict(): + expected = {"id": 1, "name": "my-template", "data": "aGVsbG8="} + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(expected) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.get_template(1) + assert result == expected + req = mock_open.call_args[0][0] + assert req.get_method() == "GET" + assert req.full_url.endswith("/templates/1") + + +def test_create_template_base64_encodes_data(): + raw = b"#!/bin/bash\necho hello" + expected_b64 = base64.b64encode(raw).decode() + created = {"id": 2, "name": "new-tmpl", "data": expected_b64} + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(created) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.create_template(name="new-tmpl", data=raw) + assert result == created + req = mock_open.call_args[0][0] + assert req.get_method() == "POST" + assert req.full_url.endswith("/templates") + body = json.loads(req.data) + assert body["data"] == expected_b64 + assert body["name"] == "new-tmpl" + assert body["os_type"] == "linux" + assert body["forge_type"] == "github" + assert body["description"] == "" + + +def test_create_template_accepts_custom_os_and_forge(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response({"id": 3}) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.create_template( + name="win-tmpl", + data=b"data", + os_type="windows", + forge_type="gitea", + description="A windows template", + ) + body = json.loads(mock_open.call_args[0][0].data) + assert body["os_type"] == "windows" + assert body["forge_type"] == "gitea" + assert body["description"] == "A windows template" + + +def test_update_template_base64_encodes_data(): + raw = b"new content" + expected_b64 = base64.b64encode(raw).decode() + updated = {"id": 1, "data": expected_b64} + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response(updated) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.update_template(1, data=raw) + assert result == updated + req = mock_open.call_args[0][0] + assert req.get_method() == "PUT" + assert req.full_url.endswith("/templates/1") + body = json.loads(req.data) + assert body["data"] == expected_b64 + assert "name" not in body + assert "description" not in body + + +def test_update_template_includes_optional_fields_when_set(): + with patch("urllib.request.urlopen") as mock_open: + mock_open.return_value = _make_response({"id": 1}) + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + client.update_template(1, data=b"x", name="updated", description="new desc") + body = json.loads(mock_open.call_args[0][0].data) + assert body["name"] == "updated" + assert body["description"] == "new desc" + assert body["data"] == base64.b64encode(b"x").decode() + + +def test_delete_template_makes_delete_request(): + with patch("urllib.request.urlopen") as mock_open: + mock_resp = MagicMock() + mock_resp.__enter__ = lambda s: s + mock_resp.__exit__ = MagicMock(return_value=False) + mock_resp.read.return_value = b"" + mock_open.return_value = mock_resp + client = GarmClient("http://localhost:9997/api/v1") + client.token = "test-token" + result = client.delete_template(5) + assert result is None + req = mock_open.call_args[0][0] + assert req.get_method() == "DELETE" + assert req.full_url.endswith("/templates/5") diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py new file mode 100644 index 00000000..3e529c02 --- /dev/null +++ b/charms/garm/tests/unit/test_runner_template.py @@ -0,0 +1,111 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the runner-install template rendering.""" + +import pytest + +from runner_template import ( + RUNNER_ENV_PATH, + RunnerConfig, + build_template_data, +) + +SAMPLE_BASE = b"#!/bin/bash\nset -e\necho original-bootstrap\n" + + +def _full_config() -> RunnerConfig: + return RunnerConfig( + dockerhub_mirror="https://mirror.example.com", + runner_http_proxy="http://proxy.example.com:3128", + aproxy_exclude_addresses="10.0.0.0/8,192.168.1.5", + aproxy_redirect_ports="80,443,8000-9000", + otel_collector_endpoint="http://otel.example.com:4318", + pre_job_script="echo hello-from-operator", + ) + + +# A fully-populated config injects every option into the template while keeping +# the base bootstrap intact and placed after the shebang but before `set -e`. +def test_build_template_data_injects_all_options(): + result = build_template_data(SAMPLE_BASE, _full_config()).decode() + lines = result.splitlines() + + assert lines[0] == "#!/bin/bash" + assert "echo original-bootstrap" in result + # Injection sits between the shebang and the base body's `set -e`. + assert result.index("charm-injected") < result.index("set -e") + + assert "mirror.example.com" in result + assert "registry-mirrors" in result + assert "proxy.example.com:3128" in result + assert "snap install aproxy" in result + assert "10.0.0.0/8" in result and "192.168.1.5" in result + assert "8000-9000" in result + assert "OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318" in result + assert "echo hello-from-operator" in result + assert "ACTIONS_RUNNER_HOOK_JOB_STARTED=" in result + assert RUNNER_ENV_PATH in result + + +# An empty config still wires the job-start hook and static host prep, but emits +# none of the optional proxy/mirror/telemetry blocks. +def test_build_template_data_empty_config_omits_optional_blocks(): + result = build_template_data(SAMPLE_BASE, RunnerConfig()).decode() + + assert "echo original-bootstrap" in result + assert "ACTIONS_RUNNER_HOOK_JOB_STARTED=" in result + assert "usermod -aG lxd,adm ubuntu" in result + + assert "snap install aproxy" not in result + assert "registry-mirrors" not in result + assert "OTEL_EXPORTER_OTLP_ENDPOINT" not in result + + +@pytest.mark.parametrize( + "config, present, absent", + [ + pytest.param( + RunnerConfig(dockerhub_mirror="https://m.test"), + "https://m.test", + "snap install aproxy", + id="dockerhub-mirror-only", + ), + pytest.param( + RunnerConfig(runner_http_proxy="http://p.test:8080"), + "http://p.test:8080", + "registry-mirrors", + id="proxy-only", + ), + pytest.param( + RunnerConfig(otel_collector_endpoint="http://o.test:4318"), + "OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318", + "snap install aproxy", + id="otel-only", + ), + pytest.param( + RunnerConfig(pre_job_script="run-my-thing --flag"), + "run-my-thing --flag", + "registry-mirrors", + id="pre-job-script-only", + ), + ], +) +def test_build_template_data_per_option(config, present, absent): + result = build_template_data(SAMPLE_BASE, config).decode() + assert present in result + assert absent not in result + + +# from_databag maps each contract key into the config and ignores absent keys; +# has_config reflects whether any option is set. +def test_runner_config_from_databag_and_has_config(): + empty = RunnerConfig.from_databag({}) + assert empty == RunnerConfig() + assert not empty.has_config() + + populated = RunnerConfig.from_databag( + {"dockerhub_mirror": " https://m.test ", "irrelevant": "x"} + ) + assert populated.dockerhub_mirror == "https://m.test" + assert populated.has_config() diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 610fa8b3..ebfc68db 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -3,21 +3,33 @@ """Unit tests for the scaleset reconciler.""" +import base64 from unittest.mock import MagicMock +from runner_template import RunnerConfig from scaleset_reconciler import ScalesetReconciler, ScalesetSpec -def _mock_client(providers=None, credentials=None, scalesets=None): +def _b64(text: str) -> str: + return base64.b64encode(text.encode()).decode() + + +def _mock_client(providers=None, credentials=None, scalesets=None, templates=None): client = MagicMock() client.list_providers.return_value = providers or [] client.list_credentials.return_value = credentials or [] client.list_scalesets.return_value = scalesets or [] + client.list_templates.return_value = templates or [] client.create_scaleset.return_value = {"id": "new-uuid", "name": "test"} client.update_scaleset.return_value = {"id": "uuid-1", "name": "test"} + client.create_template.return_value = {"id": 42, "name": "github_linux-my-scaleset"} return client +# A minimal system base template GARM seeds; data is base64 like the real API. +_SYSTEM_TEMPLATE = {"id": 1, "name": "github_linux", "data": _b64("#!/bin/bash\nset -e\n")} + + def _spec( name="my-scaleset", provider_name="openstack-demo", @@ -30,6 +42,7 @@ def _spec( labels=None, runner_group="default", pre_install_scripts=None, + runner_config=None, ): return ScalesetSpec( name=name, @@ -43,6 +56,7 @@ def _spec( labels=labels or [], runner_group=runner_group, pre_install_scripts=pre_install_scripts or {}, + runner_config=runner_config or RunnerConfig(), ) @@ -218,3 +232,96 @@ def test_existing_scaleset_not_deleted_when_provider_temporarily_missing(): reconciler = ScalesetReconciler(client) reconciler.reconcile([_spec(labels=["a-tag", "z-tag"])]) client.update_scaleset.assert_not_called() + + +# A scaleset carrying runner options copies the system template, creates a +# per-scaleset template, and references it via template_id on create. +def test_create_builds_template_and_sets_template_id(): + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[], + templates=[_SYSTEM_TEMPLATE], + ) + reconciler = ScalesetReconciler(client) + spec = _spec(runner_config=RunnerConfig(dockerhub_mirror="https://m.test")) + reconciler.reconcile([spec]) + + client.create_template.assert_called_once() + created_kwargs = client.create_template.call_args.kwargs + assert created_kwargs["name"] == "github_linux-my-scaleset" + assert b"https://m.test" in created_kwargs["data"] + assert client.create_scaleset.call_args[0][0]["template_id"] == 42 + + +# Without runner options no template is built and the scaleset keeps the default +# template (no template_id in the payload). +def test_no_template_when_runner_config_empty(): + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[], + templates=[_SYSTEM_TEMPLATE], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) + + client.create_template.assert_not_called() + assert "template_id" not in client.create_scaleset.call_args[0][0] + + +# An option change refreshes the existing template in place (PUT) without needing +# a scaleset update, since template_id is unchanged. +def test_template_updated_on_content_drift_without_scaleset_update(): + existing_scaleset = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + "template_id": 7, + } + stale_template = {"id": 7, "name": "github_linux-my-scaleset", "data": _b64("#!/bin/bash\n")} + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing_scaleset], + templates=[_SYSTEM_TEMPLATE, stale_template], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec(runner_config=RunnerConfig(dockerhub_mirror="https://m.test"))]) + + client.update_template.assert_called_once() + assert client.update_template.call_args[0][0] == 7 + client.update_scaleset.assert_not_called() + + +# Deleting an orphaned scaleset also removes its per-scaleset runner template. +def test_orphan_scaleset_deletes_custom_template(): + stale_scaleset = { + "id": "uuid-stale", + "name": "stale-scaleset", + "image_id": "ubuntu-20.04", + "flavor": "m1.small", + "max_runners": 2, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + } + stale_template = {"id": 9, "name": "github_linux-stale-scaleset", "data": _b64("x")} + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[stale_scaleset], + templates=[_SYSTEM_TEMPLATE, stale_template], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([]) + + client.delete_scaleset.assert_called_once_with("uuid-stale") + client.delete_template.assert_called_once_with(9) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 0293507d..d7e4f605 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -6,7 +6,6 @@ import base64 import json import logging -import secrets import shlex import urllib.error import urllib.request @@ -33,10 +32,6 @@ GARM_API_PORT = 8080 PEBBLE_PREFIX = "PEBBLE_SOCKET=/charm/containers/app/pebble.socket /charm/bin/pebble" -# Generated once per session so all test functions that call _garm_first_run use -# the same credentials. Format guarantees GARM's strong-password requirements: -# uppercase (A), lowercase (dmin + hex), digit (1 + hex), symbols (-, !). -_GARM_ADMIN_PASSWORD = f"Admin-{secrets.token_hex(8)}-X1!" _SCALESET_TEST_NAME = "test-scaleset" _SCALESET_TEST_CREDENTIAL_NAME = "github-app-12345" @@ -86,29 +81,42 @@ def _get_garm_address(juju: jubilant.Juju, app_name: str) -> str: return status.apps[app_name].units[unit_name].address -def _garm_first_run(address: str) -> str: - """Complete GARM first-run initialization and return an admin JWT. +def _get_charm_admin_creds(juju: jubilant.Juju) -> tuple[str, str]: + """Return the (username, password) the charm stored in the garm-secrets secret.""" + all_secrets = json.loads(juju.cli("secrets", "--format=json")) + uri = next( + (u for u, info in all_secrets.items() if info.get("label") == GARM_SECRETS_LABEL), None + ) + assert uri, f"{GARM_SECRETS_LABEL} secret not found" + content = json.loads(juju.cli("show-secret", "--reveal", "--format=json", uri)) + data = content[uri]["content"]["Data"] + return data["admin-username"], data["admin-password"] + - Calls /api/v1/first-run to create the initial admin user, then logs in - to obtain a JWT. Also configures required controller URLs so GARM will - serve operational API endpoints. +def _garm_first_run(juju: jubilant.Juju, address: str) -> str: + """Initialize GARM with the charm-managed admin credentials and return a JWT. - Retries with backoff to allow GARM time to finish starting after replan. + Reads the admin username/password the charm generated (garm-secrets Juju + secret) and uses them for first-run + login, so GARM's admin matches what the + charm itself authenticates with during scaleset reconcile. Idempotent: a 409 + means the charm (or an earlier test) already initialized GARM with the same + credentials. Also configures the controller URLs so GARM serves operational + endpoints. Retries with backoff to allow GARM time to start after replan. Args: + juju: Jubilant Juju handle (used to read the admin credentials secret). address: GARM unit IP address. Returns: JWT token string for authenticated API calls. """ base_url = f"http://{address}:{GARM_API_PORT}/api/v1" - # GARM v0.2.x requires strong passwords (min 12 chars, mixed case, digits, symbols) - password = _GARM_ADMIN_PASSWORD + username, password = _get_charm_admin_creds(juju) first_run_payload = { - "username": "admin", + "username": username, "password": password, - "email": "admin@test.local", - "full_name": "Integration Test Admin", + "email": "admin@garm.local", + "full_name": "GARM Admin", } session = requests.Session() @@ -140,7 +148,7 @@ def _post_first_run() -> None: _post_first_run() # Login to obtain JWT - login_payload = {"username": "admin", "password": password} + login_payload = {"username": username, "password": password} resp = session.post(f"{base_url}/auth/login", json=login_payload, timeout=30) logger.info("login response: status=%d body=%s", resp.status_code, resp.text[:500]) resp.raise_for_status() @@ -236,6 +244,16 @@ def _list_providers(base_url: str, token: str) -> list[dict]: return providers +def _get_template_data(base_url: str, token: str, template_id: int) -> str: + """Fetch a GARM template and return its decoded (base64) script content.""" + resp = requests.get( + f"{base_url}/templates/{template_id}", headers=_garm_auth_headers(token), timeout=30 + ) + resp.raise_for_status() + data = resp.json().get("data", "") + return base64.b64decode(data).decode() if data else "" + + def _find_scaleset(scalesets: list[dict], name: str) -> dict | None: """Return the first scaleset with the requested name.""" return next((scaleset for scaleset in scalesets if scaleset.get("name") == name), None) @@ -538,7 +556,7 @@ def test_garm_api_controller_info( address = _get_garm_address(juju, garm_app) logger.info("GARM address: %s", address) - token = _garm_first_run(address) + token = _garm_first_run(juju, address) assert token, "Expected non-empty JWT token from first-run/login" logger.info("Got admin JWT token (length=%d)", len(token)) @@ -564,7 +582,7 @@ def test_garm_api_list_scalesets( scale set query path through postgresql is functional. """ address = _get_garm_address(juju, garm_app) - token = _garm_first_run(address) + token = _garm_first_run(juju, address) base_url = f"http://{address}:{GARM_API_PORT}/api/v1" headers = {"Authorization": f"Bearer {token}"} @@ -592,7 +610,7 @@ def test_garm_api_list_providers( assert: The openstack provider is registered and visible through the API. """ address = _get_garm_address(juju, garm_app) - token = _garm_first_run(address) + token = _garm_first_run(juju, address) base_url = f"http://{address}:{GARM_API_PORT}/api/v1" headers = {"Authorization": f"Bearer {token}"} @@ -847,3 +865,62 @@ def test_scaleset_deleted_when_relation_removed( ) _wait_for_scaleset_absent(base_url, token, _SCALESET_TEST_NAME) + + +def test_runner_options_render_into_scaleset_template( + juju: jubilant.Juju, + garm_app: str, + garm_configurator_for_scaleset_tests: str, +): + """ + arrange: GARM is active with the test configurator related and a credential created. + act: Set the runner-behaviour options on the relation, pointing the scaleset at the + built-in 'openstack' provider so a scaleset (and its custom runner template) is + created via the GARM REST API. + assert: The scaleset references a custom template whose rendered content reflects each + runner option — proving the options reach GARM via live reconcile (no upgrade). + """ + _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) + address = _get_garm_address(juju, garm_app) + base_url = _garm_api_base_url(address) + token = garm_login_from_secret(juju, garm_app, base_url) + _create_test_credential(base_url, token) + relation_data = _get_scaleset_relation_data( + juju, garm_app, garm_configurator_for_scaleset_tests + ) + provider_names = {provider.get("name", "") for provider in _list_providers(base_url, token)} + provider_name = relation_data["provider_name"] + if provider_name not in provider_names: + # The provider is registered from the configurator's OpenStack config + # (rendered into GARM's TOML); without that chain the reconciler cannot + # create the scaleset, so the template cannot be observed here. Same + # guard as the sibling scaleset-creation tests. + pytest.skip( + f"requires GARM provider {provider_name!r}; available providers: {sorted(provider_names)}" + ) + + _set_scaleset_relation_data( + juju, + garm_app, + garm_configurator_for_scaleset_tests, + min_idle_runner="1", + dockerhub_mirror="https://mirror.example.com", + runner_http_proxy="http://proxy.example.com:3128", + aproxy_redirect_ports="80,443", + otel_collector_endpoint="http://otel.example.com:4318", + pre_job_script="echo integration-marker", + ) + + scaleset = _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + template_id = scaleset.get("template_id") + assert template_id, f"Expected scaleset to reference a custom template, got: {scaleset}" + + rendered = _get_template_data(base_url, token, template_id) + for expected in ( + "registry-mirrors", + "https://mirror.example.com", + "http://proxy.example.com:3128", + "OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", + "echo integration-marker", + ): + assert expected in rendered, f"Expected {expected!r} in rendered template, got:\n{rendered}" From 01a33bc901a7b399077d5a33953b75d99ccfcb9e Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:25:38 +0000 Subject: [PATCH 12/21] fix(garm): revert scaleset to default template when runner options cleared Address Copilot review on #238: - the reconciler now detaches the per-scaleset template (template_id -> 0) and deletes the unreferenced custom template when runner options are cleared, so operators can revert to the default template by clearing the config - aproxy-exclude-addresses rejects non-IP/CIDR tokens (they would fail in the nft ruleset and the failure would be swallowed) instead of silently accepting - align the runner-http-proxy and otel-collector-endpoint config descriptions with their http(s) URL validation --- charms/garm-configurator/charmcraft.yaml | 4 +-- charms/garm-configurator/src/charm_state.py | 16 +++++---- .../tests/unit/test_charm.py | 6 ++++ charms/garm/src/scaleset_reconciler.py | 33 ++++++++++++------- .../tests/unit/test_scaleset_reconciler.py | 30 +++++++++++++++++ 5 files changed, 69 insertions(+), 20 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index ec87aa5c..37859953 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -114,7 +114,7 @@ config: runner-http-proxy: type: string description: | - HTTP proxy that aproxy forwards to. + HTTP proxy that aproxy forwards to. Must be a valid http(s) URL. aproxy-exclude-addresses: type: string description: | @@ -126,7 +126,7 @@ config: otel-collector-endpoint: type: string description: | - The OTEL exporter address for the otel-collector to connect to. Must be a valid address. + The OTEL exporter address for the otel-collector to connect to. Must be a valid http(s) URL. pre-job-script: type: string description: | diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index a1b91a4f..7b917d41 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -3,7 +3,6 @@ """State of the GARM configurator charm.""" -import contextlib import ipaddress import urllib.parse @@ -371,13 +370,16 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": tokens = [t.strip() for t in str(raw_exclude).split(",")] # Reject empty tokens (e.g. trailing comma) as they signal a misconfiguration. for token in tokens: - if not token: - raise CharmConfigInvalidError( - f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} contains an empty address token" - ) - with contextlib.suppress(ValueError): - # Non-IP tokens (hostnames) are accepted on a best-effort basis. + # Each token must be a valid IP address or CIDR: the values are + # rendered into an nft ruleset, where a hostname would fail at + # runtime (and the failure would be swallowed by ``|| true``). + try: ipaddress.ip_network(token, strict=False) + except ValueError as exc: + raise CharmConfigInvalidError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} must be a comma-separated list " + f"of IP addresses or CIDRs; got invalid token: {token!r}" + ) from exc aproxy_exclude_addresses = ",".join(tokens) raw_ports = charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME) diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index babb1edb..7ab3a7eb 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -583,6 +583,12 @@ def test_garm_configurator_relation_changed_triggers_reconcile(): "aproxy-redirect-ports must be a comma-separated list of ports or N-M ranges in 1..65535", id="aproxy-redirect-ports-inverted-range", ), + pytest.param( + "aproxy-exclude-addresses", + "10.0.0.1,not-an-ip", + "aproxy-exclude-addresses must be a comma-separated list of IP addresses or CIDRs", + id="aproxy-exclude-addresses-non-ip", + ), ], ) def test_charm_blocked_invalid_runner_config( diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 415266ee..ae987a1f 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -88,13 +88,19 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: else: self._create(spec, template_id) + if not template_id: + # Runner options were cleared (or the system template is + # unavailable): the scaleset has been reverted to the default + # template above, so drop any now-unreferenced custom template. + self._delete_custom_template(spec.name, templates) + for name, scaleset in observed.items(): if name not in all_desired_names: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset["id"]) self._client.delete_scaleset(scaleset["id"]) self._delete_custom_template(name, templates) - def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> int | None: + def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> int: """Ensure the scaleset's runner template reflects its runner options. Copies the system ``github_linux`` template, injects the runner options, @@ -107,12 +113,13 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> in templates: Observed templates keyed by name. Returns: - The template id to reference from the scaleset, or None to keep the - default template (no runner options set, or the system template is - unavailable). + The custom template id to reference from the scaleset, or ``0`` to use + GARM's default template (no runner options set, or the system template + is unavailable). Returning ``0`` for a scaleset that previously had a + custom template detaches it (reverts to the default). """ if not spec.runner_config.has_config(): - return None + return 0 base = templates.get(SYSTEM_TEMPLATE_NAME) if base is None: @@ -121,7 +128,7 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> in SYSTEM_TEMPLATE_NAME, spec.name, ) - return None + return 0 new_data = build_template_data(self._template_bytes(base), spec.runner_config) custom_name = f"{SYSTEM_TEMPLATE_NAME}-{spec.name}" @@ -166,7 +173,7 @@ def _delete_custom_template(self, scaleset_name: str, templates: dict[str, dict] logger.info("Deleting orphaned runner template %s", custom["name"]) self._client.delete_template(custom["id"]) - def _create(self, spec: ScalesetSpec, template_id: int | None) -> None: + def _create(self, spec: ScalesetSpec, template_id: int) -> None: extra_specs: dict[str, object] = {} if spec.pre_install_scripts: extra_specs["pre_install_scripts"] = spec.pre_install_scripts @@ -184,13 +191,14 @@ def _create(self, spec: ScalesetSpec, template_id: int | None) -> None: "runner_group": spec.runner_group, "extra_specs": extra_specs, } - if template_id is not None: + if template_id: payload["template_id"] = template_id logger.info("Creating scaleset %s", spec.name) self._client.create_scaleset(payload) - def _maybe_update(self, observed: dict, spec: ScalesetSpec, template_id: int | None) -> None: - template_changed = template_id is not None and observed.get("template_id") != template_id + def _maybe_update(self, observed: dict, spec: ScalesetSpec, template_id: int) -> None: + observed_template_id = observed.get("template_id") or 0 + template_changed = observed_template_id != template_id if not self._needs_update(observed, spec) and not template_changed: logger.debug("Scaleset %s is up to date", spec.name) return @@ -208,7 +216,10 @@ def _maybe_update(self, observed: dict, spec: ScalesetSpec, template_id: int | N "runner_group": spec.runner_group, "extra_specs": extra_specs, } - if template_id is not None: + # Send template_id when the scaleset has, or had, a custom template — a 0 + # value detaches it (reverts to the default); omit it otherwise so an + # unrelated update never spuriously sets the field. + if template_id or observed_template_id: payload["template_id"] = template_id logger.info("Updating scaleset %s (id=%s)", spec.name, observed["id"]) self._client.update_scaleset(observed["id"], payload) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index ebfc68db..35707b8e 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -325,3 +325,33 @@ def test_orphan_scaleset_deletes_custom_template(): client.delete_scaleset.assert_called_once_with("uuid-stale") client.delete_template.assert_called_once_with(9) + + +# Clearing the runner options reverts the scaleset to GARM's default template +# (template_id -> 0) and removes the now-unreferenced custom template. +def test_template_detached_when_runner_config_cleared(): + existing_scaleset = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + "template_id": 7, + } + custom_template = {"id": 7, "name": "github_linux-my-scaleset", "data": _b64("#!/bin/bash\n")} + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing_scaleset], + templates=[_SYSTEM_TEMPLATE, custom_template], + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec()]) # default spec has no runner options + + client.update_scaleset.assert_called_once() + assert client.update_scaleset.call_args[0][1]["template_id"] == 0 + client.delete_template.assert_called_once_with(7) From 911be8331f0afb97845daeb9919b880faac6666f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:33:49 +0000 Subject: [PATCH 13/21] fix(garm): keep existing custom template when system template is missing Address Copilot review on #238: when runner options are set but the system github_linux template is transiently absent from list_templates, _ensure_template returned 0, which detached and deleted an existing per-scaleset template and lost the runner config. Now keep an existing custom template untouched in that case and only fall back to the default template when there is none to keep. --- charms/garm/src/scaleset_reconciler.py | 20 ++++++++++--- .../tests/unit/test_scaleset_reconciler.py | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index ae987a1f..f7f75ccf 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -115,14 +115,28 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> in Returns: The custom template id to reference from the scaleset, or ``0`` to use GARM's default template (no runner options set, or the system template - is unavailable). Returning ``0`` for a scaleset that previously had a - custom template detaches it (reverts to the default). + is unavailable and no custom template already exists). Returning ``0`` + for a scaleset that previously had a custom template detaches it. """ + custom_name = f"{SYSTEM_TEMPLATE_NAME}-{spec.name}" + existing = templates.get(custom_name) + if not spec.runner_config.has_config(): return 0 base = templates.get(SYSTEM_TEMPLATE_NAME) if base is None: + # The system template is not listed (transient/compat). Don't destroy + # an existing custom template over it — keep the last-rendered one + # rather than detaching and losing the runner config; only fall back + # to the default when there is nothing to keep. + if existing is not None: + logger.warning( + "System template %s not found; keeping existing custom template for %s", + SYSTEM_TEMPLATE_NAME, + spec.name, + ) + return existing["id"] logger.warning( "System template %s not found; scaleset %s will use the default template", SYSTEM_TEMPLATE_NAME, @@ -131,8 +145,6 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, dict]) -> in return 0 new_data = build_template_data(self._template_bytes(base), spec.runner_config) - custom_name = f"{SYSTEM_TEMPLATE_NAME}-{spec.name}" - existing = templates.get(custom_name) if existing is not None: if self._template_bytes(existing) != new_data: logger.info("Updating runner template %s", custom_name) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 35707b8e..25a8989d 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -355,3 +355,33 @@ def test_template_detached_when_runner_config_cleared(): client.update_scaleset.assert_called_once() assert client.update_scaleset.call_args[0][1]["template_id"] == 0 client.delete_template.assert_called_once_with(7) + + +# A transiently missing system template must not destroy an existing custom +# template: it is kept (no detach, no delete, no rebuild). +def test_existing_custom_template_kept_when_system_template_missing(): + existing_scaleset = { + "id": "uuid-1", + "name": "my-scaleset", + "image_id": "ubuntu-22.04", + "flavor": "m1.small", + "max_runners": 5, + "min_idle_runners": 0, + "tags": [], + "runner_group": "default", + "extra_specs": {}, + "template_id": 7, + } + custom_template = {"id": 7, "name": "github_linux-my-scaleset", "data": _b64("#!/bin/bash\n")} + client = _mock_client( + providers=[{"name": "openstack-demo"}], + credentials=[{"name": "github-app-12345"}], + scalesets=[existing_scaleset], + templates=[custom_template], # system github_linux template absent + ) + reconciler = ScalesetReconciler(client) + reconciler.reconcile([_spec(runner_config=RunnerConfig(dockerhub_mirror="https://m.test"))]) + + client.delete_template.assert_not_called() + client.update_template.assert_not_called() + client.update_scaleset.assert_not_called() From baa9e71ddabd366c9fa1eeabdcf42db19f56315a Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:40:19 +0000 Subject: [PATCH 14/21] fix(garm): cache template data and restrict aproxy excludes to IPv4 Address Copilot review on #238: - _template_bytes now caches data fetched via get_template back onto the template dict, avoiding redundant GETs when the system template is reused across specs - aproxy-exclude-addresses now rejects IPv6 (and hostnames): the rendered nft ruleset uses an IPv4-only `table ip`, so IPv6 would pass validation but fail at runtime; the config description is updated to say IPv4 --- charms/garm-configurator/charmcraft.yaml | 2 +- charms/garm-configurator/src/charm_state.py | 16 +++++++++++----- .../garm-configurator/tests/unit/test_charm.py | 8 +++++++- charms/garm/src/scaleset_reconciler.py | 3 +++ .../garm/tests/unit/test_scaleset_reconciler.py | 14 ++++++++++++++ 5 files changed, 36 insertions(+), 7 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 37859953..2c8c9ebb 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -118,7 +118,7 @@ config: aproxy-exclude-addresses: type: string description: | - Comma-separated IP addresses/CIDRs to exclude from aproxy forwarding. + Comma-separated IPv4 addresses/CIDRs to exclude from aproxy forwarding. aproxy-redirect-ports: type: string description: | diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 7b917d41..216344ff 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -370,16 +370,22 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": tokens = [t.strip() for t in str(raw_exclude).split(",")] # Reject empty tokens (e.g. trailing comma) as they signal a misconfiguration. for token in tokens: - # Each token must be a valid IP address or CIDR: the values are - # rendered into an nft ruleset, where a hostname would fail at - # runtime (and the failure would be swallowed by ``|| true``). + # Each token must be a valid IPv4 address or CIDR: the values are + # rendered into an nft IPv4 (`table ip`) ruleset, so a hostname or + # IPv6 address would pass validation but fail at runtime (and the + # failure would be swallowed by ``|| true``). try: - ipaddress.ip_network(token, strict=False) + network = ipaddress.ip_network(token, strict=False) except ValueError as exc: raise CharmConfigInvalidError( f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} must be a comma-separated list " - f"of IP addresses or CIDRs; got invalid token: {token!r}" + f"of IPv4 addresses or CIDRs; got invalid token: {token!r}" ) from exc + if network.version != 4: + raise CharmConfigInvalidError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} only supports IPv4 addresses or " + f"CIDRs (the aproxy nft ruleset is IPv4-only); got: {token!r}" + ) aproxy_exclude_addresses = ",".join(tokens) raw_ports = charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME) diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 7ab3a7eb..fd92e9da 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -586,9 +586,15 @@ def test_garm_configurator_relation_changed_triggers_reconcile(): pytest.param( "aproxy-exclude-addresses", "10.0.0.1,not-an-ip", - "aproxy-exclude-addresses must be a comma-separated list of IP addresses or CIDRs", + "aproxy-exclude-addresses must be a comma-separated list of IPv4 addresses or CIDRs", id="aproxy-exclude-addresses-non-ip", ), + pytest.param( + "aproxy-exclude-addresses", + "2001:db8::1", + "aproxy-exclude-addresses only supports IPv4", + id="aproxy-exclude-addresses-ipv6", + ), ], ) def test_charm_blocked_invalid_runner_config( diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index f7f75ccf..f920236d 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -171,6 +171,9 @@ def _template_bytes(self, template: dict) -> bytes: data = template.get("data") if not data: data = self._client.get_template(template["id"]).get("data", "") + # Cache it back so repeated lookups (e.g. the system template reused + # across specs in one reconcile) don't re-fetch it. + template["data"] = data return base64.b64decode(data) if data else b"" def _delete_custom_template(self, scaleset_name: str, templates: dict[str, dict]) -> None: diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 25a8989d..0598bee5 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -385,3 +385,17 @@ def test_existing_custom_template_kept_when_system_template_missing(): client.delete_template.assert_not_called() client.update_template.assert_not_called() client.update_scaleset.assert_not_called() + + +# A template returned without its data is fetched once and cached back. +def test_template_bytes_caches_fetched_data(): + client = MagicMock() + client.get_template.return_value = {"data": _b64("hello")} + reconciler = ScalesetReconciler(client) + template = {"id": 5} # no 'data' field + + assert reconciler._template_bytes(template) == b"hello" + assert reconciler._template_bytes(template) == b"hello" + + client.get_template.assert_called_once_with(5) + assert template["data"] == _b64("hello") From 0725d51052ddc9f897a924d944e09469b8c98c1f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:48:11 +0000 Subject: [PATCH 15/21] fix(garm): login before first-run and filter template listing Address Copilot review on #238: - _maybe_first_run now logs in first and only calls /first-run when login fails, avoiding repeated first-run calls (and the 409s they log) on every update-status tick while still self-healing if GARM's database is reset - the scaleset reconciler lists templates with the github_linux partial-name filter instead of pulling every template on the controller --- charms/garm/src/charm.py | 18 ++++++++++++------ charms/garm/src/scaleset_reconciler.py | 8 +++++++- charms/garm/tests/unit/test_charm.py | 25 +++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 93d0eaa6..82d3d6b1 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -251,11 +251,12 @@ def _maybe_first_run(self) -> None: """Register the GARM admin user if it does not exist yet (leader only). GARM needs an initial admin (POST /first-run) before its API accepts - authenticated calls. GarmClient.first_run is idempotent — a 409 - "already initialised" is ignored — so this is safe to call on every - event; a failure (e.g. GARM not yet accepting connections right after a - restart) is logged and retried on the next event (update-status, relation - change, or scaleset reconcile). + authenticated calls. We log in first: if that succeeds GARM is already + initialised and we do nothing, which avoids calling /first-run (and the + 409s it logs) on every update-status tick. Only when login fails do we + attempt first-run, so the call also self-heals if GARM's database is ever + reset. Failures (e.g. GARM not yet accepting connections after a restart) + are logged and retried on the next event. """ if not self.unit.is_leader(): return @@ -267,8 +268,13 @@ def _maybe_first_run(self) -> None: admin_password = content.get("admin-password") if not admin_username or not admin_password: return + client = GarmClient(f"{self._get_garm_url()}/api/v1") + try: + client.login(admin_username, admin_password) + return + except GarmApiError: + logger.info("GARM admin login failed; attempting first-run initialisation") try: - client = GarmClient(f"{self._get_garm_url()}/api/v1") client.first_run( admin_username, admin_password, GARM_ADMIN_EMAIL, GARM_ADMIN_FULL_NAME ) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index f920236d..24a9abab 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -61,7 +61,13 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: providers = {provider["name"] for provider in self._client.list_providers()} credentials = {credential["name"] for credential in self._client.list_credentials()} observed = {scaleset["name"]: scaleset for scaleset in self._client.list_scalesets()} - templates = {template["name"]: template for template in self._client.list_templates()} + # Only the system github_linux template and our per-scaleset + # github_linux- copies are relevant, so filter by partial name + # rather than pulling every template on the controller. + templates = { + template["name"]: template + for template in self._client.list_templates(partial_name=SYSTEM_TEMPLATE_NAME) + } all_desired_names: set[str] = {spec.name for spec in desired} diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 33ea0385..c072754f 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -14,6 +14,7 @@ import tomli as tomllib # type: ignore[no-redef] from charm import GARM_SECRETS_LABEL, GarmCharm, _generate_garm_secrets, render_garm_toml +from garm_api import GarmApiError _DEFAULT_PG_CONFIG = { "username": "u", @@ -287,6 +288,8 @@ def test_maybe_first_run_registers_admin_from_secret(): patch("charm.GarmClient") as mock_client_cls, ): mock_unit.return_value.is_leader.return_value = True + # Login fails => GARM is not initialised yet => first-run is attempted. + mock_client_cls.return_value.login.side_effect = GarmApiError("not initialised") charm._maybe_first_run() mock_client_cls.return_value.first_run.assert_called_once_with( @@ -294,6 +297,28 @@ def test_maybe_first_run_registers_admin_from_secret(): ) +def test_maybe_first_run_skips_first_run_when_login_succeeds(): + """When the admin can already log in, GARM is initialised and first-run is skipped.""" + charm = object.__new__(GarmCharm) + secret = MagicMock() + secret.get_content.return_value = { + "admin-username": "admin", + "admin-password": "Admin-deadbeefcafebabe-Gx1!", + } + charm._get_garm_secrets = MagicMock(return_value=secret) + charm._get_garm_url = MagicMock(return_value="http://127.0.0.1:8080") + + with ( + patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, + patch("charm.GarmClient") as mock_client_cls, + ): + mock_unit.return_value.is_leader.return_value = True + mock_client_cls.return_value.login.return_value = "token" + charm._maybe_first_run() + + mock_client_cls.return_value.first_run.assert_not_called() + + def test_maybe_first_run_skips_when_not_leader(): """Non-leader units do not attempt GARM first-run.""" charm = object.__new__(GarmCharm) From aaf74b3c8513aa0367b3f898bf36f82d47f4194f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 14:55:05 +0000 Subject: [PATCH 16/21] fix(garm-configurator): reject aproxy options without a proxy; dedupe creds helper Address Copilot review on #238: - block config that sets aproxy-exclude-addresses / aproxy-redirect-ports without runner-http-proxy, since the runner template only renders the aproxy block when a proxy is set (the options would otherwise silently no-op) - centralise the GARM admin-credentials secret lookup in a conftest helper (get_garm_admin_creds_from_secret) reused by garm_login_from_secret and the first-run integration helper --- charms/garm-configurator/src/charm_state.py | 11 ++++++++++ .../tests/unit/test_charm.py | 17 ++++++++++++++ charms/tests/integration/conftest.py | 22 ++++++++++--------- charms/tests/integration/test_garm.py | 19 +++++----------- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 216344ff..47663e7c 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -396,6 +396,17 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": _validate_port_token(token) aproxy_redirect_ports = ",".join(tokens_ports) + # The aproxy options only take effect alongside a proxy: the runner + # template renders the aproxy block solely when runner-http-proxy is set, + # so reject these options on their own rather than letting them no-op. + if (aproxy_exclude_addresses or aproxy_redirect_ports) and not url_values[ + RUNNER_HTTP_PROXY_CONFIG_NAME + ]: + raise CharmConfigInvalidError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} and {APROXY_REDIRECT_PORTS_CONFIG_NAME} " + f"require {RUNNER_HTTP_PROXY_CONFIG_NAME} to be set" + ) + raw_script = charm.config.get(PRE_JOB_SCRIPT_CONFIG_NAME) pre_job_script = str(raw_script).strip() if raw_script else None diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index fd92e9da..23c96794 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -616,6 +616,23 @@ def test_charm_blocked_invalid_runner_config( assert expected_fragment in out.unit_status.message +def test_runner_config_aproxy_options_require_proxy(): + """ + arrange: aproxy options set without runner-http-proxy. + act: Run config-changed. + assert: Unit is Blocked — the aproxy options would otherwise silently no-op. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["aproxy-redirect-ports"] = "80,443" + state = State(config=config, secrets=[secret, pk_secret]) + out = ctx.run(ctx.on.config_changed(), state) + assert isinstance(out.unit_status, ops.BlockedStatus) + assert "require runner-http-proxy to be set" in out.unit_status.message + + def test_runner_config_fields_written_to_garm_configurator_relation(): """ arrange: Valid config with all six runner config options set. diff --git a/charms/tests/integration/conftest.py b/charms/tests/integration/conftest.py index db95d44a..e474718b 100644 --- a/charms/tests/integration/conftest.py +++ b/charms/tests/integration/conftest.py @@ -351,21 +351,23 @@ def garm_configurator_charm_file_fixture(pytestconfig: pytest.Config) -> str: return configurator_charms[0] -def garm_login_from_secret(juju: jubilant.Juju, garm_app_name: str, garm_url: str) -> str: - """Log into the GARM API using admin credentials stored in Juju secrets.""" - secrets_json = juju.cli("secrets", "--format=json") - secrets = json.loads(secrets_json) +def get_garm_admin_creds_from_secret(juju: jubilant.Juju) -> tuple[str, str]: + """Return the (admin-username, admin-password) the GARM charm stored in garm-secrets.""" + secrets = json.loads(juju.cli("secrets", "--format=json")) garm_secret_uri = next( (uri for uri, info in secrets.items() if info.get("label") == GARM_SECRETS_LABEL), None, ) - assert garm_secret_uri, f"{GARM_SECRETS_LABEL} not found for {garm_app_name}" + assert garm_secret_uri, f"{GARM_SECRETS_LABEL} secret not found" + content = json.loads(juju.cli("show-secret", "--reveal", "--format=json", garm_secret_uri))[ + garm_secret_uri + ]["content"]["Data"] + return content["admin-username"], content["admin-password"] + - secret_json = juju.cli("show-secret", "--reveal", "--format=json", garm_secret_uri) - secret_data = json.loads(secret_json) - content = secret_data[garm_secret_uri]["content"]["Data"] - admin_username = content["admin-username"] - admin_password = content["admin-password"] +def garm_login_from_secret(juju: jubilant.Juju, garm_app_name: str, garm_url: str) -> str: + """Log into the GARM API using admin credentials stored in Juju secrets.""" + admin_username, admin_password = get_garm_admin_creds_from_secret(juju) base_url = garm_url.rstrip("/") if not base_url.endswith("/api/v1"): diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index d7e4f605..fa401139 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -13,7 +13,10 @@ import jubilant import pytest import requests -from charms.tests.integration.conftest import garm_login_from_secret +from charms.tests.integration.conftest import ( + garm_login_from_secret, + get_garm_admin_creds_from_secret, +) from requests.adapters import HTTPAdapter from tenacity import ( retry, @@ -81,18 +84,6 @@ def _get_garm_address(juju: jubilant.Juju, app_name: str) -> str: return status.apps[app_name].units[unit_name].address -def _get_charm_admin_creds(juju: jubilant.Juju) -> tuple[str, str]: - """Return the (username, password) the charm stored in the garm-secrets secret.""" - all_secrets = json.loads(juju.cli("secrets", "--format=json")) - uri = next( - (u for u, info in all_secrets.items() if info.get("label") == GARM_SECRETS_LABEL), None - ) - assert uri, f"{GARM_SECRETS_LABEL} secret not found" - content = json.loads(juju.cli("show-secret", "--reveal", "--format=json", uri)) - data = content[uri]["content"]["Data"] - return data["admin-username"], data["admin-password"] - - def _garm_first_run(juju: jubilant.Juju, address: str) -> str: """Initialize GARM with the charm-managed admin credentials and return a JWT. @@ -111,7 +102,7 @@ def _garm_first_run(juju: jubilant.Juju, address: str) -> str: JWT token string for authenticated API calls. """ base_url = f"http://{address}:{GARM_API_PORT}/api/v1" - username, password = _get_charm_admin_creds(juju) + username, password = get_garm_admin_creds_from_secret(juju) first_run_payload = { "username": username, "password": password, From b521d0f72abd333df129c1a28920041d494a3cb4 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 15:05:21 +0000 Subject: [PATCH 17/21] fix(garm): defensively sanitise aproxy values before rendering nft rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #238: the runner template renders relation databag values (aproxy redirect ports / exclude addresses) into a root-executed nft ruleset, so re-validate them at this trust boundary instead of relying on the configurator's validation alone — malformed and IPv6 tokens are dropped. Also document the unresolved ubuntu-vs-runner target for the static group-membership step under the existing ISD-278 TODO. --- charms/garm/src/runner_template.py | 58 ++++++++++++++++--- .../garm/tests/unit/test_runner_template.py | 17 ++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index f7f5e156..77be0274 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -22,11 +22,15 @@ is true. """ +import ipaddress import json +import re import shlex from collections.abc import Mapping from dataclasses import dataclass +_PORT_TOKEN_RE = re.compile(r"^\d{1,5}(-\d{1,5})?$") + # GARM hardcodes the runner username and actions-runner directory; the env file # read by the runner service therefore lives at the path below. RUNNER_USER = "runner" @@ -198,14 +202,15 @@ def _render_aproxy(config: RunnerConfig) -> str: Returns: A bash snippet configuring aproxy as a transparent forward proxy. """ - redirect_ports = config.aproxy_redirect_ports or "80,443" - nft_ports = ", ".join(part.strip() for part in redirect_ports.split(",") if part.strip()) + # Defensively re-validate the relation-provided values before rendering them + # into a root-executed nft ruleset: the databag is a trust boundary, so never + # rely on the configurator's validation alone — drop anything malformed. + ports = _valid_port_tokens(config.aproxy_redirect_ports) or ["80", "443"] + nft_ports = ", ".join(ports) exclude_guard = "" - if config.aproxy_exclude_addresses: - excludes = ", ".join( - part.strip() for part in config.aproxy_exclude_addresses.split(",") if part.strip() - ) - exclude_guard = f"ip daddr != {{ {excludes} }} " + excludes = _valid_ipv4_tokens(config.aproxy_exclude_addresses) + if excludes: + exclude_guard = f"ip daddr != {{ {', '.join(excludes)} }} " # TODO(ISD-278): confirm the exact aproxy listen port / nft ruleset against # the production github-runner charm cloud-init. @@ -257,10 +262,47 @@ def _render_static_host_prep() -> str: """ # TODO(ISD-278): port the remaining production cloud-init steps from # canonical/github-runner-operator: apt mirror sync self-test, tmate proxy - # setup, and the post-job metrics collector. + # setup, and the post-job metrics collector. Also confirm the target account + # for these group memberships: ISD278 specifies "ubuntu", but GARM runs the + # runner as RUNNER_USER ("runner") — reconcile against the old charm's + # cloud-init. The command is guarded by "|| true", so it is a no-op if the + # user is absent. return "\n".join( [ "# Static runner host preparation (ported from the github-runner charm).", "usermod -aG lxd,adm ubuntu >/dev/null 2>&1 || true", ] ) + + +def _valid_port_tokens(spec: str) -> list[str]: + """Return only the well-formed port / N-M range tokens from a comma list. + + Args: + spec: A comma-separated ports string (possibly empty or untrusted). + + Returns: + The subset of tokens matching ``N`` or ``N-M`` (digits only). + """ + return [token.strip() for token in spec.split(",") if _PORT_TOKEN_RE.match(token.strip())] + + +def _valid_ipv4_tokens(spec: str) -> list[str]: + """Return only the valid IPv4 address/CIDR tokens from a comma list. + + Args: + spec: A comma-separated address string (possibly empty or untrusted). + + Returns: + The subset of tokens that parse as IPv4 networks. + """ + valid: list[str] = [] + for token in spec.split(","): + token = token.strip() + try: + network = ipaddress.ip_network(token, strict=False) + except ValueError: + continue + if network.version == 4: + valid.append(token) + return valid diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index 3e529c02..b9aa549d 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -97,6 +97,23 @@ def test_build_template_data_per_option(config, present, absent): assert absent not in result +# The consumer side defensively drops malformed/IPv6 tokens (the databag is a +# trust boundary; the values are rendered into a root-executed nft ruleset). +def test_aproxy_render_drops_malformed_tokens(): + config = RunnerConfig( + runner_http_proxy="http://p.test:3128", + aproxy_redirect_ports="80,not-a-port,8000-9000,99 rm", + aproxy_exclude_addresses="10.0.0.0/8,evil;,2001:db8::1", + ) + result = build_template_data(SAMPLE_BASE, config).decode() + + assert "{ 80, 8000-9000 }" in result + assert "not-a-port" not in result + assert "10.0.0.0/8" in result + assert "evil" not in result + assert "2001:db8::1" not in result # IPv6 dropped: the nft table is IPv4-only + + # from_databag maps each contract key into the config and ignores absent keys; # has_config reflects whether any option is set. def test_runner_config_from_databag_and_has_config(): From 84b4a662cebc03d35207f0dcabf32430f57e795c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 15:11:19 +0000 Subject: [PATCH 18/21] fix(garm): drop out-of-range and inverted aproxy ports Address Copilot review on #238: the consumer-side port sanitisation checked only token shape, so out-of-range (0, 99999) or inverted (443-80) ports could still reach the nft ruleset and break it. Now also enforce 1..65535 and N<=M. --- charms/garm/src/runner_template.py | 18 +++++++++++++++--- charms/garm/tests/unit/test_runner_template.py | 4 +++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 77be0274..7598eb26 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -276,15 +276,27 @@ def _render_static_host_prep() -> str: def _valid_port_tokens(spec: str) -> list[str]: - """Return only the well-formed port / N-M range tokens from a comma list. + """Return only the in-range port / N-M range tokens from a comma list. + + A token is kept only if it is ``N`` or ``N-M`` with every port in 1..65535 + and ``N <= M`` — out-of-range or inverted tokens are dropped so they can't + break the nft ruleset. Args: spec: A comma-separated ports string (possibly empty or untrusted). Returns: - The subset of tokens matching ``N`` or ``N-M`` (digits only). + The subset of well-formed, in-range tokens. """ - return [token.strip() for token in spec.split(",") if _PORT_TOKEN_RE.match(token.strip())] + valid: list[str] = [] + for raw in spec.split(","): + token = raw.strip() + if not _PORT_TOKEN_RE.match(token): + continue + ports = [int(part) for part in token.split("-")] + if all(1 <= port <= 65535 for port in ports) and ports == sorted(ports): + valid.append(token) + return valid def _valid_ipv4_tokens(spec: str) -> list[str]: diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index b9aa549d..eef44127 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -102,13 +102,15 @@ def test_build_template_data_per_option(config, present, absent): def test_aproxy_render_drops_malformed_tokens(): config = RunnerConfig( runner_http_proxy="http://p.test:3128", - aproxy_redirect_ports="80,not-a-port,8000-9000,99 rm", + aproxy_redirect_ports="80,not-a-port,8000-9000,99 rm,99999,443-80", aproxy_exclude_addresses="10.0.0.0/8,evil;,2001:db8::1", ) result = build_template_data(SAMPLE_BASE, config).decode() assert "{ 80, 8000-9000 }" in result assert "not-a-port" not in result + assert "99999" not in result # out of range + assert "443-80" not in result # inverted range assert "10.0.0.0/8" in result assert "evil" not in result assert "2001:db8::1" not in result # IPv6 dropped: the nft table is IPv4-only From 7429e8a62ea372edce24194fad5938f01202dac8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 15:21:23 +0000 Subject: [PATCH 19/21] fix(garm): reject whitespace URLs, sanitise otel env, drop unused param Address Copilot review on #238: - _validate_http_url rejects URLs containing whitespace/control characters, which are later rendered into scripts and env files - the otel endpoint has CR/LF stripped before being written to the runner env file, so a databag value can't inject extra env entries - remove the now-unused garm_app_name parameter from garm_login_from_secret and update its call sites --- charms/garm-configurator/src/charm_state.py | 7 ++++++- charms/garm-configurator/tests/unit/test_charm.py | 6 ++++++ charms/garm/src/runner_template.py | 5 ++++- charms/garm/tests/unit/test_runner_template.py | 9 +++++++++ charms/tests/integration/conftest.py | 2 +- charms/tests/integration/test_garm.py | 10 +++++----- 6 files changed, 31 insertions(+), 8 deletions(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 47663e7c..62d150c8 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -310,8 +310,13 @@ def _validate_http_url(config_name: str, value: str) -> None: value: URL string to validate. Raises: - CharmConfigInvalidError: When the URL scheme is not http/https or netloc is empty. + CharmConfigInvalidError: When the URL has whitespace, a non-http(s) scheme, + or an empty netloc. """ + # Reject embedded whitespace/control characters: the value is later rendered + # into scripts and env files, where a newline could inject extra lines. + if any(char.isspace() for char in value): + raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") parsed = urllib.parse.urlparse(value) if parsed.scheme not in ("http", "https") or not parsed.netloc: raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 23c96794..9c0ef0ad 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -595,6 +595,12 @@ def test_garm_configurator_relation_changed_triggers_reconcile(): "aproxy-exclude-addresses only supports IPv4", id="aproxy-exclude-addresses-ipv6", ), + pytest.param( + "runner-http-proxy", + "http://pro xy.example.com", + "runner-http-proxy must be a valid http(s) URL", + id="runner-http-proxy-embedded-whitespace", + ), ], ) def test_charm_blocked_invalid_runner_config( diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 7598eb26..5e592d50 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -155,7 +155,10 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: env_entries = [f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}"] if config.otel_collector_endpoint: - env_entries.append(f"OTEL_EXPORTER_OTLP_ENDPOINT={config.otel_collector_endpoint}") + # Strip CR/LF so a databag value can't inject extra env entries (the + # databag is a trust boundary, regardless of configurator validation). + otel_endpoint = config.otel_collector_endpoint.replace("\r", "").replace("\n", "") + env_entries.append(f"OTEL_EXPORTER_OTLP_ENDPOINT={otel_endpoint}") return "\n".join( [ diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index eef44127..14463705 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -116,6 +116,15 @@ def test_aproxy_render_drops_malformed_tokens(): assert "2001:db8::1" not in result # IPv6 dropped: the nft table is IPv4-only +# A CR/LF in the otel endpoint must not inject extra lines into the env file. +def test_otel_endpoint_newline_stripped(): + config = RunnerConfig(otel_collector_endpoint="http://o.test:4318\nMALICIOUS=1") + result = build_template_data(SAMPLE_BASE, config).decode() + + assert "OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318MALICIOUS=1" in result + assert "\nMALICIOUS=1" not in result + + # from_databag maps each contract key into the config and ignores absent keys; # has_config reflects whether any option is set. def test_runner_config_from_databag_and_has_config(): diff --git a/charms/tests/integration/conftest.py b/charms/tests/integration/conftest.py index e474718b..fba79706 100644 --- a/charms/tests/integration/conftest.py +++ b/charms/tests/integration/conftest.py @@ -365,7 +365,7 @@ def get_garm_admin_creds_from_secret(juju: jubilant.Juju) -> tuple[str, str]: return content["admin-username"], content["admin-password"] -def garm_login_from_secret(juju: jubilant.Juju, garm_app_name: str, garm_url: str) -> str: +def garm_login_from_secret(juju: jubilant.Juju, garm_url: str) -> str: """Log into the GARM API using admin credentials stored in Juju secrets.""" admin_username, admin_password = get_garm_admin_creds_from_secret(juju) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index fa401139..e1c46953 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -715,7 +715,7 @@ def test_scaleset_creation_deferred_when_provider_missing( _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) address = _get_garm_address(juju, garm_app) base_url = _garm_api_base_url(address) - token = garm_login_from_secret(juju, garm_app, base_url) + token = garm_login_from_secret(juju, base_url) _create_test_credential(base_url, token) _set_scaleset_relation_data( juju, @@ -744,7 +744,7 @@ def test_scalesets_created_from_relation_data( _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) address = _get_garm_address(juju, garm_app) base_url = _garm_api_base_url(address) - token = garm_login_from_secret(juju, garm_app, base_url) + token = garm_login_from_secret(juju, base_url) _create_test_credential(base_url, token) relation_data = _get_scaleset_relation_data( juju, garm_app, garm_configurator_for_scaleset_tests @@ -779,7 +779,7 @@ def test_scaleset_updated_on_relation_change( _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) address = _get_garm_address(juju, garm_app) base_url = _garm_api_base_url(address) - token = garm_login_from_secret(juju, garm_app, base_url) + token = garm_login_from_secret(juju, base_url) _create_test_credential(base_url, token) relation_data = _get_scaleset_relation_data( juju, garm_app, garm_configurator_for_scaleset_tests @@ -829,7 +829,7 @@ def test_scaleset_deleted_when_relation_removed( _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) address = _get_garm_address(juju, garm_app) base_url = _garm_api_base_url(address) - token = garm_login_from_secret(juju, garm_app, base_url) + token = garm_login_from_secret(juju, base_url) _create_test_credential(base_url, token) relation_data = _get_scaleset_relation_data( juju, garm_app, garm_configurator_for_scaleset_tests @@ -874,7 +874,7 @@ def test_runner_options_render_into_scaleset_template( _ensure_garm_configurator_relation(juju, garm_app, garm_configurator_for_scaleset_tests) address = _get_garm_address(juju, garm_app) base_url = _garm_api_base_url(address) - token = garm_login_from_secret(juju, garm_app, base_url) + token = garm_login_from_secret(juju, base_url) _create_test_credential(base_url, token) relation_data = _get_scaleset_relation_data( juju, garm_app, garm_configurator_for_scaleset_tests From 983da3fa102f336a10a3eb8078389b157a33da2d Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 15:30:35 +0000 Subject: [PATCH 20/21] fix(garm): guard hooks on readiness, lazy template listing, safe heredocs Address Copilot review on #238: - guard the update-status and configurator-relation handlers with is_ready() so the GARM client's 30s timeout can't hang a hook while the workload is starting - only call list_templates when a spec has runner options or an existing scaleset references a custom template, avoiding an API call per reconcile otherwise - derive collision-resistant heredoc delimiters so an operator pre-job-script containing the delimiter can't truncate the generated script/env file --- charms/garm/src/charm.py | 22 ++++++++++--- charms/garm/src/runner_template.py | 31 ++++++++++++++++--- charms/garm/src/scaleset_reconciler.py | 20 +++++++----- charms/garm/tests/unit/test_charm.py | 11 +++++++ .../garm/tests/unit/test_runner_template.py | 9 ++++++ .../tests/unit/test_scaleset_reconciler.py | 1 + 6 files changed, 78 insertions(+), 16 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 82d3d6b1..171b10a6 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -155,9 +155,16 @@ def _on_install(self, _: ops.InstallEvent) -> None: """Ensure secrets exist on first install.""" self._ensure_secrets() + # Typed HookEvent (not UpdateStatusEvent) to match the PaasCharm base method + # signature — overrides cannot narrow parameter types (Liskov substitution). def _on_update_status(self, _: ops.HookEvent) -> None: - """Retry GARM admin first-run until it succeeds.""" - self._maybe_first_run() + """Retry GARM admin first-run, but only once the workload is ready. + + Skipping when not ready avoids the GARM client's 30s network timeout + hanging the periodic update-status hook while GARM is still starting. + """ + if self.is_ready(): + self._maybe_first_run() @property def _workload_config(self) -> WorkloadConfig: @@ -172,9 +179,14 @@ def _workload_config(self) -> WorkloadConfig: """ return dataclasses.replace(super()._workload_config, port=GARM_PORT, metrics_target=None) - def _on_configurator_changed(self, _: typing.Any) -> None: - """Handle garm-configurator relation changes.""" - self._reconcile_scalesets() + def _on_configurator_changed(self, _: ops.RelationEvent) -> None: + """Reconcile scalesets on garm-configurator relation changes, when ready. + + Skipping when not ready avoids the GARM client's 30s network timeout + hanging the relation hook while GARM is still starting. + """ + if self.is_ready(): + self._reconcile_scalesets() def restart(self, rerun_migrations: bool = False) -> None: """Write GARM config then restart the workload. diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 5e592d50..514aca3c 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -160,24 +160,47 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: otel_endpoint = config.otel_collector_endpoint.replace("\r", "").replace("\n", "") env_entries.append(f"OTEL_EXPORTER_OTLP_ENDPOINT={otel_endpoint}") + # Pick delimiters that don't collide with the (operator-controlled) content, + # so a pre-job-script containing the literal delimiter can't terminate the + # heredoc early. Deterministic for a given content, keeping the rendered + # template stable across reconciles. + prejob_delim = _heredoc_delimiter(hook_body, "GARM_CHARM_PREJOB") + env_delim = _heredoc_delimiter("\n".join(env_entries), "GARM_CHARM_ENV") return "\n".join( [ "", "# ===== charm-injected runner job hooks =====", f"mkdir -p {RUNNER_HOME}", - f"cat > {PRE_JOB_HOOK_PATH} <<'GARM_CHARM_PREJOB'", + f"cat > {PRE_JOB_HOOK_PATH} <<'{prejob_delim}'", hook_body, - "GARM_CHARM_PREJOB", + prejob_delim, f"chmod 0755 {PRE_JOB_HOOK_PATH}", - f"cat >> {RUNNER_ENV_PATH} <<'GARM_CHARM_ENV'", + f"cat >> {RUNNER_ENV_PATH} <<'{env_delim}'", *env_entries, - "GARM_CHARM_ENV", + env_delim, f"chown -R {RUNNER_USER}:{RUNNER_USER} {RUNNER_HOME} 2>/dev/null || true", "# ===== end charm-injected runner job hooks =====\n", ] ) +def _heredoc_delimiter(content: str, base: str) -> str: + """Return a heredoc delimiter that does not appear as a line in *content*. + + Args: + content: The heredoc body the delimiter must not collide with. + base: The preferred delimiter; extended only if it collides. + + Returns: + ``base``, suffixed with underscores until no line of *content* matches it. + """ + lines = content.splitlines() + delimiter = base + while delimiter in lines: + delimiter += "_" + return delimiter + + # The production github-runner charm pins the proxy IP once per job so it cannot # drift mid-run. The exact resolution logic is environment-specific; this is a # faithful-but-minimal port. diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 24a9abab..6eee94a3 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -61,13 +61,19 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: providers = {provider["name"] for provider in self._client.list_providers()} credentials = {credential["name"] for credential in self._client.list_credentials()} observed = {scaleset["name"]: scaleset for scaleset in self._client.list_scalesets()} - # Only the system github_linux template and our per-scaleset - # github_linux- copies are relevant, so filter by partial name - # rather than pulling every template on the controller. - templates = { - template["name"]: template - for template in self._client.list_templates(partial_name=SYSTEM_TEMPLATE_NAME) - } + # Templates are only needed when a spec carries runner options or an + # existing scaleset already references a custom template (to update or + # detach it); skip the API call entirely otherwise. When fetched, filter + # by partial name so we only pull the system github_linux template and our + # per-scaleset github_linux- copies. + templates: dict[str, dict] = {} + if any(spec.runner_config.has_config() for spec in desired) or any( + scaleset.get("template_id") for scaleset in observed.values() + ): + templates = { + template["name"]: template + for template in self._client.list_templates(partial_name=SYSTEM_TEMPLATE_NAME) + } all_desired_names: set[str] = {spec.name for spec in desired} diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index c072754f..7d0246fa 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -319,6 +319,17 @@ def test_maybe_first_run_skips_first_run_when_login_succeeds(): mock_client_cls.return_value.first_run.assert_not_called() +def test_on_update_status_skips_first_run_when_not_ready(): + """update-status does not touch GARM while the workload is not ready.""" + charm = object.__new__(GarmCharm) + charm.is_ready = MagicMock(return_value=False) + charm._maybe_first_run = MagicMock() + + charm._on_update_status(MagicMock()) + + charm._maybe_first_run.assert_not_called() + + def test_maybe_first_run_skips_when_not_leader(): """Non-leader units do not attempt GARM first-run.""" charm = object.__new__(GarmCharm) diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index 14463705..a6ef37c3 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -116,6 +116,15 @@ def test_aproxy_render_drops_malformed_tokens(): assert "2001:db8::1" not in result # IPv6 dropped: the nft table is IPv4-only +# A pre-job-script containing the heredoc delimiter must not terminate it early. +def test_heredoc_delimiter_avoids_collision_with_pre_job_script(): + config = RunnerConfig(pre_job_script="echo hi\nGARM_CHARM_PREJOB\necho bye") + result = build_template_data(SAMPLE_BASE, config).decode() + + assert "<<'GARM_CHARM_PREJOB_'" in result + assert "echo bye" in result + + # A CR/LF in the otel endpoint must not inject extra lines into the env file. def test_otel_endpoint_newline_stripped(): config = RunnerConfig(otel_collector_endpoint="http://o.test:4318\nMALICIOUS=1") diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 0598bee5..39b02ccd 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -312,6 +312,7 @@ def test_orphan_scaleset_deletes_custom_template(): "tags": [], "runner_group": "default", "extra_specs": {}, + "template_id": 9, # a scaleset with a custom template references it } stale_template = {"id": 9, "name": "github_linux-stale-scaleset", "data": _b64("x")} client = _mock_client( From 08873126da94a33f0029ee0911b4e581f202188b Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 16 Jun 2026 15:41:10 +0000 Subject: [PATCH 21/21] fix(garm-configurator): reject URLs with invalid ports Address Copilot review on #238: _validate_http_url accepted URLs with invalid ports (e.g. http://host:bad) because urlparse does not validate them. Force evaluation of parsed.port (which raises on a bad port) and require a hostname. --- charms/garm-configurator/src/charm_state.py | 8 +++++++- charms/garm-configurator/tests/unit/test_charm.py | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 62d150c8..06d3e185 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -318,7 +318,13 @@ def _validate_http_url(config_name: str, value: str) -> None: if any(char.isspace() for char in value): raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") parsed = urllib.parse.urlparse(value) - if parsed.scheme not in ("http", "https") or not parsed.netloc: + try: + # Accessing .port raises ValueError on a non-numeric / out-of-range port, + # which urlparse otherwise accepts silently. + port = parsed.port + except ValueError as exc: + raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") from exc + if parsed.scheme not in ("http", "https") or not parsed.hostname or port == 0: raise CharmConfigInvalidError(f"{config_name} must be a valid http(s) URL") diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 9c0ef0ad..0edd2f4a 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -601,6 +601,12 @@ def test_garm_configurator_relation_changed_triggers_reconcile(): "runner-http-proxy must be a valid http(s) URL", id="runner-http-proxy-embedded-whitespace", ), + pytest.param( + "runner-http-proxy", + "http://proxy.example.com:bad", + "runner-http-proxy must be a valid http(s) URL", + id="runner-http-proxy-invalid-port", + ), ], ) def test_charm_blocked_invalid_runner_config(