From be2c50da29bc5a997e5e3274d62c518324599757 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 1 Jul 2026 21:16:11 +0000 Subject: [PATCH 01/16] feat(garm): expose runner config options via garm-configurator relation (ISD-5728) Add six operator-facing runner config options (dockerhub-mirror, runner-http-proxy, aproxy-exclude-addresses, aproxy-redirect-ports, otel-collector-endpoint, pre-job-script) to the garm-configurator charm. The GARM charm processes these via the relation databag, renders them into per-scaleset runner-install templates, and applies changes through the reconciler without requiring a charm upgrade. Configurator side: - charmcraft.yaml: add six config options - charm_state.py: add RunnerConfig with validation (http(s) URLs, IPv4 CIDRs, port ranges, aproxy-requires-proxy cross-check) - charm.py: publish runner config to garm-configurator relation databag GARM side: - runner_template.py: render runner options into a GARM runner-install template (aproxy nft rules, docker registry mirror, otel env, pre-job hook with heredoc-safe delimiters) - garm_api.py: add template API wrapper methods (list/get/create/update/ delete) over the generated TemplatesApi client - scaleset_reconciler.py: add runner_config to ScalesetSpec and template lifecycle (ensure/create/update/detach/delete) to the reconcile flow - charm.py: build runner_config from the relation databag in _build_desired_scalesets Tests: - Configurator: 13 new unit tests (invalid config validation, relation data publishing, aproxy-requires-proxy) - GARM: 10 new runner_template unit tests (injection, per-option, malformed tokens, heredoc collision, otel newline stripping, from_databag) - GARM: 4 new scaleset_reconciler template lifecycle tests (create, update, detach, keep-when-system-missing) - Integration: test_runner_options_render_into_scaleset_template --- charms/garm-configurator/charmcraft.yaml | 24 ++ charms/garm-configurator/src/charm.py | 7 + charms/garm-configurator/src/charm_state.py | 174 +++++++++ .../tests/unit/test_charm.py | 172 +++++++++ charms/garm/src/charm.py | 2 + charms/garm/src/garm_api.py | 152 ++++++++ charms/garm/src/runner_template.py | 346 ++++++++++++++++++ charms/garm/src/scaleset_reconciler.py | 189 +++++++++- .../garm/tests/unit/test_runner_template.py | 148 ++++++++ .../tests/unit/test_scaleset_reconciler.py | 191 ++++++++++ charms/tests/integration/test_garm.py | 75 ++++ 11 files changed, 1468 insertions(+), 12 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 40305051..ac954e21 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -109,6 +109,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. Must be a valid http(s) URL. + aproxy-exclude-addresses: + type: string + description: | + Comma-separated IPv4 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 http(s) URL. + pre-job-script: + type: string + description: | + A bash script appended to the end of the runner pre-job script. requires: image: diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index bf65ef85..eb0f8b2d 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -146,6 +146,7 @@ def _configure_garm_relation(self, state: CharmState) -> None: return pre_install = state.scaleset_config.pre_install_scripts + rc = state.runner_config basic_data: dict[str, str] = { "name": state.scaleset_config.name, "provider_name": state.provider_config.provider_name, @@ -159,6 +160,12 @@ def _configure_garm_relation(self, state: CharmState) -> None: "pre_install_scripts": json.dumps({"pre_install.sh": pre_install}) if pre_install 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 "", } if state.scaleset_config.org: basic_data["org"] = state.scaleset_config.org diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 8b7df13c..3ed0406e 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -3,6 +3,9 @@ """State of the GARM configurator charm.""" +import ipaddress +import urllib.parse + import ops from pydantic import BaseModel @@ -30,6 +33,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" GARM_RELATION_NAME = "garm-configurator" @@ -296,6 +306,164 @@ 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 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) + 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") + + +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 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: + # 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. + try: + 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 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) + 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) + + # 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 + + 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, + ) + + class CharmState: """The charm state. @@ -303,6 +471,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. """ @@ -312,6 +481,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. @@ -320,11 +490,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 @@ -343,11 +515,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 3cd1bcdc..dc7c85e6 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -631,3 +631,175 @@ def test_reconcile_writes_optional_scaleset_fields_to_garm_relation(): {"pre_install.sh": '{"setup.sh": "#!/bin/bash\\necho hello"}'} ) assert "repo" not in garm_out.local_unit_data + + +# --------------------------------------------------------------------------- +# 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", + ), + pytest.param( + "aproxy-exclude-addresses", + "10.0.0.1,not-an-ip", + "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", + ), + 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", + ), + 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( + 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_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. + 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/src/charm.py b/charms/garm/src/charm.py index 0d83453e..7c2e2220 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -36,6 +36,7 @@ CredentialSpec, GithubReconciler, ) +from runner_template import RunnerConfig from scaleset_reconciler import ScalesetReconciler, ScalesetSpec logger = logging.getLogger(__name__) @@ -824,6 +825,7 @@ def _build_desired_scalesets(self, template_id: int | None) -> list[ScalesetSpec data.get("pre_install_scripts", "") ), template_id=template_id, + runner_config=RunnerConfig.from_databag(data), ) ) return specs diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index 49cbcf26..518630fb 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -919,3 +919,155 @@ def delete_scaleset(self, scaleset_id: int) -> None: ) from exc except urllib3.exceptions.HTTPError as exc: raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def list_templates( + self, + partial_name: str | None = None, + os_type: str | None = None, + ) -> list[Template]: + """List GARM runner templates, optionally filtered by partial name. + + Args: + partial_name: Optional partial or full template name to filter by. + os_type: Optional OS type filter (e.g. "linux"). + + Returns: + List of Template model objects. + + Raises: + GarmApiError: On API error. + """ + with self._api_client() as client: + try: + return ( + TemplatesApi(api_client=client).list_templates( + partial_name=partial_name, + os_type=os_type, + _request_timeout=_REQUEST_TIMEOUT, + ) + or [] + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to list templates ({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def get_template(self, template_id: int) -> Template: + """Fetch a single template by ID. + + Args: + template_id: Integer template ID. + + Returns: + The Template model object. + + Raises: + GarmApiError: On API error. + """ + with self._api_client() as client: + try: + return TemplatesApi(api_client=client).get_template( + template_id=template_id, + _request_timeout=_REQUEST_TIMEOUT, + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to get template {template_id} ({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def create_template( + self, + name: str, + data: bytes, + description: str = "", + forge_type: str = "github", + os_type: str = "linux", + ) -> Template: + """Create a new runner template. + + Args: + name: Template name. + data: Template script bytes. + description: Optional description. + forge_type: Forge type (default "github"). + os_type: OS type (default "linux"). + + Returns: + The created Template model object. + + Raises: + GarmApiError: On API error. + """ + params = CreateTemplateParams( + name=name, + data=base64.b64encode(data).decode("utf-8"), + description=description or None, + forge_type=forge_type, + os_type=os_type, + ) + with self._api_client() as client: + try: + return TemplatesApi(api_client=client).create_template( + body=params, + _request_timeout=_REQUEST_TIMEOUT, + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to create template {name!r} ({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def update_template(self, template_id: int, data: bytes) -> Template: + """Update an existing template's script content. + + Args: + template_id: Integer template ID. + data: New template script bytes. + + Returns: + The updated Template model object. + + Raises: + GarmApiError: On API error. + """ + params = UpdateTemplateParams(data=base64.b64encode(data).decode("utf-8")) + with self._api_client() as client: + try: + return TemplatesApi(api_client=client).update_template( + template_id=template_id, + body=params, + _request_timeout=_REQUEST_TIMEOUT, + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to update template {template_id} ({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def delete_template(self, template_id: int) -> None: + """Delete a template by ID. + + Args: + template_id: Integer template ID. + + Raises: + GarmApiError: On API error. + """ + with self._api_client() as client: + try: + TemplatesApi(api_client=client).delete_template( + template_id=template_id, + _request_timeout=_REQUEST_TIMEOUT, + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to delete template {template_id} ({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py new file mode 100644 index 00000000..514aca3c --- /dev/null +++ b/charms/garm/src/runner_template.py @@ -0,0 +1,346 @@ +#!/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 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" +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: + # 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}") + + # 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} <<'{prejob_delim}'", + hook_body, + prejob_delim, + f"chmod 0755 {PRE_JOB_HOOK_PATH}", + f"cat >> {RUNNER_ENV_PATH} <<'{env_delim}'", + *env_entries, + 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. +# 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. + """ + # 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 = "" + 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. + 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. 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 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 well-formed, in-range tokens. + """ + 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]: + """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/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 0d246640..64f9f2ca 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -11,9 +11,14 @@ from garm_client.models.create_scale_set_params import CreateScaleSetParams from garm_client.models.scale_set import ScaleSet from garm_client.models.update_scale_set_params import UpdateScaleSetParams +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: @@ -33,6 +38,7 @@ class ScalesetSpec: runner_group: str = "Default" pre_install_scripts: dict[str, str] = field(default_factory=dict) template_id: int | None = None + runner_config: RunnerConfig = field(default_factory=RunnerConfig) class ScalesetReconciler: @@ -49,16 +55,34 @@ def __init__(self, client: GarmAuthenticatedClient) -> 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 is missing or the target entity (org/repo) - is not registered in GARM, 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 is missing or the target entity + (org/repo) is not registered in GARM, 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()} - observed = {scaleset.name: scaleset for scaleset in self._client.list_scalesets()} + observed = { + (scaleset.name or ""): scaleset for scaleset in self._client.list_scalesets() + } + + # 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, object] = {} + if any(spec.runner_config.has_config() for spec in desired) or any( + scaleset.template_id for scaleset in observed.values() + ): + templates = { + (template.name or ""): template + for template in self._client.list_templates(partial_name=SYSTEM_TEMPLATE_NAME) + if template.name + } all_desired_names: set[str] = {spec.name for spec in desired} @@ -87,14 +111,23 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: ) continue + 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, entity_id, create_params) + self._create(spec, entity_id, create_params, template_id) + + if not spec.runner_config.has_config(): + # 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: self._delete_orphaned(scaleset) + self._delete_custom_template(name, templates) def _delete_orphaned(self, scaleset: ScaleSet) -> None: """Disable then delete a scaleset that is no longer in the desired set.""" @@ -132,6 +165,120 @@ def _resolve_entity_id(self, spec: ScalesetSpec) -> str | None: logger.warning("Unknown entity_type %r for scaleset %s", spec.entity_type, spec.name) return None + def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, object]) -> int: + """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 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 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 spec.template_id or 0 + + base = self._template_by_id(templates, spec.template_id) or 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 self._template_id(existing) + logger.warning( + "System template %s not found; scaleset %s will use the default template", + SYSTEM_TEMPLATE_NAME, + spec.name, + ) + return 0 + + new_data = build_template_data(self._template_bytes(base), spec.runner_config) + if existing is not None: + if self._template_bytes(existing) != new_data: + logger.info("Updating runner template %s", custom_name) + self._client.update_template(self._template_id(existing), data=new_data) + return self._template_id(existing) + + 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 or 0 + + def _template_bytes(self, template: object) -> bytes: + """Return a template's raw bytes, fetching the full object if needed. + + Args: + template: A template object, possibly without its ``data`` field populated. + + Returns: + The decoded template bytes. + """ + data = getattr(template, "data", None) + if not data: + template_id = getattr(template, "id", None) + if template_id is None: + return b"" + fetched = self._client.get_template(template_id) + fetched_data = getattr(fetched, "data", None) or [] + # Cache it back so repeated lookups don't re-fetch. + setattr(template, "data", fetched_data) + data = fetched_data + return bytes(data) + + def _template_id(self, template: object) -> int: + """Return the template's id, defaulting to 0 when absent.""" + return getattr(template, "id", None) or 0 + + def _template_by_id( + self, templates: dict[str, object], template_id: int | None + ) -> object | None: + """Return a listed template by id, or None when absent.""" + if template_id is None: + return None + return next( + ( + template + for template in templates.values() + if self._template_id(template) == template_id + ), + None, + ) + + def _delete_custom_template(self, scaleset_name: str, templates: dict[str, object]) -> 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: + custom_name = getattr(custom, "name", f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}") + logger.info("Deleting orphaned runner template %s", custom_name) + self._client.delete_template(self._template_id(custom)) + @staticmethod def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: """Build and validate CreateScaleSetParams from a ScalesetSpec. @@ -165,14 +312,24 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: } ) - def _create(self, spec: ScalesetSpec, entity_id: str, params: CreateScaleSetParams) -> None: + def _create( + self, + spec: ScalesetSpec, + entity_id: str, + params: CreateScaleSetParams, + template_id: int, + ) -> None: + if template_id: + params.template_id = template_id logger.info("Creating scaleset %s under %s %s", spec.name, spec.entity_type, entity_id) if spec.entity_type == "organization": self._client.create_org_scaleset(entity_id, params) else: self._client.create_repo_scaleset(entity_id, params) - def _maybe_update(self, observed, spec: ScalesetSpec) -> None: + def _maybe_update( + self, observed: ScaleSet, spec: ScalesetSpec, template_id: int + ) -> None: observed_labels = sorted(t.name for t in (observed.tags or []) if t.name) if observed_labels != sorted(spec.labels): # UpdateScaleSetParams has no labels field; label changes require @@ -187,7 +344,10 @@ def _maybe_update(self, observed, spec: ScalesetSpec) -> None: sorted(spec.labels), ) - if not self._needs_update(observed, spec): + observed_template_id = observed.template_id or 0 + template_changed = observed_template_id != template_id + + if not self._needs_update(observed, spec, template_id) and not template_changed: logger.debug("Scaleset %s is up to date", spec.name) return @@ -204,6 +364,11 @@ def _maybe_update(self, observed, spec: ScalesetSpec) -> None: extra_specs=extra_specs or None, template_id=spec.template_id, ) + # 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: + params.template_id = template_id logger.info("Updating scaleset %s (id=%s)", spec.name, observed.id) if observed.id is None: logger.warning("Scaleset %s has no id; skipping update", spec.name) @@ -211,7 +376,7 @@ def _maybe_update(self, observed, spec: ScalesetSpec) -> None: self._client.update_scaleset(observed.id, params) @staticmethod - def _needs_update(observed, spec: ScalesetSpec) -> bool: + def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> bool: observed_scripts = (observed.extra_specs or {}).get("pre_install_scripts", {}) return ( observed.image != spec.image @@ -220,5 +385,5 @@ def _needs_update(observed, spec: ScalesetSpec) -> bool: or observed.min_idle_runners != spec.min_idle_runners or observed.github_runner_group != (spec.runner_group or None) or observed_scripts != spec.pre_install_scripts - or (spec.template_id is not None and observed.template_id != spec.template_id) + or (observed.template_id or 0) != template_id ) 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..a6ef37c3 --- /dev/null +++ b/charms/garm/tests/unit/test_runner_template.py @@ -0,0 +1,148 @@ +# 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 + + +# 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,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 + + +# 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") + 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(): + 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 7cdc755e..40392538 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -98,6 +98,23 @@ def update_scaleset(self, scaleset_id, params): def delete_scaleset(self, scaleset_id): self.deleted.append(scaleset_id) + # Template stubs: return empty results so the reconciler's template path + # is a no-op when no runner config is set. + def list_templates(self, partial_name=None, os_type=None): + return [] + + def get_template(self, template_id): + return None + + def create_template(self, name, data, description="", os_type="linux") -> object: + return None + + def update_template(self, template_id, data): + return None + + def delete_template(self, template_id): + pass + def _spec( name="my-scaleset", @@ -113,7 +130,10 @@ def _spec( runner_group="", pre_install_scripts=None, template_id=None, + runner_config=None, ): + from runner_template import RunnerConfig + return ScalesetSpec( name=name, provider_name=provider_name, @@ -128,6 +148,7 @@ def _spec( runner_group=runner_group, pre_install_scripts=pre_install_scripts or {}, template_id=template_id, + runner_config=runner_config or RunnerConfig(), ) @@ -307,3 +328,173 @@ def test_pre_install_scripts_passed_in_create(): assert len(client.created) == 1 _, _, params = client.created[0] assert params.extra_specs == {"pre_install_scripts": scripts} + + +# --------------------------------------------------------------------------- +# Runner template lifecycle tests +# --------------------------------------------------------------------------- + + +class _FakeTemplate: + """Minimal fake for the GARM Template model used in template lifecycle tests.""" + + def __init__(self, name, tid, data=b"", description=""): + self.name = name + self.id = tid + self.data = list(data) if data else None + self.description = description + + +class _TemplateTrackingClient(FakeGarmClient): + """FakeGarmClient variant that tracks template create/update/delete operations.""" + + def __init__(self, templates=None, **kwargs): + super().__init__(**kwargs) + self._templates = list(templates or []) + self.created_templates: list[tuple[str, bytes, str]] = [] + self.updated_templates: list[tuple[int, bytes]] = [] + self.deleted_templates: list[int] = [] + + def list_templates(self, partial_name=None, os_type=None): + if partial_name is None: + return list(self._templates) + return [t for t in self._templates if t.name and partial_name in t.name] + + def get_template(self, template_id): + for t in self._templates: + if t.id == template_id: + return t + return None + + def create_template(self, name, data, description="", os_type="linux") -> _FakeTemplate: # type: ignore[override] + tid = max((t.id for t in self._templates), default=0) + 1 + template = _FakeTemplate(name, tid, data, description) + self._templates.append(template) + self.created_templates.append((name, data, description)) + return template + + def update_template(self, template_id, data): + for t in self._templates: + if t.id == template_id: + t.data = list(data) + break + self.updated_templates.append((template_id, data)) + + def delete_template(self, template_id): + self._templates = [t for t in self._templates if t.id != template_id] + self.deleted_templates.append(template_id) + + +_SYSTEM_TEMPLATE = _FakeTemplate("github_linux", tid=1, data=b"#!/bin/bash\nset -e\necho base\n") + + +def _spec_with_runner_config(**rc_kwargs): + """Build a spec with runner_config populated from the given kwargs.""" + from runner_template import RunnerConfig + + return _spec(runner_config=RunnerConfig(**rc_kwargs)) + + +def test_template_created_when_runner_config_set(): + """ + arrange: No existing scalesets or custom templates; system template exists. + act: Reconcile a spec with runner options (dockerhub_mirror). + assert: A custom template is created and the scaleset references it via template_id. + """ + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[], + templates=[_SYSTEM_TEMPLATE], + ) + _reconcile( + client, + [_spec_with_runner_config(dockerhub_mirror="https://mirror.example.com")], + ) + + assert len(client.created_templates) == 1 + name, data, _ = client.created_templates[0] + assert name == "github_linux-my-scaleset" + assert b"registry-mirrors" in data + assert len(client.created) == 1 + _, _, params = client.created[0] + # The created template's id is 2 (system=1, first custom=2) + assert params.template_id == 2 + + +def test_template_updated_when_runner_config_changes(): + """ + arrange: A scaleset with an existing custom template. + act: Reconcile with a changed runner config. + assert: The custom template is updated (not recreated); the scaleset template_id is unchanged. + """ + from runner_template import build_template_data + from runner_template import RunnerConfig + + old_config = RunnerConfig(dockerhub_mirror="https://old.example.com") + custom_template = _FakeTemplate( + "github_linux-my-scaleset", + tid=2, + data=build_template_data(b"#!/bin/bash\nset -e\necho base\n", old_config), + ) + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(template_id=2)], + templates=[_SYSTEM_TEMPLATE, custom_template], + ) + _reconcile( + client, + [_spec_with_runner_config(dockerhub_mirror="https://new.example.com")], + ) + + assert len(client.updated_templates) == 1 + template_id, data = client.updated_templates[0] + assert template_id == 2 + assert b"new.example.com" in data + assert b"old.example.com" not in data + # Scaleset should not be updated just because template content changed. + assert client.updated == [] + + +def test_template_detached_when_runner_config_cleared(): + """ + arrange: A scaleset with an existing custom template. + act: Reconcile with no runner options. + assert: The scaleset is updated with template_id=0 (detach), and the custom template is deleted. + """ + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(template_id=2)], + templates=[ + _SYSTEM_TEMPLATE, + _FakeTemplate("github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n"), + ], + ) + _reconcile(client, [_spec()]) + + assert len(client.updated) == 1 + _, params = client.updated[0] + assert params.template_id == 0 + assert 2 in client.deleted_templates + + +def test_template_kept_when_system_template_missing(): + """ + arrange: A scaleset with an existing custom template, but the system template is not listed. + act: Reconcile with runner options still set. + assert: The existing custom template is kept (not destroyed); no create/update/delete on templates. + """ + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(template_id=2)], + templates=[ + _FakeTemplate("github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n"), + ], + ) + _reconcile( + client, + [_spec_with_runner_config(dockerhub_mirror="https://m.example.com")], + ) + + assert client.created_templates == [] + assert client.updated_templates == [] + assert client.deleted_templates == [] diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 83dd64a1..c3e9790d 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -775,3 +775,78 @@ def test_github_credentials_synced_from_relation( assert "github.com" in endpoint_names, ( f"Built-in github.com endpoint must be preserved; got {sorted(endpoint_names)}" ) + + +def _get_template_data(base_url: str, token: str, template_id: int) -> str: + """Fetch a GARM template and return its decoded (base64) script content. + + Args: + base_url: GARM API base URL. + token: JWT token for authentication. + template_id: Integer template ID. + + Returns: + The decoded template script as a string. + """ + 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 test_runner_options_render_into_scaleset_template( + juju: jubilant.Juju, + configurator_garm: str, + configurator_with_image: str, + fake_github_api_url: str, +): + """ + arrange: GARM and garm-configurator are integrated, with a credential and org + registered so a scaleset can be created. + act: Set the runner-behaviour config options on the configurator charm. + assert: The scaleset references a custom template whose rendered content + reflects each runner option — proving the options reach GARM via live + reconcile (no upgrade). + """ + address = _get_garm_address(juju, configurator_garm) + base_url = _garm_api_base_url(address) + token = _garm_first_run(juju, address) + _restore_system_templates(base_url, token) + _create_test_credential(base_url, token, fake_github_api_url) + _create_test_org(base_url, token, "test-org") + + juju.config( + configurator_with_image, + values={ + "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", + }, + ) + juju.wait( + lambda status: jubilant.all_active(status, configurator_garm, configurator_with_image), + timeout=3 * 60, + delay=10, + ) + + 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 16402c67ed21f9f678e6133c1d2928dc6c11d3bb Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 05:21:26 +0000 Subject: [PATCH 02/16] Fix GARM scaleset reconciliation --- charms/garm/src/scaleset_reconciler.py | 7 +++ .../tests/unit/test_scaleset_reconciler.py | 46 +++++++++++++++++- charms/tests/integration/test_garm.py | 48 +++++++++++++++---- 3 files changed, 90 insertions(+), 11 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 64f9f2ca..8e2c651d 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -245,6 +245,10 @@ def _template_bytes(self, template: object) -> bytes: # Cache it back so repeated lookups don't re-fetch. setattr(template, "data", fetched_data) data = fetched_data + if isinstance(data, bytes): + return data + if isinstance(data, str): + return data.encode("utf-8") return bytes(data) def _template_id(self, template: object) -> int: @@ -305,6 +309,7 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: "os_type": spec.os_type, "min_idle_runners": spec.min_idle_runners, "max_runners": spec.max_runners, + "enabled": True, "labels": sorted(spec.labels), "github_runner_group": spec.runner_group or None, "extra_specs": extra_specs or None, @@ -360,6 +365,7 @@ def _maybe_update( flavor=spec.flavor, min_idle_runners=spec.min_idle_runners, max_runners=spec.max_runners, + enabled=True, runner_group=spec.runner_group or None, extra_specs=extra_specs or None, template_id=spec.template_id, @@ -383,6 +389,7 @@ def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> b or observed.flavor != spec.flavor or observed.max_runners != spec.max_runners or observed.min_idle_runners != spec.min_idle_runners + or observed.enabled is not True or observed.github_runner_group != (spec.runner_group or None) or observed_scripts != spec.pre_install_scripts or (observed.template_id or 0) != template_id diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 40392538..67c11424 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -31,6 +31,7 @@ def __init__( extra_specs=None, tags=None, template_id=None, + enabled=True, ): self.name = name self.id = sid @@ -42,6 +43,7 @@ def __init__( self.extra_specs = extra_specs or {} self.tags = [_FakeTag(t) for t in (tags or [])] self.template_id = template_id + self.enabled = enabled class FakeGarmClient: @@ -65,6 +67,7 @@ def __init__(self, providers=None, scalesets=None, org_id="org-uuid", repo_id=No extra_specs=ss.get("extra_specs", {}), tags=ss.get("tags", []), template_id=ss.get("template_id", None), + enabled=ss.get("enabled", True), ) for ss in (scalesets or []) ] @@ -185,6 +188,7 @@ def test_create_scaleset(entity_type, entity_name, create_key, expected_entity_i assert params.name == "my-scaleset" assert params.image == "ubuntu-22.04" assert params.flavor == "m1.small" + assert params.enabled is True assert client.updated == [] assert client.deleted == [] @@ -222,6 +226,7 @@ def _existing_scaleset(**overrides): github_runner_group=None, extra_specs={}, tags=[], + enabled=True, ) base.update(overrides) return base @@ -280,6 +285,23 @@ def test_no_update_when_scaleset_unchanged(): assert client.deleted == [] +def test_update_when_scaleset_disabled(): + """ + arrange: FakeGarmClient with one existing disabled scaleset that otherwise matches. + act: Reconcile the desired spec. + assert: The reconciler enables the scaleset so GARM can spawn runners. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(enabled=False)], + ) + _reconcile(client, [_spec()]) + + assert len(client.updated) == 1 + _, params = client.updated[0] + assert params.enabled is True + + def test_delete_orphaned_scaleset(): """ arrange: FakeGarmClient with an observed scaleset not present in the desired set. @@ -341,7 +363,7 @@ class _FakeTemplate: def __init__(self, name, tid, data=b"", description=""): self.name = name self.id = tid - self.data = list(data) if data else None + self.data = list(data) if isinstance(data, bytes) and data else data or None self.description = description @@ -421,6 +443,28 @@ def test_template_created_when_runner_config_set(): assert params.template_id == 2 +def test_template_created_when_garm_returns_template_data_as_string(): + """ + arrange: The system template data is returned as a string by the GARM API. + act: Reconcile a spec with runner options. + assert: A custom template is created from the string data without hook errors. + """ + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[], + templates=[_FakeTemplate("github_linux", tid=1, data="#!/bin/bash\nset -e\necho base\n")], + ) + _reconcile( + client, + [_spec_with_runner_config(dockerhub_mirror="https://mirror.example.com")], + ) + + assert len(client.created_templates) == 1 + _, data, _ = client.created_templates[0] + assert isinstance(data, bytes) + assert b"registry-mirrors" in data + + def test_template_updated_when_runner_config_changes(): """ arrange: A scaleset with an existing custom template. diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index c3e9790d..d87e5f25 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -798,6 +798,31 @@ def _get_template_data(base_url: str, token: str, template_id: int) -> str: return base64.b64decode(data).decode() if data else "" +@retry( + retry=retry_if_exception_type((AssertionError, requests.exceptions.RequestException)), + wait=wait_exponential(multiplier=1, min=2, max=20), + stop=stop_after_attempt(30), + reraise=True, +) +def _wait_for_scaleset_template_data( + base_url: str, + token: str, + scaleset_name: str, + expected_markers: tuple[str, ...], +) -> str: + """Wait until a scaleset references a template containing the expected markers.""" + scaleset = _wait_for_scaleset(base_url, token, scaleset_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) + missing = [marker for marker in expected_markers if marker not in rendered] + assert not missing, ( + f"Expected markers {missing!r} in rendered template {template_id}, got:\n{rendered}" + ) + return rendered + + def test_runner_options_render_into_scaleset_template( juju: jubilant.Juju, configurator_garm: str, @@ -815,9 +840,9 @@ def test_runner_options_render_into_scaleset_template( address = _get_garm_address(juju, configurator_garm) base_url = _garm_api_base_url(address) token = _garm_first_run(juju, address) + _detach_synced_credential(base_url, token) + _point_github_endpoint_at_mock(base_url, token, fake_github_api_url) _restore_system_templates(base_url, token) - _create_test_credential(base_url, token, fake_github_api_url) - _create_test_org(base_url, token, "test-org") juju.config( configurator_with_image, @@ -830,23 +855,26 @@ def test_runner_options_render_into_scaleset_template( }, ) juju.wait( - lambda status: jubilant.all_active(status, configurator_garm, configurator_with_image), + lambda status: jubilant.all_active(status, configurator_with_image) + and jubilant.all_agents_idle(status, configurator_garm), timeout=3 * 60, delay=10, ) - 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 ( + expected_markers = ( "registry-mirrors", "https://mirror.example.com", "http://proxy.example.com:3128", "OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", "echo integration-marker", - ): + ) + rendered = _wait_for_scaleset_template_data( + base_url, + token, + _SCALESET_TEST_NAME, + expected_markers, + ) + for expected in expected_markers: assert expected in rendered, ( f"Expected {expected!r} in rendered template, got:\n{rendered}" ) From 7dd69fd1fd38bdeb231bdd8a16d4e8a5f7921d81 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 05:45:32 +0000 Subject: [PATCH 03/16] refactor(garm): dedupe template API and simplify scaleset reconciler Remove the duplicated set of five template methods appended to GarmAuthenticatedClient (they shadowed the pre-existing ones); fold the new behaviour into the originals: optional partial_name/os_type filters on list_templates and defaults on create_template. In the scaleset reconciler, type observed templates as dict[str, Template], drop the shallow _template_id helper for attribute access, and remove the redundant template_changed check already covered by _needs_update. Simplify the configurator's pre_job_script handling and drop a redundant re-assertion loop in the garm integration test. --- charms/garm-configurator/src/charm_state.py | 4 +- charms/garm/src/garm_api.py | 181 +++----------------- charms/garm/src/scaleset_reconciler.py | 65 +++---- charms/tests/integration/test_garm.py | 8 +- 4 files changed, 55 insertions(+), 203 deletions(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 3ed0406e..5ec41c41 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -452,7 +452,7 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": ) raw_script = charm.config.get(PRE_JOB_SCRIPT_CONFIG_NAME) - pre_job_script = str(raw_script).strip() if raw_script else None + pre_job_script = (str(raw_script).strip() or None) if raw_script else None return cls( dockerhub_mirror=url_values[DOCKERHUB_MIRROR_CONFIG_NAME], @@ -460,7 +460,7 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": 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, + pre_job_script=pre_job_script, ) diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index 518630fb..9370e802 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -246,8 +246,16 @@ def _api_client(self) -> ApiClient: header_value=f"Bearer {self._token}", ) - def list_templates(self) -> list[Template]: - """List all runner install templates known to GARM. + def list_templates( + self, + partial_name: str | None = None, + os_type: str | None = None, + ) -> list[Template]: + """List runner install templates known to GARM, optionally filtered. + + Args: + partial_name: Optional partial or full template name to filter by. + os_type: Optional OS type filter (e.g. ``"linux"``). Returns: List of Template objects. @@ -258,7 +266,14 @@ def list_templates(self) -> list[Template]: with self._api_client() as client: api = TemplatesApi(api_client=client) try: - return api.list_templates(_request_timeout=_REQUEST_TIMEOUT) or [] + return ( + api.list_templates( + partial_name=partial_name, + os_type=os_type, + _request_timeout=_REQUEST_TIMEOUT, + ) + or [] + ) except ApiException as exc: raise GarmApiError( f"GARM list_templates failed ({exc.status}): {exc.body}" @@ -292,19 +307,19 @@ def get_template(self, template_id: int) -> Template: def create_template( self, name: str, - description: str, - forge_type: str, - os_type: str, data: bytes, + description: str = "", + forge_type: str = "github", + os_type: str = "linux", ) -> Template: """Create a new runner install template. Args: name: Template name. + data: Raw shell script bytes for the template body. description: Human-readable description. forge_type: Source forge type, e.g. ``"github"``. os_type: OS type, e.g. ``"linux"``. - data: Raw shell script bytes for the template body. Returns: The created Template. @@ -919,155 +934,3 @@ def delete_scaleset(self, scaleset_id: int) -> None: ) from exc except urllib3.exceptions.HTTPError as exc: raise GarmConnectionError(f"GARM connection error: {exc}") from exc - - def list_templates( - self, - partial_name: str | None = None, - os_type: str | None = None, - ) -> list[Template]: - """List GARM runner templates, optionally filtered by partial name. - - Args: - partial_name: Optional partial or full template name to filter by. - os_type: Optional OS type filter (e.g. "linux"). - - Returns: - List of Template model objects. - - Raises: - GarmApiError: On API error. - """ - with self._api_client() as client: - try: - return ( - TemplatesApi(api_client=client).list_templates( - partial_name=partial_name, - os_type=os_type, - _request_timeout=_REQUEST_TIMEOUT, - ) - or [] - ) - except ApiException as exc: - raise GarmApiError( - f"Failed to list templates ({exc.status}): {exc.body}" - ) from exc - except urllib3.exceptions.HTTPError as exc: - raise GarmConnectionError(f"GARM connection error: {exc}") from exc - - def get_template(self, template_id: int) -> Template: - """Fetch a single template by ID. - - Args: - template_id: Integer template ID. - - Returns: - The Template model object. - - Raises: - GarmApiError: On API error. - """ - with self._api_client() as client: - try: - return TemplatesApi(api_client=client).get_template( - template_id=template_id, - _request_timeout=_REQUEST_TIMEOUT, - ) - except ApiException as exc: - raise GarmApiError( - f"Failed to get template {template_id} ({exc.status}): {exc.body}" - ) from exc - except urllib3.exceptions.HTTPError as exc: - raise GarmConnectionError(f"GARM connection error: {exc}") from exc - - def create_template( - self, - name: str, - data: bytes, - description: str = "", - forge_type: str = "github", - os_type: str = "linux", - ) -> Template: - """Create a new runner template. - - Args: - name: Template name. - data: Template script bytes. - description: Optional description. - forge_type: Forge type (default "github"). - os_type: OS type (default "linux"). - - Returns: - The created Template model object. - - Raises: - GarmApiError: On API error. - """ - params = CreateTemplateParams( - name=name, - data=base64.b64encode(data).decode("utf-8"), - description=description or None, - forge_type=forge_type, - os_type=os_type, - ) - with self._api_client() as client: - try: - return TemplatesApi(api_client=client).create_template( - body=params, - _request_timeout=_REQUEST_TIMEOUT, - ) - except ApiException as exc: - raise GarmApiError( - f"Failed to create template {name!r} ({exc.status}): {exc.body}" - ) from exc - except urllib3.exceptions.HTTPError as exc: - raise GarmConnectionError(f"GARM connection error: {exc}") from exc - - def update_template(self, template_id: int, data: bytes) -> Template: - """Update an existing template's script content. - - Args: - template_id: Integer template ID. - data: New template script bytes. - - Returns: - The updated Template model object. - - Raises: - GarmApiError: On API error. - """ - params = UpdateTemplateParams(data=base64.b64encode(data).decode("utf-8")) - with self._api_client() as client: - try: - return TemplatesApi(api_client=client).update_template( - template_id=template_id, - body=params, - _request_timeout=_REQUEST_TIMEOUT, - ) - except ApiException as exc: - raise GarmApiError( - f"Failed to update template {template_id} ({exc.status}): {exc.body}" - ) from exc - except urllib3.exceptions.HTTPError as exc: - raise GarmConnectionError(f"GARM connection error: {exc}") from exc - - def delete_template(self, template_id: int) -> None: - """Delete a template by ID. - - Args: - template_id: Integer template ID. - - Raises: - GarmApiError: On API error. - """ - with self._api_client() as client: - try: - TemplatesApi(api_client=client).delete_template( - template_id=template_id, - _request_timeout=_REQUEST_TIMEOUT, - ) - except ApiException as exc: - raise GarmApiError( - f"Failed to delete template {template_id} ({exc.status}): {exc.body}" - ) from exc - except urllib3.exceptions.HTTPError as exc: - raise GarmConnectionError(f"GARM connection error: {exc}") from exc diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 8e2c651d..a655b47a 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -10,6 +10,7 @@ from garm_api import GarmApiError, GarmAuthenticatedClient from garm_client.models.create_scale_set_params import CreateScaleSetParams from garm_client.models.scale_set import ScaleSet +from garm_client.models.template import Template from garm_client.models.update_scale_set_params import UpdateScaleSetParams from runner_template import RunnerConfig, build_template_data @@ -65,16 +66,14 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: desired: The full desired set of scalesets. """ providers = {provider.name for provider in self._client.list_providers()} - observed = { - (scaleset.name or ""): scaleset for scaleset in self._client.list_scalesets() - } + observed = {(scaleset.name or ""): scaleset for scaleset in self._client.list_scalesets()} # 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, object] = {} + templates: dict[str, Template] = {} if any(spec.runner_config.has_config() for spec in desired) or any( scaleset.template_id for scaleset in observed.values() ): @@ -165,7 +164,7 @@ def _resolve_entity_id(self, spec: ScalesetSpec) -> str | None: logger.warning("Unknown entity_type %r for scaleset %s", spec.entity_type, spec.name) return None - def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, object]) -> int: + def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) -> int: """Ensure the scaleset's runner template reflects its runner options. Copies the system ``github_linux`` template, injects the runner options, @@ -203,7 +202,7 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, object]) -> SYSTEM_TEMPLATE_NAME, spec.name, ) - return self._template_id(existing) + return existing.id or 0 logger.warning( "System template %s not found; scaleset %s will use the default template", SYSTEM_TEMPLATE_NAME, @@ -215,8 +214,8 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, object]) -> if existing is not None: if self._template_bytes(existing) != new_data: logger.info("Updating runner template %s", custom_name) - self._client.update_template(self._template_id(existing), data=new_data) - return self._template_id(existing) + self._client.update_template(existing.id or 0, data=new_data) + return existing.id or 0 logger.info("Creating runner template %s", custom_name) created = self._client.create_template( @@ -226,62 +225,55 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, object]) -> ) return created.id or 0 - def _template_bytes(self, template: object) -> bytes: + def _template_bytes(self, template: Template) -> bytes: """Return a template's raw bytes, fetching the full object if needed. Args: - template: A template object, possibly without its ``data`` field populated. + template: A template, possibly without its ``data`` field populated + (the list endpoint omits the body). Returns: The decoded template bytes. """ + # data is Any at runtime: the list endpoint omits it, and it can come back + # as a base64 str or (in tests) the raw list-of-ints byte representation. data = getattr(template, "data", None) if not data: - template_id = getattr(template, "id", None) - if template_id is None: + if template.id is None: return b"" - fetched = self._client.get_template(template_id) - fetched_data = getattr(fetched, "data", None) or [] + fetched = self._client.get_template(template.id) + data = getattr(fetched, "data", None) or [] # Cache it back so repeated lookups don't re-fetch. - setattr(template, "data", fetched_data) - data = fetched_data + setattr(template, "data", data) if isinstance(data, bytes): return data if isinstance(data, str): return data.encode("utf-8") return bytes(data) - def _template_id(self, template: object) -> int: - """Return the template's id, defaulting to 0 when absent.""" - return getattr(template, "id", None) or 0 - def _template_by_id( - self, templates: dict[str, object], template_id: int | None - ) -> object | None: + self, templates: dict[str, Template], template_id: int | None + ) -> Template | None: """Return a listed template by id, or None when absent.""" if template_id is None: return None return next( - ( - template - for template in templates.values() - if self._template_id(template) == template_id - ), + (template for template in templates.values() if (template.id or 0) == template_id), None, ) - def _delete_custom_template(self, scaleset_name: str, templates: dict[str, object]) -> None: + def _delete_custom_template(self, scaleset_name: str, templates: dict[str, Template]) -> 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}") + custom_name = f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}" + custom = templates.get(custom_name) if custom is not None: - custom_name = getattr(custom, "name", f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}") - logger.info("Deleting orphaned runner template %s", custom_name) - self._client.delete_template(self._template_id(custom)) + logger.info("Deleting orphaned runner template %s", custom.name or custom_name) + self._client.delete_template(custom.id or 0) @staticmethod def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: @@ -332,9 +324,7 @@ def _create( else: self._client.create_repo_scaleset(entity_id, params) - def _maybe_update( - self, observed: ScaleSet, spec: ScalesetSpec, template_id: int - ) -> None: + def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> None: observed_labels = sorted(t.name for t in (observed.tags or []) if t.name) if observed_labels != sorted(spec.labels): # UpdateScaleSetParams has no labels field; label changes require @@ -350,9 +340,10 @@ def _maybe_update( ) observed_template_id = observed.template_id or 0 - template_changed = observed_template_id != template_id - if not self._needs_update(observed, spec, template_id) and not template_changed: + # _needs_update already covers the template id (its last clause), so an + # id change alone forces an update here. + if not self._needs_update(observed, spec, template_id): logger.debug("Scaleset %s is up to date", spec.name) return diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index d87e5f25..6f45e21c 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -868,13 +868,11 @@ def test_runner_options_render_into_scaleset_template( "OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", "echo integration-marker", ) - rendered = _wait_for_scaleset_template_data( + # Asserts each expected marker is present in the referenced template, retrying + # until GARM reflects the reconciled config. + _wait_for_scaleset_template_data( base_url, token, _SCALESET_TEST_NAME, expected_markers, ) - for expected in expected_markers: - assert expected in rendered, ( - f"Expected {expected!r} in rendered template, got:\n{rendered}" - ) From 37e218ce54c15b2ee770d387ad25f0ff4be72760 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 07:41:58 +0000 Subject: [PATCH 04/16] style(garm): remove shebang from non-executable runner_template module runner_template.py is an imported module, never executed directly, so the `#!/usr/bin/env python3` shebang was misleading. Align it with the other pure modules (charm_state.py, garm_api.py) that start with the copyright header. --- charms/garm/src/runner_template.py | 1 - 1 file changed, 1 deletion(-) diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 514aca3c..7f5f83fb 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python3 # Copyright 2026 Canonical Ltd. # See LICENSE file for licensing details. From 3672a0ea041c8a56c26100fa9bd56de4b3a8d199 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 07:41:58 +0000 Subject: [PATCH 05/16] refactor(garm-configurator): validate RunnerConfig with pydantic validators Move RunnerConfig validation off the imperative from_charm and onto the model: a model_validator normalises empty/whitespace values to None, field validators check the URL / IPv4-list / port-list options, and an after model_validator enforces the aproxy-requires-proxy rule. from_charm now just reads the config strings and converts ValidationError to CharmConfigInvalidError. Drops the two module-level validation helpers. --- charms/garm-configurator/src/charm_state.py | 238 ++++++++++---------- 1 file changed, 115 insertions(+), 123 deletions(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 5ec41c41..91a62e46 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -7,7 +7,7 @@ import urllib.parse import ops -from pydantic import BaseModel +from pydantic import BaseModel, ValidationError, ValidationInfo, field_validator, model_validator OPENSTACK_AUTH_URL_CONFIG_NAME = "openstack-auth-url" OPENSTACK_USERNAME_CONFIG_NAME = "openstack-username" @@ -306,62 +306,6 @@ 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 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) - 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") - - -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 RunnerConfig(BaseModel): """Optional runner-level configuration forwarded to the GARM scaleset. @@ -381,11 +325,108 @@ class RunnerConfig(BaseModel): otel_collector_endpoint: str | None = None pre_job_script: str | None = None + @model_validator(mode="before") + @classmethod + def _strip_to_none(cls, data: object) -> object: + """Strip whitespace and treat empty/whitespace-only values as unset.""" + if not isinstance(data, dict): + return data + return { + key: (str(value).strip() or None) if value is not None else None + for key, value in data.items() + } + + @field_validator("dockerhub_mirror", "runner_http_proxy", "otel_collector_endpoint") + @classmethod + def _validate_http_url(cls, value: str | None, info: ValidationInfo) -> str | None: + """Require a set option to be a well-formed http(s) URL.""" + if value is None: + return None + # Field names are the snake_case form of the kebab-case config option. + msg = f"{(info.field_name or '').replace('_', '-')} must be a valid http(s) URL" + # Reject whitespace: the value is rendered into scripts and env files, + # where a newline could inject extra lines. + if any(char.isspace() for char in value): + raise ValueError(msg) + parsed = urllib.parse.urlparse(value) + try: + # .port raises ValueError on a non-numeric / out-of-range port that + # urlparse otherwise accepts silently. + port = parsed.port + except ValueError as exc: + raise ValueError(msg) from exc + if parsed.scheme not in ("http", "https") or not parsed.hostname or port == 0: + raise ValueError(msg) + return value + + @field_validator("aproxy_exclude_addresses") + @classmethod + def _validate_ipv4_list(cls, value: str | None) -> str | None: + """Require a comma-separated list of IPv4 addresses/CIDRs; normalise spacing.""" + if value is None: + return None + tokens = [token.strip() for token in value.split(",")] + for token in tokens: + # The values are rendered into an nft IPv4 (table ip) ruleset, so a + # hostname or IPv6 address would validate here but fail at runtime. + try: + network = ipaddress.ip_network(token, strict=False) + except ValueError as exc: + raise ValueError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} must be a comma-separated list " + f"of IPv4 addresses or CIDRs; got invalid token: {token!r}" + ) from exc + if network.version != 4: + raise ValueError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} only supports IPv4 addresses or " + f"CIDRs (the aproxy nft ruleset is IPv4-only); got: {token!r}" + ) + return ",".join(tokens) + + @field_validator("aproxy_redirect_ports") + @classmethod + def _validate_port_list(cls, value: str | None) -> str | None: + """Require a comma-separated list of ports or N-M ranges in 1..65535.""" + if value is None: + return None + msg = ( + f"{APROXY_REDIRECT_PORTS_CONFIG_NAME} must be a comma-separated list of " + "ports or N-M ranges in 1..65535" + ) + tokens = [token.strip() for token in value.split(",")] + for token in tokens: + bounds = token.split("-") + if len(bounds) > 2: + raise ValueError(msg) + try: + ports = [int(part) for part in bounds] + except ValueError as exc: + raise ValueError(msg) from exc + if not all(1 <= port <= 65535 for port in ports) or ports != sorted(ports): + raise ValueError(msg) + return ",".join(tokens) + + @model_validator(mode="after") + def _aproxy_requires_proxy(self) -> "RunnerConfig": + """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 (self.aproxy_exclude_addresses or self.aproxy_redirect_ports) and ( + not self.runner_http_proxy + ): + raise ValueError( + f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} and {APROXY_REDIRECT_PORTS_CONFIG_NAME} " + f"require {RUNNER_HTTP_PROXY_CONFIG_NAME} to be set" + ) + return self + @classmethod def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": - """Initialize the runner config from charm, applying best-effort validation. + """Build and validate the runner config from charm configuration. - All options are optional; unset or empty values result in None fields. + All options are optional; unset, empty, or whitespace-only values become None. Args: charm: The charm instance. @@ -396,72 +437,23 @@ def from_charm(cls, charm: ops.CharmBase) -> "RunnerConfig": 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: - # 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. - try: - 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 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) - 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) - - # 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() or None) if raw_script else None + def config_str(name: str) -> str | None: + """Return a string config value, or None when unset.""" + value = charm.config.get(name) + return None if value is None else str(value) - 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, - ) + try: + return cls( + dockerhub_mirror=config_str(DOCKERHUB_MIRROR_CONFIG_NAME), + runner_http_proxy=config_str(RUNNER_HTTP_PROXY_CONFIG_NAME), + aproxy_exclude_addresses=config_str(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME), + aproxy_redirect_ports=config_str(APROXY_REDIRECT_PORTS_CONFIG_NAME), + otel_collector_endpoint=config_str(OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME), + pre_job_script=config_str(PRE_JOB_SCRIPT_CONFIG_NAME), + ) + except ValidationError as exc: + raise CharmConfigInvalidError(str(exc)) from exc class CharmState: From d700c8bda7756148b51214ade9d8770ee863cd22 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 07:57:14 +0000 Subject: [PATCH 06/16] fix(garm): render runner-install template to match production templates The runner-option rendering was a hand-written approximation with wrong values and missing pieces. Port it faithfully from the production github-runner-manager templates (env.j2, openstack-userdata.sh.j2, pre-job.j2): - env: emit DOCKERHUB_MIRROR + CONTAINER_REGISTRY_URL, and fix the OTEL variable name to ACTION_OTEL_EXPORTER_OTLP_ENDPOINT. - aproxy: listen on :54969, write /etc/nftables.conf with the default-ipv4 gateway lookup, a named exclude set (always including 127.0.0.0/8), and both prerouting and output DNAT chains; enable nftables.service. - docker mirror: add systemctl daemon-reload before restart. - static host prep: adduser the runner account to lxd/adm. - pre-job hook: drop the fabricated proxy-pinning snippet; set up the OpenTelemetry collector config and run the operator pre-job-script via the production temp-file pattern. Removes the TODO(ISD-278) stubs. Tests updated to assert the ported output. --- charms/garm/src/runner_template.py | 337 +++++++++++++++--- .../garm/tests/unit/test_runner_template.py | 36 +- charms/tests/integration/test_garm.py | 2 +- 3 files changed, 317 insertions(+), 58 deletions(-) diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 7f5f83fb..3536d560 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -16,6 +16,11 @@ runner ``env`` file under ``/home/runner/actions-runner`` (GARM hardcodes the ``runner`` user and that path). +Both blocks are ported line-for-line from the production +``canonical/github-runner-operator`` templates (``openstack-userdata.sh.j2``, +``env.j2``, ``pre-job.j2``) so the runner behaves identically to the previous +machine-charm implementation. + 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. @@ -138,9 +143,10 @@ def render_pre_install(config: RunnerConfig) -> str: 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. + The hook file (``pre-job.j2`` in production) sets up the OpenTelemetry + collector, when configured, and appends the operator's ``pre-job-script``. + The docker mirror and OTEL endpoint are exported via the runner ``env`` file + (``env.j2`` in production), read once per job by the runner service. Args: config: The runner options to render. @@ -148,16 +154,22 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: 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}"] + otel_endpoint = "" if 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). + # Strip CR/LF so a databag value can't inject extra env entries or extra + # lines into the otelcol config (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}") + + hook_body = _render_pre_job_hook_body(config, otel_endpoint) + + env_entries = [] + if config.dockerhub_mirror: + env_entries.append(f"DOCKERHUB_MIRROR={config.dockerhub_mirror}") + env_entries.append(f"CONTAINER_REGISTRY_URL={config.dockerhub_mirror}") + env_entries.append(f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}") + if otel_endpoint: + env_entries.append(f"ACTION_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 @@ -170,6 +182,9 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: "", "# ===== charm-injected runner job hooks =====", f"mkdir -p {RUNNER_HOME}", + # Quoted delimiter: the hook file's own heredocs and $VARS must stay + # literal here and expand only when the runner executes the hook, + # once per job. f"cat > {PRE_JOB_HOOK_PATH} <<'{prejob_delim}'", hook_body, prejob_delim, @@ -183,6 +198,26 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: ) +def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: + """Render the contents of the pre-job hook file, ported from ``pre-job.j2``. + + Args: + config: The runner options to render. + otel_endpoint: The sanitised OTEL endpoint, or "" if unset. + + Returns: + The full contents of the hook script (no trailing newline). + """ + lines = ["#!/usr/bin/env bash", "set +e"] + if otel_endpoint: + lines.append("") + lines.append(_OTEL_COLLECTOR_SETUP.format(endpoint=otel_endpoint)) + if config.pre_job_script: + lines.append("") + lines.append(_render_custom_pre_job_script(config.pre_job_script)) + return "\n".join(lines) + + def _heredoc_delimiter(content: str, base: str) -> str: """Return a heredoc delimiter that does not appear as a line in *content*. @@ -200,27 +235,209 @@ def _heredoc_delimiter(content: str, base: str) -> str: 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. -# 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""" +# Ported verbatim from pre-job.j2 (the `{% if otel_collector_endpoint %}` block, +# lines 136-290): sets up the OpenTelemetry collector's hostmetrics/otlp/loki +# pipelines. `$GITHUB_*`, `$RUNNER_NAME` and `$(uname -m)` are left as literal +# shell syntax — they sit inside the *inner*, unquoted `tee < + set(resource.attributes["loki.resource.labels"], + Concat([resource.attributes["loki.resource.labels"], + ", service.name, github.repository, github.runner, github.workflow, github.job, \ +github.run.id, github.run.attempt, host.arch"], "")) + where resource.attributes["loki.resource.labels"] != nil + - > + set(resource.attributes["loki.resource.labels"], + "service.name, github.repository, github.runner, github.workflow, github.job, \ +github.run.id, github.run.attempt, host.arch") + where resource.attributes["loki.resource.labels"] == nil + batch: +exporters: + otlp/self_hosted_runner: + endpoint: {endpoint} + tls: + insecure: true +service: + pipelines: + metrics: + receivers: [hostmetrics] + processors: [attributes/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] + metrics/otlp: + receivers: [otlp] + processors: [attributes/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] + logs/otlp: + receivers: [otlp] + processors: + - resource/self_hosted_runner_github_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + logs/loki: + receivers: [loki] + processors: + - resource/self_hosted_runner_github_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + logs/aproxy: + receivers: [filelog/aproxy] + processors: + - resource/self_hosted_runner_github_labels + - resource/self_hosted_runner_github_aproxy_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + traces: + receivers: [otlp] + processors: [resource/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] +EOF + +/usr/bin/sudo /usr/bin/snap enable opentelemetry-collector +/usr/bin/sudo /usr/bin/snap start opentelemetry-collector""" + + +def _render_custom_pre_job_script(script: str) -> str: + """Render the custom pre-job script block, ported from ``pre-job.j2``. + + Writes the operator script to a temp file, runs it, and removes it — a + failure is logged but never aborts the job (``pre-job.j2`` lines 300-308). + + Args: + script: The operator-provided pre-job-script content, inserted verbatim + (mirrors the template's ``| safe`` filter). + + Returns: + A bash snippet. + """ + delim = _heredoc_delimiter(script, "GARM_CHARM_CUSTOM_PREJOB") + return "\n".join( + [ + f"cat > /tmp/custom_pre_job_script <<'{delim}'", + script, + delim, + "chmod +x /tmp/custom_pre_job_script", + 'logger -s "Running custom pre-job script"', + '/tmp/custom_pre_job_script || logger -s "Custom pre-job script failed, ' + 'continuing with the job"', + 'rm /tmp/custom_pre_job_script || logger -s "Failed to remove custom pre-job script"', + ] + ) def _render_aproxy(config: RunnerConfig) -> str: """Render aproxy install + nftables redirect rules. + Ported from ``openstack-userdata.sh.j2`` lines 13-38: aproxy listens on + ``:54969`` and an nftables DNAT ruleset (written to ``/etc/nftables.conf``) + redirects egress traffic to it, both for locally-initiated connections + (``output`` chain) and forwarded ones (``prerouting`` chain). + Args: config: The runner options (uses proxy, exclude addresses, redirect ports). @@ -232,26 +449,44 @@ def _render_aproxy(config: RunnerConfig) -> str: # 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 = "" excludes = _valid_ipv4_tokens(config.aproxy_exclude_addresses) - if excludes: - exclude_guard = f"ip daddr != {{ {', '.join(excludes)} }} " + exclude_elements = ", ".join(["127.0.0.0/8", *excludes]) + dnat_rule = ( + f"ip daddr != @exclude tcp dport {{ {nft_ports} }} counter dnat to \\$default-ipv4:54969" + ) - # 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", + "snap install aproxy --edge", + f"snap set aproxy proxy={shlex.quote(config.runner_http_proxy)} listen=:54969", + # Unquoted heredoc: `$(ip route ...)` must expand now, at boot, to + # compute the default gateway. `\$default-ipv4` stays escaped so the + # literal two characters `$default-ipv4` land in the file, which nft + # itself resolves against the `define` above when it loads the file. + "cat << EOF > /etc/nftables.conf", + "define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 " + "| grep -oP 'via \\K\\S+') | grep -oP 'src \\K\\S+')", + "table ip aproxy", + "flush table ip aproxy", "table ip aproxy {", - " chain output {", - " type nat hook output priority -100; policy accept;", - f" {exclude_guard}tcp dport {{ {nft_ports} }} counter redirect to :8443", - " }", + " set exclude {", + " type ipv4_addr;", + " flags interval; auto-merge;", + f" elements = {{ {exclude_elements} }}", + " }", + " chain prerouting {", + " type nat hook prerouting priority dstnat; policy accept;", + f" {dnat_rule}", + " }", + " chain output {", + " type nat hook output priority -100; policy accept;", + f" {dnat_rule}", + " }", "}", - "GARM_CHARM_NFT", + "EOF", + "systemctl enable nftables.service", + "nft -f /etc/nftables.conf", ] ) @@ -259,6 +494,8 @@ def _render_aproxy(config: RunnerConfig) -> str: def _render_dockerhub_mirror(mirror: str) -> str: """Render Docker daemon config pointing at the registry mirror. + Ported from ``openstack-userdata.sh.j2`` lines 77-81. + Args: mirror: The registry mirror URL. @@ -273,7 +510,8 @@ def _render_dockerhub_mirror(mirror: str) -> str: "cat > /etc/docker/daemon.json <<'GARM_CHARM_DOCKER'", daemon_json, "GARM_CHARM_DOCKER", - "systemctl restart docker >/dev/null 2>&1 || true", + "systemctl daemon-reload", + "systemctl restart docker", ] ) @@ -281,21 +519,18 @@ def _render_dockerhub_mirror(mirror: str) -> str: def _render_static_host_prep() -> str: """Render the always-on host-preparation steps ported from the old charm. + Ported from ``openstack-userdata.sh.j2`` lines 74-75 (``adduser ubuntu + lxd``/``adduser ubuntu adm``), targeting GARM's runner account instead. + Returns: - A bash snippet. Group membership is applied here; the remaining - production steps are stubbed pending a faithful port. + A bash snippet. Each command is guarded by ``|| true`` so it is a no-op + if the runner account doesn't exist yet. """ - # 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. 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", + f"adduser {RUNNER_USER} lxd || true", + f"adduser {RUNNER_USER} adm || true", ] ) diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index a6ef37c3..5c781797 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -42,11 +42,34 @@ def test_build_template_data_injects_all_options(): 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 "ACTION_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 + # aproxy: correct listen port, nftables file (not a piped `nft -f -`), and + # both the prerouting and output DNAT chains. + assert "listen=:54969" in result + assert "default-ipv4" in result + assert "/etc/nftables.conf" in result + assert "prerouting" in result + assert "127.0.0.0/8" in result + assert "counter dnat to \\$default-ipv4:54969" in result + + # docker mirror: both env vars and a full daemon-reload + restart. + assert "DOCKERHUB_MIRROR=https://mirror.example.com" in result + assert "CONTAINER_REGISTRY_URL=https://mirror.example.com" in result + assert "systemctl daemon-reload" in result + + # otel collector config is written to disk and enabled. + assert "/etc/otelcol/config.d/github.yaml" in result + assert "snap enable opentelemetry-collector" in result + + # custom pre-job script: temp-file / run / cleanup pattern. + assert "cat > /tmp/custom_pre_job_script <<" in result + assert "chmod +x /tmp/custom_pre_job_script" in result + assert "rm /tmp/custom_pre_job_script" 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. @@ -55,11 +78,12 @@ def test_build_template_data_empty_config_omits_optional_blocks(): assert "echo original-bootstrap" in result assert "ACTIONS_RUNNER_HOOK_JOB_STARTED=" in result - assert "usermod -aG lxd,adm ubuntu" in result + assert "adduser runner lxd || true" in result + assert "adduser runner adm || true" in result assert "snap install aproxy" not in result assert "registry-mirrors" not in result - assert "OTEL_EXPORTER_OTLP_ENDPOINT" not in result + assert "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT" not in result @pytest.mark.parametrize( @@ -79,7 +103,7 @@ def test_build_template_data_empty_config_omits_optional_blocks(): ), pytest.param( RunnerConfig(otel_collector_endpoint="http://o.test:4318"), - "OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318", + "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318", "snap install aproxy", id="otel-only", ), @@ -107,7 +131,7 @@ def test_aproxy_render_drops_malformed_tokens(): ) result = build_template_data(SAMPLE_BASE, config).decode() - assert "{ 80, 8000-9000 }" in result + assert "tcp dport { 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 @@ -130,7 +154,7 @@ 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 "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318MALICIOUS=1" in result assert "\nMALICIOUS=1" not in result diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 6f45e21c..1bafcb4a 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -865,7 +865,7 @@ def test_runner_options_render_into_scaleset_template( "registry-mirrors", "https://mirror.example.com", "http://proxy.example.com:3128", - "OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", + "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", "echo integration-marker", ) # Asserts each expected marker is present in the referenced template, retrying From db70f0c4e0519b4b7edd58f9bae1798c042e48c0 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 08:03:34 +0000 Subject: [PATCH 07/16] docs(garm): make runner_template comments self-contained Describe what each rendered block does directly, instead of citing the legacy github-runner charm templates and line numbers. The new charm should stand on its own; readers shouldn't need the legacy charm to understand this module. --- charms/garm/src/runner_template.py | 46 ++++++++++++------------------ 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 3536d560..fe26bc94 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -16,11 +16,6 @@ runner ``env`` file under ``/home/runner/actions-runner`` (GARM hardcodes the ``runner`` user and that path). -Both blocks are ported line-for-line from the production -``canonical/github-runner-operator`` templates (``openstack-userdata.sh.j2``, -``env.j2``, ``pre-job.j2``) so the runner behaves identically to the previous -machine-charm implementation. - 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. @@ -143,10 +138,9 @@ def render_pre_install(config: RunnerConfig) -> str: def render_pre_job_hooks(config: RunnerConfig) -> str: """Render the runner ``env`` file and GitHub job-start hook. - The hook file (``pre-job.j2`` in production) sets up the OpenTelemetry - collector, when configured, and appends the operator's ``pre-job-script``. - The docker mirror and OTEL endpoint are exported via the runner ``env`` file - (``env.j2`` in production), read once per job by the runner service. + The hook file sets up the OpenTelemetry collector, when configured, and runs + the operator's ``pre-job-script``. The docker mirror and OTEL endpoint are + exported via the runner ``env`` file, read once per job by the runner service. Args: config: The runner options to render. @@ -199,7 +193,7 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: - """Render the contents of the pre-job hook file, ported from ``pre-job.j2``. + """Render the contents of the pre-job hook file (the GitHub job-start hook). Args: config: The runner options to render. @@ -235,12 +229,11 @@ def _heredoc_delimiter(content: str, base: str) -> str: return delimiter -# Ported verbatim from pre-job.j2 (the `{% if otel_collector_endpoint %}` block, -# lines 136-290): sets up the OpenTelemetry collector's hostmetrics/otlp/loki -# pipelines. `$GITHUB_*`, `$RUNNER_NAME` and `$(uname -m)` are left as literal -# shell syntax — they sit inside the *inner*, unquoted `tee < str: def _render_custom_pre_job_script(script: str) -> str: - """Render the custom pre-job script block, ported from ``pre-job.j2``. + """Render the custom pre-job script block. Writes the operator script to a temp file, runs it, and removes it — a - failure is logged but never aborts the job (``pre-job.j2`` lines 300-308). + failure is logged but never aborts the job. Args: script: The operator-provided pre-job-script content, inserted verbatim @@ -433,10 +426,10 @@ def _render_custom_pre_job_script(script: str) -> str: def _render_aproxy(config: RunnerConfig) -> str: """Render aproxy install + nftables redirect rules. - Ported from ``openstack-userdata.sh.j2`` lines 13-38: aproxy listens on - ``:54969`` and an nftables DNAT ruleset (written to ``/etc/nftables.conf``) - redirects egress traffic to it, both for locally-initiated connections - (``output`` chain) and forwarded ones (``prerouting`` chain). + aproxy listens on ``:54969`` and an nftables DNAT ruleset (written to + ``/etc/nftables.conf``) redirects egress traffic to it, both for + locally-initiated connections (``output`` chain) and forwarded ones + (``prerouting`` chain). Args: config: The runner options (uses proxy, exclude addresses, redirect ports). @@ -494,8 +487,6 @@ def _render_aproxy(config: RunnerConfig) -> str: def _render_dockerhub_mirror(mirror: str) -> str: """Render Docker daemon config pointing at the registry mirror. - Ported from ``openstack-userdata.sh.j2`` lines 77-81. - Args: mirror: The registry mirror URL. @@ -517,10 +508,9 @@ def _render_dockerhub_mirror(mirror: str) -> str: def _render_static_host_prep() -> str: - """Render the always-on host-preparation steps ported from the old charm. + """Render the always-on host-preparation steps. - Ported from ``openstack-userdata.sh.j2`` lines 74-75 (``adduser ubuntu - lxd``/``adduser ubuntu adm``), targeting GARM's runner account instead. + Adds the runner account to the ``lxd`` and ``adm`` groups. Returns: A bash snippet. Each command is guarded by ``|| true`` so it is a no-op @@ -528,7 +518,7 @@ def _render_static_host_prep() -> str: """ return "\n".join( [ - "# Static runner host preparation (ported from the github-runner charm).", + "# Static runner host preparation.", f"adduser {RUNNER_USER} lxd || true", f"adduser {RUNNER_USER} adm || true", ] From 36aa6b55bf34c7c526539d80121e3cd0ecc39ec0 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 08:03:34 +0000 Subject: [PATCH 08/16] test(garm): cover all six runner options in integration test Add aproxy-exclude-addresses (previously untested) and tighten the asserted markers so each config option has a distinct, specific marker in the rendered scaleset template (docker env var, aproxy listener + nft ruleset, otel env var, pre-job hook). --- charms/tests/integration/test_garm.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 1bafcb4a..ddee831d 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -849,6 +849,7 @@ def test_runner_options_render_into_scaleset_template( values={ "dockerhub-mirror": "https://mirror.example.com", "runner-http-proxy": "http://proxy.example.com:3128", + "aproxy-exclude-addresses": "192.168.0.0/16", "aproxy-redirect-ports": "80,443", "otel-collector-endpoint": "http://otel.example.com:4318", "pre-job-script": "echo integration-marker", @@ -861,10 +862,16 @@ def test_runner_options_render_into_scaleset_template( delay=10, ) + # One marker per config option, proving each reaches GARM via live reconcile: + # dockerhub-mirror (daemon.json + env var), runner-http-proxy + aproxy-* + # (aproxy listener and nftables ruleset), otel-collector-endpoint (env var), + # and pre-job-script (job-start hook). expected_markers = ( "registry-mirrors", - "https://mirror.example.com", - "http://proxy.example.com:3128", + "DOCKERHUB_MIRROR=https://mirror.example.com", + "proxy=http://proxy.example.com:3128 listen=:54969", + "192.168.0.0/16", + "tcp dport { 80, 443 }", "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT=http://otel.example.com:4318", "echo integration-marker", ) From 8549252d3e82186bbea6c453ffec2d6b685a7eb8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 08:59:28 +0000 Subject: [PATCH 09/16] refactor(garm): move RunnerConfig into charm_state.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RunnerConfig is relation-derived desired state, so per ISD014 (managing charm complexity) it belongs in charm_state.py alongside CharmState and SSHDebugInfo — not in the rendering module. runner_template.py now imports it and only renders it; charm_state.py stays free of rendering/reconciler imports to avoid a cycle. Also hoist the large _OTEL_COLLECTOR_SETUP constant to the top of runner_template.py with the other module constants, and document the value-object placement rule in the garm AGENTS.md. --- charms/garm/AGENTS.md | 8 +- charms/garm/src/charm.py | 2 +- charms/garm/src/charm_state.py | 58 +++ charms/garm/src/runner_template.py | 331 ++++++++---------- charms/garm/src/scaleset_reconciler.py | 3 +- .../garm/tests/unit/test_runner_template.py | 2 +- .../tests/unit/test_scaleset_reconciler.py | 6 +- 7 files changed, 209 insertions(+), 201 deletions(-) diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 1f5d9f9b..14a64118 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -9,12 +9,18 @@ charm conventions; this file lists only what's specific to `garm`. - **Secrets (owner).** `garm` owns two labelled juju secrets — `GARM_SECRETS_LABEL` and `GARM_ADMIN_CREDENTIALS_LABEL` — created leader-only in `_ensure_secrets`. - **DO** read them with plain `get_content()` (`_get_secrets`, `_get_admin_credentials`). - **DON'T** pass `refresh=True` — that's an observer concept (see root `AGENTS.md`). -- **Domain logic is factored out of `charm.py`**: `src/garm_api.py` and `src/garm_client/` - **Domain logic is factored out of `charm.py`**: `src/garm_api.py` and `src/garm_client/` (the GARM HTTP client), the per-resource reconcilers (`src/github_reconciler.py`, `src/entity_reconciler.py`, `src/scaleset_reconciler.py`), and relation-derived desired state (`src/charm_state.py` — `CharmState.from_charm`). Extend these rather than growing the charm class. TOML rendering: `render_garm_toml()` in `src/charm.py`. + - **Relation-/config-derived value objects belong in `src/charm_state.py`** (per + [ISD014](https://discourse.charmhub.io/t/specification-isd014-managing-charm-complexity/11619)): + keep desired-state types parsed from relation or config data (`CharmState`, `SSHDebugInfo`, + `RunnerConfig`) there. **DON'T** define them in rendering/helper modules — e.g. + `runner_template.py` only *renders* a `RunnerConfig`, it doesn't own it. Rendering and + reconciler modules import these types; **DO** keep `charm_state.py` free of + rendering/reconciler imports so it can't form an import cycle. - **`src/garm_client/` is generated** by `scripts/generate_client.sh` (openapi-generator) — **DON'T** hand-edit it. The script **patches the GARM swagger spec** before generating so the template `data` field (Go `[]byte`, base64 string on the wire) becomes `StrictStr` diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 7c2e2220..4e26a669 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -24,6 +24,7 @@ DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME, CharmState, + RunnerConfig, credential_name, ) from entity_reconciler import EntityReconciler @@ -36,7 +37,6 @@ CredentialSpec, GithubReconciler, ) -from runner_template import RunnerConfig from scaleset_reconciler import ScalesetReconciler, ScalesetSpec logger = logging.getLogger(__name__) diff --git a/charms/garm/src/charm_state.py b/charms/garm/src/charm_state.py index d777e702..6e594c13 100644 --- a/charms/garm/src/charm_state.py +++ b/charms/garm/src/charm_state.py @@ -13,6 +13,7 @@ import dataclasses import logging import typing +from collections.abc import Mapping import ops @@ -41,6 +42,63 @@ class SSHDebugInfo: ed25519_fingerprint: str +# Databag keys written by the garm-configurator charm — the single source of +# truth for the configurator→garm relation contract for runner options. +RUNNER_OPTION_DATABAG_KEYS: typing.Final = ( + "dockerhub_mirror", + "runner_http_proxy", + "aproxy_exclude_addresses", + "aproxy_redirect_ports", + "otel_collector_endpoint", + "pre_job_script", +) + + +@dataclasses.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 consuming them here (and in + the template renderer) 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 RUNNER_OPTION_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 RUNNER_OPTION_DATABAG_KEYS) + + def credential_name(app_id: int, installation_id: int) -> str: """Return the GARM credential name for a GitHub App installation. diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index fe26bc94..6e495050 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -25,8 +25,8 @@ import json import re import shlex -from collections.abc import Mapping -from dataclasses import dataclass + +from charm_state import RunnerConfig _PORT_TOKEN_RE = re.compile(r"^\d{1,5}(-\d{1,5})?$") @@ -37,198 +37,6 @@ 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 file sets up the OpenTelemetry collector, when configured, and runs - the operator's ``pre-job-script``. The docker mirror and OTEL endpoint are - exported via the runner ``env`` file, read once per job by the runner service. - - Args: - config: The runner options to render. - - Returns: - A bash snippet, terminated by a newline. - """ - otel_endpoint = "" - if config.otel_collector_endpoint: - # Strip CR/LF so a databag value can't inject extra env entries or extra - # lines into the otelcol config (the databag is a trust boundary, - # regardless of configurator validation). - otel_endpoint = config.otel_collector_endpoint.replace("\r", "").replace("\n", "") - - hook_body = _render_pre_job_hook_body(config, otel_endpoint) - - env_entries = [] - if config.dockerhub_mirror: - env_entries.append(f"DOCKERHUB_MIRROR={config.dockerhub_mirror}") - env_entries.append(f"CONTAINER_REGISTRY_URL={config.dockerhub_mirror}") - env_entries.append(f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}") - if otel_endpoint: - env_entries.append(f"ACTION_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}", - # Quoted delimiter: the hook file's own heredocs and $VARS must stay - # literal here and expand only when the runner executes the hook, - # once per job. - f"cat > {PRE_JOB_HOOK_PATH} <<'{prejob_delim}'", - hook_body, - prejob_delim, - f"chmod 0755 {PRE_JOB_HOOK_PATH}", - f"cat >> {RUNNER_ENV_PATH} <<'{env_delim}'", - *env_entries, - env_delim, - f"chown -R {RUNNER_USER}:{RUNNER_USER} {RUNNER_HOME} 2>/dev/null || true", - "# ===== end charm-injected runner job hooks =====\n", - ] - ) - - -def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: - """Render the contents of the pre-job hook file (the GitHub job-start hook). - - Args: - config: The runner options to render. - otel_endpoint: The sanitised OTEL endpoint, or "" if unset. - - Returns: - The full contents of the hook script (no trailing newline). - """ - lines = ["#!/usr/bin/env bash", "set +e"] - if otel_endpoint: - lines.append("") - lines.append(_OTEL_COLLECTOR_SETUP.format(endpoint=otel_endpoint)) - if config.pre_job_script: - lines.append("") - lines.append(_render_custom_pre_job_script(config.pre_job_script)) - return "\n".join(lines) - - -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 - - # OpenTelemetry collector config: hostmetrics/otlp/loki pipelines exporting to # the configured endpoint. `$GITHUB_*`, `$RUNNER_NAME` and `$(uname -m)` are left # as literal shell syntax — they sit inside the *inner*, unquoted `tee < str: /usr/bin/sudo /usr/bin/snap start opentelemetry-collector""" +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 file sets up the OpenTelemetry collector, when configured, and runs + the operator's ``pre-job-script``. The docker mirror and OTEL endpoint are + exported via the runner ``env`` file, read once per job by the runner service. + + Args: + config: The runner options to render. + + Returns: + A bash snippet, terminated by a newline. + """ + otel_endpoint = "" + if config.otel_collector_endpoint: + # Strip CR/LF so a databag value can't inject extra env entries or extra + # lines into the otelcol config (the databag is a trust boundary, + # regardless of configurator validation). + otel_endpoint = config.otel_collector_endpoint.replace("\r", "").replace("\n", "") + + hook_body = _render_pre_job_hook_body(config, otel_endpoint) + + env_entries = [] + if config.dockerhub_mirror: + env_entries.append(f"DOCKERHUB_MIRROR={config.dockerhub_mirror}") + env_entries.append(f"CONTAINER_REGISTRY_URL={config.dockerhub_mirror}") + env_entries.append(f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}") + if otel_endpoint: + env_entries.append(f"ACTION_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}", + # Quoted delimiter: the hook file's own heredocs and $VARS must stay + # literal here and expand only when the runner executes the hook, + # once per job. + f"cat > {PRE_JOB_HOOK_PATH} <<'{prejob_delim}'", + hook_body, + prejob_delim, + f"chmod 0755 {PRE_JOB_HOOK_PATH}", + f"cat >> {RUNNER_ENV_PATH} <<'{env_delim}'", + *env_entries, + env_delim, + f"chown -R {RUNNER_USER}:{RUNNER_USER} {RUNNER_HOME} 2>/dev/null || true", + "# ===== end charm-injected runner job hooks =====\n", + ] + ) + + +def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: + """Render the contents of the pre-job hook file (the GitHub job-start hook). + + Args: + config: The runner options to render. + otel_endpoint: The sanitised OTEL endpoint, or "" if unset. + + Returns: + The full contents of the hook script (no trailing newline). + """ + lines = ["#!/usr/bin/env bash", "set +e"] + if otel_endpoint: + lines.append("") + lines.append(_OTEL_COLLECTOR_SETUP.format(endpoint=otel_endpoint)) + if config.pre_job_script: + lines.append("") + lines.append(_render_custom_pre_job_script(config.pre_job_script)) + return "\n".join(lines) + + +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 + + def _render_custom_pre_job_script(script: str) -> str: """Render the custom pre-job script block. diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index a655b47a..3d318cd1 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -7,12 +7,13 @@ import logging from dataclasses import dataclass, field +from charm_state import RunnerConfig from garm_api import GarmApiError, GarmAuthenticatedClient from garm_client.models.create_scale_set_params import CreateScaleSetParams from garm_client.models.scale_set import ScaleSet from garm_client.models.template import Template from garm_client.models.update_scale_set_params import UpdateScaleSetParams -from runner_template import RunnerConfig, build_template_data +from runner_template import build_template_data logger = logging.getLogger(__name__) diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index 5c781797..630581e1 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -5,9 +5,9 @@ import pytest +from charm_state import RunnerConfig from runner_template import ( RUNNER_ENV_PATH, - RunnerConfig, build_template_data, ) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 67c11424..e0f61b1d 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -135,7 +135,7 @@ def _spec( template_id=None, runner_config=None, ): - from runner_template import RunnerConfig + from charm_state import RunnerConfig return ScalesetSpec( name=name, @@ -412,7 +412,7 @@ def delete_template(self, template_id): def _spec_with_runner_config(**rc_kwargs): """Build a spec with runner_config populated from the given kwargs.""" - from runner_template import RunnerConfig + from charm_state import RunnerConfig return _spec(runner_config=RunnerConfig(**rc_kwargs)) @@ -472,7 +472,7 @@ def test_template_updated_when_runner_config_changes(): assert: The custom template is updated (not recreated); the scaleset template_id is unchanged. """ from runner_template import build_template_data - from runner_template import RunnerConfig + from charm_state import RunnerConfig old_config = RunnerConfig(dockerhub_mirror="https://old.example.com") custom_template = _FakeTemplate( From 1c7e8c3699f0e61a5a3317e747112dda69b8ed19 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 6 Jul 2026 09:16:53 +0000 Subject: [PATCH 10/16] Align garm test docs with AAA style --- charms/garm-configurator/charmcraft.yaml | 10 +- charms/garm-configurator/src/charm_state.py | 30 ++-- .../tests/unit/test_charm.py | 10 +- charms/garm/src/runner_template.py | 28 ++-- charms/garm/src/scaleset_reconciler.py | 49 ++++-- .../garm/tests/unit/test_runner_template.py | 61 ++++++-- .../tests/unit/test_scaleset_reconciler.py | 140 ++++++++++++++++-- 7 files changed, 258 insertions(+), 70 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index ac954e21..5bd30783 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -116,23 +116,23 @@ config: runner-http-proxy: type: string description: | - HTTP proxy that aproxy forwards to. Must be a valid http(s) URL. + Optional HTTP proxy that aproxy forwards to. Must be a valid http(s) URL. aproxy-exclude-addresses: type: string description: | - Comma-separated IPv4 addresses/CIDRs to exclude from aproxy forwarding. + Optional comma-separated IPv4 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. + Optional 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 http(s) URL. + Optional OTEL exporter address for the otel-collector to connect to. Must be a valid http(s) URL. pre-job-script: type: string description: | - A bash script appended to the end of the runner pre-job script. + Optional bash script appended to the end of the runner pre-job script. requires: image: diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 91a62e46..b189331a 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -3,11 +3,17 @@ """State of the GARM configurator charm.""" -import ipaddress -import urllib.parse - import ops -from pydantic import BaseModel, ValidationError, ValidationInfo, field_validator, model_validator +from pydantic import ( + BaseModel, + HttpUrl, + IPvAnyNetwork, + TypeAdapter, + ValidationError, + ValidationInfo, + field_validator, + model_validator, +) OPENSTACK_AUTH_URL_CONFIG_NAME = "openstack-auth-url" OPENSTACK_USERNAME_CONFIG_NAME = "openstack-username" @@ -40,6 +46,9 @@ OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME = "otel-collector-endpoint" PRE_JOB_SCRIPT_CONFIG_NAME = "pre-job-script" +_HTTP_URL_ADAPTER = TypeAdapter(HttpUrl) +_IP_NETWORK_ADAPTER = TypeAdapter(IPvAnyNetwork) + IMAGE_RELATION_NAME = "image" GARM_RELATION_NAME = "garm-configurator" @@ -348,14 +357,11 @@ def _validate_http_url(cls, value: str | None, info: ValidationInfo) -> str | No # where a newline could inject extra lines. if any(char.isspace() for char in value): raise ValueError(msg) - parsed = urllib.parse.urlparse(value) try: - # .port raises ValueError on a non-numeric / out-of-range port that - # urlparse otherwise accepts silently. - port = parsed.port - except ValueError as exc: + parsed = _HTTP_URL_ADAPTER.validate_python(value) + except ValidationError as exc: raise ValueError(msg) from exc - if parsed.scheme not in ("http", "https") or not parsed.hostname or port == 0: + if parsed.port == 0: raise ValueError(msg) return value @@ -370,8 +376,8 @@ def _validate_ipv4_list(cls, value: str | None) -> str | None: # The values are rendered into an nft IPv4 (table ip) ruleset, so a # hostname or IPv6 address would validate here but fail at runtime. try: - network = ipaddress.ip_network(token, strict=False) - except ValueError as exc: + network = _IP_NETWORK_ADAPTER.validate_python(token) + except ValidationError as exc: raise ValueError( f"{APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} must be a comma-separated list " f"of IPv4 addresses or CIDRs; got invalid token: {token!r}" diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index dc7c85e6..d719c895 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -632,12 +632,6 @@ def test_reconcile_writes_optional_scaleset_fields_to_garm_relation(): ) assert "repo" not in garm_out.local_unit_data - -# --------------------------------------------------------------------------- -# Runner config — invalid value tests -# --------------------------------------------------------------------------- - - @pytest.mark.parametrize( "config_key, bad_value, expected_fragment", [ @@ -777,9 +771,7 @@ 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. + assert: The six runner config keys are absent from the databag. """ ctx = Context(GarmConfiguratorCharm) secret = _make_secret() diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 6e495050..63ab2dad 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -3,7 +3,7 @@ """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 +GARM 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 @@ -44,7 +44,7 @@ # Only the exporter endpoint is substituted at render time. _OTEL_COLLECTOR_SETUP = """\ /usr/bin/logger -s "OpenTelemetry collector is enabled." -/usr/bin/logger -s "Additional OpenTelemetery collector configuration can be added." +/usr/bin/logger -s "Additional OpenTelemetry collector configuration can be added." /usr/bin/logger -s "The exporter endpoint is at the environment variable \ ACTION_OTEL_EXPORTER_OTLP_ENDPOINT." /usr/bin/sudo /usr/bin/mkdir -p /etc/otelcol/config.d @@ -257,19 +257,15 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: Returns: A bash snippet, terminated by a newline. """ - otel_endpoint = "" - if config.otel_collector_endpoint: - # Strip CR/LF so a databag value can't inject extra env entries or extra - # lines into the otelcol config (the databag is a trust boundary, - # regardless of configurator validation). - otel_endpoint = config.otel_collector_endpoint.replace("\r", "").replace("\n", "") + otel_endpoint = _flatten_env_value(config.otel_collector_endpoint) + dockerhub_mirror = _flatten_env_value(config.dockerhub_mirror) hook_body = _render_pre_job_hook_body(config, otel_endpoint) env_entries = [] - if config.dockerhub_mirror: - env_entries.append(f"DOCKERHUB_MIRROR={config.dockerhub_mirror}") - env_entries.append(f"CONTAINER_REGISTRY_URL={config.dockerhub_mirror}") + if dockerhub_mirror: + env_entries.append(f"DOCKERHUB_MIRROR={dockerhub_mirror}") + env_entries.append(f"CONTAINER_REGISTRY_URL={dockerhub_mirror}") env_entries.append(f"ACTIONS_RUNNER_HOOK_JOB_STARTED={PRE_JOB_HOOK_PATH}") if otel_endpoint: env_entries.append(f"ACTION_OTEL_EXPORTER_OTLP_ENDPOINT={otel_endpoint}") @@ -321,6 +317,13 @@ def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: return "\n".join(lines) +def _flatten_env_value(value: str | None) -> str: + """Return a value safe for a single-line env file entry.""" + if value is None: + return "" + return value.replace("\r", "").replace("\n", "") + + def _heredoc_delimiter(content: str, base: str) -> str: """Return a heredoc delimiter that does not appear as a line in *content*. @@ -400,11 +403,10 @@ def _render_aproxy(config: RunnerConfig) -> str: # compute the default gateway. `\$default-ipv4` stays escaped so the # literal two characters `$default-ipv4` land in the file, which nft # itself resolves against the `define` above when it loads the file. + "nft delete table ip aproxy 2>/dev/null || true", "cat << EOF > /etc/nftables.conf", "define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 " "| grep -oP 'via \\K\\S+') | grep -oP 'src \\K\\S+')", - "table ip aproxy", - "flush table ip aproxy", "table ip aproxy {", " set exclude {", " type ipv4_addr;", diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 3d318cd1..d03509b8 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -4,6 +4,7 @@ """Scaleset reconciler: diffs desired vs observed GARM scalesets and applies changes.""" +import base64 import logging from dataclasses import dataclass, field @@ -67,20 +68,23 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: desired: The full desired set of scalesets. """ providers = {provider.name for provider in self._client.list_providers()} - observed = {(scaleset.name or ""): scaleset for scaleset in self._client.list_scalesets()} + observed: dict[str, ScaleSet] = {} + for scaleset in self._client.list_scalesets(): + if not scaleset.name: + logger.warning("Skipping observed scaleset with missing name (id=%s)", scaleset.id) + continue + observed[scaleset.name] = scaleset # 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. + # detach it); skip the API call entirely otherwise. templates: dict[str, Template] = {} if any(spec.runner_config.has_config() for spec in desired) or any( scaleset.template_id for scaleset in observed.values() ): templates = { (template.name or ""): template - for template in self._client.list_templates(partial_name=SYSTEM_TEMPLATE_NAME) + for template in self._client.list_templates() if template.name } @@ -213,10 +217,17 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - new_data = build_template_data(self._template_bytes(base), spec.runner_config) if existing is not None: + if existing.id is None: + logger.warning( + "Runner template %s has no id; scaleset %s will use the default template", + custom_name, + spec.name, + ) + return 0 if self._template_bytes(existing) != new_data: logger.info("Updating runner template %s", custom_name) - self._client.update_template(existing.id or 0, data=new_data) - return existing.id or 0 + 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( @@ -243,13 +254,17 @@ def _template_bytes(self, template: Template) -> bytes: if template.id is None: return b"" fetched = self._client.get_template(template.id) - data = getattr(fetched, "data", None) or [] - # Cache it back so repeated lookups don't re-fetch. - setattr(template, "data", data) + data = getattr(fetched, "data", None) + # Cache it back so repeated lookups don't re-fetch, but only when + # the generated model can accept the assigned value. + if isinstance(data, str): + setattr(template, "data", data) + if not data: + return b"" if isinstance(data, bytes): return data if isinstance(data, str): - return data.encode("utf-8") + return base64.b64decode(data) return bytes(data) def _template_by_id( @@ -273,8 +288,18 @@ def _delete_custom_template(self, scaleset_name: str, templates: dict[str, Templ custom_name = f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}" custom = templates.get(custom_name) if custom is not None: + if custom.id is None: + logger.warning("Skipping delete for runner template %s: missing id", custom_name) + return logger.info("Deleting orphaned runner template %s", custom.name or custom_name) - self._client.delete_template(custom.id or 0) + try: + self._client.delete_template(custom.id) + except GarmApiError as exc: + logger.warning( + "Could not delete runner template %s (will retry on next reconcile): %s", + custom.name or custom_name, + exc, + ) @staticmethod def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index 630581e1..b0718660 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -25,9 +25,12 @@ def _full_config() -> RunnerConfig: ) -# 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(): + """ + arrange: A bootstrap script and a runner config with every optional field set. + act: Build the runner template data. + assert: The rendered template preserves the bootstrap and injects every block. + """ result = build_template_data(SAMPLE_BASE, _full_config()).decode() lines = result.splitlines() @@ -52,6 +55,8 @@ def test_build_template_data_injects_all_options(): assert "listen=:54969" in result assert "default-ipv4" in result assert "/etc/nftables.conf" in result + assert "table ip aproxy\nflush table ip aproxy" not in result + assert "nft delete table ip aproxy 2>/dev/null || true" in result assert "prerouting" in result assert "127.0.0.0/8" in result assert "counter dnat to \\$default-ipv4:54969" in result @@ -71,9 +76,12 @@ def test_build_template_data_injects_all_options(): assert "rm /tmp/custom_pre_job_script" 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(): + """ + arrange: A bootstrap script and an empty runner config. + act: Build the runner template data. + assert: The static setup remains and all optional blocks are omitted. + """ result = build_template_data(SAMPLE_BASE, RunnerConfig()).decode() assert "echo original-bootstrap" in result @@ -116,14 +124,22 @@ def test_build_template_data_empty_config_omits_optional_blocks(): ], ) def test_build_template_data_per_option(config, present, absent): + """ + arrange: A bootstrap script and a runner config with exactly one optional field set. + act: Build the runner template data. + assert: The selected block is present and an unrelated optional block is absent. + """ result = build_template_data(SAMPLE_BASE, config).decode() assert present in result 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(): + """ + arrange: A runner config with a proxy plus malformed ports and exclude addresses. + act: Build the runner template data. + assert: Only valid IPv4-oriented aproxy rules are rendered. + """ config = RunnerConfig( runner_http_proxy="http://p.test:3128", aproxy_redirect_ports="80,not-a-port,8000-9000,99 rm,99999,443-80", @@ -140,8 +156,12 @@ 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(): + """ + arrange: A pre-job script containing the default heredoc delimiter token. + act: Build the runner template data. + assert: The renderer chooses a non-colliding heredoc delimiter. + """ config = RunnerConfig(pre_job_script="echo hi\nGARM_CHARM_PREJOB\necho bye") result = build_template_data(SAMPLE_BASE, config).decode() @@ -149,8 +169,12 @@ def test_heredoc_delimiter_avoids_collision_with_pre_job_script(): 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(): + """ + arrange: An OTEL endpoint containing a newline and extra assignment text. + act: Build the runner template data. + assert: The endpoint is flattened into one env-file line without line injection. + """ config = RunnerConfig(otel_collector_endpoint="http://o.test:4318\nMALICIOUS=1") result = build_template_data(SAMPLE_BASE, config).decode() @@ -158,9 +182,26 @@ def test_otel_endpoint_newline_stripped(): 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_dockerhub_mirror_newline_stripped(): + """ + arrange: A Docker mirror URL containing a newline and extra assignment text. + act: Build the runner template data. + assert: The mirror value is flattened so it cannot inject extra env-file lines. + """ + config = RunnerConfig(dockerhub_mirror="https://m.test\nMALICIOUS=1") + result = build_template_data(SAMPLE_BASE, config).decode() + + assert "DOCKERHUB_MIRROR=https://m.testMALICIOUS=1" in result + assert "CONTAINER_REGISTRY_URL=https://m.testMALICIOUS=1" in result + assert "\nMALICIOUS=1" not in result + + def test_runner_config_from_databag_and_has_config(): + """ + arrange: Empty and populated runner-config databags. + act: Build RunnerConfig instances from the databags and inspect has_config. + assert: Known keys are mapped, unknown keys are ignored, and has_config reflects content. + """ empty = RunnerConfig.from_databag({}) assert empty == RunnerConfig() assert not empty.has_config() diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index e0f61b1d..f26916d5 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -3,8 +3,11 @@ """Unit tests for the scaleset reconciler.""" +import base64 + import pytest +from garm_client.models.template import Template from scaleset_reconciler import ScalesetReconciler, ScalesetSpec @@ -317,6 +320,21 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +def test_unnamed_observed_scaleset_is_skipped(): + """ + arrange: FakeGarmClient with one observed scaleset lacking a name. + act: Reconcile an unrelated desired spec. + assert: The unnamed observed scaleset is ignored rather than deleted under an empty-name key. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name=None, id=42)], + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted == [] + + @pytest.mark.parametrize( "providers, scalesets, desired, expected_deleted", [ @@ -351,12 +369,6 @@ def test_pre_install_scripts_passed_in_create(): _, _, params = client.created[0] assert params.extra_specs == {"pre_install_scripts": scripts} - -# --------------------------------------------------------------------------- -# Runner template lifecycle tests -# --------------------------------------------------------------------------- - - class _FakeTemplate: """Minimal fake for the GARM Template model used in template lifecycle tests.""" @@ -407,6 +419,15 @@ def delete_template(self, template_id): self.deleted_templates.append(template_id) +class _DeleteFailingTemplateClient(_TemplateTrackingClient): + """TemplateTrackingClient variant whose delete_template raises a GARM API error.""" + + def delete_template(self, template_id): + from garm_api import GarmApiError + + raise GarmApiError(f"delete failed for template {template_id}") + + _SYSTEM_TEMPLATE = _FakeTemplate("github_linux", tid=1, data=b"#!/bin/bash\nset -e\necho base\n") @@ -445,14 +466,20 @@ def test_template_created_when_runner_config_set(): def test_template_created_when_garm_returns_template_data_as_string(): """ - arrange: The system template data is returned as a string by the GARM API. + arrange: The system template data is returned as a base64 string by the GARM API. act: Reconcile a spec with runner options. - assert: A custom template is created from the string data without hook errors. + assert: A custom template is created from the decoded script data without hook errors. """ client = _TemplateTrackingClient( providers=["openstack-demo"], scalesets=[], - templates=[_FakeTemplate("github_linux", tid=1, data="#!/bin/bash\nset -e\necho base\n")], + templates=[ + _FakeTemplate( + "github_linux", + tid=1, + data=base64.b64encode(b"#!/bin/bash\nset -e\necho base\n").decode(), + ) + ], ) _reconcile( client, @@ -463,6 +490,43 @@ def test_template_created_when_garm_returns_template_data_as_string(): _, data, _ = client.created_templates[0] assert isinstance(data, bytes) assert b"registry-mirrors" in data + assert b"#!/bin/bash" in data + + +def test_template_created_from_charmed_template_when_scaleset_references_it(): + """ + arrange: A scaleset references the charm-managed template and runner options are set. + act: Reconcile the desired spec. + assert: The custom template is derived from the charmed template rather than the bare system one. + """ + from charm_state import RunnerConfig + + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[], + templates=[ + _SYSTEM_TEMPLATE, + _FakeTemplate( + "github_linux_charmed", + tid=7, + data=base64.b64encode(b"#!/bin/bash\nset -e\necho charmed-base\n").decode(), + ), + ], + ) + _reconcile( + client, + [ + _spec( + template_id=7, + runner_config=RunnerConfig(dockerhub_mirror="https://mirror.example.com"), + ) + ], + ) + + assert len(client.created_templates) == 1 + _, data, _ = client.created_templates[0] + assert b"echo charmed-base" in data + assert b"echo base" not in data def test_template_updated_when_runner_config_changes(): @@ -542,3 +606,61 @@ def test_template_kept_when_system_template_missing(): assert client.created_templates == [] assert client.updated_templates == [] assert client.deleted_templates == [] + + +def test_template_with_missing_id_is_not_updated(): + """ + arrange: A custom template exists without an id and its rendered content differs. + act: Reconcile a spec with runner options. + assert: The reconciler skips the update and falls back to the default template id. + """ + client = _TemplateTrackingClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(template_id=2)], + templates=[ + _SYSTEM_TEMPLATE, + _FakeTemplate("github_linux-my-scaleset", tid=None, data=b"stale"), + ], + ) + _reconcile( + client, + [_spec_with_runner_config(dockerhub_mirror="https://mirror.example.com")], + ) + + assert client.updated_templates == [] + assert len(client.updated) == 1 + _, params = client.updated[0] + assert params.template_id == 0 + + +def test_template_bytes_does_not_cache_invalid_placeholder_on_generated_model(): + """ + arrange: A generated Template model whose list response omits data and whose fetched body is still missing. + act: Read template bytes through the reconciler helper. + assert: Empty bytes are returned and the Template model is not assigned a non-string placeholder. + """ + client = _TemplateTrackingClient() + reconciler = ScalesetReconciler(client) + listed = Template(id=5, name="github_linux") + client._templates = [listed] + + assert reconciler._template_bytes(listed) == b"" + assert listed.data is None + + +def test_delete_custom_template_skips_missing_id_and_api_errors(): + """ + arrange: One custom template has no id and another raises on delete. + act: Reconcile removal of the associated scalesets. + assert: Missing-id templates are skipped and delete errors do not abort reconcile. + """ + missing_id_client = _TemplateTrackingClient( + templates=[_FakeTemplate("github_linux-stale", tid=None, data=b"x")], + ) + _reconcile(missing_id_client, []) + assert missing_id_client.deleted_templates == [] + + failing_client = _DeleteFailingTemplateClient( + templates=[_FakeTemplate("github_linux-stale", tid=7, data=b"x")], + ) + _reconcile(failing_client, []) From ed85f8f150aa0b3819bb9bf8ed1c321b96b55ad4 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 07:03:47 +0000 Subject: [PATCH 11/16] refactor(garm): extract reconcile helpers for the complexity gate Split ScalesetReconciler.reconcile into _load_templates (the conditional template fetch) and _reconcile_one (the per-spec body), dropping its cyclomatic complexity from 12 to 6. Pure extraction, no behaviour change. --- charms/garm/src/scaleset_reconciler.py | 107 +++++++++++------- .../tests/unit/test_scaleset_reconciler.py | 11 +- 2 files changed, 77 insertions(+), 41 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index d03509b8..58734e7d 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -75,6 +75,29 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: continue observed[scaleset.name] = scaleset + templates = self._load_templates(desired, observed) + all_desired_names: set[str] = {spec.name for spec in desired} + + for spec in desired: + self._reconcile_one(spec, providers, observed, templates) + + for name, scaleset in observed.items(): + if name not in all_desired_names: + self._delete_orphaned(scaleset) + self._delete_custom_template(name, templates) + + def _load_templates( + self, desired: list[ScalesetSpec], observed: dict[str, ScaleSet] + ) -> dict[str, Template]: + """Fetch observed templates keyed by name, only when a reconcile pass needs them. + + Args: + desired: The full desired set of scalesets. + observed: Observed scalesets keyed by name. + + Returns: + Observed templates keyed by name, or an empty dict when none are needed. + """ # 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. @@ -87,51 +110,59 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: for template in self._client.list_templates() if template.name } + return templates - all_desired_names: set[str] = {spec.name for spec in desired} - - for spec in desired: - try: - create_params = self._to_create_params(spec) - except Exception as exc: - logger.warning("Skipping scaleset %s: spec validation failed: %s", spec.name, exc) - continue + def _reconcile_one( + self, + spec: ScalesetSpec, + providers: set[str | None], + observed: dict[str, ScaleSet], + templates: dict[str, Template], + ) -> None: + """Reconcile a single desired scaleset: validate, create or update, and sync its template. - if spec.provider_name not in providers: - logger.warning( - "Skipping scaleset %s: provider %s not registered yet", - spec.name, - spec.provider_name, - ) - continue + Args: + spec: The desired scaleset. + providers: Names of providers currently registered in GARM. + observed: Observed scalesets keyed by name. + templates: Observed templates keyed by name. + """ + try: + create_params = self._to_create_params(spec) + except Exception as exc: + logger.warning("Skipping scaleset %s: spec validation failed: %s", spec.name, exc) + return - entity_id = self._resolve_entity_id(spec) - if entity_id is None: - logger.warning( - "Skipping scaleset %s: %s '%s' not registered in GARM yet", - spec.name, - spec.entity_type, - spec.entity_name, - ) - continue + if spec.provider_name not in providers: + logger.warning( + "Skipping scaleset %s: provider %s not registered yet", + spec.name, + spec.provider_name, + ) + return - template_id = self._ensure_template(spec, templates) + entity_id = self._resolve_entity_id(spec) + if entity_id is None: + logger.warning( + "Skipping scaleset %s: %s '%s' not registered in GARM yet", + spec.name, + spec.entity_type, + spec.entity_name, + ) + return - if spec.name in observed: - self._maybe_update(observed[spec.name], spec, template_id) - else: - self._create(spec, entity_id, create_params, template_id) + template_id = self._ensure_template(spec, templates) - if not spec.runner_config.has_config(): - # 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) + if spec.name in observed: + self._maybe_update(observed[spec.name], spec, template_id) + else: + self._create(spec, entity_id, create_params, template_id) - for name, scaleset in observed.items(): - if name not in all_desired_names: - self._delete_orphaned(scaleset) - self._delete_custom_template(name, templates) + if not spec.runner_config.has_config(): + # 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) def _delete_orphaned(self, scaleset: ScaleSet) -> None: """Disable then delete a scaleset that is no longer in the desired set.""" diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index f26916d5..34614830 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -369,6 +369,7 @@ def test_pre_install_scripts_passed_in_create(): _, _, params = client.created[0] assert params.extra_specs == {"pre_install_scripts": scripts} + class _FakeTemplate: """Minimal fake for the GARM Template model used in template lifecycle tests.""" @@ -535,8 +536,8 @@ def test_template_updated_when_runner_config_changes(): act: Reconcile with a changed runner config. assert: The custom template is updated (not recreated); the scaleset template_id is unchanged. """ - from runner_template import build_template_data from charm_state import RunnerConfig + from runner_template import build_template_data old_config = RunnerConfig(dockerhub_mirror="https://old.example.com") custom_template = _FakeTemplate( @@ -574,7 +575,9 @@ def test_template_detached_when_runner_config_cleared(): scalesets=[_existing_scaleset(template_id=2)], templates=[ _SYSTEM_TEMPLATE, - _FakeTemplate("github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n"), + _FakeTemplate( + "github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n" + ), ], ) _reconcile(client, [_spec()]) @@ -595,7 +598,9 @@ def test_template_kept_when_system_template_missing(): providers=["openstack-demo"], scalesets=[_existing_scaleset(template_id=2)], templates=[ - _FakeTemplate("github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n"), + _FakeTemplate( + "github_linux-my-scaleset", tid=2, data=b"#!/bin/bash\nset -e\necho x\n" + ), ], ) _reconcile( From cd75155ee66e76f66f827725be04033fa8d346a1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 07:03:52 +0000 Subject: [PATCH 12/16] refactor(garm): validate runner options in charm_state, render via Jinja2 Move port/address/env-value sanitisation from the renderer into RunnerConfig.from_databag so relation-sourced values are cleaned at the boundary. Render the runner-install script from Jinja2 templates in src/templates/*.j2 (FileSystemLoader) instead of inline string concatenation. --- charms/garm/requirements.txt | 1 + charms/garm/src/charm_state.py | 74 +++- charms/garm/src/runner_template.py | 389 ++++-------------- charms/garm/src/templates/aproxy.j2 | 24 ++ .../src/templates/custom_pre_job_script.j2 | 7 + charms/garm/src/templates/dockerhub_mirror.j2 | 7 + charms/garm/src/templates/hook_body.j2 | 12 + .../src/templates/otel_collector_setup.j2 | 155 +++++++ charms/garm/src/templates/pre_install.j2 | 10 + charms/garm/src/templates/pre_job_hooks.j2 | 14 + charms/garm/src/templates/static_host_prep.j2 | 3 + charms/garm/tests/unit/test_charm_state.py | 71 +++- .../garm/tests/unit/test_runner_template.py | 74 ++-- 13 files changed, 468 insertions(+), 373 deletions(-) create mode 100644 charms/garm/src/templates/aproxy.j2 create mode 100644 charms/garm/src/templates/custom_pre_job_script.j2 create mode 100644 charms/garm/src/templates/dockerhub_mirror.j2 create mode 100644 charms/garm/src/templates/hook_body.j2 create mode 100644 charms/garm/src/templates/otel_collector_setup.j2 create mode 100644 charms/garm/src/templates/pre_install.j2 create mode 100644 charms/garm/src/templates/pre_job_hooks.j2 create mode 100644 charms/garm/src/templates/static_host_prep.j2 diff --git a/charms/garm/requirements.txt b/charms/garm/requirements.txt index e4a148ae..bfc20646 100644 --- a/charms/garm/requirements.txt +++ b/charms/garm/requirements.txt @@ -1,5 +1,6 @@ ops==3.8.0 paas-charm==1.11.3 +Jinja2==3.1.6 tomli-w==1.2.0 PyYAML==6.0.3 urllib3==2.7.0 diff --git a/charms/garm/src/charm_state.py b/charms/garm/src/charm_state.py index 6e594c13..30f44c74 100644 --- a/charms/garm/src/charm_state.py +++ b/charms/garm/src/charm_state.py @@ -11,7 +11,9 @@ """ import dataclasses +import ipaddress import logging +import re import typing from collections.abc import Mapping @@ -21,6 +23,8 @@ logger = logging.getLogger(__name__) +_PORT_TOKEN_RE = re.compile(r"^\d{1,5}(-\d{1,5})?$") + DEBUG_SSH_INTEGRATION_NAME: typing.Final[str] = "debug-ssh" GARM_CONFIGURATOR_RELATION_NAME: typing.Final[str] = "garm-configurator" @@ -58,9 +62,9 @@ class SSHDebugInfo: 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 consuming them here (and in - the template renderer) is purely mechanical. + Every field is a plain string; an empty string means "unset". + :meth:`from_databag` sanitises the security-sensitive fields (ports, + addresses, env-file values) at parse time. Attributes: dockerhub_mirror: Docker registry mirror URL, or "". @@ -86,9 +90,19 @@ def from_databag(cls, data: Mapping[str, str]) -> "RunnerConfig": data: The relation unit databag (or any mapping). Returns: - A RunnerConfig with each field taken from its databag key, "" if absent. + A RunnerConfig with each field taken from its databag key, "" if absent, and the + security-sensitive fields sanitised (see class docstring). """ - return cls(**{key: (data.get(key) or "").strip() for key in RUNNER_OPTION_DATABAG_KEYS}) + values = {key: (data.get(key) or "").strip() for key in RUNNER_OPTION_DATABAG_KEYS} + values["aproxy_redirect_ports"] = ",".join( + _valid_port_tokens(values["aproxy_redirect_ports"]) + ) + values["aproxy_exclude_addresses"] = ",".join( + _valid_ipv4_tokens(values["aproxy_exclude_addresses"]) + ) + values["otel_collector_endpoint"] = _strip_newlines(values["otel_collector_endpoint"]) + values["dockerhub_mirror"] = _strip_newlines(values["dockerhub_mirror"]) + return cls(**values) def has_config(self) -> bool: """Whether any runner option is set (i.e. a custom template is needed). @@ -99,6 +113,56 @@ def has_config(self) -> bool: return any(getattr(self, key) for key in RUNNER_OPTION_DATABAG_KEYS) +def _strip_newlines(value: str) -> str: + """Flatten a value so it is safe as a single-line env-file entry.""" + return value.replace("\r", "").replace("\n", "") + + +def _valid_port_tokens(spec: str) -> list[str]: + """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 rendered nft ruleset. + + Args: + spec: A comma-separated ports string (possibly empty or untrusted). + + Returns: + The subset of well-formed, in-range tokens. + """ + 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]: + """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 + + def credential_name(app_id: int, installation_id: int) -> str: """Return the GARM credential name for a GitHub App installation. diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index 63ab2dad..6da1a531 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -19,16 +19,20 @@ 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. + +Blocks are rendered from Jinja2 templates in ``src/templates/*.j2``, loaded via a +``FileSystemLoader`` rooted next to this module; ``src/`` is packed verbatim into +the rock, so the template files ship alongside the charm code. ``autoescape`` is +off throughout: the output is shell/YAML, not HTML. """ -import ipaddress import json -import re +import pathlib import shlex -from charm_state import RunnerConfig +import jinja2 -_PORT_TOKEN_RE = re.compile(r"^\d{1,5}(-\d{1,5})?$") +from charm_state import RunnerConfig # GARM hardcodes the runner username and actions-runner directory; the env file # read by the runner service therefore lives at the path below. @@ -37,170 +41,34 @@ PRE_JOB_HOOK_PATH = f"{RUNNER_HOME}/pre-job.sh" RUNNER_ENV_PATH = f"{RUNNER_HOME}/env" +_TEMPLATES_DIR = pathlib.Path(__file__).parent / "templates" +_JINJA_ENV = jinja2.Environment( + loader=jinja2.FileSystemLoader(str(_TEMPLATES_DIR)), + autoescape=False, + trim_blocks=True, + lstrip_blocks=True, + keep_trailing_newline=True, + undefined=jinja2.StrictUndefined, +) + # OpenTelemetry collector config: hostmetrics/otlp/loki pipelines exporting to # the configured endpoint. `$GITHUB_*`, `$RUNNER_NAME` and `$(uname -m)` are left # as literal shell syntax — they sit inside the *inner*, unquoted `tee < - set(resource.attributes["loki.resource.labels"], - Concat([resource.attributes["loki.resource.labels"], - ", service.name, github.repository, github.runner, github.workflow, github.job, \ -github.run.id, github.run.attempt, host.arch"], "")) - where resource.attributes["loki.resource.labels"] != nil - - > - set(resource.attributes["loki.resource.labels"], - "service.name, github.repository, github.runner, github.workflow, github.job, \ -github.run.id, github.run.attempt, host.arch") - where resource.attributes["loki.resource.labels"] == nil - batch: -exporters: - otlp/self_hosted_runner: - endpoint: {endpoint} - tls: - insecure: true -service: - pipelines: - metrics: - receivers: [hostmetrics] - processors: [attributes/self_hosted_runner_github_labels, batch] - exporters: [otlp/self_hosted_runner] - metrics/otlp: - receivers: [otlp] - processors: [attributes/self_hosted_runner_github_labels, batch] - exporters: [otlp/self_hosted_runner] - logs/otlp: - receivers: [otlp] - processors: - - resource/self_hosted_runner_github_labels - - transform/self_hosted_runner_loki_labels - - batch - exporters: [otlp/self_hosted_runner] - logs/loki: - receivers: [loki] - processors: - - resource/self_hosted_runner_github_labels - - transform/self_hosted_runner_loki_labels - - batch - exporters: [otlp/self_hosted_runner] - logs/aproxy: - receivers: [filelog/aproxy] - processors: - - resource/self_hosted_runner_github_labels - - resource/self_hosted_runner_github_aproxy_labels - - transform/self_hosted_runner_loki_labels - - batch - exporters: [otlp/self_hosted_runner] - traces: - receivers: [otlp] - processors: [resource/self_hosted_runner_github_labels, batch] - exporters: [otlp/self_hosted_runner] -EOF - -/usr/bin/sudo /usr/bin/snap enable opentelemetry-collector -/usr/bin/sudo /usr/bin/snap start opentelemetry-collector""" +_OTEL_COLLECTOR_SETUP_TEMPLATE = _JINJA_ENV.get_template("otel_collector_setup.j2") + +# Dense inline conditionals: `trim_blocks` unconditionally eats exactly one +# newline right after every `{% %}` tag, so a tag-only line contributes nothing +# to the output regardless of which branch runs — that's what lets the +# optional blocks below disappear cleanly (no stray blank line) when unset. +_HOOK_BODY_TEMPLATE = _JINJA_ENV.get_template("hook_body.j2") +_CUSTOM_PRE_JOB_SCRIPT_TEMPLATE = _JINJA_ENV.get_template("custom_pre_job_script.j2") +_APROXY_TEMPLATE = _JINJA_ENV.get_template("aproxy.j2") +_DOCKERHUB_MIRROR_TEMPLATE = _JINJA_ENV.get_template("dockerhub_mirror.j2") +_STATIC_HOST_PREP_TEMPLATE = _JINJA_ENV.get_template("static_host_prep.j2") +_PRE_INSTALL_TEMPLATE = _JINJA_ENV.get_template("pre_install.j2") +_PRE_JOB_HOOKS_TEMPLATE = _JINJA_ENV.get_template("pre_job_hooks.j2") def build_template_data(base: bytes, config: RunnerConfig) -> bytes: @@ -234,14 +102,13 @@ def render_pre_install(config: RunnerConfig) -> str: 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) + aproxy = _render_aproxy(config) if config.runner_http_proxy else "" + dockerhub = ( + _render_dockerhub_mirror(config.dockerhub_mirror) if config.dockerhub_mirror else "" + ) + return _PRE_INSTALL_TEMPLATE.render( + aproxy=aproxy, dockerhub=dockerhub, static_prep=_render_static_host_prep() + ) def render_pre_job_hooks(config: RunnerConfig) -> str: @@ -257,8 +124,8 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: Returns: A bash snippet, terminated by a newline. """ - otel_endpoint = _flatten_env_value(config.otel_collector_endpoint) - dockerhub_mirror = _flatten_env_value(config.dockerhub_mirror) + otel_endpoint = config.otel_collector_endpoint + dockerhub_mirror = config.dockerhub_mirror hook_body = _render_pre_job_hook_body(config, otel_endpoint) @@ -273,27 +140,19 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: # 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. + # template stable across reconciles. Computed in Python (two-phase render) + # since the delimiter depends on the already-rendered hook body. 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}", - # Quoted delimiter: the hook file's own heredocs and $VARS must stay - # literal here and expand only when the runner executes the hook, - # once per job. - f"cat > {PRE_JOB_HOOK_PATH} <<'{prejob_delim}'", - hook_body, - prejob_delim, - f"chmod 0755 {PRE_JOB_HOOK_PATH}", - f"cat >> {RUNNER_ENV_PATH} <<'{env_delim}'", - *env_entries, - env_delim, - f"chown -R {RUNNER_USER}:{RUNNER_USER} {RUNNER_HOME} 2>/dev/null || true", - "# ===== end charm-injected runner job hooks =====\n", - ] + return _PRE_JOB_HOOKS_TEMPLATE.render( + runner_home=RUNNER_HOME, + hook_path=PRE_JOB_HOOK_PATH, + env_path=RUNNER_ENV_PATH, + runner_user=RUNNER_USER, + hook_body=hook_body, + env_entries=env_entries, + prejob_delim=prejob_delim, + env_delim=env_delim, ) @@ -307,21 +166,11 @@ def _render_pre_job_hook_body(config: RunnerConfig, otel_endpoint: str) -> str: Returns: The full contents of the hook script (no trailing newline). """ - lines = ["#!/usr/bin/env bash", "set +e"] - if otel_endpoint: - lines.append("") - lines.append(_OTEL_COLLECTOR_SETUP.format(endpoint=otel_endpoint)) - if config.pre_job_script: - lines.append("") - lines.append(_render_custom_pre_job_script(config.pre_job_script)) - return "\n".join(lines) - - -def _flatten_env_value(value: str | None) -> str: - """Return a value safe for a single-line env file entry.""" - if value is None: - return "" - return value.replace("\r", "").replace("\n", "") + otel = _OTEL_COLLECTOR_SETUP_TEMPLATE.render(endpoint=otel_endpoint) if otel_endpoint else "" + custom_script = ( + _render_custom_pre_job_script(config.pre_job_script) if config.pre_job_script else "" + ) + return _HOOK_BODY_TEMPLATE.render(otel=otel, custom_script=custom_script) def _heredoc_delimiter(content: str, base: str) -> str: @@ -355,18 +204,7 @@ def _render_custom_pre_job_script(script: str) -> str: A bash snippet. """ delim = _heredoc_delimiter(script, "GARM_CHARM_CUSTOM_PREJOB") - return "\n".join( - [ - f"cat > /tmp/custom_pre_job_script <<'{delim}'", - script, - delim, - "chmod +x /tmp/custom_pre_job_script", - 'logger -s "Running custom pre-job script"', - '/tmp/custom_pre_job_script || logger -s "Custom pre-job script failed, ' - 'continuing with the job"', - 'rm /tmp/custom_pre_job_script || logger -s "Failed to remove custom pre-job script"', - ] - ) + return _CUSTOM_PRE_JOB_SCRIPT_TEMPLATE.render(delim=delim, script=script) def _render_aproxy(config: RunnerConfig) -> str: @@ -383,49 +221,23 @@ def _render_aproxy(config: RunnerConfig) -> str: Returns: A bash snippet configuring aproxy as a transparent forward proxy. """ - # 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"] + # Values are already validated at parse time (RunnerConfig.from_databag); only + # shell-safe embedding (quoting, the empty-string split guard below) happens here. + ports = [t for t in config.aproxy_redirect_ports.split(",") if t] or ["80", "443"] nft_ports = ", ".join(ports) - excludes = _valid_ipv4_tokens(config.aproxy_exclude_addresses) + excludes = [t for t in config.aproxy_exclude_addresses.split(",") if t] exclude_elements = ", ".join(["127.0.0.0/8", *excludes]) + # `\$default-ipv4` stays escaped so the literal two characters `$default-ipv4` + # land in the file, which nft itself resolves against the `define` above when + # it loads the file. dnat_rule = ( f"ip daddr != @exclude tcp dport {{ {nft_ports} }} counter dnat to \\$default-ipv4:54969" ) - return "\n".join( - [ - "# Transparently forward runner egress through the configured HTTP proxy.", - "snap install aproxy --edge", - f"snap set aproxy proxy={shlex.quote(config.runner_http_proxy)} listen=:54969", - # Unquoted heredoc: `$(ip route ...)` must expand now, at boot, to - # compute the default gateway. `\$default-ipv4` stays escaped so the - # literal two characters `$default-ipv4` land in the file, which nft - # itself resolves against the `define` above when it loads the file. - "nft delete table ip aproxy 2>/dev/null || true", - "cat << EOF > /etc/nftables.conf", - "define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 " - "| grep -oP 'via \\K\\S+') | grep -oP 'src \\K\\S+')", - "table ip aproxy {", - " set exclude {", - " type ipv4_addr;", - " flags interval; auto-merge;", - f" elements = {{ {exclude_elements} }}", - " }", - " chain prerouting {", - " type nat hook prerouting priority dstnat; policy accept;", - f" {dnat_rule}", - " }", - " chain output {", - " type nat hook output priority -100; policy accept;", - f" {dnat_rule}", - " }", - "}", - "EOF", - "systemctl enable nftables.service", - "nft -f /etc/nftables.conf", - ] + return _APROXY_TEMPLATE.render( + proxy=shlex.quote(config.runner_http_proxy), + exclude_elements=exclude_elements, + dnat_rule=dnat_rule, ) @@ -439,17 +251,7 @@ def _render_dockerhub_mirror(mirror: str) -> str: 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 daemon-reload", - "systemctl restart docker", - ] - ) + return _DOCKERHUB_MIRROR_TEMPLATE.render(daemon_json=daemon_json) def _render_static_host_prep() -> str: @@ -461,55 +263,4 @@ def _render_static_host_prep() -> str: A bash snippet. Each command is guarded by ``|| true`` so it is a no-op if the runner account doesn't exist yet. """ - return "\n".join( - [ - "# Static runner host preparation.", - f"adduser {RUNNER_USER} lxd || true", - f"adduser {RUNNER_USER} adm || true", - ] - ) - - -def _valid_port_tokens(spec: str) -> list[str]: - """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 well-formed, in-range tokens. - """ - 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]: - """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 + return _STATIC_HOST_PREP_TEMPLATE.render(runner_user=RUNNER_USER) diff --git a/charms/garm/src/templates/aproxy.j2 b/charms/garm/src/templates/aproxy.j2 new file mode 100644 index 00000000..f1a79e20 --- /dev/null +++ b/charms/garm/src/templates/aproxy.j2 @@ -0,0 +1,24 @@ +# Transparently forward runner egress through the configured HTTP proxy. +snap install aproxy --edge +snap set aproxy proxy={{ proxy }} listen=:54969 +nft delete table ip aproxy 2>/dev/null || true +cat << EOF > /etc/nftables.conf +define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 | grep -oP 'via \K\S+') | grep -oP 'src \K\S+') +table ip aproxy { + set exclude { + type ipv4_addr; + flags interval; auto-merge; + elements = { {{ exclude_elements }} } + } + chain prerouting { + type nat hook prerouting priority dstnat; policy accept; + {{ dnat_rule }} + } + chain output { + type nat hook output priority -100; policy accept; + {{ dnat_rule }} + } +} +EOF +systemctl enable nftables.service +nft -f /etc/nftables.conf \ No newline at end of file diff --git a/charms/garm/src/templates/custom_pre_job_script.j2 b/charms/garm/src/templates/custom_pre_job_script.j2 new file mode 100644 index 00000000..c9a182f5 --- /dev/null +++ b/charms/garm/src/templates/custom_pre_job_script.j2 @@ -0,0 +1,7 @@ +cat > /tmp/custom_pre_job_script <<'{{ delim }}' +{{ script }} +{{ delim }} +chmod +x /tmp/custom_pre_job_script +logger -s "Running custom pre-job script" +/tmp/custom_pre_job_script || logger -s "Custom pre-job script failed, continuing with the job" +rm /tmp/custom_pre_job_script || logger -s "Failed to remove custom pre-job script" \ No newline at end of file diff --git a/charms/garm/src/templates/dockerhub_mirror.j2 b/charms/garm/src/templates/dockerhub_mirror.j2 new file mode 100644 index 00000000..6d31f932 --- /dev/null +++ b/charms/garm/src/templates/dockerhub_mirror.j2 @@ -0,0 +1,7 @@ +# 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 daemon-reload +systemctl restart docker \ No newline at end of file diff --git a/charms/garm/src/templates/hook_body.j2 b/charms/garm/src/templates/hook_body.j2 new file mode 100644 index 00000000..6f53370c --- /dev/null +++ b/charms/garm/src/templates/hook_body.j2 @@ -0,0 +1,12 @@ +#!/usr/bin/env bash +set +e +{%- if otel %} + + +{{ otel }} +{%- endif %} +{%- if custom_script %} + + +{{ custom_script }} +{%- endif %} \ No newline at end of file diff --git a/charms/garm/src/templates/otel_collector_setup.j2 b/charms/garm/src/templates/otel_collector_setup.j2 new file mode 100644 index 00000000..6c888254 --- /dev/null +++ b/charms/garm/src/templates/otel_collector_setup.j2 @@ -0,0 +1,155 @@ +/usr/bin/logger -s "OpenTelemetry collector is enabled." +/usr/bin/logger -s "Additional OpenTelemetry collector configuration can be added." +/usr/bin/logger -s "The exporter endpoint is at the environment variable ACTION_OTEL_EXPORTER_OTLP_ENDPOINT." +/usr/bin/sudo /usr/bin/mkdir -p /etc/otelcol/config.d +/usr/bin/sudo /usr/bin/touch /etc/otelcol/config.d/github.yaml +/usr/bin/sudo /usr/bin/tee /etc/otelcol/config.d/github.yaml < + set(resource.attributes["loki.resource.labels"], + Concat([resource.attributes["loki.resource.labels"], + ", service.name, github.repository, github.runner, github.workflow, github.job, github.run.id, github.run.attempt, host.arch"], "")) + where resource.attributes["loki.resource.labels"] != nil + - > + set(resource.attributes["loki.resource.labels"], + "service.name, github.repository, github.runner, github.workflow, github.job, github.run.id, github.run.attempt, host.arch") + where resource.attributes["loki.resource.labels"] == nil + batch: +exporters: + otlp/self_hosted_runner: + endpoint: {{ endpoint }} + tls: + insecure: true +service: + pipelines: + metrics: + receivers: [hostmetrics] + processors: [attributes/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] + metrics/otlp: + receivers: [otlp] + processors: [attributes/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] + logs/otlp: + receivers: [otlp] + processors: + - resource/self_hosted_runner_github_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + logs/loki: + receivers: [loki] + processors: + - resource/self_hosted_runner_github_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + logs/aproxy: + receivers: [filelog/aproxy] + processors: + - resource/self_hosted_runner_github_labels + - resource/self_hosted_runner_github_aproxy_labels + - transform/self_hosted_runner_loki_labels + - batch + exporters: [otlp/self_hosted_runner] + traces: + receivers: [otlp] + processors: [resource/self_hosted_runner_github_labels, batch] + exporters: [otlp/self_hosted_runner] +EOF + +/usr/bin/sudo /usr/bin/snap enable opentelemetry-collector +/usr/bin/sudo /usr/bin/snap start opentelemetry-collector \ No newline at end of file diff --git a/charms/garm/src/templates/pre_install.j2 b/charms/garm/src/templates/pre_install.j2 new file mode 100644 index 00000000..c2ed5f62 --- /dev/null +++ b/charms/garm/src/templates/pre_install.j2 @@ -0,0 +1,10 @@ + +# ===== charm-injected pre-install setup (runs as root) ===== +{% if aproxy %} +{{ aproxy }} +{% endif %} +{% if dockerhub %} +{{ dockerhub }} +{% endif %} +{{ static_prep }} +# ===== end charm-injected pre-install setup ===== diff --git a/charms/garm/src/templates/pre_job_hooks.j2 b/charms/garm/src/templates/pre_job_hooks.j2 new file mode 100644 index 00000000..a325c956 --- /dev/null +++ b/charms/garm/src/templates/pre_job_hooks.j2 @@ -0,0 +1,14 @@ + +# ===== charm-injected runner job hooks ===== +mkdir -p {{ runner_home }} +cat > {{ hook_path }} <<'{{ prejob_delim }}' +{{ hook_body }} +{{ prejob_delim }} +chmod 0755 {{ hook_path }} +cat >> {{ env_path }} <<'{{ env_delim }}' +{% for entry in env_entries %} +{{ entry }} +{% endfor %} +{{ env_delim }} +chown -R {{ runner_user }}:{{ runner_user }} {{ runner_home }} 2>/dev/null || true +# ===== end charm-injected runner job hooks ===== diff --git a/charms/garm/src/templates/static_host_prep.j2 b/charms/garm/src/templates/static_host_prep.j2 new file mode 100644 index 00000000..545923e1 --- /dev/null +++ b/charms/garm/src/templates/static_host_prep.j2 @@ -0,0 +1,3 @@ +# Static runner host preparation. +adduser {{ runner_user }} lxd || true +adduser {{ runner_user }} adm || true \ No newline at end of file diff --git a/charms/garm/tests/unit/test_charm_state.py b/charms/garm/tests/unit/test_charm_state.py index 5e2e60af..7b3ccfeb 100644 --- a/charms/garm/tests/unit/test_charm_state.py +++ b/charms/garm/tests/unit/test_charm_state.py @@ -7,7 +7,7 @@ import pytest -from charm_state import CharmState, SSHDebugInfo, _get_ssh_debug_connections +from charm_state import CharmState, RunnerConfig, SSHDebugInfo, _get_ssh_debug_connections _COMPLETE = { "host": "10.0.0.1", @@ -257,3 +257,72 @@ def test_desired_entities_skip_incomplete_unit(unit_data): assert: No entity is built. """ assert _entities([unit_data]) == [] + + +def test_runner_config_from_databag_and_has_config(): + """ + arrange: Empty and populated runner-config databags. + act: Build RunnerConfig instances from the databags and inspect has_config. + assert: Known keys are mapped, unknown keys are ignored, and has_config reflects content. + """ + 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() + + +def test_runner_config_from_databag_drops_malformed_port_and_address_tokens(): + """ + arrange: A databag with well-formed, malformed, out-of-range and IPv6 tokens. + act: Build a RunnerConfig from the databag. + assert: Only the well-formed, in-range IPv4 tokens survive. + """ + config = RunnerConfig.from_databag( + { + "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", + } + ) + + assert config.aproxy_redirect_ports == "80,8000-9000" + assert config.aproxy_exclude_addresses == "10.0.0.0/8" + + +def test_runner_config_from_databag_strips_newlines_from_otel_endpoint(): + """ + arrange: A databag whose OTEL endpoint contains a newline and extra assignment text. + act: Build a RunnerConfig from the databag. + assert: The endpoint is flattened into a single line, preventing env-file line injection. + """ + config = RunnerConfig.from_databag( + {"otel_collector_endpoint": "http://o.test:4318\nMALICIOUS=1"} + ) + + assert config.otel_collector_endpoint == "http://o.test:4318MALICIOUS=1" + + +def test_runner_config_from_databag_strips_newlines_from_dockerhub_mirror(): + """ + arrange: A databag whose Docker mirror URL contains a newline and extra assignment text. + act: Build a RunnerConfig from the databag. + assert: The mirror value is flattened, preventing env-file line injection. + """ + config = RunnerConfig.from_databag({"dockerhub_mirror": "https://m.test\nMALICIOUS=1"}) + + assert config.dockerhub_mirror == "https://m.testMALICIOUS=1" + + +def test_runner_config_from_databag_preserves_multiline_pre_job_script(): + """ + arrange: A databag whose pre_job_script is legitimately multi-line. + act: Build a RunnerConfig from the databag. + assert: Newlines in pre_job_script are preserved (unlike the single-line env fields). + """ + config = RunnerConfig.from_databag({"pre_job_script": "echo one\necho two"}) + + assert config.pre_job_script == "echo one\necho two" diff --git a/charms/garm/tests/unit/test_runner_template.py b/charms/garm/tests/unit/test_runner_template.py index b0718660..feaddc8a 100644 --- a/charms/garm/tests/unit/test_runner_template.py +++ b/charms/garm/tests/unit/test_runner_template.py @@ -6,10 +6,7 @@ import pytest from charm_state import RunnerConfig -from runner_template import ( - RUNNER_ENV_PATH, - build_template_data, -) +from runner_template import RUNNER_ENV_PATH, build_template_data SAMPLE_BASE = b"#!/bin/bash\nset -e\necho original-bootstrap\n" @@ -134,26 +131,21 @@ def test_build_template_data_per_option(config, present, absent): assert absent not in result -def test_aproxy_render_drops_malformed_tokens(): +def test_aproxy_render_uses_configured_ports_and_excludes(): """ - arrange: A runner config with a proxy plus malformed ports and exclude addresses. + arrange: A runner config with a proxy plus well-formed redirect ports and excludes. act: Build the runner template data. - assert: Only valid IPv4-oriented aproxy rules are rendered. + assert: The nft ruleset embeds exactly those ports and addresses. """ config = RunnerConfig( runner_http_proxy="http://p.test:3128", - 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", + aproxy_redirect_ports="80,8000-9000", + aproxy_exclude_addresses="10.0.0.0/8", ) result = build_template_data(SAMPLE_BASE, config).decode() assert "tcp dport { 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 def test_heredoc_delimiter_avoids_collision_with_pre_job_script(): @@ -169,45 +161,31 @@ def test_heredoc_delimiter_avoids_collision_with_pre_job_script(): assert "echo bye" in result -def test_otel_endpoint_newline_stripped(): +def test_build_template_data_from_untrusted_databag_drops_malformed_values(): """ - arrange: An OTEL endpoint containing a newline and extra assignment text. - act: Build the runner template data. - assert: The endpoint is flattened into one env-file line without line injection. + arrange: A raw relation databag with malformed ports/addresses and newline-bearing values. + act: Parse it via RunnerConfig.from_databag and build the runner template data. + assert: The malformed/injected content never reaches the rendered script. """ - config = RunnerConfig(otel_collector_endpoint="http://o.test:4318\nMALICIOUS=1") + config = RunnerConfig.from_databag( + { + "runner_http_proxy": "http://p.test:3128", + "aproxy_redirect_ports": "80,not-a-port,8000-9000,99999,443-80", + "aproxy_exclude_addresses": "10.0.0.0/8,evil;,2001:db8::1", + "otel_collector_endpoint": "http://o.test:4318\nMALICIOUS=1", + "dockerhub_mirror": "https://m.test\nMALICIOUS=1", + } + ) result = build_template_data(SAMPLE_BASE, config).decode() + assert "tcp dport { 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 assert "ACTION_OTEL_EXPORTER_OTLP_ENDPOINT=http://o.test:4318MALICIOUS=1" in result - assert "\nMALICIOUS=1" not in result - - -def test_dockerhub_mirror_newline_stripped(): - """ - arrange: A Docker mirror URL containing a newline and extra assignment text. - act: Build the runner template data. - assert: The mirror value is flattened so it cannot inject extra env-file lines. - """ - config = RunnerConfig(dockerhub_mirror="https://m.test\nMALICIOUS=1") - result = build_template_data(SAMPLE_BASE, config).decode() - assert "DOCKERHUB_MIRROR=https://m.testMALICIOUS=1" in result assert "CONTAINER_REGISTRY_URL=https://m.testMALICIOUS=1" in result assert "\nMALICIOUS=1" not in result - - -def test_runner_config_from_databag_and_has_config(): - """ - arrange: Empty and populated runner-config databags. - act: Build RunnerConfig instances from the databags and inspect has_config. - assert: Known keys are mapped, unknown keys are ignored, and has_config reflects content. - """ - 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() From 282f788be57eb3edb3c736ad3546b1bdd60a2547 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 07:19:07 +0000 Subject: [PATCH 13/16] fix(garm-configurator): annotate IPvAnyNetwork TypeAdapter for mypy mypy cannot infer the generic parameter of TypeAdapter(IPvAnyNetwork) (IPvAnyNetwork is a union special form), so the static check failed with var-annotated. Add the explicit TypeAdapter[IPvAnyNetwork] annotation. --- charms/garm-configurator/src/charm_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 3a8d2064..1d988710 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -47,7 +47,7 @@ PRE_JOB_SCRIPT_CONFIG_NAME = "pre-job-script" _HTTP_URL_ADAPTER = TypeAdapter(HttpUrl) -_IP_NETWORK_ADAPTER = TypeAdapter(IPvAnyNetwork) +_IP_NETWORK_ADAPTER: TypeAdapter[IPvAnyNetwork] = TypeAdapter(IPvAnyNetwork) IMAGE_RELATION_NAME = "image" GARM_RELATION_NAME = "garm-configurator" From 0b7ca45dc001356078e36d7e5575db498506e54e Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 08:07:29 +0000 Subject: [PATCH 14/16] test(garm): delete scalesets before org in integration cleanup _detach_synced_credential could not delete test-org once a prior test left a scaleset attached: GARM 400s an org delete while a scaleset references it. Remove scalesets first; with min-idle-runners at zero they hold no runners and delete cleanly, so the org (and then the credential) can be removed to repoint github.com. --- charms/tests/integration/test_garm.py | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index c2846e13..7052ba7a 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -631,13 +631,16 @@ def _point_github_endpoint_at_mock(base_url: str, token: str, mock_base_url: str def _detach_synced_credential(base_url: str, token: str) -> None: - """Delete the charm-synced org and credential so github.com can be repointed. + """Delete the charm-synced scalesets, org and credential so github.com can be repointed. - GARM locks an endpoint's URLs while a credential references it. The charm syncs its credential - onto the built-in github.com endpoint, so it must be removed (along with any org bound to it) - before the endpoint can be pointed at the mock. Deletion tolerates a not-yet-created entity - (404); a real failure surfaces later as the repoint's 400. + GARM locks an endpoint's URLs while a credential references it, and refuses to delete an org + (or credential) while a scaleset still references it. The charm syncs its credential onto the + built-in github.com endpoint, so the whole chain — scalesets first, then the org bound to the + credential, then the credential — must be removed before the endpoint can be pointed at the + mock. Deletion tolerates a not-yet-created entity (404); a real failure surfaces later as the + repoint's 400. """ + _delete_scalesets(base_url, token) for org in _list_orgs(base_url, token): if org.get("name") == "test-org" and org.get("id"): _delete_if_present(f"{base_url}/organizations/{org['id']}", token) @@ -653,6 +656,19 @@ def _delete_if_present(url: str, token: str) -> None: resp.raise_for_status() +def _delete_scalesets(base_url: str, token: str) -> None: + """Delete every scaleset so the org that owns it can be removed. + + GARM refuses to delete an org while a scaleset references it. The test scalesets run with + ``min-idle-runners`` at zero, so they hold no runners and delete without a drain cycle; a + scaleset already gone is tolerated (404). + """ + for scaleset in _list_scalesets(base_url, token): + scaleset_id = scaleset.get("id") + if scaleset_id is not None: + _delete_if_present(f"{base_url}/scalesets/{scaleset_id}", token) + + def _restore_system_templates(garm_url: str, token: str) -> None: """Restore GARM's built-in system templates so scaleset creation can find a template. From 387d4c8869c26117515efd79eb88958a40f1bd06 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 08:35:03 +0000 Subject: [PATCH 15/16] test(garm): disable scalesets before deleting them in integration cleanup The scaleset delete added in the previous commit 400s while GARM still has the scaleset enabled/draining. Mirror the charm's _delete_orphaned pattern: disable (idle count to zero) to trigger the drain, then retry the delete until GARM drains it. Also log GARM's response body on a failed delete to aid diagnosis. --- charms/tests/integration/test_garm.py | 39 +++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 7052ba7a..71eea215 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -652,21 +652,44 @@ def _detach_synced_credential(base_url: str, token: str) -> None: def _delete_if_present(url: str, token: str) -> None: """DELETE a GARM resource, treating 404 as already-gone.""" resp = requests.delete(url, headers=_garm_auth_headers(token), timeout=30) - if resp.status_code != requests.codes.not_found: - resp.raise_for_status() + if resp.status_code == requests.codes.not_found: + return + if not resp.ok: + logger.warning("DELETE %s -> %s: %s", url, resp.status_code, resp.text) + resp.raise_for_status() def _delete_scalesets(base_url: str, token: str) -> None: - """Delete every scaleset so the org that owns it can be removed. + """Disable and delete every scaleset so the org that owns it can be removed. - GARM refuses to delete an org while a scaleset references it. The test scalesets run with - ``min-idle-runners`` at zero, so they hold no runners and delete without a drain cycle; a - scaleset already gone is tolerated (404). + GARM refuses to delete an org while a scaleset references it, and 400s a scaleset delete + while the scaleset is still enabled or draining runners. Disable each scaleset (idle count + to zero) to trigger the drain, then delete once GARM has drained it; a scaleset already gone + is tolerated (404). """ for scaleset in _list_scalesets(base_url, token): scaleset_id = scaleset.get("id") - if scaleset_id is not None: - _delete_if_present(f"{base_url}/scalesets/{scaleset_id}", token) + if scaleset_id is None: + continue + resp = requests.put( + f"{base_url}/scalesets/{scaleset_id}", + json={"enabled": False, "min_idle_runners": 0}, + headers=_garm_auth_headers(token), + timeout=30, + ) + resp.raise_for_status() + _delete_scaleset_drained(base_url, token, scaleset_id) + + +@retry( + retry=retry_if_exception_type(requests.exceptions.RequestException), + wait=wait_exponential(multiplier=1, min=2, max=15), + stop=stop_after_attempt(20), + reraise=True, +) +def _delete_scaleset_drained(base_url: str, token: str, scaleset_id: int) -> None: + """DELETE a scaleset, retrying while GARM drains it (400); 404 means already gone.""" + _delete_if_present(f"{base_url}/scalesets/{scaleset_id}", token) def _restore_system_templates(garm_url: str, token: str) -> None: From b155a498c5317e5aa44d7b67dbebd726064bd687 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Jul 2026 10:32:49 +0000 Subject: [PATCH 16/16] docs(garm): move teardown rationale from docstrings into technical comments Keep _detach_synced_credential and _delete_scalesets docstrings to their interface; put the GARM referential-integrity/drain rationale as technical comments at the call site and at the disable step where it applies. --- charms/tests/integration/test_garm.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 71eea215..6a59719c 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -631,15 +631,12 @@ def _point_github_endpoint_at_mock(base_url: str, token: str, mock_base_url: str def _detach_synced_credential(base_url: str, token: str) -> None: - """Delete the charm-synced scalesets, org and credential so github.com can be repointed. - - GARM locks an endpoint's URLs while a credential references it, and refuses to delete an org - (or credential) while a scaleset still references it. The charm syncs its credential onto the - built-in github.com endpoint, so the whole chain — scalesets first, then the org bound to the - credential, then the credential — must be removed before the endpoint can be pointed at the - mock. Deletion tolerates a not-yet-created entity (404); a real failure surfaces later as the - repoint's 400. - """ + """Delete the charm-synced scalesets, org and credential so github.com can be repointed.""" + # GARM locks an endpoint's URLs while a credential references it, refuses to delete a + # credential while an org is bound to it, and refuses to delete an org while a scaleset + # references it. The charm syncs its credential onto the built-in github.com endpoint, so the + # chain must be unwound in dependency order — scalesets, then org, then credential — before + # the endpoint can be repointed at the mock. _delete_scalesets(base_url, token) for org in _list_orgs(base_url, token): if org.get("name") == "test-org" and org.get("id"): @@ -660,17 +657,13 @@ def _delete_if_present(url: str, token: str) -> None: def _delete_scalesets(base_url: str, token: str) -> None: - """Disable and delete every scaleset so the org that owns it can be removed. - - GARM refuses to delete an org while a scaleset references it, and 400s a scaleset delete - while the scaleset is still enabled or draining runners. Disable each scaleset (idle count - to zero) to trigger the drain, then delete once GARM has drained it; a scaleset already gone - is tolerated (404). - """ + """Disable and delete every scaleset.""" for scaleset in _list_scalesets(base_url, token): scaleset_id = scaleset.get("id") if scaleset_id is None: continue + # GARM 400s a scaleset delete while the scaleset is still enabled or draining runners, so + # disable it (idle count to zero) to trigger the drain before deleting. resp = requests.put( f"{base_url}/scalesets/{scaleset_id}", json={"enabled": False, "min_idle_runners": 0},