From f74d8a22f637495754e886956ebcac5dcb24a60c Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 04:29:01 +0000 Subject: [PATCH 01/26] feat(garm-configurator): manage a security group per scale set Add the optional security-group-rules option. When it is set, the charm creates one OpenStack security group named garm-, keeps its rules equal to the configured list, and publishes the group id as security_group_id on the garm-configurator relation. All egress is added when the operator configures no egress rule: OpenStack attaches this group instead of the project's default group, so without it a runner could not reach GitHub. The group is never deleted, and leaving the option unset keeps deployments on the project's default group. A group that cannot be reconciled blocks the unit rather than publishing an id the charm could not confirm. The charm talks to the Neutron API through a small standard-library client rather than an OpenStack SDK: charmcraft builds this charm's dependencies from source, and the SDK's cryptography dependency does not build without OpenSSL development headers, which the charm build environment lacks. --- charms/garm-configurator/charmcraft.yaml | 19 + charms/garm-configurator/src/charm.py | 74 ++- charms/garm-configurator/src/charm_state.py | 119 +++++ .../garm-configurator/src/openstack_client.py | 316 ++++++++++++ .../src/openstack_security_group.py | 230 +++++++++ .../tests/unit/test_charm.py | 184 +++++++ .../tests/unit/test_openstack_client.py | 482 ++++++++++++++++++ .../unit/test_openstack_security_group.py | 319 ++++++++++++ docs/changelog.md | 4 + docs/reference/charms.md | 22 + docs/reference/security.md | 4 + 11 files changed, 1766 insertions(+), 7 deletions(-) create mode 100644 charms/garm-configurator/src/openstack_client.py create mode 100644 charms/garm-configurator/src/openstack_security_group.py create mode 100644 charms/garm-configurator/tests/unit/test_openstack_client.py create mode 100644 charms/garm-configurator/tests/unit/test_openstack_security_group.py diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index dd14772d..f4a2d739 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -133,6 +133,25 @@ config: description: | A Python dict-like string containing key-value pairs of script name to bash script to be run prior to runner installation. + security-group-rules: + type: string + description: | + Optional JSON list of rules for the OpenStack security group the charm manages + for this scaleset's runners. Each rule takes: + "direction" ("ingress" or "egress", required), "protocol" ("tcp", "udp", + "icmp", "icmpv6", or "any", default "any"), "port_min" and "port_max" + (inclusive, 1-65535, tcp and udp only; "port_min" alone means that one port), + and "remote_ip_prefix" (a CIDR such as "10.0.0.0/8", default every address). + For example: + [{"direction": "ingress", "protocol": "tcp", "port_min": 22, "remote_ip_prefix": "10.0.0.0/8"}]. + The charm creates a group named "garm-" and keeps its rules equal to + this list, so rules added to the group by hand are removed. When no egress + rule is listed the charm allows all egress, because OpenStack attaches this + group instead of the project's default group and a runner that cannot reach + GitHub never runs a job. The group is never deleted: clearing this option, or + removing the application, leaves it in place for the operator to remove. + Requires OpenStack credentials allowed to create security groups and to create + and delete security group rules; it never deletes the group itself. dockerhub-mirror: type: string description: | diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 99c779b2..688dd582 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -16,6 +16,11 @@ CharmConfigInvalidError, CharmState, ) +from openstack_security_group import ( + SecurityGroupReconcileError, + SecurityGroupReconciler, + security_group_name, +) logger = logging.getLogger(__name__) @@ -45,9 +50,9 @@ def _reconcile(self, event: ops.EventBase) -> None: """Handle all charm events by reconciling configuration and relation data. Reads the current charm state, forwards OpenStack credentials to the - image-builder relation, publishes scaleset configuration (and secrets - when the image reference is available) to all garm-configurator relations, - then sets unit status to reflect readiness. + image-builder relation, reconciles the scaleset's security group, publishes + scaleset configuration (and secrets when the image reference is available) to + all garm-configurator relations, then sets unit status to reflect readiness. Args: event: The triggering event. @@ -61,9 +66,40 @@ def _reconcile(self, event: ops.EventBase) -> None: self.unit.status = ops.BlockedStatus(e.msg) return + if not self._configure(state): + return + self._set_ready_status(state) + + def _configure(self, state: CharmState) -> bool: + """Publish the resolved state, reconciling the scaleset's security group first. + + Args: + state: Current resolved charm state. + + Returns: + Whether the state was published. False means the unit is blocked on a security + group that could not be reconciled: the relations are then left untouched, so + GARM never acts on a group id the charm could not confirm. + """ self._update_image_relation(state) - self._configure_garm_relation(state) + try: + security_group_id = self._ensure_security_group(state) + except SecurityGroupReconcileError as e: + name = security_group_name(state.scaleset_config.name) + logger.warning("Security group %s could not be reconciled: %s", name, e) + self.unit.status = ops.BlockedStatus( + f"Security group {name} could not be reconciled: {e}" + ) + return False + self._configure_garm_relation(state, security_group_id or "") + return True + + def _set_ready_status(self, state: CharmState) -> None: + """Report readiness, which depends on where the runner image came from. + Args: + state: Current resolved charm state. + """ if state.image_overrides_builder: self.unit.status = ops.ActiveStatus("Ready; configured image overrides image builder") elif state.image is not None: @@ -95,6 +131,26 @@ def _update_image_relation(self, state: CharmState) -> None: } ) + def _ensure_security_group(self, state: CharmState) -> str | None: + """Reconcile the security group the scaleset's runners are attached to. + + Args: + state: Current resolved charm state. + + Returns: + The id of the group, or None when the operator configured no rules — the + runners then keep the project's default security group. + + Raises: + SecurityGroupReconcileError: If OpenStack could not be reconciled with. + """ + rules = state.scaleset_config.security_group_rules + if rules is None: + return None + return SecurityGroupReconciler(state.provider_config).reconcile( + security_group_name(state.scaleset_config.name), rules + ) + def _ensure_relation_secret( self, relation: ops.Relation, @@ -129,13 +185,14 @@ def _ensure_relation_secret( return secret - def _configure_garm_relation(self, state: CharmState) -> None: + def _configure_garm_relation(self, state: CharmState, security_group_id: str) -> None: """Publish scaleset configuration to the garm-configurator relation. Writes non-secret scaleset fields (name, provider, credentials, image, image visibility, flavor, arch, runner counts, labels, runner group, - remote-shell toggle, and pre-install scripts) to the relation. The ``image_id`` wire field - carries either an OpenStack image name or ID for compatibility. + remote-shell toggle, pre-install scripts, and the managed security group) to + the relation. The ``image_id`` wire field carries either an OpenStack image + name or ID for compatibility. The optional ``org`` and ``repo`` fields are included only when set. When the image reference is present and this unit holds leadership, also @@ -149,6 +206,8 @@ def _configure_garm_relation(self, state: CharmState) -> None: Args: state: Current resolved charm state. + security_group_id: The id of the configurator-managed security group, or an + empty string when the charm manages none. """ garm_relation = self.model.get_relation(GARM_RELATION_NAME) if garm_relation is None: @@ -177,6 +236,7 @@ def _configure_garm_relation(self, state: CharmState) -> None: "aproxy_redirect_ports": rc.aproxy_redirect_ports or "", "otel_collector_endpoint": rc.otel_collector_endpoint or "", "pre_job_script": rc.pre_job_script or "", + "security_group_id": security_group_id, } 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 f8b987dd..e2b3489c 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -4,10 +4,13 @@ """State of the GARM configurator charm.""" import ipaddress +import json +from typing import Literal import ops from pydantic import ( BaseModel, + ConfigDict, HttpUrl, TypeAdapter, ValidationError, @@ -42,6 +45,7 @@ SCALESET_RUNNER_GROUP_CONFIG_NAME = "runner-group" SCALESET_ENABLE_SHELL_CONFIG_NAME = "enable-shell" SCALESET_PRE_INSTALL_SCRIPTS_CONFIG_NAME = "pre-install-scripts" +SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME = "security-group-rules" DOCKERHUB_MIRROR_CONFIG_NAME = "dockerhub-mirror" RUNNER_HTTP_PROXY_CONFIG_NAME = "runner-http-proxy" @@ -218,6 +222,69 @@ def from_charm(cls, charm: ops.CharmBase) -> "GithubAppConfig": ) +class SecurityGroupRule(BaseModel): + """One rule of the OpenStack security group the charm manages for a scaleset. + + Attributes: + direction: Whether the rule admits ingress or egress traffic. + protocol: Transport protocol the rule applies to, or "any" for every protocol. + port_min: First port of the range, or None to cover every port. + port_max: Last port of the range, or None to cover only port_min. + remote_ip_prefix: CIDR the rule applies to, or None to cover every address. + """ + + # An unknown key is rejected rather than dropped: a misspelt ``remote_ip_prefix`` + # would otherwise silently widen the rule to every address. + model_config = ConfigDict(extra="forbid") + + direction: Literal["ingress", "egress"] + protocol: Literal["tcp", "udp", "icmp", "icmpv6", "any"] = "any" + port_min: int | None = None + port_max: int | None = None + remote_ip_prefix: str | None = None + + @field_validator("remote_ip_prefix") + @classmethod + def _validate_cidr(cls, value: str | None) -> str | None: + """Require a CIDR, normalised to the network address Neutron expects.""" + if value is None: + return None + try: + # strict, because Neutron rejects a prefix with host bits set; a bare + # address is read as a single-host network. + return str(ipaddress.ip_network(value.strip(), strict=True)) + except ValueError as exc: + raise ValueError( + f"must be a CIDR such as 10.0.0.0/8, 192.0.2.1/32, or ::/0; got {value!r}" + ) from exc + + @model_validator(mode="after") + def _validate_ports(self) -> "SecurityGroupRule": + """Reject a port range the protocol cannot carry, and fill in a single port.""" + if self.port_min is None and self.port_max is None: + return self + if self.protocol not in ("tcp", "udp"): + raise ValueError( + f"port_min and port_max require protocol tcp or udp, not {self.protocol}" + ) + if self.port_min is None: + raise ValueError("port_max requires port_min") + port_max = self.port_min if self.port_max is None else self.port_max + if not 1 <= self.port_min <= 65535 or not 1 <= port_max <= 65535: + raise ValueError("port_min and port_max must be in 1..65535") + if self.port_min > port_max: + raise ValueError("port_min must not be greater than port_max") + self.port_max = port_max + return self + + @property + def ethertype(self) -> str: + """The ethertype the remote prefix implies, which Neutron defaults to IPv4.""" + if self.remote_ip_prefix and ":" in self.remote_ip_prefix: + return "IPv6" + return "IPv4" + + class ScalesetConfig(BaseModel): """Scaleset configuration. @@ -234,6 +301,8 @@ class ScalesetConfig(BaseModel): enable_shell: Whether GARM's remote shell is enabled on the scaleset. pre_install_scripts: Script name to bash script pairs for pre-installation. image_visibility: Glance visibility the provider searches when resolving an image name. + security_group_rules: Rules of the security group the charm manages for the runners, + or None to manage no security group. """ name: str @@ -248,6 +317,7 @@ class ScalesetConfig(BaseModel): enable_shell: bool = False pre_install_scripts: str | None = None image_visibility: str | None = None + security_group_rules: list[SecurityGroupRule] | None = None @classmethod def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": @@ -329,9 +399,58 @@ def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": enable_shell=bool(charm.config.get(SCALESET_ENABLE_SHELL_CONFIG_NAME, False)), pre_install_scripts=pre_install_scripts, image_visibility=image_visibility, + security_group_rules=_parse_security_group_rules(charm), ) +def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule] | None: + """Parse the optional security-group-rules JSON document. + + Args: + charm: The charm instance. + + Raises: + CharmConfigInvalidError: If the value is not a JSON list of valid rules. + + Returns: + The parsed rules, or None when the option is unset — the charm then manages no + security group, so runners keep the project's default one. + """ + document = _get_optional_string_config(charm, SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME) + if document is None: + return None + try: + entries = json.loads(document) + except ValueError as exc: + raise CharmConfigInvalidError( + f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} must be a JSON list of rules" + ) from exc + if not isinstance(entries, list): + raise CharmConfigInvalidError( + f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} must be a JSON list of rules" + ) + + rules: list[SecurityGroupRule] = [] + for index, entry in enumerate(entries): + try: + rules.append(SecurityGroupRule.model_validate(entry)) + except ValidationError as exc: + raise CharmConfigInvalidError( + f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} rule {index}: " + f"{_describe_rule_error(exc)}" + ) from exc + return rules + + +def _describe_rule_error(exc: ValidationError) -> str: + """Render one rule's first validation error as a single status line.""" + error = exc.errors()[0] + field = ".".join(str(part) for part in error["loc"]) + # Pydantic prefixes the message of an error raised by a custom validator. + message = " ".join(str(error["msg"]).removeprefix("Value error, ").split()) + return f"{field}: {message}" if field else message + + def _get_optional_string_config(charm: ops.CharmBase, key: str) -> str | None: """Return a stripped optional string configuration value.""" value = charm.config.get(key) diff --git a/charms/garm-configurator/src/openstack_client.py b/charms/garm-configurator/src/openstack_client.py new file mode 100644 index 00000000..1b7a1370 --- /dev/null +++ b/charms/garm-configurator/src/openstack_client.py @@ -0,0 +1,316 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Minimal Keystone-authenticated client for the OpenStack networking API. + +The charm needs six calls — list, create, and delete security group rules, plus the group +itself — so it talks to Neutron's REST API directly rather than depending on a full OpenStack +SDK. charmcraft builds this charm's Python dependencies from source, and the SDK's +``cryptography`` dependency does not build without OpenSSL development headers, which the charm +build environment does not carry. + +Only the public interface and a project-scoped Keystone v3 password token are supported. +""" + +import dataclasses +import json +import typing +import urllib.error +import urllib.parse +import urllib.request + +from charm_state import ProviderConfig + +DEFAULT_TIMEOUT_SECONDS = 30 +_NETWORK_SERVICE_TYPE = "network" +_PUBLIC_INTERFACE = "public" +# Longest error detail copied into a status or log line. +MAX_DETAIL_LENGTH = 200 + + +class OpenStackClientError(Exception): + """Raised when OpenStack rejects a request or cannot be reached.""" + + +@dataclasses.dataclass(frozen=True) +class _Session: + """A Keystone token and the networking endpoint it authorises.""" + + token: str + endpoint: str + + +def _urlopen_transport( + method: str, url: str, headers: dict[str, str], body: bytes | None +) -> tuple[int, dict[str, str], bytes]: + """Send one request with the standard library, reporting HTTP errors as responses. + + Args: + method: The HTTP method. + url: The absolute URL to call. + headers: The request headers. + body: The request body, if any. + + Returns: + The status, response headers, and response body. A 4xx or 5xx from the server is + returned rather than raised, so the caller can report OpenStack's own message. + + Raises: + OpenStackClientError: If the server cannot be reached at all. + """ + if urllib.parse.urlsplit(url).scheme not in ("http", "https"): + raise OpenStackClientError(f"refusing to call a non-HTTP URL: {url}") + # The scheme is checked immediately above, so urlopen cannot be pointed at a file: URL. + request = urllib.request.Request( # noqa: S310 + url, data=body, headers=headers, method=method + ) + try: + with urllib.request.urlopen( # noqa: S310 # nosec B310 + request, timeout=DEFAULT_TIMEOUT_SECONDS + ) as response: + return response.status, dict(response.headers), response.read() + except urllib.error.HTTPError as exc: + return exc.code, dict(exc.headers or {}), exc.read() + except urllib.error.URLError as exc: + raise OpenStackClientError(f"cannot reach {url}: {exc.reason}") from exc + + +class NetworkClient: + """Talks to the Neutron API of the configured OpenStack project.""" + + def __init__( + self, + provider_config: ProviderConfig, + transport: typing.Callable[..., tuple[int, dict[str, str], bytes]] | None = None, + ) -> None: + """Initialise the client. + + Args: + provider_config: The OpenStack credentials and region to authenticate with. + transport: The HTTP transport, overridden by tests. + """ + self._config = provider_config + self._transport = transport or _urlopen_transport + self._session: _Session | None = None + + def security_groups(self) -> list[dict[str, typing.Any]]: + """Return every security group of the project.""" + return self._json_list("GET", "/security-groups", "security_groups") + + def create_security_group(self, name: str, description: str) -> dict[str, typing.Any]: + """Create a security group and return it, as Neutron reports it.""" + return self._json_object( + "POST", + "/security-groups", + "security_group", + {"security_group": {"name": name, "description": description}}, + ) + + def security_group_rules(self, security_group_id: str) -> list[dict[str, typing.Any]]: + """Return the rules of one security group.""" + query = urllib.parse.urlencode({"security_group_id": security_group_id}) + return self._json_list("GET", f"/security-group-rules?{query}", "security_group_rules") + + def create_security_group_rule( + self, security_group_id: str, fields: dict[str, object] + ) -> dict[str, typing.Any]: + """Create one rule of a security group and return it.""" + return self._json_object( + "POST", + "/security-group-rules", + "security_group_rule", + {"security_group_rule": {"security_group_id": security_group_id, **fields}}, + ) + + def delete_security_group_rule(self, rule_id: str) -> None: + """Delete one security group rule.""" + self._json("DELETE", f"/security-group-rules/{rule_id}") + + def _json( + self, method: str, path: str, body: dict[str, object] | None = None + ) -> dict[str, typing.Any]: + """Send a Neutron request and decode its JSON response.""" + payload = self._request(method, path, body) + return _decode(payload) if payload else {} + + def _json_list( + self, method: str, path: str, key: str, body: dict[str, object] | None = None + ) -> list[dict[str, typing.Any]]: + """Send a Neutron request and return the list it nests under *key*.""" + value = self._json(method, path, body).get(key) + if not isinstance(value, list): + raise OpenStackClientError(f"{method} {path} returned no {key!r} list") + return value + + def _json_object( + self, method: str, path: str, key: str, body: dict[str, object] | None = None + ) -> dict[str, typing.Any]: + """Send a Neutron request and return the object it nests under *key*.""" + value = self._json(method, path, body).get(key) + if not isinstance(value, dict): + raise OpenStackClientError(f"{method} {path} returned no {key!r} object") + return value + + def _request(self, method: str, path: str, body: dict[str, object] | None = None) -> bytes: + """Send one request to Neutron, authenticating first and once more on a 401.""" + data = None if body is None else json.dumps(body).encode("utf-8") + status, _, payload = self._send(method, path, data) + if status == 401: + # Keystone tokens expire, and the charm may hold one across a long reconcile. + self._session = None + status, _, payload = self._send(method, path, data) + if status >= 400: + raise OpenStackClientError( + f"{method} {path} failed with {status}: {_error_detail(payload)}" + ) + return payload + + def _send( + self, method: str, path: str, data: bytes | None + ) -> tuple[int, dict[str, str], bytes]: + """Send one request authenticated with the current session.""" + session = self._current_session() + headers = {"Accept": "application/json", "X-Auth-Token": session.token} + if data is not None: + headers["Content-Type"] = "application/json" + return self._transport(method, f"{session.endpoint}{path}", headers, data) + + def _current_session(self) -> _Session: + """Return the Keystone session, authenticating on first use.""" + session = self._session + if session is None: + session = self._authenticate() + return session + + def _authenticate(self) -> _Session: + """Request a Keystone token scoped to the configured project. + + Returns: + The session the token authorises. + + Raises: + OpenStackClientError: If Keystone rejects the credentials or returns no token or + no networking endpoint. + """ + config = self._config + body = { + "auth": { + "identity": { + "methods": ["password"], + "password": { + "user": { + "name": config.username, + "domain": {"name": config.user_domain_name}, + "password": config.password, + } + }, + }, + "scope": { + "project": { + "name": config.project_name, + "domain": {"name": config.project_domain_name}, + } + }, + } + } + url = f"{config.auth_url.rstrip('/')}/auth/tokens" + status, headers, payload = self._transport( + "POST", url, {"Content-Type": "application/json"}, json.dumps(body).encode("utf-8") + ) + if status >= 400: + raise OpenStackClientError( + f"Keystone rejected the credentials ({status}): {_error_detail(payload)}" + ) + token = _header(headers, "X-Subject-Token") + if not token: + raise OpenStackClientError("Keystone returned no token") + session = _Session(token=token, endpoint=_network_endpoint(payload, config.region_name)) + self._session = session + return session + + +def _header(headers: dict[str, str], name: str) -> str: + """Return a response header, matched case-insensitively.""" + lowered = name.lower() + for key, value in headers.items(): + if key.lower() == lowered: + return value + return "" + + +def _network_endpoint(payload: bytes, region_name: str) -> str: + """Return the public networking endpoint of the token's service catalog. + + Args: + payload: The Keystone token document. + region_name: The region to use, or an empty string for the first one listed. + + Returns: + The endpoint URL, without a trailing slash. + + Raises: + OpenStackClientError: If the catalog lists no matching endpoint. + """ + for endpoint in _network_endpoints(_decode(payload)): + url = _endpoint_url(endpoint, region_name) + if url: + return url + raise OpenStackClientError( + f"the Keystone catalog has no public networking endpoint for region {region_name or 'any'}" + ) + + +def _network_endpoints(document: dict[str, typing.Any]) -> list[dict[str, typing.Any]]: + """Return the endpoints the catalog lists for the networking service.""" + return [ + endpoint + for service in document.get("token", {}).get("catalog", []) + if service.get("type") == _NETWORK_SERVICE_TYPE + for endpoint in service.get("endpoints", []) + ] + + +def _endpoint_url(endpoint: dict[str, typing.Any], region_name: str) -> str: + """Return an endpoint's URL when it is the public one of the configured region.""" + if endpoint.get("interface") != _PUBLIC_INTERFACE: + return "" + if region_name and endpoint.get("region") != region_name: + return "" + return str(endpoint.get("url") or "").rstrip("/") + + +def _decode(payload: bytes) -> dict[str, typing.Any]: + """Decode a JSON object, or report that the response was not one.""" + try: + document = json.loads(payload) + except ValueError as exc: + raise OpenStackClientError("OpenStack returned a response that is not JSON") from exc + if not isinstance(document, dict): + raise OpenStackClientError("OpenStack returned a response that is not a JSON object") + return document + + +def _error_detail(payload: bytes) -> str: + """Return OpenStack's explanation of a failed request, bounded to one short line.""" + detail = " ".join(_error_message(payload).split()) + return detail[:MAX_DETAIL_LENGTH] or "no detail" + + +def _error_message(payload: bytes) -> str: + """Return the message inside a failed response, or the response itself.""" + try: + document = json.loads(payload) + except ValueError: + return payload.decode("utf-8", "replace") + if not isinstance(document, dict): + return str(document) + return _document_message(document) + + +def _document_message(document: dict[str, typing.Any]) -> str: + """Return the message of an error document, whatever it nests it under.""" + # Neutron nests its message under NeutronError; Keystone under error. + error = document.get("NeutronError") or document.get("error") or document + if isinstance(error, dict): + return str(error.get("message", error)) + return str(error) diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py new file mode 100644 index 00000000..033e1cdd --- /dev/null +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -0,0 +1,230 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Reconcile the OpenStack security group a scaleset's runners are attached to. + +Network isolation for the runners is policy, so the configurator owns it: it creates one +security group per scaleset and keeps its rules in line with the operator's +``security-group-rules``. The ``garm`` charm passes the group's id to the OpenStack provider +as the scaleset's ``security_groups`` extra spec. +""" + +import logging +import typing + +from charm_state import ProviderConfig, SecurityGroupRule +from openstack_client import NetworkClient, OpenStackClientError + +logger = logging.getLogger(__name__) + +SECURITY_GROUP_NAME_PREFIX = "garm-" +SECURITY_GROUP_DESCRIPTION = ( + "Managed by the garm-configurator charm; its rules follow the security-group-rules option." +) + +# The egress the charm grants when the operator configures none. Attaching a security +# group replaces the project's default group, which allows all egress; without this a +# runner could not reach GitHub and would never register. +DEFAULT_EGRESS_PREFIXES = ("0.0.0.0/0", "::/0") + +# What identifies a rule. Everything else OpenStack reports about one — its id, tenant, +# description — is either assigned by OpenStack or irrelevant to the desired state, so +# comparing only these makes the diff immune to it. +RULE_FIELDS = ( + "direction", + "ethertype", + "protocol", + "port_range_min", + "port_range_max", + "remote_ip_prefix", + "remote_group_id", + "remote_address_group_id", +) + +# Juju shows only the first line of a status message. +MAX_STATUS_REASON_LENGTH = 200 + + +class SecurityGroupReconcileError(Exception): + """Raised when the managed security group could not be reconciled with OpenStack.""" + + +def security_group_name(scaleset_name: str) -> str: + """Return the OpenStack name of the security group managed for a scaleset. + + The name is derived from the scaleset name alone, so a scaleset keeps the same group + across charm upgrades and restarts. + + Args: + scaleset_name: The scaleset name the operator configured. + + Returns: + The name of the group that carries the scaleset's rules. + """ + return f"{SECURITY_GROUP_NAME_PREFIX}{scaleset_name}" + + +class SecurityGroupReconciler: + """Creates and converges one charm-managed OpenStack security group.""" + + def __init__( + self, provider_config: ProviderConfig, client: NetworkClient | None = None + ) -> None: + """Initialise the reconciler. + + Args: + provider_config: The OpenStack credentials and region to reconcile in. + client: The OpenStack client, overridden by tests. + """ + self._provider_config = provider_config + self._client = client + + def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: + """Create the group if it is missing, then converge its rules. + + Args: + name: The group name, as returned by :func:`security_group_name`. + rules: The rules the group should hold. + + Returns: + The group's id, which the ``garm`` charm hands to the provider. + + Raises: + SecurityGroupReconcileError: If OpenStack could not be reached or refused a + change. The caller reports it and retries on the next event, because a + rejected call says nothing about the group's final state. + """ + client = self._client or NetworkClient(self._provider_config) + try: + group = self._ensure_group(client, name) + self._reconcile_rules(client, group, rules) + return str(group["id"]) + except OpenStackClientError as exc: + raise SecurityGroupReconcileError(_reason(exc)) from exc + + def _ensure_group(self, client: NetworkClient, name: str) -> dict[str, typing.Any]: + """Return the named group, creating it when the project has none. + + Args: + client: The OpenStack client. + name: The group name. + + Returns: + The security group, as Neutron reports it. + + Raises: + SecurityGroupReconcileError: If the name matches more than one group, which would + make the reconcile pick one at random. + """ + groups = [group for group in client.security_groups() if group.get("name") == name] + if len(groups) > 1: + raise SecurityGroupReconcileError( + f"OpenStack holds {len(groups)} security groups named {name}" + ) + if groups: + return groups[0] + logger.info("Creating security group %s", name) + return client.create_security_group(name, SECURITY_GROUP_DESCRIPTION) + + def _reconcile_rules( + self, + client: NetworkClient, + group: dict[str, typing.Any], + rules: list[SecurityGroupRule], + ) -> None: + """Add the configured rules, then remove the ones no longer configured. + + Additions come first so the group never loses a rule the runners may still be + using — every runner of the scaleset carries this group. + + Args: + client: The OpenStack client. + group: The security group to converge. + rules: The rules the group should hold. + """ + desired = _desired_rule_fields(rules) + observed = { + _rule_identity(_observed_rule_fields(rule)): rule + for rule in client.security_group_rules(str(group["id"])) + } + for identity, fields in desired.items(): + if identity in observed: + continue + logger.info("Adding rule %s to security group %s", identity, group.get("name")) + client.create_security_group_rule(str(group["id"]), fields) + for identity, rule in observed.items(): + if identity in desired: + continue + logger.info("Removing rule %s from security group %s", identity, group.get("name")) + client.delete_security_group_rule(str(rule["id"])) + + +def _desired_rule_fields(rules: list[SecurityGroupRule]) -> dict[tuple, dict[str, object]]: + """Return the fields to create for each rule, keyed by rule identity. + + A rule the operator lists twice yields one entry, so the group does not collect + duplicate rules that the next reconcile would then have to remove. + + Args: + rules: The rules the group should hold. + + Returns: + The rule fields to create, keyed by identity. + """ + desired: dict[tuple, dict[str, object]] = {} + for rule in _with_default_egress(rules): + fields = _rule_fields(rule) + desired.setdefault(_rule_identity(fields), fields) + return desired + + +def _with_default_egress(rules: list[SecurityGroupRule]) -> list[SecurityGroupRule]: + """Add the charm's default egress rules when the operator configured none. + + Args: + rules: The rules the operator configured. + + Returns: + The configured rules, plus all-egress rules for IPv4 and IPv6 when no egress rule + was configured, so a runner can always reach GitHub. + """ + if any(rule.direction == "egress" for rule in rules): + return list(rules) + return [ + *rules, + *( + SecurityGroupRule(direction="egress", remote_ip_prefix=prefix) + for prefix in DEFAULT_EGRESS_PREFIXES + ), + ] + + +def _rule_fields(rule: SecurityGroupRule) -> dict[str, object]: + """Translate a configured rule into the fields Neutron creates it from.""" + return { + "direction": rule.direction, + "ethertype": rule.ethertype, + # Neutron spells "every protocol" as a null protocol. + "protocol": None if rule.protocol == "any" else rule.protocol, + "port_range_min": rule.port_min, + "port_range_max": rule.port_max, + "remote_ip_prefix": rule.remote_ip_prefix, + } + + +def _observed_rule_fields(rule: dict[str, typing.Any]) -> dict[str, object]: + """Read the comparable fields of a rule as Neutron reports it.""" + return {field: rule.get(field) for field in RULE_FIELDS} + + +def _rule_identity(fields: dict[str, object]) -> tuple: + """Return what distinguishes one rule from another.""" + return tuple(fields.get(field) for field in RULE_FIELDS) + + +def _reason(exc: Exception) -> str: + """Return a bounded, single-line reason for a status message.""" + reason = " ".join(str(exc).split()) or type(exc).__name__ + if len(reason) > MAX_STATUS_REASON_LENGTH: + reason = f"{reason[:MAX_STATUS_REASON_LENGTH]}..." + return reason diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 5fd075e4..a50db574 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -11,6 +11,7 @@ from scenario import Context, Relation, Secret, State from charm import GarmConfiguratorCharm +from openstack_security_group import SecurityGroupReconcileError def _make_secret(): @@ -321,6 +322,34 @@ def test_clearing_configured_image_without_relation_withdraws_only_image(): "image-visibility must be one of: public, private, shared, community, all", id="invalid-image-visibility", ), + pytest.param( + {"security-group-rules": "not json"}, + "security-group-rules must be a JSON list of rules", + id="security-group-rules-not-json", + ), + pytest.param( + {"security-group-rules": '{"direction": "ingress"}'}, + "security-group-rules must be a JSON list of rules", + id="security-group-rules-not-a-list", + ), + pytest.param( + {"security-group-rules": '[{"direction": "sideways"}]'}, + "security-group-rules rule 0: direction: Input should be 'ingress' or 'egress'", + id="security-group-rules-invalid-direction", + ), + pytest.param( + { + "security-group-rules": '[{"direction": "ingress", "protocol": "icmp", "port_min": 22}]' + }, + "security-group-rules rule 0: port_min and port_max require protocol tcp or udp, " + "not icmp", + id="security-group-rules-ports-on-icmp", + ), + pytest.param( + {"security-group-rules": '[{"direction": "ingress", "remote_prefix": "0.0.0.0/0"}]'}, + "security-group-rules rule 0: remote_prefix: Extra inputs are not permitted", + id="security-group-rules-misspelt-field", + ), ], ) def test_charm_blocked_invalid_config(config_mutations: dict, expected_message: str): @@ -1042,3 +1071,158 @@ def test_image_visibility_written_to_garm_configurator_relation(configured, expe assert out.get_relation(garm_relation.id).local_unit_data.get("image_visibility", "") == ( expected ) + + +def _security_group_rules_config() -> str: + """Return a minimal valid security-group-rules document.""" + return '[{"direction": "ingress", "protocol": "tcp", "port_min": 22}]' + + +def test_security_group_id_written_to_garm_configurator_relation(monkeypatch): + """ + arrange: Rules configured for the managed group, and a reconciler that creates it. + act: Run config-changed with a garm-configurator relation present. + assert: The relation carries the group's id, and the reconciler was asked for the group + named after the scaleset, so GARM can attach the group to the runners. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["image"] = "runner-noble-amd64" + config["security-group-rules"] = _security_group_rules_config() + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + leader=True, + ) + calls = [] + + class _FakeReconciler: + def __init__(self, provider_config): + calls.append(("network", provider_config.network)) + + def reconcile(self, name, rules): + calls.append((name, [rule.direction for rule in rules])) + return "sg-1234" + + monkeypatch.setattr("charm.SecurityGroupReconciler", _FakeReconciler) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.unit_status == ops.ActiveStatus("Ready") + assert out.get_relation(garm_relation.id).local_unit_data["security_group_id"] == "sg-1234" + assert calls == [("network", "external-net"), ("garm-my-scaleset", ["ingress"])] + + +def test_security_group_not_managed_without_rules(monkeypatch): + """ + arrange: No security-group-rules configured, as in a deployment predating the option. + act: Run config-changed. + assert: No group is reconciled and the relation carries no id, so the runners keep the + project's default security group and existing deployments are unchanged. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["image"] = "runner-noble-amd64" + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + leader=True, + ) + + class _UnexpectedReconciler: + def __init__(self, provider_config): + raise AssertionError("no security group must be reconciled without rules") + + monkeypatch.setattr("charm.SecurityGroupReconciler", _UnexpectedReconciler) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.unit_status == ops.ActiveStatus("Ready") + assert "security_group_id" not in out.get_relation(garm_relation.id).local_unit_data + + +def test_clearing_security_group_rules_withdraws_the_id(monkeypatch): + """ + arrange: A deployment whose managed group id is on the relation. + act: Clear security-group-rules and run config-changed again. + assert: The id is withdrawn, so GARM stops attaching the group and the runners return to + the project's default security group. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["image"] = "runner-noble-amd64" + config["security-group-rules"] = _security_group_rules_config() + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + leader=True, + ) + + class _FakeReconciler: + def __init__(self, provider_config): + pass + + def reconcile(self, name, rules): + return "sg-1234" + + monkeypatch.setattr("charm.SecurityGroupReconciler", _FakeReconciler) + configured = ctx.run(ctx.on.config_changed(), state) + cleared_config = dict(config) + cleared_config["security-group-rules"] = "" + + cleared = ctx.run( + ctx.on.config_changed(), + dataclasses.replace(configured, config=cleared_config), + ) + + assert "security_group_id" not in cleared.get_relation(garm_relation.id).local_unit_data + + +def test_security_group_failure_blocks_and_publishes_nothing(monkeypatch): + """ + arrange: OpenStack refuses the reconcile. + act: Run config-changed. + assert: The unit blocks naming the group and withholds the scaleset configuration, so + GARM never acts on a group id the charm could not confirm. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["image"] = "runner-noble-amd64" + config["security-group-rules"] = _security_group_rules_config() + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + leader=True, + ) + + class _FailingReconciler: + def __init__(self, provider_config): + pass + + def reconcile(self, name, rules): + raise SecurityGroupReconcileError("keystone refused the token") + + monkeypatch.setattr("charm.SecurityGroupReconciler", _FailingReconciler) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.unit_status == ops.BlockedStatus( + "Security group garm-my-scaleset could not be reconciled: keystone refused the token" + ) + assert "name" not in out.get_relation(garm_relation.id).local_unit_data diff --git a/charms/garm-configurator/tests/unit/test_openstack_client.py b/charms/garm-configurator/tests/unit/test_openstack_client.py new file mode 100644 index 00000000..dc8ce673 --- /dev/null +++ b/charms/garm-configurator/tests/unit/test_openstack_client.py @@ -0,0 +1,482 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the minimal OpenStack networking client.""" + +import io +import json +import urllib.error + +import pytest + +from charm_state import ProviderConfig +from openstack_client import ( + MAX_DETAIL_LENGTH, + NetworkClient, + OpenStackClientError, + _urlopen_transport, +) + +CATALOG = { + "token": { + "catalog": [ + { + "type": "identity", + "endpoints": [ + { + "interface": "public", + "region": "RegionOne", + "url": "https://keystone.example.com:5000/v3", + } + ], + }, + { + "type": "network", + "endpoints": [ + { + "interface": "internal", + "region": "RegionOne", + "url": "http://neutron.internal:9696", + }, + { + "interface": "public", + "region": "RegionTwo", + "url": "https://neutron.two:9696", + }, + { + "interface": "public", + "region": "RegionOne", + "url": "https://neutron.example.com:9696/", + }, + ], + }, + ] + } +} + + +class FakeTransport: + """A transport that answers from a recorded queue of responses.""" + + def __init__(self, responses): + self.responses = list(responses) + self.requests = [] + + def __call__(self, method, url, headers, body): + self.requests.append({"method": method, "url": url, "headers": headers, "body": body}) + return self.responses.pop(0) + + +def _response(status=200, payload=None, headers=None): + """Build a transport response, JSON-encoding the payload.""" + body = b"" if payload is None else json.dumps(payload).encode("utf-8") + return status, headers or {}, body + + +def _provider_config(region="RegionOne"): + """Build the OpenStack credentials the client authenticates with.""" + return ProviderConfig( + auth_url="https://keystone.example.com:5000/v3", + username="admin", + password="s3cr3t", + project_name="myproject", + user_domain_name="Default", + project_domain_name="Default", + region_name=region, + network="external-net", + provider_name="garm-configurator-0", + ) + + +def _client(responses, region="RegionOne"): + """Build a client whose transport answers with *responses*.""" + transport = FakeTransport(responses) + return NetworkClient(_provider_config(region), transport=transport), transport + + +def _auth_response(): + """Return a successful Keystone token response.""" + return _response(201, CATALOG, {"X-Subject-Token": "token-1"}) + + +def test_authentication_scopes_a_password_token_to_the_project(): + """ + arrange: A Keystone that issues a token, and a project to scope it to. + act: List the project's security groups. + assert: The token request carries the password grant, the user and project domains, and + the project scope, so the token cannot act outside the configured project. + """ + client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) + + client.security_groups() + + auth = json.loads(transport.requests[0]["body"]) + assert transport.requests[0]["method"] == "POST" + assert transport.requests[0]["url"] == "https://keystone.example.com:5000/v3/auth/tokens" + assert auth == { + "auth": { + "identity": { + "methods": ["password"], + "password": { + "user": { + "name": "admin", + "domain": {"name": "Default"}, + "password": "s3cr3t", + } + }, + }, + "scope": {"project": {"name": "myproject", "domain": {"name": "Default"}}}, + } + } + + +def test_requests_use_the_public_endpoint_of_the_configured_region(): + """ + arrange: A catalog listing an internal endpoint and two public ones in different regions. + act: List the project's security groups. + assert: The request goes to the public endpoint of the configured region, without a + doubled slash. + """ + client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) + + client.security_groups() + + assert transport.requests[1]["url"] == "https://neutron.example.com:9696/security-groups" + + +def test_requests_carry_the_token(): + """ + arrange: A Keystone that issues a token. + act: List the project's security groups. + assert: The Neutron request authenticates with that token, so it acts as the configured + user rather than anonymously. + """ + client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) + + client.security_groups() + + assert transport.requests[1]["headers"]["X-Auth-Token"] == "token-1" + + +def test_token_is_reused_across_requests(): + """ + arrange: A client that has already authenticated. + act: Make two Neutron requests. + assert: Only one token is requested, so a reconcile does not re-authenticate per call. + """ + client, transport = _client( + [ + _auth_response(), + _response(200, {"security_groups": []}), + _response(200, {"security_group_rules": []}), + ] + ) + + client.security_groups() + client.security_group_rules("sg-1") + + assert [request["method"] for request in transport.requests] == ["POST", "GET", "GET"] + + +def test_expired_token_is_refreshed_once(): + """ + arrange: A Neutron that rejects the cached token as expired. + act: List the project's security groups. + assert: The client authenticates again and retries, so a token expiring between hooks + does not fail the reconcile. + """ + client, transport = _client( + [ + _auth_response(), + _response(401, {"error": {"message": "token expired"}}), + _auth_response(), + _response(200, {"security_groups": [{"id": "sg-1", "name": "garm-runner"}]}), + ] + ) + + groups = client.security_groups() + + assert groups == [{"id": "sg-1", "name": "garm-runner"}] + assert [request["method"] for request in transport.requests] == ["POST", "GET", "POST", "GET"] + assert transport.requests[3]["headers"]["X-Auth-Token"] == "token-1" + + +def test_rejected_credentials_are_reported_once_not_retried(): + """ + arrange: A Keystone that rejects the password. + act: List the project's security groups. + assert: The failure reports Keystone's own message, and no request is retried — bad + credentials do not resolve within a reconcile. + """ + client, transport = _client( + [ + _response( + 401, {"error": {"message": "The request you have made requires authentication"}} + ) + ] + ) + + with pytest.raises(OpenStackClientError) as exc_info: + client.security_groups() + + assert "Keystone rejected the credentials (401)" in str(exc_info.value) + assert "requires authentication" in str(exc_info.value) + assert len(transport.requests) == 1 + + +def test_missing_token_header_is_reported(): + """ + arrange: A Keystone that answers 201 but omits the token header. + act: List the project's security groups. + assert: The failure is reported rather than the client sending an empty token. + """ + client, _ = _client([_response(201, CATALOG)]) + + with pytest.raises(OpenStackClientError) as exc_info: + client.security_groups() + + assert str(exc_info.value) == "Keystone returned no token" + + +def test_missing_networking_endpoint_is_reported(): + """ + arrange: A catalog with no public networking endpoint for the configured region. + act: List the project's security groups. + assert: The failure names the region, so an operator can see which catalog entry is + missing. + """ + client, _ = _client([_auth_response()], region="RegionThree") + + with pytest.raises(OpenStackClientError) as exc_info: + client.security_groups() + + assert str(exc_info.value) == ( + "the Keystone catalog has no public networking endpoint for region RegionThree" + ) + + +def test_neutron_error_message_is_reported(): + """ + arrange: A Neutron that rejects a rule. + act: Create a rule. + assert: The failure carries Neutron's own explanation, which names the invalid field. + """ + client, _ = _client( + [ + _auth_response(), + _response(400, {"NeutronError": {"message": "Security group rule already exists"}}), + ] + ) + + with pytest.raises(OpenStackClientError) as exc_info: + client.create_security_group_rule("sg-1", {"direction": "ingress"}) + + assert "POST /security-group-rules failed with 400" in str(exc_info.value) + assert "Security group rule already exists" in str(exc_info.value) + + +def test_non_json_error_body_is_truncated(): + """ + arrange: A proxy answering with an HTML error page instead of JSON. + act: List the project's security groups. + assert: The reported detail is bounded, so a page of HTML cannot fill the status message. + """ + client, _ = _client( + [ + _auth_response(), + (502, {}, b"" + b"x" * (MAX_DETAIL_LENGTH * 2) + b""), + ] + ) + + with pytest.raises(OpenStackClientError) as exc_info: + client.security_groups() + + detail = str(exc_info.value).split(": ", 1)[1] + assert len(detail) == MAX_DETAIL_LENGTH + + +def test_rule_creation_sends_the_group_and_fields(): + """ + arrange: A group to add a rule to. + act: Create one rule. + assert: The body nests the fields under the Neutron resource name, with the group id. + """ + client, transport = _client( + [_auth_response(), _response(201, {"security_group_rule": {"id": "rule-1"}})] + ) + + client.create_security_group_rule("sg-1", {"direction": "ingress", "protocol": "tcp"}) + + assert json.loads(transport.requests[1]["body"]) == { + "security_group_rule": { + "security_group_id": "sg-1", + "direction": "ingress", + "protocol": "tcp", + } + } + + +def test_rule_listing_filters_by_group(): + """ + arrange: A group whose rules are wanted. + act: List the rules. + assert: The request filters by group id, so the reconcile never considers another group's + rules. + """ + client, transport = _client([_auth_response(), _response(200, {"security_group_rules": []})]) + + client.security_group_rules("sg 1") + + assert transport.requests[1]["url"] == ( + "https://neutron.example.com:9696/security-group-rules?security_group_id=sg+1" + ) + + +def test_rule_deletion_sends_no_body(): + """ + arrange: A rule to delete. + act: Delete it. + assert: The request is a bodyless DELETE, which Neutron accepts without a JSON body. + """ + client, transport = _client([_auth_response(), (204, {}, b"")]) + + client.delete_security_group_rule("rule-1") + + assert transport.requests[1]["method"] == "DELETE" + assert transport.requests[1]["url"] == ( + "https://neutron.example.com:9696/security-group-rules/rule-1" + ) + assert transport.requests[1]["body"] is None + + +def test_missing_response_key_is_reported(): + """ + arrange: A Neutron answering 200 with a body that lacks the expected key. + act: List the project's security groups. + assert: The failure is reported as a client error, so a malformed response blocks the unit + instead of crashing the hook with a KeyError. + """ + client, _ = _client([_auth_response(), _response(200, {"unexpected": []})]) + + with pytest.raises(OpenStackClientError) as exc_info: + client.security_groups() + + assert str(exc_info.value) == ("GET /security-groups returned no 'security_groups' list") + + +def test_group_creation_sends_the_name_and_description(): + """ + arrange: A project without the managed group. + act: Create the group. + assert: The body names it and marks it charm-managed, so an operator reading the OpenStack + console knows not to edit it by hand. + """ + client, transport = _client( + [_auth_response(), _response(201, {"security_group": {"id": "sg-1"}})] + ) + + client.create_security_group("garm-my-scaleset", "Managed by the charm") + + assert json.loads(transport.requests[1]["body"]) == { + "security_group": {"name": "garm-my-scaleset", "description": "Managed by the charm"} + } + + +class _FakeResponse: + """A response, as urlopen returns one.""" + + status = 200 + headers = {"Content-Type": "application/json"} + + def read(self): + return b'{"ok": true}' + + def __enter__(self): + return self + + def __exit__(self, *_exception): + return False + + +def test_transport_returns_the_response(monkeypatch): + """ + arrange: A server answering successfully. + act: Send a request through the default transport. + assert: The status, headers, and body reach the caller, so the client can decode them. + """ + sent = {} + + def urlopen(request, timeout=None): + sent["url"] = request.full_url + sent["timeout"] = timeout + return _FakeResponse() + + monkeypatch.setattr("urllib.request.urlopen", urlopen) + + status, headers, body = _urlopen_transport( + "GET", "https://neutron.example.com:9696/security-groups", {"Accept": "x"}, None + ) + + assert (status, headers, body) == (200, {"Content-Type": "application/json"}, b'{"ok": true}') + assert sent["url"] == "https://neutron.example.com:9696/security-groups" + + +def test_transport_returns_http_errors_as_responses(monkeypatch): + """ + arrange: A server rejecting the request. + act: Send it through the default transport. + assert: The error is returned rather than raised, so the client can report OpenStack's own + explanation instead of a bare traceback. + """ + + def urlopen(request, timeout=None): + raise urllib.error.HTTPError( + request.full_url, + 403, + "Forbidden", + {"X-Test": "1"}, + io.BytesIO(b'{"error": {"message": "Policy does not allow this request"}}'), + ) + + monkeypatch.setattr("urllib.request.urlopen", urlopen) + + status, headers, body = _urlopen_transport( + "POST", "https://neutron.example.com:9696/security-groups", {}, b"{}" + ) + + assert status == 403 + assert headers["X-Test"] == "1" + assert b"Policy does not allow" in body + + +def test_transport_reports_an_unreachable_server(monkeypatch): + """ + arrange: A server that cannot be reached at all. + act: Send a request through the default transport. + assert: The failure names the URL and the reason, so the blocked status tells an operator + what to check. + """ + + def urlopen(request, timeout=None): + raise urllib.error.URLError("connection refused") + + monkeypatch.setattr("urllib.request.urlopen", urlopen) + + with pytest.raises(OpenStackClientError) as exc_info: + _urlopen_transport("GET", "https://neutron.example.com:9696/security-groups", {}, None) + + assert str(exc_info.value) == ( + "cannot reach https://neutron.example.com:9696/security-groups: connection refused" + ) + + +def test_transport_refuses_a_non_http_url(): + """ + arrange: A URL whose scheme is not http or https. + act: Send a request through the default transport. + assert: The transport refuses, so a malformed catalog endpoint cannot read a local file. + """ + with pytest.raises(OpenStackClientError) as exc_info: + _urlopen_transport("GET", "file:///etc/passwd", {}, None) + + assert str(exc_info.value) == "refusing to call a non-HTTP URL: file:///etc/passwd" diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py new file mode 100644 index 00000000..ce12a933 --- /dev/null +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -0,0 +1,319 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the OpenStack security group reconciler.""" + +import typing + +import pytest + +from charm_state import ProviderConfig, SecurityGroupRule +from openstack_client import OpenStackClientError +from openstack_security_group import ( + MAX_STATUS_REASON_LENGTH, + SecurityGroupReconcileError, + SecurityGroupReconciler, + security_group_name, +) + +SCALESET_NAME = "my-scaleset" +GROUP_NAME = "garm-my-scaleset" + + +class FakeNetworkClient: + """An in-memory Neutron API holding one project's security groups.""" + + def __init__(self, groups: typing.Sequence[dict] = ()): + self.groups = [dict(group, rules=list(group.get("rules", []))) for group in groups] + self.created_groups: list[str] = [] + self.created_rules: list[dict] = [] + self.deleted_rules: list[str] = [] + self._next_id = 1 + + def security_groups(self): + return [{"id": group["id"], "name": group["name"]} for group in self.groups] + + def create_security_group(self, name, description): + group = {"id": f"sg-{self._next_id}", "name": name, "rules": []} + self._next_id += 1 + self.groups.append(group) + self.created_groups.append(name) + return group + + def security_group_rules(self, security_group_id): + return list(self._group(security_group_id)["rules"]) + + def create_security_group_rule(self, security_group_id, fields): + rule = {"id": f"rule-{self._next_id}", **fields} + self._next_id += 1 + self._group(security_group_id)["rules"].append(rule) + self.created_rules.append(fields) + return rule + + def delete_security_group_rule(self, rule_id): + self.deleted_rules.append(rule_id) + for group in self.groups: + group["rules"] = [rule for rule in group["rules"] if rule["id"] != rule_id] + + def _group(self, group_id): + return next(group for group in self.groups if group["id"] == group_id) + + +def _provider_config(): + """Build the OpenStack credentials the reconciler authenticates with.""" + return ProviderConfig( + auth_url="https://keystone.example.com:5000/v3", + username="admin", + password="s3cr3t", + project_name="myproject", + user_domain_name="Default", + project_domain_name="Default", + region_name="RegionOne", + network="external-net", + provider_name="garm-configurator-0", + ) + + +def _reconciler(client): + """Build a reconciler over *client*.""" + return SecurityGroupReconciler(_provider_config(), client=client) + + +def _rule_fields( + direction, protocol=None, port_min=None, port_max=None, prefix=None, ethertype="IPv4" +): + """Build the fields Neutron creates one rule from.""" + return { + "direction": direction, + "ethertype": ethertype, + "protocol": protocol, + "port_range_min": port_min, + "port_range_max": port_max, + "remote_ip_prefix": prefix, + } + + +def _stored_rule(**fields): + """Build a rule as Neutron reports it, with the id it assigned.""" + return {"id": f"rule-{fields.pop('rule_id', 99)}", **fields} + + +EGRESS_V4 = _rule_fields("egress", prefix="0.0.0.0/0") +EGRESS_V6 = _rule_fields("egress", prefix="::/0", ethertype="IPv6") +SSH_INGRESS = _rule_fields("ingress", protocol="tcp", port_min=22, port_max=22) +SSH_INGRESS_FROM_SUBNET = _rule_fields( + "ingress", protocol="tcp", port_min=22, port_max=22, prefix="10.0.0.0/8" +) + + +def test_security_group_name_is_derived_from_the_scaleset_name(): + """ + arrange: A scaleset name. + act: Derive the managed security group name. + assert: The name is prefixed, so it is recognisably charm-managed and stable across + restarts and upgrades. + """ + assert security_group_name(SCALESET_NAME) == GROUP_NAME + + +def test_missing_group_is_created_with_the_configured_and_default_egress_rules(): + """ + arrange: A project with no managed security group yet. + act: Reconcile one ingress rule. + assert: The group is created, holds that rule, and also allows all egress — without it a + runner could not reach GitHub, because OpenStack attaches this group instead of the + project's default group. + """ + client = FakeNetworkClient() + + group_id = _reconciler(client).reconcile( + GROUP_NAME, + [ + SecurityGroupRule( + direction="ingress", + protocol="tcp", + port_min=22, + port_max=22, + remote_ip_prefix="10.0.0.0/8", + ) + ], + ) + + assert client.created_groups == [GROUP_NAME] + assert client.created_rules == [SSH_INGRESS_FROM_SUBNET, EGRESS_V4, EGRESS_V6] + assert group_id == "sg-1" + + +def test_configured_egress_rule_replaces_the_default_egress(): + """ + arrange: A project with no managed security group, and rules that state their own egress. + act: Reconcile the rules. + assert: Only the configured egress rule is created, so the operator's narrower egress is + not widened by the charm's default. + """ + client = FakeNetworkClient() + + _reconciler(client).reconcile( + GROUP_NAME, + [SecurityGroupRule(direction="egress", protocol="tcp", port_min=443, port_max=443)], + ) + + assert client.created_rules == [ + _rule_fields("egress", protocol="tcp", port_min=443, port_max=443) + ] + + +def test_duplicate_configured_rules_are_created_once(): + """ + arrange: Rules that list the same ingress rule twice. + act: Reconcile the rules. + assert: One rule is created, so the group does not carry a duplicate the next reconcile + would then have to remove. + """ + client = FakeNetworkClient() + ssh = SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22) + + _reconciler(client).reconcile(GROUP_NAME, [ssh, ssh]) + + assert client.created_rules == [SSH_INGRESS, EGRESS_V4, EGRESS_V6] + + +def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(): + """ + arrange: A group that already holds the desired rules, with the ids Neutron assigned. + act: Reconcile the same rules again. + assert: The existing group and its rules are kept, so reconciling unchanged configuration + only adds the rule that is missing. + """ + client = FakeNetworkClient( + [ + { + "id": "sg-existing", + "name": GROUP_NAME, + "rules": [_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_V4)], + } + ] + ) + + group_id = _reconciler(client).reconcile( + GROUP_NAME, [SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22)] + ) + + assert group_id == "sg-existing" + assert client.created_groups == [] + assert client.deleted_rules == [] + assert client.created_rules == [EGRESS_V6] + + +def test_rules_no_longer_configured_are_deleted(): + """ + arrange: A group holding a rule the operator has since removed. + act: Reconcile the remaining rules. + assert: The stale rule is deleted and the kept one is not recreated, so the group matches + the option exactly. + """ + client = FakeNetworkClient( + [ + { + "id": "sg-existing", + "name": GROUP_NAME, + "rules": [ + _stored_rule(**SSH_INGRESS, rule_id=1), + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), + rule_id=2, + ), + ], + } + ] + ) + + _reconciler(client).reconcile( + GROUP_NAME, [SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22)] + ) + + assert client.deleted_rules == ["rule-2"] + assert client.created_rules == [EGRESS_V4, EGRESS_V6] + + +def test_a_rule_added_by_hand_is_removed(): + """ + arrange: A group holding a rule the operator never configured, such as one added with the + OpenStack CLI. + act: Reconcile. + assert: The rule is removed, because the option is the only source of the group's rules. + """ + client = FakeNetworkClient( + [ + { + "id": "sg-existing", + "name": GROUP_NAME, + "rules": [ + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), + rule_id=7, + ) + ], + } + ] + ) + + _reconciler(client).reconcile(GROUP_NAME, []) + + assert client.deleted_rules == ["rule-7"] + assert client.created_rules == [EGRESS_V4, EGRESS_V6] + + +def test_ambiguous_group_name_is_reported_rather_than_guessed(): + """ + arrange: Two security groups sharing the managed name. + act: Reconcile. + assert: The reconcile fails, because picking one of the two would converge the wrong group. + """ + client = FakeNetworkClient( + [ + {"id": "sg-1", "name": GROUP_NAME}, + {"id": "sg-2", "name": GROUP_NAME}, + ] + ) + + with pytest.raises(SecurityGroupReconcileError) as exc_info: + _reconciler(client).reconcile(GROUP_NAME, []) + + assert str(exc_info.value) == f"OpenStack holds 2 security groups named {GROUP_NAME}" + + +def test_openstack_failure_is_reported_as_a_single_line_reason(): + """ + arrange: An OpenStack that cannot be reached. + act: Reconcile. + assert: A SecurityGroupReconcileError carries the reason on one line, so the Blocked + status the charm sets stays readable. + """ + + class _UnreachableClient: + def security_groups(self): + raise OpenStackClientError("cannot reach https://keystone:\nconnection refused") + + with pytest.raises(SecurityGroupReconcileError) as exc_info: + _reconciler(_UnreachableClient()).reconcile(GROUP_NAME, []) + + assert str(exc_info.value) == "cannot reach https://keystone: connection refused" + + +def test_openstack_failure_reason_is_bounded(): + """ + arrange: An OpenStack call that fails with an over-long message. + act: Reconcile. + assert: The reported reason is truncated, so it cannot crowd out the rest of a status + message Juju shortens anyway. + """ + + class _UnreachableClient: + def security_groups(self): + raise OpenStackClientError("x" * (MAX_STATUS_REASON_LENGTH * 2)) + + with pytest.raises(SecurityGroupReconcileError) as exc_info: + _reconciler(_UnreachableClient()).reconcile(GROUP_NAME, []) + + assert len(str(exc_info.value)) == MAX_STATUS_REASON_LENGTH + len("...") diff --git a/docs/changelog.md b/docs/changelog.md index eb236167..c04e7a01 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -8,6 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Each revision is versioned by the date of the revision. +## 2026-10-20 + +- `garm-configurator`: add the optional `security-group-rules` configuration, which makes the charm create and maintain one OpenStack security group per scale set and publish its id to the `garm` charm, which passes it to the OpenStack provider so that every runner is attached to it. The group is named `garm-` and its rules are kept equal to the configured list, so rules added to it by hand are removed. The charm allows all egress when no egress rule is listed, because OpenStack attaches this group instead of the project's default group and a runner that cannot reach GitHub never runs a job. The group is never deleted: clearing the option, or removing the application, leaves it in place for the operator to remove. A rule change reaches runners created after the next reconcile; a runner that already exists keeps the group it was launched with. Deployments that do not set the option are unchanged and keep the project's default security group. + ## 2026-10-16 - `garm`: name runners after their scale set (for example `my-scaleset-dvnyp5dm3sm`) instead of GARM's default `garm-`. The prefix is the scale set name in lowercase, with every run of characters other than letters and digits replaced by a hyphen, and truncated to 40 characters so the runner name still fits a 63-character hostname. Existing scale sets are updated in place; runners created before the update keep their names. diff --git a/docs/reference/charms.md b/docs/reference/charms.md index 45d305ec..652786d5 100644 --- a/docs/reference/charms.md +++ b/docs/reference/charms.md @@ -74,6 +74,28 @@ options and defaults before applying the configuration. [Configurations for GARM configurator charm](https://charmhub.io/garm-configurator/configurations) ``` +### Security groups + +The optional `security-group-rules` option makes the charm manage one OpenStack security group +per scale set. The charm creates the group, named `garm-`, and keeps its rules equal to +the configured list: a rule that is not listed is removed, including one added by hand. The +group's id is published to the GARM charm, which passes it to the OpenStack provider so that +every runner of the scale set is attached to it. + +OpenStack attaches the configured groups instead of the project's default security group, so +the rules must allow the runner's outbound traffic to GitHub. When no egress rule is listed the +charm allows all egress for that reason. + +A rule change reaches new runners on the next reconcile. A runner that already exists keeps the +group it was launched with until GARM replaces it. + +The charm never deletes the group. Clearing the option detaches it from the scale set and +leaves it in place, as does removing the application, so an operator decides when to remove it. + +The OpenStack credentials need the `create_security_group`, `create_security_group_rule`, and +`delete_security_group_rule` permissions. The charm does not delete security groups, so it does +not need `delete_security_group`. + ### Integrations The GARM configurator charm integrates with a GARM charm. The GARM charm manages diff --git a/docs/reference/security.md b/docs/reference/security.md index 510e0075..1c33442e 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -39,6 +39,10 @@ operator responsibilities, see the {ref}`security overview Date: Tue, 20 Oct 2026 04:29:03 +0000 Subject: [PATCH 02/26] feat(garm): attach the managed security group to runners Read security_group_id from the configurator databag and pass it to the OpenStack provider as the scaleset's security_groups extra spec, so a runner boots with the group the configurator manages instead of the project's default group. A databag without the key leaves the extra spec unset, so existing deployments are unchanged. --- charms/garm/src/charm.py | 1 + charms/garm/src/scaleset_reconciler.py | 13 ++++- charms/garm/tests/unit/test_charm.py | 24 ++++++++ .../tests/unit/test_scaleset_reconciler.py | 57 +++++++++++++++++++ 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index a2c0ed2e..487dd7a1 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -732,6 +732,7 @@ def _build_scaleset_spec( template_id=template_id, runner_config=RunnerConfig.from_databag(data), image_visibility=data.get("image_visibility", "").strip(), + security_group_id=data.get("security_group_id", "").strip(), ) def _build_desired_credentials(self) -> list[CredentialSpec]: diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 2ad9399b..b49a683e 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -149,6 +149,7 @@ class ScalesetSpec: template_id: int | None = None runner_config: RunnerConfig = field(default_factory=RunnerConfig) image_visibility: str = "" + security_group_id: str = "" class ScalesetReconciler: @@ -1143,6 +1144,8 @@ def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> b or bool(observed_extra.get("disable_updates")) != bool(desired_extra.get("disable_updates")) or observed_extra.get("image_visibility") != desired_extra.get("image_visibility") + or (observed_extra.get("security_groups") or []) + != (desired_extra.get("security_groups") or []) or (observed.template_id or 0) != template_id or observed.runner_prefix != runner_prefix(spec.name) ) @@ -1332,8 +1335,9 @@ def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: Single source of truth for create, update, and drift detection: combines the operator-supplied pre-install scripts with the charm's aproxy bootstrap, the - ``disable_updates`` flag when a runner proxy is configured, and the image - visibility the provider searches when resolving an image name. + ``disable_updates`` flag when a runner proxy is configured, the image + visibility the provider searches when resolving an image name, and the + configurator-managed security group. Args: spec: The desired scaleset. @@ -1360,4 +1364,9 @@ def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: } if spec.image_visibility: extra_specs["image_visibility"] = spec.image_visibility + if spec.security_group_id: + # Nova attaches the groups it is given *instead of* the project's default + # group, which is why the configurator only publishes an id for a group it + # has already opened egress on. + extra_specs["security_groups"] = [spec.security_group_id] return extra_specs diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index e1a8f4df..9fe494d5 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1397,6 +1397,30 @@ def test_image_visibility_follows_the_configurator_databag( assert [spec.image_visibility for spec in specs] == [expected] +@pytest.mark.parametrize( + "databag_value, expected", + [("sg-1234", "sg-1234"), (" sg-1234 ", "sg-1234"), ("", ""), (None, "")], + ids=["set", "stripped", "empty", "absent"], +) +def test_security_group_follows_the_configurator_databag( + ctx: Context, garm_api: _GarmApiMocks, databag_value: str | None, expected: str +): + """ + arrange: A configurator unit publishing a managed security group id, a padded one, an + empty value, or nothing — the last two being a configurator that manages no group. + act: Run update-status. + assert: The scaleset spec carries the id, or none so GARM leaves the provider's default + group in place. + """ + data = {**_PROVIDER_UNIT_DATA, **_SCALESET_UNIT_DATA} + if databag_value is not None: + data["security_group_id"] = databag_value + ctx.run(ctx.on.update_status(), _state(configurator_units_data={0: data})) + + specs = garm_api.scaleset.return_value.reconcile.call_args[0][0] + assert [spec.security_group_id for spec in specs] == [expected] + + # --- Event wiring ------------------------------------------------------------------------- diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 99912ba5..c996fda8 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -258,6 +258,7 @@ def _spec( template_id=None, runner_config=None, image_visibility="", + security_group_id="", ): return ScalesetSpec( name=name, @@ -276,6 +277,7 @@ def _spec( template_id=template_id, runner_config=runner_config or RunnerConfig(), image_visibility=image_visibility, + security_group_id=security_group_id, ) @@ -1338,6 +1340,34 @@ def test_create_sends_image_visibility_as_an_extra_spec(): assert params.extra_specs == {"image_visibility": "private"} +def test_create_sends_the_configurator_security_group_as_an_extra_spec(): + """ + arrange: A spec whose configurator manages a security group for the scaleset. + act: Reconcile a create. + assert: The extra_specs attach that group, since Nova otherwise attaches the project's + default group to every runner. + """ + client = FakeGarmClient(providers=["openstack-demo"], scalesets=[]) + _reconcile(client, [_spec(security_group_id="sg-1234")]) + + _, _, params = client.created[0] + assert params.extra_specs == {"security_groups": ["sg-1234"]} + + +def test_create_omits_security_groups_when_unmanaged(): + """ + arrange: A spec from a configurator that manages no security group. + act: Reconcile a create. + assert: No security_groups extra spec is sent, leaving Nova's default group in place, so + deployments predating the option are unchanged. + """ + client = FakeGarmClient(providers=["openstack-demo"], scalesets=[]) + _reconcile(client, [_spec()]) + + _, _, params = client.created[0] + assert params.extra_specs is None + + @pytest.mark.parametrize( "observed_extra, desired, expected_extra", [ @@ -1365,6 +1395,33 @@ def test_update_converges_image_visibility(observed_extra, desired, expected_ext assert params.extra_specs == expected_extra +@pytest.mark.parametrize( + "observed_extra, desired, expected_extra", + [ + ({}, "sg-1234", {"security_groups": ["sg-1234"]}), + ({"security_groups": ["sg-old"]}, "sg-1234", {"security_groups": ["sg-1234"]}), + ({"security_groups": ["sg-1234"]}, "", {}), + ], + ids=["set", "changed", "cleared"], +) +def test_update_converges_the_security_group(observed_extra, desired, expected_extra): + """ + arrange: An existing scaleset attached to a different group, or to none. + act: Reconcile the spec. + assert: One update brings the extra_specs to the desired group, clearing it with an + explicit empty dict when the configurator no longer manages one. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(extra_specs=observed_extra)], + ) + _reconcile(client, [_spec(security_group_id=desired)]) + + assert len(client.updated) == 1 + _, params = client.updated[0] + assert params.extra_specs == expected_extra + + def test_no_update_when_image_visibility_already_matches(): """ arrange: An existing scaleset already carrying the spec's image_visibility. From 0155bc96817194dc34121adaecf34caf690510ee Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 05:18:47 +0000 Subject: [PATCH 03/26] fix(garm-configurator): use the OpenStack SDK and address review feedback Address the initial review: - Use the official OpenStack SDK instead of a hand-written Keystone and Neutron client. charmcraft installs requirements from source, so the charm now declares libssl-dev and pkg-config as build packages for the SDK's cryptography dependency. - Keep the reconcile control flow in _reconcile rather than a _configure helper. - Delete duplicate security groups that share the managed name, keeping the oldest, instead of blocking on the ambiguity. - Give security-group-rules a charmcraft default: all egress, no ingress, which is what a runner strictly needs. The implicit egress the reconciler added is gone; a configured list without an egress rule is rejected instead. - Restructure the port validation so each branch is visibly needed, and catch JSONDecodeError explicitly. - Shorten the changelog entry. --- charms/garm-configurator/charmcraft.yaml | 15 +- charms/garm-configurator/requirements.txt | 1 + charms/garm-configurator/src/charm.py | 37 +- charms/garm-configurator/src/charm_state.py | 32 +- .../garm-configurator/src/openstack_client.py | 316 ------------ .../src/openstack_security_group.py | 137 ++--- .../garm-configurator/tests/unit/conftest.py | 31 ++ .../tests/unit/test_charm.py | 101 ++-- .../tests/unit/test_openstack_client.py | 482 ------------------ .../unit/test_openstack_security_group.py | 352 ++++++++----- charms/garm/src/scaleset_reconciler.py | 8 +- .../tests/unit/test_scaleset_reconciler.py | 7 +- docs/changelog.md | 2 +- docs/reference/charms.md | 14 +- docs/reference/security.md | 1 + 15 files changed, 434 insertions(+), 1102 deletions(-) delete mode 100644 charms/garm-configurator/src/openstack_client.py create mode 100644 charms/garm-configurator/tests/unit/conftest.py delete mode 100644 charms/garm-configurator/tests/unit/test_openstack_client.py diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index f4a2d739..1805d86c 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -135,6 +135,7 @@ config: to bash script to be run prior to runner installation. security-group-rules: type: string + default: '[{"direction": "egress", "remote_ip_prefix": "0.0.0.0/0"}, {"direction": "egress", "remote_ip_prefix": "::/0"}]' description: | Optional JSON list of rules for the OpenStack security group the charm manages for this scaleset's runners. Each rule takes: @@ -148,10 +149,11 @@ config: this list, so rules added to the group by hand are removed. When no egress rule is listed the charm allows all egress, because OpenStack attaches this group instead of the project's default group and a runner that cannot reach - GitHub never runs a job. The group is never deleted: clearing this option, or - removing the application, leaves it in place for the operator to remove. - Requires OpenStack credentials allowed to create security groups and to create - and delete security group rules; it never deletes the group itself. + GitHub never runs a job. The group this charm manages is never deleted: clearing + this option, or removing the application, leaves it in place for the operator to + remove. A duplicate group that shares the name is deleted, keeping the oldest, + because OpenStack allows duplicate names. Requires OpenStack credentials allowed to + create and delete security groups and security group rules. dockerhub-mirror: type: string description: | @@ -194,3 +196,8 @@ parts: charm: source: . plugin: charm + # charmcraft installs requirements.txt with --no-binary=:all:, so the OpenStack SDK's + # cryptography dependency is built from source and needs the OpenSSL headers. + build-packages: + - libssl-dev + - pkg-config diff --git a/charms/garm-configurator/requirements.txt b/charms/garm-configurator/requirements.txt index 8879c8d8..f61f052c 100644 --- a/charms/garm-configurator/requirements.txt +++ b/charms/garm-configurator/requirements.txt @@ -1,2 +1,3 @@ +openstacksdk==4.21.0 ops==3.9.0 pydantic==2.14.1 diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 688dd582..c2ec5dac 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -66,21 +66,6 @@ def _reconcile(self, event: ops.EventBase) -> None: self.unit.status = ops.BlockedStatus(e.msg) return - if not self._configure(state): - return - self._set_ready_status(state) - - def _configure(self, state: CharmState) -> bool: - """Publish the resolved state, reconciling the scaleset's security group first. - - Args: - state: Current resolved charm state. - - Returns: - Whether the state was published. False means the unit is blocked on a security - group that could not be reconciled: the relations are then left untouched, so - GARM never acts on a group id the charm could not confirm. - """ self._update_image_relation(state) try: security_group_id = self._ensure_security_group(state) @@ -90,16 +75,9 @@ def _configure(self, state: CharmState) -> bool: self.unit.status = ops.BlockedStatus( f"Security group {name} could not be reconciled: {e}" ) - return False - self._configure_garm_relation(state, security_group_id or "") - return True - - def _set_ready_status(self, state: CharmState) -> None: - """Report readiness, which depends on where the runner image came from. + return + self._configure_garm_relation(state, security_group_id) - Args: - state: Current resolved charm state. - """ if state.image_overrides_builder: self.unit.status = ops.ActiveStatus("Ready; configured image overrides image builder") elif state.image is not None: @@ -131,24 +109,21 @@ def _update_image_relation(self, state: CharmState) -> None: } ) - def _ensure_security_group(self, state: CharmState) -> str | None: + def _ensure_security_group(self, state: CharmState) -> str: """Reconcile the security group the scaleset's runners are attached to. Args: state: Current resolved charm state. Returns: - The id of the group, or None when the operator configured no rules — the - runners then keep the project's default security group. + The id of the group the runners are attached to. Raises: SecurityGroupReconcileError: If OpenStack could not be reconciled with. """ - rules = state.scaleset_config.security_group_rules - if rules is None: - return None return SecurityGroupReconciler(state.provider_config).reconcile( - security_group_name(state.scaleset_config.name), rules + security_group_name(state.scaleset_config.name), + state.scaleset_config.security_group_rules, ) def _ensure_relation_secret( diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index e2b3489c..63352c6c 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -261,14 +261,15 @@ def _validate_cidr(cls, value: str | None) -> str | None: @model_validator(mode="after") def _validate_ports(self) -> "SecurityGroupRule": """Reject a port range the protocol cannot carry, and fill in a single port.""" - if self.port_min is None and self.port_max is None: + # A range needs a lower bound; Neutron has no "everything up to" form. + if self.port_max is not None and self.port_min is None: + raise ValueError("port_max requires port_min") + if self.port_min is None: return self if self.protocol not in ("tcp", "udp"): raise ValueError( f"port_min and port_max require protocol tcp or udp, not {self.protocol}" ) - if self.port_min is None: - raise ValueError("port_max requires port_min") port_max = self.port_min if self.port_max is None else self.port_max if not 1 <= self.port_min <= 65535 or not 1 <= port_max <= 65535: raise ValueError("port_min and port_max must be in 1..65535") @@ -301,8 +302,7 @@ class ScalesetConfig(BaseModel): enable_shell: Whether GARM's remote shell is enabled on the scaleset. pre_install_scripts: Script name to bash script pairs for pre-installation. image_visibility: Glance visibility the provider searches when resolving an image name. - security_group_rules: Rules of the security group the charm manages for the runners, - or None to manage no security group. + security_group_rules: Rules of the security group the charm manages for the runners. """ name: str @@ -317,7 +317,7 @@ class ScalesetConfig(BaseModel): enable_shell: bool = False pre_install_scripts: str | None = None image_visibility: str | None = None - security_group_rules: list[SecurityGroupRule] | None = None + security_group_rules: list[SecurityGroupRule] @classmethod def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": @@ -403,25 +403,27 @@ def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": ) -def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule] | None: +def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule]: """Parse the optional security-group-rules JSON document. Args: charm: The charm instance. Raises: - CharmConfigInvalidError: If the value is not a JSON list of valid rules. + CharmConfigInvalidError: If the value is not a JSON list of valid rules, or lists no + egress rule. Returns: - The parsed rules, or None when the option is unset — the charm then manages no - security group, so runners keep the project's default one. + The parsed rules. The charmcraft default lists all egress, so the option is always + set in practice. """ document = _get_optional_string_config(charm, SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME) if document is None: - return None + # Unset only if the charmcraft default is dropped from the metadata. + return [] try: entries = json.loads(document) - except ValueError as exc: + except json.JSONDecodeError as exc: raise CharmConfigInvalidError( f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} must be a JSON list of rules" ) from exc @@ -439,6 +441,12 @@ def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule] f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} rule {index}: " f"{_describe_rule_error(exc)}" ) from exc + if not any(rule.direction == "egress" for rule in rules): + raise CharmConfigInvalidError( + f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} needs at least one egress rule: " + f"OpenStack attaches this group instead of the project's default group, so a " + f"runner with no egress cannot reach GitHub" + ) return rules diff --git a/charms/garm-configurator/src/openstack_client.py b/charms/garm-configurator/src/openstack_client.py deleted file mode 100644 index 1b7a1370..00000000 --- a/charms/garm-configurator/src/openstack_client.py +++ /dev/null @@ -1,316 +0,0 @@ -# Copyright 2026 Canonical Ltd. -# See LICENSE file for licensing details. - -"""Minimal Keystone-authenticated client for the OpenStack networking API. - -The charm needs six calls — list, create, and delete security group rules, plus the group -itself — so it talks to Neutron's REST API directly rather than depending on a full OpenStack -SDK. charmcraft builds this charm's Python dependencies from source, and the SDK's -``cryptography`` dependency does not build without OpenSSL development headers, which the charm -build environment does not carry. - -Only the public interface and a project-scoped Keystone v3 password token are supported. -""" - -import dataclasses -import json -import typing -import urllib.error -import urllib.parse -import urllib.request - -from charm_state import ProviderConfig - -DEFAULT_TIMEOUT_SECONDS = 30 -_NETWORK_SERVICE_TYPE = "network" -_PUBLIC_INTERFACE = "public" -# Longest error detail copied into a status or log line. -MAX_DETAIL_LENGTH = 200 - - -class OpenStackClientError(Exception): - """Raised when OpenStack rejects a request or cannot be reached.""" - - -@dataclasses.dataclass(frozen=True) -class _Session: - """A Keystone token and the networking endpoint it authorises.""" - - token: str - endpoint: str - - -def _urlopen_transport( - method: str, url: str, headers: dict[str, str], body: bytes | None -) -> tuple[int, dict[str, str], bytes]: - """Send one request with the standard library, reporting HTTP errors as responses. - - Args: - method: The HTTP method. - url: The absolute URL to call. - headers: The request headers. - body: The request body, if any. - - Returns: - The status, response headers, and response body. A 4xx or 5xx from the server is - returned rather than raised, so the caller can report OpenStack's own message. - - Raises: - OpenStackClientError: If the server cannot be reached at all. - """ - if urllib.parse.urlsplit(url).scheme not in ("http", "https"): - raise OpenStackClientError(f"refusing to call a non-HTTP URL: {url}") - # The scheme is checked immediately above, so urlopen cannot be pointed at a file: URL. - request = urllib.request.Request( # noqa: S310 - url, data=body, headers=headers, method=method - ) - try: - with urllib.request.urlopen( # noqa: S310 # nosec B310 - request, timeout=DEFAULT_TIMEOUT_SECONDS - ) as response: - return response.status, dict(response.headers), response.read() - except urllib.error.HTTPError as exc: - return exc.code, dict(exc.headers or {}), exc.read() - except urllib.error.URLError as exc: - raise OpenStackClientError(f"cannot reach {url}: {exc.reason}") from exc - - -class NetworkClient: - """Talks to the Neutron API of the configured OpenStack project.""" - - def __init__( - self, - provider_config: ProviderConfig, - transport: typing.Callable[..., tuple[int, dict[str, str], bytes]] | None = None, - ) -> None: - """Initialise the client. - - Args: - provider_config: The OpenStack credentials and region to authenticate with. - transport: The HTTP transport, overridden by tests. - """ - self._config = provider_config - self._transport = transport or _urlopen_transport - self._session: _Session | None = None - - def security_groups(self) -> list[dict[str, typing.Any]]: - """Return every security group of the project.""" - return self._json_list("GET", "/security-groups", "security_groups") - - def create_security_group(self, name: str, description: str) -> dict[str, typing.Any]: - """Create a security group and return it, as Neutron reports it.""" - return self._json_object( - "POST", - "/security-groups", - "security_group", - {"security_group": {"name": name, "description": description}}, - ) - - def security_group_rules(self, security_group_id: str) -> list[dict[str, typing.Any]]: - """Return the rules of one security group.""" - query = urllib.parse.urlencode({"security_group_id": security_group_id}) - return self._json_list("GET", f"/security-group-rules?{query}", "security_group_rules") - - def create_security_group_rule( - self, security_group_id: str, fields: dict[str, object] - ) -> dict[str, typing.Any]: - """Create one rule of a security group and return it.""" - return self._json_object( - "POST", - "/security-group-rules", - "security_group_rule", - {"security_group_rule": {"security_group_id": security_group_id, **fields}}, - ) - - def delete_security_group_rule(self, rule_id: str) -> None: - """Delete one security group rule.""" - self._json("DELETE", f"/security-group-rules/{rule_id}") - - def _json( - self, method: str, path: str, body: dict[str, object] | None = None - ) -> dict[str, typing.Any]: - """Send a Neutron request and decode its JSON response.""" - payload = self._request(method, path, body) - return _decode(payload) if payload else {} - - def _json_list( - self, method: str, path: str, key: str, body: dict[str, object] | None = None - ) -> list[dict[str, typing.Any]]: - """Send a Neutron request and return the list it nests under *key*.""" - value = self._json(method, path, body).get(key) - if not isinstance(value, list): - raise OpenStackClientError(f"{method} {path} returned no {key!r} list") - return value - - def _json_object( - self, method: str, path: str, key: str, body: dict[str, object] | None = None - ) -> dict[str, typing.Any]: - """Send a Neutron request and return the object it nests under *key*.""" - value = self._json(method, path, body).get(key) - if not isinstance(value, dict): - raise OpenStackClientError(f"{method} {path} returned no {key!r} object") - return value - - def _request(self, method: str, path: str, body: dict[str, object] | None = None) -> bytes: - """Send one request to Neutron, authenticating first and once more on a 401.""" - data = None if body is None else json.dumps(body).encode("utf-8") - status, _, payload = self._send(method, path, data) - if status == 401: - # Keystone tokens expire, and the charm may hold one across a long reconcile. - self._session = None - status, _, payload = self._send(method, path, data) - if status >= 400: - raise OpenStackClientError( - f"{method} {path} failed with {status}: {_error_detail(payload)}" - ) - return payload - - def _send( - self, method: str, path: str, data: bytes | None - ) -> tuple[int, dict[str, str], bytes]: - """Send one request authenticated with the current session.""" - session = self._current_session() - headers = {"Accept": "application/json", "X-Auth-Token": session.token} - if data is not None: - headers["Content-Type"] = "application/json" - return self._transport(method, f"{session.endpoint}{path}", headers, data) - - def _current_session(self) -> _Session: - """Return the Keystone session, authenticating on first use.""" - session = self._session - if session is None: - session = self._authenticate() - return session - - def _authenticate(self) -> _Session: - """Request a Keystone token scoped to the configured project. - - Returns: - The session the token authorises. - - Raises: - OpenStackClientError: If Keystone rejects the credentials or returns no token or - no networking endpoint. - """ - config = self._config - body = { - "auth": { - "identity": { - "methods": ["password"], - "password": { - "user": { - "name": config.username, - "domain": {"name": config.user_domain_name}, - "password": config.password, - } - }, - }, - "scope": { - "project": { - "name": config.project_name, - "domain": {"name": config.project_domain_name}, - } - }, - } - } - url = f"{config.auth_url.rstrip('/')}/auth/tokens" - status, headers, payload = self._transport( - "POST", url, {"Content-Type": "application/json"}, json.dumps(body).encode("utf-8") - ) - if status >= 400: - raise OpenStackClientError( - f"Keystone rejected the credentials ({status}): {_error_detail(payload)}" - ) - token = _header(headers, "X-Subject-Token") - if not token: - raise OpenStackClientError("Keystone returned no token") - session = _Session(token=token, endpoint=_network_endpoint(payload, config.region_name)) - self._session = session - return session - - -def _header(headers: dict[str, str], name: str) -> str: - """Return a response header, matched case-insensitively.""" - lowered = name.lower() - for key, value in headers.items(): - if key.lower() == lowered: - return value - return "" - - -def _network_endpoint(payload: bytes, region_name: str) -> str: - """Return the public networking endpoint of the token's service catalog. - - Args: - payload: The Keystone token document. - region_name: The region to use, or an empty string for the first one listed. - - Returns: - The endpoint URL, without a trailing slash. - - Raises: - OpenStackClientError: If the catalog lists no matching endpoint. - """ - for endpoint in _network_endpoints(_decode(payload)): - url = _endpoint_url(endpoint, region_name) - if url: - return url - raise OpenStackClientError( - f"the Keystone catalog has no public networking endpoint for region {region_name or 'any'}" - ) - - -def _network_endpoints(document: dict[str, typing.Any]) -> list[dict[str, typing.Any]]: - """Return the endpoints the catalog lists for the networking service.""" - return [ - endpoint - for service in document.get("token", {}).get("catalog", []) - if service.get("type") == _NETWORK_SERVICE_TYPE - for endpoint in service.get("endpoints", []) - ] - - -def _endpoint_url(endpoint: dict[str, typing.Any], region_name: str) -> str: - """Return an endpoint's URL when it is the public one of the configured region.""" - if endpoint.get("interface") != _PUBLIC_INTERFACE: - return "" - if region_name and endpoint.get("region") != region_name: - return "" - return str(endpoint.get("url") or "").rstrip("/") - - -def _decode(payload: bytes) -> dict[str, typing.Any]: - """Decode a JSON object, or report that the response was not one.""" - try: - document = json.loads(payload) - except ValueError as exc: - raise OpenStackClientError("OpenStack returned a response that is not JSON") from exc - if not isinstance(document, dict): - raise OpenStackClientError("OpenStack returned a response that is not a JSON object") - return document - - -def _error_detail(payload: bytes) -> str: - """Return OpenStack's explanation of a failed request, bounded to one short line.""" - detail = " ".join(_error_message(payload).split()) - return detail[:MAX_DETAIL_LENGTH] or "no detail" - - -def _error_message(payload: bytes) -> str: - """Return the message inside a failed response, or the response itself.""" - try: - document = json.loads(payload) - except ValueError: - return payload.decode("utf-8", "replace") - if not isinstance(document, dict): - return str(document) - return _document_message(document) - - -def _document_message(document: dict[str, typing.Any]) -> str: - """Return the message of an error document, whatever it nests it under.""" - # Neutron nests its message under NeutronError; Keystone under error. - error = document.get("NeutronError") or document.get("error") or document - if isinstance(error, dict): - return str(error.get("message", error)) - return str(error) diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py index 033e1cdd..627db4fd 100644 --- a/charms/garm-configurator/src/openstack_security_group.py +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -12,8 +12,9 @@ import logging import typing +import openstack + from charm_state import ProviderConfig, SecurityGroupRule -from openstack_client import NetworkClient, OpenStackClientError logger = logging.getLogger(__name__) @@ -22,11 +23,6 @@ "Managed by the garm-configurator charm; its rules follow the security-group-rules option." ) -# The egress the charm grants when the operator configures none. Attaching a security -# group replaces the project's default group, which allows all egress; without this a -# runner could not reach GitHub and would never register. -DEFAULT_EGRESS_PREFIXES = ("0.0.0.0/0", "::/0") - # What identifies a rule. Everything else OpenStack reports about one — its id, tenant, # description — is either assigned by OpenStack or irrelevant to the desired state, so # comparing only these makes the diff immune to it. @@ -68,16 +64,20 @@ class SecurityGroupReconciler: """Creates and converges one charm-managed OpenStack security group.""" def __init__( - self, provider_config: ProviderConfig, client: NetworkClient | None = None + self, + provider_config: ProviderConfig, + connect: typing.Callable[..., typing.Any] | None = None, ) -> None: """Initialise the reconciler. Args: provider_config: The OpenStack credentials and region to reconcile in. - client: The OpenStack client, overridden by tests. + connect: The OpenStack SDK entrypoint, overridden by tests. """ self._provider_config = provider_config - self._client = client + # Typed as an opaque callable: the SDK's own signature is narrower than the + # keyword arguments a connection takes, but is what must be called at runtime. + self._connect: typing.Callable[..., typing.Any] = connect or openstack.connect def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: """Create the group if it is missing, then converge its rules. @@ -94,43 +94,71 @@ def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: change. The caller reports it and retries on the next event, because a rejected call says nothing about the group's final state. """ - client = self._client or NetworkClient(self._provider_config) + # Nothing here may crash the hook: every failure is reported and retried, so the + # next reconcile looks again either way. try: - group = self._ensure_group(client, name) - self._reconcile_rules(client, group, rules) - return str(group["id"]) - except OpenStackClientError as exc: + connection = self._connect(**self._auth_fields()) + try: + group = self._ensure_group(connection, name) + self._reconcile_rules(connection, group, rules) + return str(group.id) + finally: + connection.close() + except Exception as exc: raise SecurityGroupReconcileError(_reason(exc)) from exc - def _ensure_group(self, client: NetworkClient, name: str) -> dict[str, typing.Any]: + def _auth_fields(self) -> dict[str, str]: + """Return the keyword arguments the OpenStack SDK authenticates with.""" + config = self._provider_config + return { + "auth_url": config.auth_url, + "username": config.username, + "password": config.password, + "project_name": config.project_name, + "user_domain_name": config.user_domain_name, + "project_domain_name": config.project_domain_name, + "region_name": config.region_name, + "interface": "public", + } + + def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: """Return the named group, creating it when the project has none. + The oldest group of that name wins, so two configurators racing to create it settle + on the same survivor instead of deleting each other's group on every reconcile. + Neutron refuses to delete a group a port still references, so an obsolete duplicate + that something else attached to blocks the reconcile rather than breaking that + port's networking. + Args: - client: The OpenStack client. + connection: The authenticated OpenStack connection. name: The group name. Returns: - The security group, as Neutron reports it. - - Raises: - SecurityGroupReconcileError: If the name matches more than one group, which would - make the reconcile pick one at random. + The security group this charm manages. """ - groups = [group for group in client.security_groups() if group.get("name") == name] - if len(groups) > 1: - raise SecurityGroupReconcileError( - f"OpenStack holds {len(groups)} security groups named {name}" + groups = sorted( + (group for group in connection.network.security_groups() if group.name == name), + key=_age, + ) + for obsolete in groups[1:]: + logger.warning( + "Deleting security group %s (%s): %s (%s) is the group this charm manages", + name, + obsolete.id, + name, + groups[0].id, ) + connection.network.delete_security_group(obsolete.id) if groups: return groups[0] logger.info("Creating security group %s", name) - return client.create_security_group(name, SECURITY_GROUP_DESCRIPTION) + return connection.network.create_security_group( + name=name, description=SECURITY_GROUP_DESCRIPTION + ) def _reconcile_rules( - self, - client: NetworkClient, - group: dict[str, typing.Any], - rules: list[SecurityGroupRule], + self, connection: typing.Any, group: typing.Any, rules: list[SecurityGroupRule] ) -> None: """Add the configured rules, then remove the ones no longer configured. @@ -138,25 +166,25 @@ def _reconcile_rules( using — every runner of the scaleset carries this group. Args: - client: The OpenStack client. + connection: The authenticated OpenStack connection. group: The security group to converge. rules: The rules the group should hold. """ desired = _desired_rule_fields(rules) observed = { _rule_identity(_observed_rule_fields(rule)): rule - for rule in client.security_group_rules(str(group["id"])) + for rule in connection.network.security_group_rules(security_group_id=group.id) } for identity, fields in desired.items(): if identity in observed: continue - logger.info("Adding rule %s to security group %s", identity, group.get("name")) - client.create_security_group_rule(str(group["id"]), fields) + logger.info("Adding rule %s to security group %s", identity, group.name) + connection.network.create_security_group_rule(security_group_id=group.id, **fields) for identity, rule in observed.items(): if identity in desired: continue - logger.info("Removing rule %s from security group %s", identity, group.get("name")) - client.delete_security_group_rule(str(rule["id"])) + logger.info("Removing rule %s from security group %s", identity, group.name) + connection.network.delete_security_group_rule(rule) def _desired_rule_fields(rules: list[SecurityGroupRule]) -> dict[tuple, dict[str, object]]: @@ -172,35 +200,14 @@ def _desired_rule_fields(rules: list[SecurityGroupRule]) -> dict[tuple, dict[str The rule fields to create, keyed by identity. """ desired: dict[tuple, dict[str, object]] = {} - for rule in _with_default_egress(rules): + for rule in rules: fields = _rule_fields(rule) desired.setdefault(_rule_identity(fields), fields) return desired -def _with_default_egress(rules: list[SecurityGroupRule]) -> list[SecurityGroupRule]: - """Add the charm's default egress rules when the operator configured none. - - Args: - rules: The rules the operator configured. - - Returns: - The configured rules, plus all-egress rules for IPv4 and IPv6 when no egress rule - was configured, so a runner can always reach GitHub. - """ - if any(rule.direction == "egress" for rule in rules): - return list(rules) - return [ - *rules, - *( - SecurityGroupRule(direction="egress", remote_ip_prefix=prefix) - for prefix in DEFAULT_EGRESS_PREFIXES - ), - ] - - def _rule_fields(rule: SecurityGroupRule) -> dict[str, object]: - """Translate a configured rule into the fields Neutron creates it from.""" + """Translate a configured rule into the fields OpenStack creates it from.""" return { "direction": rule.direction, "ethertype": rule.ethertype, @@ -212,9 +219,9 @@ def _rule_fields(rule: SecurityGroupRule) -> dict[str, object]: } -def _observed_rule_fields(rule: dict[str, typing.Any]) -> dict[str, object]: - """Read the comparable fields of a rule as Neutron reports it.""" - return {field: rule.get(field) for field in RULE_FIELDS} +def _observed_rule_fields(rule: typing.Any) -> dict[str, object]: + """Read the comparable fields of a rule as OpenStack reports it.""" + return {field: getattr(rule, field, None) for field in RULE_FIELDS} def _rule_identity(fields: dict[str, object]) -> tuple: @@ -222,6 +229,12 @@ def _rule_identity(fields: dict[str, object]) -> tuple: return tuple(fields.get(field) for field in RULE_FIELDS) +def _age(group: typing.Any) -> str: + """Return the sort key that puts the oldest security group first.""" + created = getattr(group, "created_at", None) + return "" if created is None else str(created) + + def _reason(exc: Exception) -> str: """Return a bounded, single-line reason for a status message.""" reason = " ".join(str(exc).split()) or type(exc).__name__ diff --git a/charms/garm-configurator/tests/unit/conftest.py b/charms/garm-configurator/tests/unit/conftest.py new file mode 100644 index 00000000..455709b9 --- /dev/null +++ b/charms/garm-configurator/tests/unit/conftest.py @@ -0,0 +1,31 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Shared fixtures for the charm's unit tests.""" + +import pytest + +DEFAULT_SECURITY_GROUP_ID = "sg-1234" + + +@pytest.fixture(autouse=True) +def security_group_calls(monkeypatch): + """Keep the charm's OpenStack calls out of the unit tests. + + ops applies the charmcraft default of ``security-group-rules``, so every reconcile + manages the scale set's security group. A test that cares about the call asks for this + fixture and inspects the recorded calls; one that cares about a failure replaces the + reconciler itself. + """ + calls = [] + + class _FakeReconciler: + def __init__(self, provider_config): + calls.append(("init", provider_config)) + + def reconcile(self, name, rules): + calls.append((name, rules)) + return DEFAULT_SECURITY_GROUP_ID + + monkeypatch.setattr("charm.SecurityGroupReconciler", _FakeReconciler) + return calls diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index a50db574..3d98d1d0 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -350,6 +350,20 @@ def test_clearing_configured_image_without_relation_withdraws_only_image(): "security-group-rules rule 0: remote_prefix: Extra inputs are not permitted", id="security-group-rules-misspelt-field", ), + pytest.param( + { + "security-group-rules": '[{"direction": "egress", "protocol": "tcp", "port_max": 80}]' + }, + "security-group-rules rule 0: port_max requires port_min", + id="security-group-rules-port-max-without-min", + ), + pytest.param( + {"security-group-rules": '[{"direction": "ingress"}]'}, + "security-group-rules needs at least one egress rule: OpenStack attaches this " + "group instead of the project's default group, so a runner with no egress " + "cannot reach GitHub", + id="security-group-rules-without-egress", + ), ], ) def test_charm_blocked_invalid_config(config_mutations: dict, expected_message: str): @@ -1075,15 +1089,16 @@ def test_image_visibility_written_to_garm_configurator_relation(configured, expe def _security_group_rules_config() -> str: """Return a minimal valid security-group-rules document.""" - return '[{"direction": "ingress", "protocol": "tcp", "port_min": 22}]' + return '[{"direction": "ingress", "protocol": "tcp", "port_min": 22}, {"direction": "egress"}]' -def test_security_group_id_written_to_garm_configurator_relation(monkeypatch): +def test_security_group_id_written_to_garm_configurator_relation(security_group_calls): """ - arrange: Rules configured for the managed group, and a reconciler that creates it. + arrange: Rules configured for the managed group. act: Run config-changed with a garm-configurator relation present. - assert: The relation carries the group's id, and the reconciler was asked for the group - named after the scaleset, so GARM can attach the group to the runners. + assert: The relation carries the reconciled group's id, and the reconcile was asked for + the group named after the scaleset with the parsed rules, so GARM attaches exactly + the group the charm manages. """ ctx = Context(GarmConfiguratorCharm) secret = _make_secret() @@ -1098,31 +1113,26 @@ def test_security_group_id_written_to_garm_configurator_relation(monkeypatch): relations=[garm_relation], leader=True, ) - calls = [] - - class _FakeReconciler: - def __init__(self, provider_config): - calls.append(("network", provider_config.network)) - - def reconcile(self, name, rules): - calls.append((name, [rule.direction for rule in rules])) - return "sg-1234" - - monkeypatch.setattr("charm.SecurityGroupReconciler", _FakeReconciler) out = ctx.run(ctx.on.config_changed(), state) assert out.unit_status == ops.ActiveStatus("Ready") assert out.get_relation(garm_relation.id).local_unit_data["security_group_id"] == "sg-1234" - assert calls == [("network", "external-net"), ("garm-my-scaleset", ["ingress"])] + group_calls = security_group_calls[1:] + assert group_calls[0][0] == "garm-my-scaleset" + assert [(rule.direction, rule.protocol, rule.port_min) for rule in group_calls[0][1]] == [ + ("ingress", "tcp", 22), + ("egress", "any", None), + ] -def test_security_group_not_managed_without_rules(monkeypatch): +def test_default_rules_isolate_the_runners_without_configuration(security_group_calls): """ - arrange: No security-group-rules configured, as in a deployment predating the option. + arrange: No security-group-rules set, so the charmcraft default applies. act: Run config-changed. - assert: No group is reconciled and the relation carries no id, so the runners keep the - project's default security group and existing deployments are unchanged. + assert: The reconcile runs with the default rules — all egress, no ingress — so a + deployment is isolated without the operator writing any rule, and a runner can still + reach GitHub. """ ctx = Context(GarmConfiguratorCharm) secret = _make_secret() @@ -1137,31 +1147,29 @@ def test_security_group_not_managed_without_rules(monkeypatch): leader=True, ) - class _UnexpectedReconciler: - def __init__(self, provider_config): - raise AssertionError("no security group must be reconciled without rules") - - monkeypatch.setattr("charm.SecurityGroupReconciler", _UnexpectedReconciler) - out = ctx.run(ctx.on.config_changed(), state) assert out.unit_status == ops.ActiveStatus("Ready") - assert "security_group_id" not in out.get_relation(garm_relation.id).local_unit_data + assert out.get_relation(garm_relation.id).local_unit_data["security_group_id"] == "sg-1234" + rules = security_group_calls[1][1] + assert [(rule.direction, rule.protocol, rule.remote_ip_prefix) for rule in rules] == [ + ("egress", "any", "0.0.0.0/0"), + ("egress", "any", "::/0"), + ] -def test_clearing_security_group_rules_withdraws_the_id(monkeypatch): +def test_rule_change_is_published_on_the_next_reconcile(security_group_calls): """ - arrange: A deployment whose managed group id is on the relation. - act: Clear security-group-rules and run config-changed again. - assert: The id is withdrawn, so GARM stops attaching the group and the runners return to - the project's default security group. + arrange: A deployment already publishing one group id. + act: Change the rules and run config-changed again. + assert: The reconcile runs with the new rules and the relation keeps the reconciled id, + so a rule edit is applied without a unit restart. """ ctx = Context(GarmConfiguratorCharm) secret = _make_secret() pk_secret = _make_private_key_secret() config = _valid_config(secret, pk_secret) config["image"] = "runner-noble-amd64" - config["security-group-rules"] = _security_group_rules_config() garm_relation = _make_garm_configurator_relation() state = State( config=config, @@ -1169,25 +1177,19 @@ def test_clearing_security_group_rules_withdraws_the_id(monkeypatch): relations=[garm_relation], leader=True, ) + first = ctx.run(ctx.on.config_changed(), state) + changed_config = dict(config) + changed_config["security-group-rules"] = _security_group_rules_config() - class _FakeReconciler: - def __init__(self, provider_config): - pass - - def reconcile(self, name, rules): - return "sg-1234" - - monkeypatch.setattr("charm.SecurityGroupReconciler", _FakeReconciler) - configured = ctx.run(ctx.on.config_changed(), state) - cleared_config = dict(config) - cleared_config["security-group-rules"] = "" - - cleared = ctx.run( + changed = ctx.run( ctx.on.config_changed(), - dataclasses.replace(configured, config=cleared_config), + dataclasses.replace(first, config=changed_config), ) - assert "security_group_id" not in cleared.get_relation(garm_relation.id).local_unit_data + assert changed.get_relation(garm_relation.id).local_unit_data["security_group_id"] == ( + "sg-1234" + ) + assert len(security_group_calls[-1][1]) == 2 def test_security_group_failure_blocks_and_publishes_nothing(monkeypatch): @@ -1202,7 +1204,6 @@ def test_security_group_failure_blocks_and_publishes_nothing(monkeypatch): pk_secret = _make_private_key_secret() config = _valid_config(secret, pk_secret) config["image"] = "runner-noble-amd64" - config["security-group-rules"] = _security_group_rules_config() garm_relation = _make_garm_configurator_relation() state = State( config=config, diff --git a/charms/garm-configurator/tests/unit/test_openstack_client.py b/charms/garm-configurator/tests/unit/test_openstack_client.py deleted file mode 100644 index dc8ce673..00000000 --- a/charms/garm-configurator/tests/unit/test_openstack_client.py +++ /dev/null @@ -1,482 +0,0 @@ -# Copyright 2026 Canonical Ltd. -# See LICENSE file for licensing details. - -"""Unit tests for the minimal OpenStack networking client.""" - -import io -import json -import urllib.error - -import pytest - -from charm_state import ProviderConfig -from openstack_client import ( - MAX_DETAIL_LENGTH, - NetworkClient, - OpenStackClientError, - _urlopen_transport, -) - -CATALOG = { - "token": { - "catalog": [ - { - "type": "identity", - "endpoints": [ - { - "interface": "public", - "region": "RegionOne", - "url": "https://keystone.example.com:5000/v3", - } - ], - }, - { - "type": "network", - "endpoints": [ - { - "interface": "internal", - "region": "RegionOne", - "url": "http://neutron.internal:9696", - }, - { - "interface": "public", - "region": "RegionTwo", - "url": "https://neutron.two:9696", - }, - { - "interface": "public", - "region": "RegionOne", - "url": "https://neutron.example.com:9696/", - }, - ], - }, - ] - } -} - - -class FakeTransport: - """A transport that answers from a recorded queue of responses.""" - - def __init__(self, responses): - self.responses = list(responses) - self.requests = [] - - def __call__(self, method, url, headers, body): - self.requests.append({"method": method, "url": url, "headers": headers, "body": body}) - return self.responses.pop(0) - - -def _response(status=200, payload=None, headers=None): - """Build a transport response, JSON-encoding the payload.""" - body = b"" if payload is None else json.dumps(payload).encode("utf-8") - return status, headers or {}, body - - -def _provider_config(region="RegionOne"): - """Build the OpenStack credentials the client authenticates with.""" - return ProviderConfig( - auth_url="https://keystone.example.com:5000/v3", - username="admin", - password="s3cr3t", - project_name="myproject", - user_domain_name="Default", - project_domain_name="Default", - region_name=region, - network="external-net", - provider_name="garm-configurator-0", - ) - - -def _client(responses, region="RegionOne"): - """Build a client whose transport answers with *responses*.""" - transport = FakeTransport(responses) - return NetworkClient(_provider_config(region), transport=transport), transport - - -def _auth_response(): - """Return a successful Keystone token response.""" - return _response(201, CATALOG, {"X-Subject-Token": "token-1"}) - - -def test_authentication_scopes_a_password_token_to_the_project(): - """ - arrange: A Keystone that issues a token, and a project to scope it to. - act: List the project's security groups. - assert: The token request carries the password grant, the user and project domains, and - the project scope, so the token cannot act outside the configured project. - """ - client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) - - client.security_groups() - - auth = json.loads(transport.requests[0]["body"]) - assert transport.requests[0]["method"] == "POST" - assert transport.requests[0]["url"] == "https://keystone.example.com:5000/v3/auth/tokens" - assert auth == { - "auth": { - "identity": { - "methods": ["password"], - "password": { - "user": { - "name": "admin", - "domain": {"name": "Default"}, - "password": "s3cr3t", - } - }, - }, - "scope": {"project": {"name": "myproject", "domain": {"name": "Default"}}}, - } - } - - -def test_requests_use_the_public_endpoint_of_the_configured_region(): - """ - arrange: A catalog listing an internal endpoint and two public ones in different regions. - act: List the project's security groups. - assert: The request goes to the public endpoint of the configured region, without a - doubled slash. - """ - client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) - - client.security_groups() - - assert transport.requests[1]["url"] == "https://neutron.example.com:9696/security-groups" - - -def test_requests_carry_the_token(): - """ - arrange: A Keystone that issues a token. - act: List the project's security groups. - assert: The Neutron request authenticates with that token, so it acts as the configured - user rather than anonymously. - """ - client, transport = _client([_auth_response(), _response(200, {"security_groups": []})]) - - client.security_groups() - - assert transport.requests[1]["headers"]["X-Auth-Token"] == "token-1" - - -def test_token_is_reused_across_requests(): - """ - arrange: A client that has already authenticated. - act: Make two Neutron requests. - assert: Only one token is requested, so a reconcile does not re-authenticate per call. - """ - client, transport = _client( - [ - _auth_response(), - _response(200, {"security_groups": []}), - _response(200, {"security_group_rules": []}), - ] - ) - - client.security_groups() - client.security_group_rules("sg-1") - - assert [request["method"] for request in transport.requests] == ["POST", "GET", "GET"] - - -def test_expired_token_is_refreshed_once(): - """ - arrange: A Neutron that rejects the cached token as expired. - act: List the project's security groups. - assert: The client authenticates again and retries, so a token expiring between hooks - does not fail the reconcile. - """ - client, transport = _client( - [ - _auth_response(), - _response(401, {"error": {"message": "token expired"}}), - _auth_response(), - _response(200, {"security_groups": [{"id": "sg-1", "name": "garm-runner"}]}), - ] - ) - - groups = client.security_groups() - - assert groups == [{"id": "sg-1", "name": "garm-runner"}] - assert [request["method"] for request in transport.requests] == ["POST", "GET", "POST", "GET"] - assert transport.requests[3]["headers"]["X-Auth-Token"] == "token-1" - - -def test_rejected_credentials_are_reported_once_not_retried(): - """ - arrange: A Keystone that rejects the password. - act: List the project's security groups. - assert: The failure reports Keystone's own message, and no request is retried — bad - credentials do not resolve within a reconcile. - """ - client, transport = _client( - [ - _response( - 401, {"error": {"message": "The request you have made requires authentication"}} - ) - ] - ) - - with pytest.raises(OpenStackClientError) as exc_info: - client.security_groups() - - assert "Keystone rejected the credentials (401)" in str(exc_info.value) - assert "requires authentication" in str(exc_info.value) - assert len(transport.requests) == 1 - - -def test_missing_token_header_is_reported(): - """ - arrange: A Keystone that answers 201 but omits the token header. - act: List the project's security groups. - assert: The failure is reported rather than the client sending an empty token. - """ - client, _ = _client([_response(201, CATALOG)]) - - with pytest.raises(OpenStackClientError) as exc_info: - client.security_groups() - - assert str(exc_info.value) == "Keystone returned no token" - - -def test_missing_networking_endpoint_is_reported(): - """ - arrange: A catalog with no public networking endpoint for the configured region. - act: List the project's security groups. - assert: The failure names the region, so an operator can see which catalog entry is - missing. - """ - client, _ = _client([_auth_response()], region="RegionThree") - - with pytest.raises(OpenStackClientError) as exc_info: - client.security_groups() - - assert str(exc_info.value) == ( - "the Keystone catalog has no public networking endpoint for region RegionThree" - ) - - -def test_neutron_error_message_is_reported(): - """ - arrange: A Neutron that rejects a rule. - act: Create a rule. - assert: The failure carries Neutron's own explanation, which names the invalid field. - """ - client, _ = _client( - [ - _auth_response(), - _response(400, {"NeutronError": {"message": "Security group rule already exists"}}), - ] - ) - - with pytest.raises(OpenStackClientError) as exc_info: - client.create_security_group_rule("sg-1", {"direction": "ingress"}) - - assert "POST /security-group-rules failed with 400" in str(exc_info.value) - assert "Security group rule already exists" in str(exc_info.value) - - -def test_non_json_error_body_is_truncated(): - """ - arrange: A proxy answering with an HTML error page instead of JSON. - act: List the project's security groups. - assert: The reported detail is bounded, so a page of HTML cannot fill the status message. - """ - client, _ = _client( - [ - _auth_response(), - (502, {}, b"" + b"x" * (MAX_DETAIL_LENGTH * 2) + b""), - ] - ) - - with pytest.raises(OpenStackClientError) as exc_info: - client.security_groups() - - detail = str(exc_info.value).split(": ", 1)[1] - assert len(detail) == MAX_DETAIL_LENGTH - - -def test_rule_creation_sends_the_group_and_fields(): - """ - arrange: A group to add a rule to. - act: Create one rule. - assert: The body nests the fields under the Neutron resource name, with the group id. - """ - client, transport = _client( - [_auth_response(), _response(201, {"security_group_rule": {"id": "rule-1"}})] - ) - - client.create_security_group_rule("sg-1", {"direction": "ingress", "protocol": "tcp"}) - - assert json.loads(transport.requests[1]["body"]) == { - "security_group_rule": { - "security_group_id": "sg-1", - "direction": "ingress", - "protocol": "tcp", - } - } - - -def test_rule_listing_filters_by_group(): - """ - arrange: A group whose rules are wanted. - act: List the rules. - assert: The request filters by group id, so the reconcile never considers another group's - rules. - """ - client, transport = _client([_auth_response(), _response(200, {"security_group_rules": []})]) - - client.security_group_rules("sg 1") - - assert transport.requests[1]["url"] == ( - "https://neutron.example.com:9696/security-group-rules?security_group_id=sg+1" - ) - - -def test_rule_deletion_sends_no_body(): - """ - arrange: A rule to delete. - act: Delete it. - assert: The request is a bodyless DELETE, which Neutron accepts without a JSON body. - """ - client, transport = _client([_auth_response(), (204, {}, b"")]) - - client.delete_security_group_rule("rule-1") - - assert transport.requests[1]["method"] == "DELETE" - assert transport.requests[1]["url"] == ( - "https://neutron.example.com:9696/security-group-rules/rule-1" - ) - assert transport.requests[1]["body"] is None - - -def test_missing_response_key_is_reported(): - """ - arrange: A Neutron answering 200 with a body that lacks the expected key. - act: List the project's security groups. - assert: The failure is reported as a client error, so a malformed response blocks the unit - instead of crashing the hook with a KeyError. - """ - client, _ = _client([_auth_response(), _response(200, {"unexpected": []})]) - - with pytest.raises(OpenStackClientError) as exc_info: - client.security_groups() - - assert str(exc_info.value) == ("GET /security-groups returned no 'security_groups' list") - - -def test_group_creation_sends_the_name_and_description(): - """ - arrange: A project without the managed group. - act: Create the group. - assert: The body names it and marks it charm-managed, so an operator reading the OpenStack - console knows not to edit it by hand. - """ - client, transport = _client( - [_auth_response(), _response(201, {"security_group": {"id": "sg-1"}})] - ) - - client.create_security_group("garm-my-scaleset", "Managed by the charm") - - assert json.loads(transport.requests[1]["body"]) == { - "security_group": {"name": "garm-my-scaleset", "description": "Managed by the charm"} - } - - -class _FakeResponse: - """A response, as urlopen returns one.""" - - status = 200 - headers = {"Content-Type": "application/json"} - - def read(self): - return b'{"ok": true}' - - def __enter__(self): - return self - - def __exit__(self, *_exception): - return False - - -def test_transport_returns_the_response(monkeypatch): - """ - arrange: A server answering successfully. - act: Send a request through the default transport. - assert: The status, headers, and body reach the caller, so the client can decode them. - """ - sent = {} - - def urlopen(request, timeout=None): - sent["url"] = request.full_url - sent["timeout"] = timeout - return _FakeResponse() - - monkeypatch.setattr("urllib.request.urlopen", urlopen) - - status, headers, body = _urlopen_transport( - "GET", "https://neutron.example.com:9696/security-groups", {"Accept": "x"}, None - ) - - assert (status, headers, body) == (200, {"Content-Type": "application/json"}, b'{"ok": true}') - assert sent["url"] == "https://neutron.example.com:9696/security-groups" - - -def test_transport_returns_http_errors_as_responses(monkeypatch): - """ - arrange: A server rejecting the request. - act: Send it through the default transport. - assert: The error is returned rather than raised, so the client can report OpenStack's own - explanation instead of a bare traceback. - """ - - def urlopen(request, timeout=None): - raise urllib.error.HTTPError( - request.full_url, - 403, - "Forbidden", - {"X-Test": "1"}, - io.BytesIO(b'{"error": {"message": "Policy does not allow this request"}}'), - ) - - monkeypatch.setattr("urllib.request.urlopen", urlopen) - - status, headers, body = _urlopen_transport( - "POST", "https://neutron.example.com:9696/security-groups", {}, b"{}" - ) - - assert status == 403 - assert headers["X-Test"] == "1" - assert b"Policy does not allow" in body - - -def test_transport_reports_an_unreachable_server(monkeypatch): - """ - arrange: A server that cannot be reached at all. - act: Send a request through the default transport. - assert: The failure names the URL and the reason, so the blocked status tells an operator - what to check. - """ - - def urlopen(request, timeout=None): - raise urllib.error.URLError("connection refused") - - monkeypatch.setattr("urllib.request.urlopen", urlopen) - - with pytest.raises(OpenStackClientError) as exc_info: - _urlopen_transport("GET", "https://neutron.example.com:9696/security-groups", {}, None) - - assert str(exc_info.value) == ( - "cannot reach https://neutron.example.com:9696/security-groups: connection refused" - ) - - -def test_transport_refuses_a_non_http_url(): - """ - arrange: A URL whose scheme is not http or https. - act: Send a request through the default transport. - assert: The transport refuses, so a malformed catalog endpoint cannot read a local file. - """ - with pytest.raises(OpenStackClientError) as exc_info: - _urlopen_transport("GET", "file:///etc/passwd", {}, None) - - assert str(exc_info.value) == "refusing to call a non-HTTP URL: file:///etc/passwd" diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py index ce12a933..448179cd 100644 --- a/charms/garm-configurator/tests/unit/test_openstack_security_group.py +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -8,7 +8,6 @@ import pytest from charm_state import ProviderConfig, SecurityGroupRule -from openstack_client import OpenStackClientError from openstack_security_group import ( MAX_STATUS_REASON_LENGTH, SecurityGroupReconcileError, @@ -18,45 +17,91 @@ SCALESET_NAME = "my-scaleset" GROUP_NAME = "garm-my-scaleset" +RULE_ATTRIBUTES = ( + "direction", + "ethertype", + "protocol", + "port_range_min", + "port_range_max", + "remote_ip_prefix", + "remote_group_id", + "remote_address_group_id", +) + + +class _FakeRule: + """A security group rule as the SDK reports it.""" + + def __init__(self, **fields): + for field in RULE_ATTRIBUTES: + setattr(self, field, fields.get(field)) + # Fields OpenStack assigns, which a diff must ignore. + self.id = fields.get("id", "rule-id") + self.description = fields.get("description", "") -class FakeNetworkClient: - """An in-memory Neutron API holding one project's security groups.""" +class _FakeGroup: + """A security group as the SDK reports it.""" - def __init__(self, groups: typing.Sequence[dict] = ()): - self.groups = [dict(group, rules=list(group.get("rules", []))) for group in groups] - self.created_groups: list[str] = [] - self.created_rules: list[dict] = [] - self.deleted_rules: list[str] = [] - self._next_id = 1 + def __init__(self, name, gid="sg-1", created_at="2026-01-01T00:00:00Z", rules=()): + self.id = gid + self.name = name + self.created_at = created_at + self.rules = list(rules) + + +class _FakeNetwork: + """The network service of a fake OpenStack connection.""" + + def __init__(self, groups=()): + self.groups = list(groups) + self.created_groups = [] + self.deleted_groups = [] + self.created_rules = [] + self.deleted_rules = [] def security_groups(self): - return [{"id": group["id"], "name": group["name"]} for group in self.groups] + return self.groups def create_security_group(self, name, description): - group = {"id": f"sg-{self._next_id}", "name": name, "rules": []} - self._next_id += 1 + group = _FakeGroup(name, gid=f"sg-{name}") self.groups.append(group) self.created_groups.append(name) return group + def delete_security_group(self, group_id): + self.deleted_groups.append(group_id) + self.groups = [group for group in self.groups if group.id != group_id] + def security_group_rules(self, security_group_id): - return list(self._group(security_group_id)["rules"]) + return list(self._group(security_group_id).rules) - def create_security_group_rule(self, security_group_id, fields): - rule = {"id": f"rule-{self._next_id}", **fields} - self._next_id += 1 - self._group(security_group_id)["rules"].append(rule) + def create_security_group_rule(self, security_group_id, **fields): self.created_rules.append(fields) + rule = _FakeRule(**fields) + self._group(security_group_id).rules.append(rule) return rule - def delete_security_group_rule(self, rule_id): - self.deleted_rules.append(rule_id) - for group in self.groups: - group["rules"] = [rule for rule in group["rules"] if rule["id"] != rule_id] + def delete_security_group_rule(self, rule): + self.deleted_rules.append(rule.id) + self._group_by_rule(rule).rules.remove(rule) def _group(self, group_id): - return next(group for group in self.groups if group["id"] == group_id) + return next(group for group in self.groups if group.id == group_id) + + def _group_by_rule(self, rule): + return next(group for group in self.groups if rule in group.rules) + + +class _FakeConnection: + """A fake OpenStack connection holding one network service.""" + + def __init__(self, network): + self.network = network + self.closed = False + + def close(self): + self.closed = True def _provider_config(): @@ -74,15 +119,22 @@ def _provider_config(): ) -def _reconciler(client): - """Build a reconciler over *client*.""" - return SecurityGroupReconciler(_provider_config(), client=client) +def _reconciler(network): + """Build a reconciler over *network*, plus the connection and auth arguments it used.""" + connection = _FakeConnection(network) + calls: dict[str, typing.Any] = {} + + def connect(**auth_fields): + calls["auth"] = auth_fields + return connection + + return SecurityGroupReconciler(_provider_config(), connect=connect), connection, calls def _rule_fields( direction, protocol=None, port_min=None, port_max=None, prefix=None, ethertype="IPv4" ): - """Build the fields Neutron creates one rule from.""" + """Build the fields OpenStack creates one rule from.""" return { "direction": direction, "ethertype": ethertype, @@ -94,16 +146,17 @@ def _rule_fields( def _stored_rule(**fields): - """Build a rule as Neutron reports it, with the id it assigned.""" - return {"id": f"rule-{fields.pop('rule_id', 99)}", **fields} + """Build a rule as OpenStack reports it, with the id it assigned.""" + return _FakeRule(id=f"rule-{fields.pop('rule_id', 99)}", **fields) -EGRESS_V4 = _rule_fields("egress", prefix="0.0.0.0/0") -EGRESS_V6 = _rule_fields("egress", prefix="::/0", ethertype="IPv6") +SSH = SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22) +EGRESS_ALL_V4_RULE = SecurityGroupRule(direction="egress", remote_ip_prefix="0.0.0.0/0") SSH_INGRESS = _rule_fields("ingress", protocol="tcp", port_min=22, port_max=22) SSH_INGRESS_FROM_SUBNET = _rule_fields( "ingress", protocol="tcp", port_min=22, port_max=22, prefix="10.0.0.0/8" ) +EGRESS_ALL_V4 = _rule_fields("egress", prefix="0.0.0.0/0") def test_security_group_name_is_derived_from_the_scaleset_name(): @@ -116,17 +169,17 @@ def test_security_group_name_is_derived_from_the_scaleset_name(): assert security_group_name(SCALESET_NAME) == GROUP_NAME -def test_missing_group_is_created_with_the_configured_and_default_egress_rules(): +def test_missing_group_is_created_with_the_configured_rules(): """ arrange: A project with no managed security group yet. - act: Reconcile one ingress rule. - assert: The group is created, holds that rule, and also allows all egress — without it a - runner could not reach GitHub, because OpenStack attaches this group instead of the - project's default group. + act: Reconcile one ingress rule and one egress rule. + assert: The group is created holding exactly those rules, because the option is the only + source of the group's rules. """ - client = FakeNetworkClient() + network = _FakeNetwork() + reconciler, _, _ = _reconciler(network) - group_id = _reconciler(client).reconcile( + group_id = reconciler.reconcile( GROUP_NAME, [ SecurityGroupRule( @@ -135,32 +188,14 @@ def test_missing_group_is_created_with_the_configured_and_default_egress_rules() port_min=22, port_max=22, remote_ip_prefix="10.0.0.0/8", - ) + ), + EGRESS_ALL_V4_RULE, ], ) - assert client.created_groups == [GROUP_NAME] - assert client.created_rules == [SSH_INGRESS_FROM_SUBNET, EGRESS_V4, EGRESS_V6] - assert group_id == "sg-1" - - -def test_configured_egress_rule_replaces_the_default_egress(): - """ - arrange: A project with no managed security group, and rules that state their own egress. - act: Reconcile the rules. - assert: Only the configured egress rule is created, so the operator's narrower egress is - not widened by the charm's default. - """ - client = FakeNetworkClient() - - _reconciler(client).reconcile( - GROUP_NAME, - [SecurityGroupRule(direction="egress", protocol="tcp", port_min=443, port_max=443)], - ) - - assert client.created_rules == [ - _rule_fields("egress", protocol="tcp", port_min=443, port_max=443) - ] + assert network.created_groups == [GROUP_NAME] + assert network.created_rules == [SSH_INGRESS_FROM_SUBNET, EGRESS_ALL_V4] + assert group_id == f"sg-{GROUP_NAME}" def test_duplicate_configured_rules_are_created_once(): @@ -170,70 +205,59 @@ def test_duplicate_configured_rules_are_created_once(): assert: One rule is created, so the group does not carry a duplicate the next reconcile would then have to remove. """ - client = FakeNetworkClient() - ssh = SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22) + network = _FakeNetwork() + reconciler, _, _ = _reconciler(network) - _reconciler(client).reconcile(GROUP_NAME, [ssh, ssh]) + reconciler.reconcile(GROUP_NAME, [SSH, SSH, EGRESS_ALL_V4_RULE]) - assert client.created_rules == [SSH_INGRESS, EGRESS_V4, EGRESS_V6] + assert network.created_rules == [SSH_INGRESS, EGRESS_ALL_V4] def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(): """ - arrange: A group that already holds the desired rules, with the ids Neutron assigned. + arrange: A group that already holds the desired rules, with the ids OpenStack assigned. act: Reconcile the same rules again. assert: The existing group and its rules are kept, so reconciling unchanged configuration - only adds the rule that is missing. + makes no change at all. """ - client = FakeNetworkClient( - [ - { - "id": "sg-existing", - "name": GROUP_NAME, - "rules": [_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_V4)], - } - ] + group = _FakeGroup( + GROUP_NAME, + gid="sg-existing", + rules=[_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_ALL_V4)], ) + network = _FakeNetwork([group]) + reconciler, _, _ = _reconciler(network) - group_id = _reconciler(client).reconcile( - GROUP_NAME, [SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22)] - ) + group_id = reconciler.reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) assert group_id == "sg-existing" - assert client.created_groups == [] - assert client.deleted_rules == [] - assert client.created_rules == [EGRESS_V6] + assert network.created_groups == [] + assert network.created_rules == [] + assert network.deleted_rules == [] def test_rules_no_longer_configured_are_deleted(): """ arrange: A group holding a rule the operator has since removed. act: Reconcile the remaining rules. - assert: The stale rule is deleted and the kept one is not recreated, so the group matches - the option exactly. + assert: The stale rule is deleted, so the group matches the option exactly. """ - client = FakeNetworkClient( - [ - { - "id": "sg-existing", - "name": GROUP_NAME, - "rules": [ - _stored_rule(**SSH_INGRESS, rule_id=1), - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), - rule_id=2, - ), - ], - } - ] + group = _FakeGroup( + GROUP_NAME, + rules=[ + _stored_rule(**SSH_INGRESS, rule_id=1), + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), rule_id=2 + ), + ], ) + network = _FakeNetwork([group]) + reconciler, _, _ = _reconciler(network) - _reconciler(client).reconcile( - GROUP_NAME, [SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22)] - ) + reconciler.reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) - assert client.deleted_rules == ["rule-2"] - assert client.created_rules == [EGRESS_V4, EGRESS_V6] + assert network.deleted_rules == ["rule-2"] + assert network.created_rules == [EGRESS_ALL_V4] def test_a_rule_added_by_hand_is_removed(): @@ -243,44 +267,106 @@ def test_a_rule_added_by_hand_is_removed(): act: Reconcile. assert: The rule is removed, because the option is the only source of the group's rules. """ - client = FakeNetworkClient( + group = _FakeGroup( + GROUP_NAME, + rules=[ + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), rule_id=7 + ) + ], + ) + network = _FakeNetwork([group]) + reconciler, _, _ = _reconciler(network) + + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert network.deleted_rules == ["rule-7"] + assert network.created_rules == [EGRESS_ALL_V4] + + +def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(): + """ + arrange: Two groups sharing the managed name, as a race between two configurators leaves. + act: Reconcile. + assert: The older group is adopted and the newer one deleted, so the charm converges on one + group instead of failing, and two configurators racing pick the same survivor. + """ + network = _FakeNetwork( [ - { - "id": "sg-existing", - "name": GROUP_NAME, - "rules": [ - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), - rule_id=7, - ) - ], - } + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ] ) + reconciler, _, _ = _reconciler(network) - _reconciler(client).reconcile(GROUP_NAME, []) + group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert client.deleted_rules == ["rule-7"] - assert client.created_rules == [EGRESS_V4, EGRESS_V6] + assert group_id == "sg-older" + assert network.deleted_groups == ["sg-newer"] + assert network.created_groups == [] -def test_ambiguous_group_name_is_reported_rather_than_guessed(): +def test_a_duplicate_that_cannot_be_deleted_blocks_the_reconcile(): """ - arrange: Two security groups sharing the managed name. + arrange: Two groups sharing the managed name, and an OpenStack that refuses the delete + because a port still references the obsolete one. act: Reconcile. - assert: The reconcile fails, because picking one of the two would converge the wrong group. + assert: The failure reaches the caller as a reconcile error, so the unit blocks instead of + silently converging on one of two groups. """ - client = FakeNetworkClient( + + class _RefusesDelete(_FakeNetwork): + def delete_security_group(self, group_id): + raise RuntimeError("Security Group sg-newer in use") + + network = _RefusesDelete( [ - {"id": "sg-1", "name": GROUP_NAME}, - {"id": "sg-2", "name": GROUP_NAME}, + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ] ) + reconciler, _, _ = _reconciler(network) with pytest.raises(SecurityGroupReconcileError) as exc_info: - _reconciler(client).reconcile(GROUP_NAME, []) + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert str(exc_info.value) == f"OpenStack holds 2 security groups named {GROUP_NAME}" + assert "in use" in str(exc_info.value) + + +def test_connection_is_closed_after_a_successful_reconcile(): + """ + arrange: A working connection. + act: Reconcile. + assert: The connection is closed, so repeated reconciles do not leak sessions. + """ + reconciler, connection, _ = _reconciler(_FakeNetwork()) + + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert connection.closed is True + + +def test_authentication_uses_the_provider_config(): + """ + arrange: The charm state's provider credentials. + act: Reconcile. + assert: The SDK is given those credentials, the configured region, and the public endpoint + interface. + """ + reconciler, _, calls = _reconciler(_FakeNetwork()) + + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert calls["auth"] == { + "auth_url": "https://keystone.example.com:5000/v3", + "username": "admin", + "password": "s3cr3t", + "project_name": "myproject", + "user_domain_name": "Default", + "project_domain_name": "Default", + "region_name": "RegionOne", + "interface": "public", + } def test_openstack_failure_is_reported_as_a_single_line_reason(): @@ -291,14 +377,15 @@ def test_openstack_failure_is_reported_as_a_single_line_reason(): status the charm sets stays readable. """ - class _UnreachableClient: - def security_groups(self): - raise OpenStackClientError("cannot reach https://keystone:\nconnection refused") + def connect(**_auth_fields): + raise RuntimeError("keystone unreachable:\nconnection refused") + + reconciler = SecurityGroupReconciler(_provider_config(), connect=connect) with pytest.raises(SecurityGroupReconcileError) as exc_info: - _reconciler(_UnreachableClient()).reconcile(GROUP_NAME, []) + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert str(exc_info.value) == "cannot reach https://keystone: connection refused" + assert str(exc_info.value) == "keystone unreachable: connection refused" def test_openstack_failure_reason_is_bounded(): @@ -309,11 +396,12 @@ def test_openstack_failure_reason_is_bounded(): message Juju shortens anyway. """ - class _UnreachableClient: - def security_groups(self): - raise OpenStackClientError("x" * (MAX_STATUS_REASON_LENGTH * 2)) + def connect(**_auth_fields): + raise RuntimeError("x" * (MAX_STATUS_REASON_LENGTH * 2)) + + reconciler = SecurityGroupReconciler(_provider_config(), connect=connect) with pytest.raises(SecurityGroupReconcileError) as exc_info: - _reconciler(_UnreachableClient()).reconcile(GROUP_NAME, []) + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert len(str(exc_info.value)) == MAX_STATUS_REASON_LENGTH + len("...") diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index b49a683e..37b9ed82 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -1365,8 +1365,10 @@ def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: if spec.image_visibility: extra_specs["image_visibility"] = spec.image_visibility if spec.security_group_id: - # Nova attaches the groups it is given *instead of* the project's default - # group, which is why the configurator only publishes an id for a group it - # has already opened egress on. + # Nova attaches the groups it is given *instead of* the project's default group, + # which is why the configurator only publishes an id for a group it has already + # opened egress on. With no id the key is left out entirely, and the provider + # (garm-provider-openstack v0.1.5, provider/spec.go: `if len(spec.SecurityGroups) + # > 0`) then sets no groups on the instance, so Nova keeps the project's default. extra_specs["security_groups"] = [spec.security_group_id] return extra_specs diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index c996fda8..d67d1efe 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -1356,10 +1356,11 @@ def test_create_sends_the_configurator_security_group_as_an_extra_spec(): def test_create_omits_security_groups_when_unmanaged(): """ - arrange: A spec from a configurator that manages no security group. + arrange: A spec from a configurator that publishes no security group id. act: Reconcile a create. - assert: No security_groups extra spec is sent, leaving Nova's default group in place, so - deployments predating the option are unchanged. + assert: No security_groups extra spec is sent, so the provider sets no groups on the + instance and Nova attaches the project's default group — the behaviour deployments + without the option rely on. """ client = FakeGarmClient(providers=["openstack-demo"], scalesets=[]) _reconcile(client, [_spec()]) diff --git a/docs/changelog.md b/docs/changelog.md index c04e7a01..166b10c2 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-10-20 -- `garm-configurator`: add the optional `security-group-rules` configuration, which makes the charm create and maintain one OpenStack security group per scale set and publish its id to the `garm` charm, which passes it to the OpenStack provider so that every runner is attached to it. The group is named `garm-` and its rules are kept equal to the configured list, so rules added to it by hand are removed. The charm allows all egress when no egress rule is listed, because OpenStack attaches this group instead of the project's default group and a runner that cannot reach GitHub never runs a job. The group is never deleted: clearing the option, or removing the application, leaves it in place for the operator to remove. A rule change reaches runners created after the next reconcile; a runner that already exists keeps the group it was launched with. Deployments that do not set the option are unchanged and keep the project's default security group. +- `garm-configurator`: add the `security-group-rules` configuration, which makes the charm create and maintain one OpenStack security group (`garm-`) per scale set and pass its id to the `garm` charm, so runners are attached to it. The default rules allow all egress and no ingress. The group is never deleted, and a rule change reaches only runners created after the next reconcile. ## 2026-10-16 diff --git a/docs/reference/charms.md b/docs/reference/charms.md index 652786d5..cf9ec9eb 100644 --- a/docs/reference/charms.md +++ b/docs/reference/charms.md @@ -89,12 +89,14 @@ charm allows all egress for that reason. A rule change reaches new runners on the next reconcile. A runner that already exists keeps the group it was launched with until GARM replaces it. -The charm never deletes the group. Clearing the option detaches it from the scale set and -leaves it in place, as does removing the application, so an operator decides when to remove it. - -The OpenStack credentials need the `create_security_group`, `create_security_group_rule`, and -`delete_security_group_rule` permissions. The charm does not delete security groups, so it does -not need `delete_security_group`. +The charm never deletes the group it manages: clearing the option detaches it from the scale +set and leaves it in place, as does removing the application, so an operator decides when to +remove it. OpenStack allows duplicate group names, so when several groups share the managed +name the charm keeps the oldest and deletes the others. A duplicate still referenced by a port +cannot be deleted, and the charm reports that instead of converging on one of two groups. + +The OpenStack credentials need the `create_security_group`, `delete_security_group`, +`create_security_group_rule`, and `delete_security_group_rule` permissions. ### Integrations diff --git a/docs/reference/security.md b/docs/reference/security.md index 1c33442e..f6303152 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -43,6 +43,7 @@ operator responsibilities, see the {ref}`security overview Date: Tue, 20 Oct 2026 05:58:37 +0000 Subject: [PATCH 04/26] fix(garm-configurator): never delete a security group a port attaches Duplicates of the managed group name are now resolved by looking at the ports: the survivor is the oldest group a port already attaches, or the oldest when none does, because that is the group protecting live runners. Only duplicates no port uses are deleted, so the reconcile never asks OpenStack for a delete it would refuse, and a duplicate in use is logged rather than blocking the unit. The port filter is re-applied client side, because OpenStack ignores a query parameter it does not know and would otherwise report every port in the project as attached. --- charms/garm-configurator/charmcraft.yaml | 7 +- .../src/openstack_security_group.py | 84 ++++++++++--- .../unit/test_openstack_security_group.py | 110 +++++++++++++++--- docs/reference/charms.md | 5 +- docs/reference/security.md | 3 +- 5 files changed, 169 insertions(+), 40 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 1805d86c..62c91c3f 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -151,9 +151,10 @@ config: group instead of the project's default group and a runner that cannot reach GitHub never runs a job. The group this charm manages is never deleted: clearing this option, or removing the application, leaves it in place for the operator to - remove. A duplicate group that shares the name is deleted, keeping the oldest, - because OpenStack allows duplicate names. Requires OpenStack credentials allowed to - create and delete security groups and security group rules. + remove. OpenStack allows duplicate names, so when several groups share it the + charm keeps the one a port already attaches — the one protecting live runners — + or the oldest, and deletes the duplicates no port uses. Requires OpenStack + credentials allowed to create and delete security groups and security group rules. dockerhub-mirror: type: string description: | diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py index 627db4fd..a9ea4b6a 100644 --- a/charms/garm-configurator/src/openstack_security_group.py +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -124,12 +124,6 @@ def _auth_fields(self) -> dict[str, str]: def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: """Return the named group, creating it when the project has none. - The oldest group of that name wins, so two configurators racing to create it settle - on the same survivor instead of deleting each other's group on every reconcile. - Neutron refuses to delete a group a port still references, so an obsolete duplicate - that something else attached to blocks the reconcile rather than breaking that - port's networking. - Args: connection: The authenticated OpenStack connection. name: The group name. @@ -141,21 +135,64 @@ def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: (group for group in connection.network.security_groups() if group.name == name), key=_age, ) - for obsolete in groups[1:]: + if not groups: + logger.info("Creating security group %s", name) + return connection.network.create_security_group( + name=name, description=SECURITY_GROUP_DESCRIPTION + ) + return self._keep_one_group(connection, name, groups) + + def _keep_one_group( + self, connection: typing.Any, name: str, groups: list[typing.Any] + ) -> typing.Any: + """Pick the group to converge and remove the duplicates nothing is using. + + OpenStack allows duplicate names, so two configurators can each create one. The + survivor is the oldest group a port already attaches — the one protecting live + runners — or the oldest group when none is attached. Every configurator makes the + same choice, so the duplicates settle instead of deleting each other. + + A duplicate a port still attaches is left alone rather than deleted: OpenStack + refuses that delete anyway, and trying it would block the reconcile over a group + this charm does not manage. The warning names it, so an operator can move the port + and remove the group. + + Args: + connection: The authenticated OpenStack connection. + name: The group name. + groups: Every group of that name, oldest first. + + Returns: + The security group this charm manages. + """ + if len(groups) == 1: + return groups[0] + attached = {group.id: _ports_using(connection, group.id) for group in groups} + survivor = next((group for group in groups if attached[group.id]), groups[0]) + for group in groups: + if group.id == survivor.id: + continue + if attached[group.id]: + logger.warning( + "Leaving security group %s (%s) in place: %d port(s) still attach it, and" + " %s (%s) is the group this charm manages", + name, + group.id, + len(attached[group.id]), + name, + survivor.id, + ) + continue logger.warning( - "Deleting security group %s (%s): %s (%s) is the group this charm manages", + "Deleting duplicate security group %s (%s); %s (%s) is the group this charm" + " manages", name, - obsolete.id, + group.id, name, - groups[0].id, + survivor.id, ) - connection.network.delete_security_group(obsolete.id) - if groups: - return groups[0] - logger.info("Creating security group %s", name) - return connection.network.create_security_group( - name=name, description=SECURITY_GROUP_DESCRIPTION - ) + connection.network.delete_security_group(group.id) + return survivor def _reconcile_rules( self, connection: typing.Any, group: typing.Any, rules: list[SecurityGroupRule] @@ -229,6 +266,19 @@ def _rule_identity(fields: dict[str, object]) -> tuple: return tuple(fields.get(field) for field in RULE_FIELDS) +def _ports_using(connection: typing.Any, group_id: str) -> list[typing.Any]: + """Return the ports that attach a security group. + + The filter is applied again here because OpenStack ignores a query parameter it does + not know, which would otherwise report every port in the project as attached. + """ + return [ + port + for port in connection.network.ports(security_group_ids=[group_id]) + if group_id in (port.security_group_ids or []) + ] + + def _age(group: typing.Any) -> str: """Return the sort key that puts the oldest security group first.""" created = getattr(group, "created_at", None) diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py index 448179cd..fc683ccf 100644 --- a/charms/garm-configurator/tests/unit/test_openstack_security_group.py +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -50,11 +50,22 @@ def __init__(self, name, gid="sg-1", created_at="2026-01-01T00:00:00Z", rules=() self.rules = list(rules) +class _FakePort: + """A port as the SDK reports it.""" + + def __init__(self, *security_group_ids): + self.security_group_ids = list(security_group_ids) + + class _FakeNetwork: """The network service of a fake OpenStack connection.""" - def __init__(self, groups=()): + def __init__(self, groups=(), ports=(), ignore_port_filter=False): self.groups = list(groups) + self._ports = list(ports) + # Models an OpenStack that ignores the filter it does not know. + self._ignore_port_filter = ignore_port_filter + self.port_queries = [] self.created_groups = [] self.deleted_groups = [] self.created_rules = [] @@ -73,6 +84,14 @@ def delete_security_group(self, group_id): self.deleted_groups.append(group_id) self.groups = [group for group in self.groups if group.id != group_id] + def ports(self, security_group_ids=None): + self.port_queries.append(security_group_ids) + if self._ignore_port_filter: + return list(self._ports) + return [ + port for port in self._ports if set(security_group_ids) & set(port.security_group_ids) + ] + def security_group_rules(self, security_group_id): return list(self._group(security_group_id).rules) @@ -286,10 +305,11 @@ def test_a_rule_added_by_hand_is_removed(): def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(): """ - arrange: Two groups sharing the managed name, as a race between two configurators leaves. + arrange: Two groups sharing the managed name and no port attaching either, as a race + between two configurators leaves. act: Reconcile. - assert: The older group is adopted and the newer one deleted, so the charm converges on one - group instead of failing, and two configurators racing pick the same survivor. + assert: The older group is adopted and the unused newer one deleted, so the charm + converges on one group instead of failing. """ network = _FakeNetwork( [ @@ -306,31 +326,87 @@ def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(): assert network.created_groups == [] -def test_a_duplicate_that_cannot_be_deleted_blocks_the_reconcile(): +def test_duplicate_a_port_still_attaches_is_kept_rather_than_deleted(): """ - arrange: Two groups sharing the managed name, and an OpenStack that refuses the delete - because a port still references the obsolete one. + arrange: Two groups sharing the managed name, with runners attached to the newer one. act: Reconcile. - assert: The failure reaches the caller as a reconcile error, so the unit blocks instead of - silently converging on one of two groups. + assert: The attached group is adopted and the unused one deleted, so the charm converges + on the group protecting live runners and never asks OpenStack to delete a group a port + still uses. """ + network = _FakeNetwork( + [ + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), + ], + ports=[_FakePort("sg-newer")], + ) + reconciler, _, _ = _reconciler(network) + + group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert group_id == "sg-newer" + assert network.deleted_groups == ["sg-older"] - class _RefusesDelete(_FakeNetwork): - def delete_security_group(self, group_id): - raise RuntimeError("Security Group sg-newer in use") - network = _RefusesDelete( +def test_two_attached_duplicates_are_both_left_in_place(): + """ + arrange: Two groups sharing the managed name, with a port attaching each. + act: Reconcile. + assert: Neither is deleted — OpenStack would refuse — and the reconcile still succeeds on + the oldest, leaving the operator to move the other port. + """ + network = _FakeNetwork( [ _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), - ] + ], + ports=[_FakePort("sg-newer"), _FakePort("sg-older")], ) reconciler, _, _ = _reconciler(network) - with pytest.raises(SecurityGroupReconcileError) as exc_info: - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert "in use" in str(exc_info.value) + assert group_id == "sg-older" + assert network.deleted_groups == [] + assert network.created_groups == [] + + +def test_ports_are_only_listed_when_the_name_is_duplicated(): + """ + arrange: A single group with the managed name, the normal case. + act: Reconcile. + assert: No port listing happens, so a steady-state reconcile costs no extra API calls. + """ + network = _FakeNetwork([_FakeGroup(GROUP_NAME, gid="sg-only")]) + reconciler, _, _ = _reconciler(network) + + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert network.port_queries == [] + + +def test_port_filter_is_applied_locally_too(): + """ + arrange: An OpenStack that ignores the security group filter and returns every port. + act: Reconcile a duplicated name. + assert: Only ports naming the group count as attached, so an ignored filter cannot make an + unused duplicate look used and keep it forever. + """ + network = _FakeNetwork( + [ + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), + ], + ports=[_FakePort("some-other-group")], + ignore_port_filter=True, + ) + reconciler, _, _ = _reconciler(network) + + group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert group_id == "sg-older" + assert network.deleted_groups == ["sg-newer"] def test_connection_is_closed_after_a_successful_reconcile(): diff --git a/docs/reference/charms.md b/docs/reference/charms.md index cf9ec9eb..92840ed9 100644 --- a/docs/reference/charms.md +++ b/docs/reference/charms.md @@ -92,8 +92,9 @@ group it was launched with until GARM replaces it. The charm never deletes the group it manages: clearing the option detaches it from the scale set and leaves it in place, as does removing the application, so an operator decides when to remove it. OpenStack allows duplicate group names, so when several groups share the managed -name the charm keeps the oldest and deletes the others. A duplicate still referenced by a port -cannot be deleted, and the charm reports that instead of converging on one of two groups. +name the charm keeps the one a port already attaches, or the oldest when none does, and +deletes the duplicates that no port uses. A duplicate that a port still attaches is left in +place and reported in the log, so the charm never detaches a running runner's security group. The OpenStack credentials need the `create_security_group`, `delete_security_group`, `create_security_group_rule`, and `delete_security_group_rule` permissions. diff --git a/docs/reference/security.md b/docs/reference/security.md index f6303152..54ac0755 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -43,7 +43,8 @@ operator responsibilities, see the {ref}`security overview Date: Tue, 20 Oct 2026 06:00:06 +0000 Subject: [PATCH 05/26] fix(garm-configurator): tolerate a port attaching a duplicate during the delete The port check and the delete cannot be atomic, so OpenStack can still answer the delete with a conflict. That is the same condition the check guards against, so it is logged and left in place rather than raised: a group a port uses must never block the reconcile. Any other delete failure still propagates. --- .../src/openstack_security_group.py | 21 +++++++- .../unit/test_openstack_security_group.py | 54 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py index a9ea4b6a..c6b8f491 100644 --- a/charms/garm-configurator/src/openstack_security_group.py +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -191,7 +191,7 @@ def _keep_one_group( name, survivor.id, ) - connection.network.delete_security_group(group.id) + _delete_unused_group(connection, group) return survivor def _reconcile_rules( @@ -266,6 +266,25 @@ def _rule_identity(fields: dict[str, object]) -> tuple: return tuple(fields.get(field) for field in RULE_FIELDS) +def _delete_unused_group(connection: typing.Any, group: typing.Any) -> None: + """Delete a duplicate group, tolerating a port that attached it since the check. + + The port check above and this delete cannot be atomic, so OpenStack can still answer + with a conflict. That is the same condition the check guards against, so it is logged + rather than raised: a group a port uses must not block the reconcile. Every other + failure — a refused permission, an unreachable API — still propagates. + """ + try: + connection.network.delete_security_group(group.id) + except openstack.exceptions.ConflictException: + logger.warning( + "Could not delete security group %s (%s): a port attached it during the" + " reconcile. Leaving it in place", + group.name, + group.id, + ) + + def _ports_using(connection: typing.Any, group_id: str) -> list[typing.Any]: """Return the ports that attach a security group. diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py index fc683ccf..9903ae4a 100644 --- a/charms/garm-configurator/tests/unit/test_openstack_security_group.py +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -5,6 +5,7 @@ import typing +import openstack.exceptions import pytest from charm_state import ProviderConfig, SecurityGroupRule @@ -372,6 +373,59 @@ def test_two_attached_duplicates_are_both_left_in_place(): assert network.created_groups == [] +def test_a_port_attaching_during_the_reconcile_does_not_block(): + """ + arrange: An unused duplicate, and an OpenStack that reports a conflict because a port + attached it between the port check and the delete. + act: Reconcile. + assert: The reconcile still succeeds on the survivor and the duplicate is left in place, + so a race with a runner launch cannot block the unit. + """ + + class _Conflicts(_FakeNetwork): + def delete_security_group(self, group_id): + raise openstack.exceptions.ConflictException("Security Group in use") + + network = _Conflicts( + [ + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), + ] + ) + reconciler, _, _ = _reconciler(network) + + group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert group_id == "sg-older" + assert network.deleted_groups == [] + + +def test_a_duplicate_delete_refused_for_another_reason_still_blocks(): + """ + arrange: An OpenStack that refuses the duplicate delete with a non-conflict error, such as + a missing permission. + act: Reconcile. + assert: The failure blocks the unit, because it will not resolve on its own. + """ + + class _Forbidden(_FakeNetwork): + def delete_security_group(self, group_id): + raise openstack.exceptions.ForbiddenException("Policy does not allow this request") + + network = _Forbidden( + [ + _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), + _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), + ] + ) + reconciler, _, _ = _reconciler(network) + + with pytest.raises(SecurityGroupReconcileError) as exc_info: + reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + + assert "Policy does not allow" in str(exc_info.value) + + def test_ports_are_only_listed_when_the_name_is_duplicated(): """ arrange: A single group with the managed name, the normal case. From b2d9d09fc9fba274589f79bb3b18973b223e0902 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 06:31:01 +0000 Subject: [PATCH 06/26] fix(garm-configurator): let an empty rule list opt out of the managed group The charmcraft default turns the managed group on, which CI cannot exercise: the integration tests run with fake OpenStack credentials, so the configurator blocked on an unreachable Keystone and the garm and garm-configurator suites failed at setup. An empty security-group-rules list now means "manage no group": no OpenStack call, no relation key, and the runners keep the project's default group. The integration fixtures use it, and the option description and reference docs document it as the opt-out for deployments that cannot reach the OpenStack API. --- charms/garm-configurator/charmcraft.yaml | 20 +++++++------ charms/garm-configurator/src/charm.py | 13 +++++---- charms/garm-configurator/src/charm_state.py | 12 +++++--- .../tests/unit/test_charm.py | 28 +++++++++++++++++++ charms/tests/integration/conftest.py | 3 ++ .../integration/test_garm_configurator.py | 3 ++ docs/reference/charms.md | 4 +++ 7 files changed, 65 insertions(+), 18 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 62c91c3f..5614b3e8 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -146,15 +146,17 @@ config: For example: [{"direction": "ingress", "protocol": "tcp", "port_min": 22, "remote_ip_prefix": "10.0.0.0/8"}]. The charm creates a group named "garm-" and keeps its rules equal to - this list, so rules added to the group by hand are removed. When no egress - rule is listed the charm allows all egress, because OpenStack attaches this - group instead of the project's default group and a runner that cannot reach - GitHub never runs a job. The group this charm manages is never deleted: clearing - this option, or removing the application, leaves it in place for the operator to - remove. OpenStack allows duplicate names, so when several groups share it the - charm keeps the one a port already attaches — the one protecting live runners — - or the oldest, and deletes the duplicates no port uses. Requires OpenStack - credentials allowed to create and delete security groups and security group rules. + this list, so rules added to the group by hand are removed. A list without an + egress rule is rejected, because OpenStack attaches this group instead of the + project's default group and a runner with no egress cannot reach GitHub. Set the + option to an empty list to manage no security group, which leaves the runners on + the project's default group and makes no OpenStack call. The group this charm + manages is never deleted: clearing this option, or removing the application, + leaves it in place for the operator to remove. OpenStack allows duplicate names, + so when several groups share it the charm keeps the one a port already attaches — + the one protecting live runners — or the oldest, and deletes the duplicates no + port uses. Requires OpenStack credentials allowed to create and delete security + groups and security group rules. dockerhub-mirror: type: string description: | diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index c2ec5dac..607e1ac9 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -76,7 +76,7 @@ def _reconcile(self, event: ops.EventBase) -> None: f"Security group {name} could not be reconciled: {e}" ) return - self._configure_garm_relation(state, security_group_id) + self._configure_garm_relation(state, security_group_id or "") if state.image_overrides_builder: self.unit.status = ops.ActiveStatus("Ready; configured image overrides image builder") @@ -109,21 +109,24 @@ def _update_image_relation(self, state: CharmState) -> None: } ) - def _ensure_security_group(self, state: CharmState) -> str: + def _ensure_security_group(self, state: CharmState) -> str | None: """Reconcile the security group the scaleset's runners are attached to. Args: state: Current resolved charm state. Returns: - The id of the group the runners are attached to. + The id of the group the runners are attached to, or None when the operator + configured no rules — the runners then keep the project's default group. Raises: SecurityGroupReconcileError: If OpenStack could not be reconciled with. """ + rules = state.scaleset_config.security_group_rules + if not rules: + return None return SecurityGroupReconciler(state.provider_config).reconcile( - security_group_name(state.scaleset_config.name), - state.scaleset_config.security_group_rules, + security_group_name(state.scaleset_config.name), rules ) def _ensure_relation_secret( diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 63352c6c..a27e300d 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -302,7 +302,8 @@ class ScalesetConfig(BaseModel): enable_shell: Whether GARM's remote shell is enabled on the scaleset. pre_install_scripts: Script name to bash script pairs for pre-installation. image_visibility: Glance visibility the provider searches when resolving an image name. - security_group_rules: Rules of the security group the charm manages for the runners. + security_group_rules: Rules of the security group the charm manages for the runners, + or empty when it manages none. """ name: str @@ -414,8 +415,8 @@ def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule] egress rule. Returns: - The parsed rules. The charmcraft default lists all egress, so the option is always - set in practice. + The parsed rules, empty when the operator opts out of managing a group. The + charmcraft default lists all egress, so the group is managed unless it is cleared. """ document = _get_optional_string_config(charm, SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME) if document is None: @@ -441,7 +442,10 @@ def _parse_security_group_rules(charm: ops.CharmBase) -> list[SecurityGroupRule] f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} rule {index}: " f"{_describe_rule_error(exc)}" ) from exc - if not any(rule.direction == "egress" for rule in rules): + # An empty list is the opt-out: the charm manages no group and the runners keep the + # project's default one. A configured group needs egress, though, or it would isolate + # the runner from GitHub entirely. + if rules and not any(rule.direction == "egress" for rule in rules): raise CharmConfigInvalidError( f"{SCALESET_SECURITY_GROUP_RULES_CONFIG_NAME} needs at least one egress rule: " f"OpenStack attaches this group instead of the project's default group, so a " diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 3d98d1d0..c6cd4fe2 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -1192,6 +1192,34 @@ def test_rule_change_is_published_on_the_next_reconcile(security_group_calls): assert len(security_group_calls[-1][1]) == 2 +def test_an_empty_rule_list_manages_no_security_group(security_group_calls): + """ + arrange: security-group-rules set to an empty list, the documented opt-out. + act: Run config-changed. + assert: No group is reconciled and the relation carries no id, so the runners keep the + project's default group and a deployment that cannot reach OpenStack still works. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + config["image"] = "runner-noble-amd64" + config["security-group-rules"] = "[]" + garm_relation = _make_garm_configurator_relation() + state = State( + config=config, + secrets=[secret, pk_secret], + relations=[garm_relation], + leader=True, + ) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.unit_status == ops.ActiveStatus("Ready") + assert "security_group_id" not in out.get_relation(garm_relation.id).local_unit_data + assert security_group_calls == [] + + def test_security_group_failure_blocks_and_publishes_nothing(monkeypatch): """ arrange: OpenStack refuses the reconcile. diff --git a/charms/tests/integration/conftest.py b/charms/tests/integration/conftest.py index ab713d1b..4be008d1 100644 --- a/charms/tests/integration/conftest.py +++ b/charms/tests/integration/conftest.py @@ -844,6 +844,9 @@ def deploy_configurator_with_image_fixture( "min-idle-runner": "0", "max-runner": "5", "org": "test-org", + # No OpenStack runs in CI: opt out of the charm-managed security group so the + # configurator does not block on an unreachable Keystone. + "security-group-rules": "[]", } _deploy_configurator( diff --git a/charms/tests/integration/test_garm_configurator.py b/charms/tests/integration/test_garm_configurator.py index c0fc03b1..703ee2cf 100644 --- a/charms/tests/integration/test_garm_configurator.py +++ b/charms/tests/integration/test_garm_configurator.py @@ -57,6 +57,9 @@ def deploy_garm_configurator_app_fixture( "os-arch": "amd64", "max-runner": "1", "repo": "myorg/myrepo", + # No OpenStack runs in CI: opt out of the charm-managed security group so the + # configurator does not block on an unreachable Keystone. + "security-group-rules": "[]", }, ) juju.wait( diff --git a/docs/reference/charms.md b/docs/reference/charms.md index 92840ed9..4e23006c 100644 --- a/docs/reference/charms.md +++ b/docs/reference/charms.md @@ -89,6 +89,10 @@ charm allows all egress for that reason. A rule change reaches new runners on the next reconcile. A runner that already exists keeps the group it was launched with until GARM replaces it. +Set the option to an empty list to manage no security group: the runners then keep the +project's default group and the charm makes no OpenStack call. That is also how a deployment +opts out where the configurator cannot reach the OpenStack API. + The charm never deletes the group it manages: clearing the option detaches it from the scale set and leaves it in place, as does removing the application, so an operator decides when to remove it. OpenStack allows duplicate group names, so when several groups share the managed From 2c026aa2569b55dc9d36e2f1e3d1c03cc69cd9b7 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 09:33:01 +0000 Subject: [PATCH 07/26] refactor(garm-configurator): tighten the security group reconciler Second review pass: - Keep the unit status short: the exception carries the whole single-line reason for the log, and status_reason() truncates it to 60 characters for the status message. - Shorten the security-group-rules description and keep it to one example. - Drop the test-only connect parameter: the reconciler calls openstack.connect directly and the tests stub the module attribute. - Remove the single-use helpers (_rule_fields, _observed_rule_fields, _rule_identity, _desired_rule_fields, _age). A rule's identity is now the tuple of RULE_FIELDS read off SecurityGroupRule.neutron_fields, documented where RULE_FIELDS is declared. --- charms/garm-configurator/charmcraft.yaml | 32 +-- charms/garm-configurator/src/charm.py | 3 +- charms/garm-configurator/src/charm_state.py | 13 + .../src/openstack_security_group.py | 116 +++------ .../unit/test_openstack_security_group.py | 222 +++++++++--------- 5 files changed, 178 insertions(+), 208 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index 5614b3e8..a93e09bd 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -137,26 +137,18 @@ config: type: string default: '[{"direction": "egress", "remote_ip_prefix": "0.0.0.0/0"}, {"direction": "egress", "remote_ip_prefix": "::/0"}]' description: | - Optional JSON list of rules for the OpenStack security group the charm manages - for this scaleset's runners. Each rule takes: - "direction" ("ingress" or "egress", required), "protocol" ("tcp", "udp", - "icmp", "icmpv6", or "any", default "any"), "port_min" and "port_max" - (inclusive, 1-65535, tcp and udp only; "port_min" alone means that one port), - and "remote_ip_prefix" (a CIDR such as "10.0.0.0/8", default every address). - For example: - [{"direction": "ingress", "protocol": "tcp", "port_min": 22, "remote_ip_prefix": "10.0.0.0/8"}]. - The charm creates a group named "garm-" and keeps its rules equal to - this list, so rules added to the group by hand are removed. A list without an - egress rule is rejected, because OpenStack attaches this group instead of the - project's default group and a runner with no egress cannot reach GitHub. Set the - option to an empty list to manage no security group, which leaves the runners on - the project's default group and makes no OpenStack call. The group this charm - manages is never deleted: clearing this option, or removing the application, - leaves it in place for the operator to remove. OpenStack allows duplicate names, - so when several groups share it the charm keeps the one a port already attaches — - the one protecting live runners — or the oldest, and deletes the duplicates no - port uses. Requires OpenStack credentials allowed to create and delete security - groups and security group rules. + JSON list of rules for the OpenStack security group this charm manages for the + scaleset's runners. The charm keeps the group's rules equal to this list, so a + rule added by hand is removed. A rule takes: "direction" (required, "ingress" or + "egress"), "protocol" ("tcp", "udp", "icmp", "icmpv6", or "any", default "any"), + "port_min"/"port_max" (tcp and udp only), and "remote_ip_prefix" (a CIDR, default + every address), for example: + [{"direction": "ingress", "protocol": "tcp", "port_min": 22, "remote_ip_prefix": "10.0.0.0/8"}] + The default allows all egress and no ingress, which is what a runner needs to reach + GitHub. OpenStack attaches this group instead of the project's default group, so a + list without an egress rule is rejected. Set an empty list to manage no group. + Requires OpenStack credentials that may create and delete security groups and + their rules. dockerhub-mirror: type: string description: | diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 607e1ac9..249427d9 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -20,6 +20,7 @@ SecurityGroupReconcileError, SecurityGroupReconciler, security_group_name, + status_reason, ) logger = logging.getLogger(__name__) @@ -73,7 +74,7 @@ def _reconcile(self, event: ops.EventBase) -> None: name = security_group_name(state.scaleset_config.name) logger.warning("Security group %s could not be reconciled: %s", name, e) self.unit.status = ops.BlockedStatus( - f"Security group {name} could not be reconciled: {e}" + f"Security group {name} could not be reconciled: {status_reason(e)}" ) return self._configure_garm_relation(state, security_group_id or "") diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index a27e300d..b69eb593 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -285,6 +285,19 @@ def ethertype(self) -> str: return "IPv6" return "IPv4" + @property + def neutron_fields(self) -> dict[str, object]: + """This rule as OpenStack names its fields, for creating and comparing it.""" + return { + "direction": self.direction, + "ethertype": self.ethertype, + # Neutron spells "every protocol" as a null protocol. + "protocol": None if self.protocol == "any" else self.protocol, + "port_range_min": self.port_min, + "port_range_max": self.port_max, + "remote_ip_prefix": self.remote_ip_prefix, + } + class ScalesetConfig(BaseModel): """Scaleset configuration. diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py index c6b8f491..ba6b79f3 100644 --- a/charms/garm-configurator/src/openstack_security_group.py +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -23,9 +23,9 @@ "Managed by the garm-configurator charm; its rules follow the security-group-rules option." ) -# What identifies a rule. Everything else OpenStack reports about one — its id, tenant, -# description — is either assigned by OpenStack or irrelevant to the desired state, so -# comparing only these makes the diff immune to it. +# A rule's identity is the value of these fields, in this order. OpenStack assigns the id, +# tenant, and description itself, and reports the rest back exactly as configured, so two +# rules with the same identity are the same rule. RULE_FIELDS = ( "direction", "ethertype", @@ -37,8 +37,9 @@ "remote_address_group_id", ) -# Juju shows only the first line of a status message. -MAX_STATUS_REASON_LENGTH = 200 +# Juju shows a status message on one short line, so the unit status keeps only this much of +# a failure; the log line carries the rest. +MAX_STATUS_REASON_LENGTH = 60 class SecurityGroupReconcileError(Exception): @@ -63,21 +64,13 @@ def security_group_name(scaleset_name: str) -> str: class SecurityGroupReconciler: """Creates and converges one charm-managed OpenStack security group.""" - def __init__( - self, - provider_config: ProviderConfig, - connect: typing.Callable[..., typing.Any] | None = None, - ) -> None: + def __init__(self, provider_config: ProviderConfig) -> None: """Initialise the reconciler. Args: provider_config: The OpenStack credentials and region to reconcile in. - connect: The OpenStack SDK entrypoint, overridden by tests. """ self._provider_config = provider_config - # Typed as an opaque callable: the SDK's own signature is narrower than the - # keyword arguments a connection takes, but is what must be called at runtime. - self._connect: typing.Callable[..., typing.Any] = connect or openstack.connect def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: """Create the group if it is missing, then converge its rules. @@ -97,7 +90,10 @@ def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: # Nothing here may crash the hook: every failure is reported and retried, so the # next reconcile looks again either way. try: - connection = self._connect(**self._auth_fields()) + # The SDK's signature is narrower than the keyword arguments a connection + # accepts at runtime, so it is called through an opaque callable. + connect = typing.cast(typing.Callable[..., typing.Any], openstack.connect) + connection = connect(**self._auth_fields()) try: group = self._ensure_group(connection, name) self._reconcile_rules(connection, group, rules) @@ -133,7 +129,8 @@ def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: """ groups = sorted( (group for group in connection.network.security_groups() if group.name == name), - key=_age, + # Oldest first. An absent timestamp reads as oldest, which is only ever a tie-break. + key=lambda group: (str(group.created_at or ""), str(group.id)), ) if not groups: logger.info("Creating security group %s", name) @@ -145,17 +142,11 @@ def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: def _keep_one_group( self, connection: typing.Any, name: str, groups: list[typing.Any] ) -> typing.Any: - """Pick the group to converge and remove the duplicates nothing is using. - - OpenStack allows duplicate names, so two configurators can each create one. The - survivor is the oldest group a port already attaches — the one protecting live - runners — or the oldest group when none is attached. Every configurator makes the - same choice, so the duplicates settle instead of deleting each other. + """Pick the group to converge and delete the duplicates nothing is using. - A duplicate a port still attaches is left alone rather than deleted: OpenStack - refuses that delete anyway, and trying it would block the reconcile over a group - this charm does not manage. The warning names it, so an operator can move the port - and remove the group. + The survivor is the oldest group a port already attaches, or the oldest when none + is, so two configurators racing to create one settle on the same group. A duplicate + a port attaches is left in place and logged: OpenStack refuses that delete anyway. Args: connection: The authenticated OpenStack connection. @@ -200,23 +191,30 @@ def _reconcile_rules( """Add the configured rules, then remove the ones no longer configured. Additions come first so the group never loses a rule the runners may still be - using — every runner of the scaleset carries this group. + using — every runner of the scaleset carries this group. A rule the operator lists + twice is added once. Args: connection: The authenticated OpenStack connection. group: The security group to converge. rules: The rules the group should hold. """ - desired = _desired_rule_fields(rules) observed = { - _rule_identity(_observed_rule_fields(rule)): rule + _observed_identity(rule): rule for rule in connection.network.security_group_rules(security_group_id=group.id) } - for identity, fields in desired.items(): + desired: set[tuple] = set() + for rule in rules: + identity = tuple(rule.neutron_fields.get(field) for field in RULE_FIELDS) + if identity in desired: + continue + desired.add(identity) if identity in observed: continue logger.info("Adding rule %s to security group %s", identity, group.name) - connection.network.create_security_group_rule(security_group_id=group.id, **fields) + connection.network.create_security_group_rule( + security_group_id=group.id, **rule.neutron_fields + ) for identity, rule in observed.items(): if identity in desired: continue @@ -224,46 +222,9 @@ def _reconcile_rules( connection.network.delete_security_group_rule(rule) -def _desired_rule_fields(rules: list[SecurityGroupRule]) -> dict[tuple, dict[str, object]]: - """Return the fields to create for each rule, keyed by rule identity. - - A rule the operator lists twice yields one entry, so the group does not collect - duplicate rules that the next reconcile would then have to remove. - - Args: - rules: The rules the group should hold. - - Returns: - The rule fields to create, keyed by identity. - """ - desired: dict[tuple, dict[str, object]] = {} - for rule in rules: - fields = _rule_fields(rule) - desired.setdefault(_rule_identity(fields), fields) - return desired - - -def _rule_fields(rule: SecurityGroupRule) -> dict[str, object]: - """Translate a configured rule into the fields OpenStack creates it from.""" - return { - "direction": rule.direction, - "ethertype": rule.ethertype, - # Neutron spells "every protocol" as a null protocol. - "protocol": None if rule.protocol == "any" else rule.protocol, - "port_range_min": rule.port_min, - "port_range_max": rule.port_max, - "remote_ip_prefix": rule.remote_ip_prefix, - } - - -def _observed_rule_fields(rule: typing.Any) -> dict[str, object]: - """Read the comparable fields of a rule as OpenStack reports it.""" - return {field: getattr(rule, field, None) for field in RULE_FIELDS} - - -def _rule_identity(fields: dict[str, object]) -> tuple: - """Return what distinguishes one rule from another.""" - return tuple(fields.get(field) for field in RULE_FIELDS) +def _observed_identity(rule: typing.Any) -> tuple: + """Return the identity of a rule as OpenStack reports it.""" + return tuple(getattr(rule, field, None) for field in RULE_FIELDS) def _delete_unused_group(connection: typing.Any, group: typing.Any) -> None: @@ -298,15 +259,14 @@ def _ports_using(connection: typing.Any, group_id: str) -> list[typing.Any]: ] -def _age(group: typing.Any) -> str: - """Return the sort key that puts the oldest security group first.""" - created = getattr(group, "created_at", None) - return "" if created is None else str(created) +def _reason(exc: Exception) -> str: + """Return the failure as one line, for the log.""" + return " ".join(str(exc).split()) or type(exc).__name__ -def _reason(exc: Exception) -> str: - """Return a bounded, single-line reason for a status message.""" - reason = " ".join(str(exc).split()) or type(exc).__name__ +def status_reason(exc: Exception) -> str: + """Return as much of the failure as a unit status can show, for the operator.""" + reason = _reason(exc) if len(reason) > MAX_STATUS_REASON_LENGTH: reason = f"{reason[:MAX_STATUS_REASON_LENGTH]}..." return reason diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py index 9903ae4a..3b27ce43 100644 --- a/charms/garm-configurator/tests/unit/test_openstack_security_group.py +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -14,27 +14,27 @@ SecurityGroupReconcileError, SecurityGroupReconciler, security_group_name, + status_reason, ) SCALESET_NAME = "my-scaleset" GROUP_NAME = "garm-my-scaleset" -RULE_ATTRIBUTES = ( - "direction", - "ethertype", - "protocol", - "port_range_min", - "port_range_max", - "remote_ip_prefix", - "remote_group_id", - "remote_address_group_id", -) class _FakeRule: """A security group rule as the SDK reports it.""" def __init__(self, **fields): - for field in RULE_ATTRIBUTES: + for field in ( + "direction", + "ethertype", + "protocol", + "port_range_min", + "port_range_max", + "remote_ip_prefix", + "remote_group_id", + "remote_address_group_id", + ): setattr(self, field, fields.get(field)) # Fields OpenStack assigns, which a diff must ignore. self.id = fields.get("id", "rule-id") @@ -124,6 +124,34 @@ def close(self): self.closed = True +class _FakeSdk: + """The stubbed SDK entrypoint: the connection it hands out, and the auth it was given.""" + + def __init__(self, monkeypatch): + self.connection = _FakeConnection(_FakeNetwork()) + self.auth = None + + def connect(**auth_fields): + self.auth = auth_fields + return self.connection + + monkeypatch.setattr("openstack_security_group.openstack.connect", connect) + + +@pytest.fixture(name="sdk", autouse=True) +def sdk_fixture(monkeypatch): + """Keep every test off the real OpenStack SDK entrypoint.""" + return _FakeSdk(monkeypatch) + + +def _reconciler(sdk, network=None): + """Build a reconciler whose stubbed connection serves *network*, when given.""" + if network is not None: + sdk.connection.network = network + sdk.connection.closed = False + return SecurityGroupReconciler(_provider_config()) + + def _provider_config(): """Build the OpenStack credentials the reconciler authenticates with.""" return ProviderConfig( @@ -139,18 +167,6 @@ def _provider_config(): ) -def _reconciler(network): - """Build a reconciler over *network*, plus the connection and auth arguments it used.""" - connection = _FakeConnection(network) - calls: dict[str, typing.Any] = {} - - def connect(**auth_fields): - calls["auth"] = auth_fields - return connection - - return SecurityGroupReconciler(_provider_config(), connect=connect), connection, calls - - def _rule_fields( direction, protocol=None, port_min=None, port_max=None, prefix=None, ethertype="IPv4" ): @@ -189,7 +205,7 @@ def test_security_group_name_is_derived_from_the_scaleset_name(): assert security_group_name(SCALESET_NAME) == GROUP_NAME -def test_missing_group_is_created_with_the_configured_rules(): +def test_missing_group_is_created_with_the_configured_rules(sdk): """ arrange: A project with no managed security group yet. act: Reconcile one ingress rule and one egress rule. @@ -197,9 +213,8 @@ def test_missing_group_is_created_with_the_configured_rules(): source of the group's rules. """ network = _FakeNetwork() - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile( + group_id = _reconciler(sdk, network).reconcile( GROUP_NAME, [ SecurityGroupRule( @@ -218,7 +233,7 @@ def test_missing_group_is_created_with_the_configured_rules(): assert group_id == f"sg-{GROUP_NAME}" -def test_duplicate_configured_rules_are_created_once(): +def test_duplicate_configured_rules_are_created_once(sdk): """ arrange: Rules that list the same ingress rule twice. act: Reconcile the rules. @@ -226,29 +241,30 @@ def test_duplicate_configured_rules_are_created_once(): would then have to remove. """ network = _FakeNetwork() - reconciler, _, _ = _reconciler(network) - reconciler.reconcile(GROUP_NAME, [SSH, SSH, EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, SSH, EGRESS_ALL_V4_RULE]) assert network.created_rules == [SSH_INGRESS, EGRESS_ALL_V4] -def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(): +def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(sdk): """ arrange: A group that already holds the desired rules, with the ids OpenStack assigned. act: Reconcile the same rules again. assert: The existing group and its rules are kept, so reconciling unchanged configuration makes no change at all. """ - group = _FakeGroup( - GROUP_NAME, - gid="sg-existing", - rules=[_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_ALL_V4)], + network = _FakeNetwork( + [ + _FakeGroup( + GROUP_NAME, + gid="sg-existing", + rules=[_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_ALL_V4)], + ) + ] ) - network = _FakeNetwork([group]) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) assert group_id == "sg-existing" assert network.created_groups == [] @@ -256,55 +272,61 @@ def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(): assert network.deleted_rules == [] -def test_rules_no_longer_configured_are_deleted(): +def test_rules_no_longer_configured_are_deleted(sdk): """ arrange: A group holding a rule the operator has since removed. act: Reconcile the remaining rules. assert: The stale rule is deleted, so the group matches the option exactly. """ - group = _FakeGroup( - GROUP_NAME, - rules=[ - _stored_rule(**SSH_INGRESS, rule_id=1), - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), rule_id=2 - ), - ], + network = _FakeNetwork( + [ + _FakeGroup( + GROUP_NAME, + rules=[ + _stored_rule(**SSH_INGRESS, rule_id=1), + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), + rule_id=2, + ), + ], + ) + ] ) - network = _FakeNetwork([group]) - reconciler, _, _ = _reconciler(network) - reconciler.reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) assert network.deleted_rules == ["rule-2"] assert network.created_rules == [EGRESS_ALL_V4] -def test_a_rule_added_by_hand_is_removed(): +def test_a_rule_added_by_hand_is_removed(sdk): """ arrange: A group holding a rule the operator never configured, such as one added with the OpenStack CLI. act: Reconcile. assert: The rule is removed, because the option is the only source of the group's rules. """ - group = _FakeGroup( - GROUP_NAME, - rules=[ - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), rule_id=7 + network = _FakeNetwork( + [ + _FakeGroup( + GROUP_NAME, + rules=[ + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), + rule_id=7, + ) + ], ) - ], + ] ) - network = _FakeNetwork([group]) - reconciler, _, _ = _reconciler(network) - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert network.deleted_rules == ["rule-7"] assert network.created_rules == [EGRESS_ALL_V4] -def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(): +def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(sdk): """ arrange: Two groups sharing the managed name and no port attaching either, as a race between two configurators leaves. @@ -318,16 +340,15 @@ def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(): _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ] ) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert group_id == "sg-older" assert network.deleted_groups == ["sg-newer"] assert network.created_groups == [] -def test_duplicate_a_port_still_attaches_is_kept_rather_than_deleted(): +def test_duplicate_a_port_still_attaches_is_kept_rather_than_deleted(sdk): """ arrange: Two groups sharing the managed name, with runners attached to the newer one. act: Reconcile. @@ -342,15 +363,14 @@ def test_duplicate_a_port_still_attaches_is_kept_rather_than_deleted(): ], ports=[_FakePort("sg-newer")], ) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert group_id == "sg-newer" assert network.deleted_groups == ["sg-older"] -def test_two_attached_duplicates_are_both_left_in_place(): +def test_two_attached_duplicates_are_both_left_in_place(sdk): """ arrange: Two groups sharing the managed name, with a port attaching each. act: Reconcile. @@ -364,16 +384,15 @@ def test_two_attached_duplicates_are_both_left_in_place(): ], ports=[_FakePort("sg-newer"), _FakePort("sg-older")], ) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert group_id == "sg-older" assert network.deleted_groups == [] assert network.created_groups == [] -def test_a_port_attaching_during_the_reconcile_does_not_block(): +def test_a_port_attaching_during_the_reconcile_does_not_block(sdk): """ arrange: An unused duplicate, and an OpenStack that reports a conflict because a port attached it between the port check and the delete. @@ -392,15 +411,14 @@ def delete_security_group(self, group_id): _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ] ) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert group_id == "sg-older" assert network.deleted_groups == [] -def test_a_duplicate_delete_refused_for_another_reason_still_blocks(): +def test_a_duplicate_delete_refused_for_another_reason_still_blocks(sdk): """ arrange: An OpenStack that refuses the duplicate delete with a non-conflict error, such as a missing permission. @@ -418,29 +436,27 @@ def delete_security_group(self, group_id): _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ] ) - reconciler, _, _ = _reconciler(network) with pytest.raises(SecurityGroupReconcileError) as exc_info: - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert "Policy does not allow" in str(exc_info.value) -def test_ports_are_only_listed_when_the_name_is_duplicated(): +def test_ports_are_only_listed_when_the_name_is_duplicated(sdk): """ arrange: A single group with the managed name, the normal case. act: Reconcile. assert: No port listing happens, so a steady-state reconcile costs no extra API calls. """ network = _FakeNetwork([_FakeGroup(GROUP_NAME, gid="sg-only")]) - reconciler, _, _ = _reconciler(network) - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert network.port_queries == [] -def test_port_filter_is_applied_locally_too(): +def test_port_filter_is_applied_locally_too(sdk): """ arrange: An OpenStack that ignores the security group filter and returns every port. act: Reconcile a duplicated name. @@ -455,39 +471,34 @@ def test_port_filter_is_applied_locally_too(): ports=[_FakePort("some-other-group")], ignore_port_filter=True, ) - reconciler, _, _ = _reconciler(network) - group_id = reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert group_id == "sg-older" assert network.deleted_groups == ["sg-newer"] -def test_connection_is_closed_after_a_successful_reconcile(): +def test_connection_is_closed_after_a_successful_reconcile(sdk): """ arrange: A working connection. act: Reconcile. assert: The connection is closed, so repeated reconciles do not leak sessions. """ - reconciler, connection, _ = _reconciler(_FakeNetwork()) - - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, _FakeNetwork()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert connection.closed is True + assert sdk.connection.closed is True -def test_authentication_uses_the_provider_config(): +def test_authentication_uses_the_provider_config(sdk): """ arrange: The charm state's provider credentials. act: Reconcile. assert: The SDK is given those credentials, the configured region, and the public endpoint interface. """ - reconciler, _, calls = _reconciler(_FakeNetwork()) - - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, _FakeNetwork()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - assert calls["auth"] == { + assert sdk.auth == { "auth_url": "https://keystone.example.com:5000/v3", "username": "admin", "password": "s3cr3t", @@ -499,39 +510,32 @@ def test_authentication_uses_the_provider_config(): } -def test_openstack_failure_is_reported_as_a_single_line_reason(): +def test_openstack_failure_is_reported_as_a_single_line_reason(monkeypatch): """ arrange: An OpenStack that cannot be reached. act: Reconcile. - assert: A SecurityGroupReconcileError carries the reason on one line, so the Blocked - status the charm sets stays readable. + assert: The error carries the whole reason on one line for the log, so debug-log shows what + went wrong without a multi-line traceback. """ def connect(**_auth_fields): raise RuntimeError("keystone unreachable:\nconnection refused") - reconciler = SecurityGroupReconciler(_provider_config(), connect=connect) + monkeypatch.setattr("openstack_security_group.openstack.connect", connect) with pytest.raises(SecurityGroupReconcileError) as exc_info: - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + SecurityGroupReconciler(_provider_config()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) assert str(exc_info.value) == "keystone unreachable: connection refused" -def test_openstack_failure_reason_is_bounded(): +def test_status_reason_is_bounded(): """ - arrange: An OpenStack call that fails with an over-long message. - act: Reconcile. - assert: The reported reason is truncated, so it cannot crowd out the rest of a status - message Juju shortens anyway. + arrange: A failure with a message far longer than a status line. + act: Render it for the unit status. + assert: The status text is truncated, so Juju shows the start of the reason rather than + cutting it at an arbitrary point. """ + exc = SecurityGroupReconcileError("x" * (MAX_STATUS_REASON_LENGTH * 2)) - def connect(**_auth_fields): - raise RuntimeError("x" * (MAX_STATUS_REASON_LENGTH * 2)) - - reconciler = SecurityGroupReconciler(_provider_config(), connect=connect) - - with pytest.raises(SecurityGroupReconcileError) as exc_info: - reconciler.reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert len(str(exc_info.value)) == MAX_STATUS_REASON_LENGTH + len("...") + assert len(status_reason(exc)) == MAX_STATUS_REASON_LENGTH + len("...") From 89273fdb29c306509360d9d83c751b92ea6a5659 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 09:33:46 +0000 Subject: [PATCH 08/26] docs: state what the security group feature does in the changelog Lead with the isolation it gives the scale sets and keep the lifecycle details to the reference page. --- docs/changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.md b/docs/changelog.md index 166b10c2..00eae729 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-10-20 -- `garm-configurator`: add the `security-group-rules` configuration, which makes the charm create and maintain one OpenStack security group (`garm-`) per scale set and pass its id to the `garm` charm, so runners are attached to it. The default rules allow all egress and no ingress. The group is never deleted, and a rule change reaches only runners created after the next reconcile. +- `garm-configurator`: add the `security-group-rules` configuration, which isolates a scale set's runners in their own OpenStack security group instead of the project's default group. The charm creates and maintains the group and passes it to the `garm` charm for the provider to attach to every runner. ## 2026-10-16 From f13a2d7ae81375f0698cf80b6e3c97ac3493fb25 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 10:15:33 +0000 Subject: [PATCH 09/26] refactor(garm-configurator): name the reconcile arguments and table the rule tests Final review: - Point the blocked status at debug-log rather than truncating the failure into it; the warning keeps the whole reason. - Reinstate _get_identity(rule) and rename the observed reader to _get_observed_identity, so the rule diff reads as identity against identity. - Use named arguments at the calls this change introduced. - Collapse the rule and duplicate-group unit tests into two parametrised tables of inputs and expectations. - Record next to the module and in the reference docs that the remote shell and debug-ssh sessions are outbound, so neither needs an ingress rule. --- charms/garm-configurator/src/charm.py | 11 +- .../src/openstack_security_group.py | 43 +- .../tests/unit/test_charm.py | 2 +- .../unit/test_openstack_security_group.py | 390 +++++++----------- docs/reference/charms.md | 5 + 5 files changed, 178 insertions(+), 273 deletions(-) diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 249427d9..9c40a343 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -20,7 +20,6 @@ SecurityGroupReconcileError, SecurityGroupReconciler, security_group_name, - status_reason, ) logger = logging.getLogger(__name__) @@ -71,13 +70,13 @@ def _reconcile(self, event: ops.EventBase) -> None: try: security_group_id = self._ensure_security_group(state) except SecurityGroupReconcileError as e: - name = security_group_name(state.scaleset_config.name) + name = security_group_name(scaleset_name=state.scaleset_config.name) logger.warning("Security group %s could not be reconciled: %s", name, e) self.unit.status = ops.BlockedStatus( - f"Security group {name} could not be reconciled: {status_reason(e)}" + f"Security group {name} could not be reconciled (see debug-log)" ) return - self._configure_garm_relation(state, security_group_id or "") + self._configure_garm_relation(state=state, security_group_id=security_group_id or "") if state.image_overrides_builder: self.unit.status = ops.ActiveStatus("Ready; configured image overrides image builder") @@ -126,8 +125,8 @@ def _ensure_security_group(self, state: CharmState) -> str | None: rules = state.scaleset_config.security_group_rules if not rules: return None - return SecurityGroupReconciler(state.provider_config).reconcile( - security_group_name(state.scaleset_config.name), rules + return SecurityGroupReconciler(provider_config=state.provider_config).reconcile( + name=security_group_name(scaleset_name=state.scaleset_config.name), rules=rules ) def _ensure_relation_secret( diff --git a/charms/garm-configurator/src/openstack_security_group.py b/charms/garm-configurator/src/openstack_security_group.py index ba6b79f3..7e000291 100644 --- a/charms/garm-configurator/src/openstack_security_group.py +++ b/charms/garm-configurator/src/openstack_security_group.py @@ -7,6 +7,10 @@ security group per scaleset and keeps its rules in line with the operator's ``security-group-rules``. The ``garm`` charm passes the group's id to the OpenStack provider as the scaleset's ``security_groups`` extra spec. + +Nothing the charms provide needs an ingress rule. GARM's remote shell rides the agent's +connection, which the runner opens to ``agent_url``, and a ``debug-ssh`` session is an +outbound tmate client on the runner; both are traffic the runner originates. """ import logging @@ -37,10 +41,6 @@ "remote_address_group_id", ) -# Juju shows a status message on one short line, so the unit status keeps only this much of -# a failure; the log line carries the rest. -MAX_STATUS_REASON_LENGTH = 60 - class SecurityGroupReconcileError(Exception): """Raised when the managed security group could not be reconciled with OpenStack.""" @@ -95,8 +95,8 @@ def reconcile(self, name: str, rules: list[SecurityGroupRule]) -> str: connect = typing.cast(typing.Callable[..., typing.Any], openstack.connect) connection = connect(**self._auth_fields()) try: - group = self._ensure_group(connection, name) - self._reconcile_rules(connection, group, rules) + group = self._ensure_group(connection=connection, name=name) + self._reconcile_rules(connection=connection, group=group, rules=rules) return str(group.id) finally: connection.close() @@ -137,7 +137,7 @@ def _ensure_group(self, connection: typing.Any, name: str) -> typing.Any: return connection.network.create_security_group( name=name, description=SECURITY_GROUP_DESCRIPTION ) - return self._keep_one_group(connection, name, groups) + return self._keep_one_group(connection=connection, name=name, groups=groups) def _keep_one_group( self, connection: typing.Any, name: str, groups: list[typing.Any] @@ -158,7 +158,9 @@ def _keep_one_group( """ if len(groups) == 1: return groups[0] - attached = {group.id: _ports_using(connection, group.id) for group in groups} + attached = { + group.id: _ports_using(connection=connection, group_id=group.id) for group in groups + } survivor = next((group for group in groups if attached[group.id]), groups[0]) for group in groups: if group.id == survivor.id: @@ -182,7 +184,7 @@ def _keep_one_group( name, survivor.id, ) - _delete_unused_group(connection, group) + _delete_unused_group(connection=connection, group=group) return survivor def _reconcile_rules( @@ -200,12 +202,12 @@ def _reconcile_rules( rules: The rules the group should hold. """ observed = { - _observed_identity(rule): rule + _get_observed_identity(rule=rule): rule for rule in connection.network.security_group_rules(security_group_id=group.id) } desired: set[tuple] = set() for rule in rules: - identity = tuple(rule.neutron_fields.get(field) for field in RULE_FIELDS) + identity = _get_identity(rule=rule) if identity in desired: continue desired.add(identity) @@ -219,10 +221,15 @@ def _reconcile_rules( if identity in desired: continue logger.info("Removing rule %s from security group %s", identity, group.name) - connection.network.delete_security_group_rule(rule) + connection.network.delete_security_group_rule(security_group_rule=rule) + +def _get_identity(rule: SecurityGroupRule) -> tuple: + """Return a configured rule's identity: the value of RULE_FIELDS, in order.""" + return tuple(rule.neutron_fields.get(field) for field in RULE_FIELDS) -def _observed_identity(rule: typing.Any) -> tuple: + +def _get_observed_identity(rule: typing.Any) -> tuple: """Return the identity of a rule as OpenStack reports it.""" return tuple(getattr(rule, field, None) for field in RULE_FIELDS) @@ -236,7 +243,7 @@ def _delete_unused_group(connection: typing.Any, group: typing.Any) -> None: failure — a refused permission, an unreachable API — still propagates. """ try: - connection.network.delete_security_group(group.id) + connection.network.delete_security_group(security_group=group.id) except openstack.exceptions.ConflictException: logger.warning( "Could not delete security group %s (%s): a port attached it during the" @@ -262,11 +269,3 @@ def _ports_using(connection: typing.Any, group_id: str) -> list[typing.Any]: def _reason(exc: Exception) -> str: """Return the failure as one line, for the log.""" return " ".join(str(exc).split()) or type(exc).__name__ - - -def status_reason(exc: Exception) -> str: - """Return as much of the failure as a unit status can show, for the operator.""" - reason = _reason(exc) - if len(reason) > MAX_STATUS_REASON_LENGTH: - reason = f"{reason[:MAX_STATUS_REASON_LENGTH]}..." - return reason diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index c6cd4fe2..d6af9d44 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -1252,6 +1252,6 @@ def reconcile(self, name, rules): out = ctx.run(ctx.on.config_changed(), state) assert out.unit_status == ops.BlockedStatus( - "Security group garm-my-scaleset could not be reconciled: keystone refused the token" + "Security group garm-my-scaleset could not be reconciled (see debug-log)" ) assert "name" not in out.get_relation(garm_relation.id).local_unit_data diff --git a/charms/garm-configurator/tests/unit/test_openstack_security_group.py b/charms/garm-configurator/tests/unit/test_openstack_security_group.py index 3b27ce43..26388983 100644 --- a/charms/garm-configurator/tests/unit/test_openstack_security_group.py +++ b/charms/garm-configurator/tests/unit/test_openstack_security_group.py @@ -10,11 +10,9 @@ from charm_state import ProviderConfig, SecurityGroupRule from openstack_security_group import ( - MAX_STATUS_REASON_LENGTH, SecurityGroupReconcileError, SecurityGroupReconciler, security_group_name, - status_reason, ) SCALESET_NAME = "my-scaleset" @@ -81,9 +79,9 @@ def create_security_group(self, name, description): self.created_groups.append(name) return group - def delete_security_group(self, group_id): - self.deleted_groups.append(group_id) - self.groups = [group for group in self.groups if group.id != group_id] + def delete_security_group(self, security_group): + self.deleted_groups.append(security_group) + self.groups = [group for group in self.groups if group.id != security_group] def ports(self, security_group_ids=None): self.port_queries.append(security_group_ids) @@ -102,9 +100,9 @@ def create_security_group_rule(self, security_group_id, **fields): self._group(security_group_id).rules.append(rule) return rule - def delete_security_group_rule(self, rule): - self.deleted_rules.append(rule.id) - self._group_by_rule(rule).rules.remove(rule) + def delete_security_group_rule(self, security_group_rule): + self.deleted_rules.append(security_group_rule.id) + self._group_by_rule(security_group_rule).rules.remove(security_group_rule) def _group(self, group_id): return next(group for group in self.groups if group.id == group_id) @@ -187,6 +185,9 @@ def _stored_rule(**fields): SSH = SecurityGroupRule(direction="ingress", protocol="tcp", port_min=22) +SSH_FROM_SUBNET_RULE = SecurityGroupRule( + direction="ingress", protocol="tcp", port_min=22, port_max=22, remote_ip_prefix="10.0.0.0/8" +) EGRESS_ALL_V4_RULE = SecurityGroupRule(direction="egress", remote_ip_prefix="0.0.0.0/0") SSH_INGRESS = _rule_fields("ingress", protocol="tcp", port_min=22, port_max=22) SSH_INGRESS_FROM_SUBNET = _rule_fields( @@ -205,242 +206,153 @@ def test_security_group_name_is_derived_from_the_scaleset_name(): assert security_group_name(SCALESET_NAME) == GROUP_NAME -def test_missing_group_is_created_with_the_configured_rules(sdk): - """ - arrange: A project with no managed security group yet. - act: Reconcile one ingress rule and one egress rule. - assert: The group is created holding exactly those rules, because the option is the only - source of the group's rules. - """ - network = _FakeNetwork() - - group_id = _reconciler(sdk, network).reconcile( - GROUP_NAME, - [ - SecurityGroupRule( - direction="ingress", - protocol="tcp", - port_min=22, - port_max=22, - remote_ip_prefix="10.0.0.0/8", - ), - EGRESS_ALL_V4_RULE, - ], - ) - - assert network.created_groups == [GROUP_NAME] - assert network.created_rules == [SSH_INGRESS_FROM_SUBNET, EGRESS_ALL_V4] - assert group_id == f"sg-{GROUP_NAME}" - - -def test_duplicate_configured_rules_are_created_once(sdk): - """ - arrange: Rules that list the same ingress rule twice. - act: Reconcile the rules. - assert: One rule is created, so the group does not carry a duplicate the next reconcile - would then have to remove. - """ - network = _FakeNetwork() - - _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, SSH, EGRESS_ALL_V4_RULE]) - - assert network.created_rules == [SSH_INGRESS, EGRESS_ALL_V4] - - -def test_existing_group_is_reused_and_its_matching_rules_are_left_alone(sdk): - """ - arrange: A group that already holds the desired rules, with the ids OpenStack assigned. - act: Reconcile the same rules again. - assert: The existing group and its rules are kept, so reconciling unchanged configuration - makes no change at all. - """ - network = _FakeNetwork( - [ - _FakeGroup( - GROUP_NAME, - gid="sg-existing", - rules=[_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_ALL_V4)], - ) - ] - ) - - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) - - assert group_id == "sg-existing" - assert network.created_groups == [] - assert network.created_rules == [] - assert network.deleted_rules == [] - - -def test_rules_no_longer_configured_are_deleted(sdk): - """ - arrange: A group holding a rule the operator has since removed. - act: Reconcile the remaining rules. - assert: The stale rule is deleted, so the group matches the option exactly. - """ - network = _FakeNetwork( - [ - _FakeGroup( - GROUP_NAME, - rules=[ - _stored_rule(**SSH_INGRESS, rule_id=1), - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), - rule_id=2, - ), - ], - ) - ] - ) - - _reconciler(sdk, network).reconcile(GROUP_NAME, [SSH, EGRESS_ALL_V4_RULE]) - - assert network.deleted_rules == ["rule-2"] - assert network.created_rules == [EGRESS_ALL_V4] - - -def test_a_rule_added_by_hand_is_removed(sdk): - """ - arrange: A group holding a rule the operator never configured, such as one added with the - OpenStack CLI. - act: Reconcile. - assert: The rule is removed, because the option is the only source of the group's rules. - """ - network = _FakeNetwork( - [ - _FakeGroup( - GROUP_NAME, - rules=[ - _stored_rule( - **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), - rule_id=7, - ) - ], - ) - ] - ) - - _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert network.deleted_rules == ["rule-7"] - assert network.created_rules == [EGRESS_ALL_V4] - - -def test_duplicate_groups_keep_the_oldest_and_delete_the_rest(sdk): - """ - arrange: Two groups sharing the managed name and no port attaching either, as a race - between two configurators leaves. - act: Reconcile. - assert: The older group is adopted and the unused newer one deleted, so the charm - converges on one group instead of failing. - """ - network = _FakeNetwork( - [ - _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), - _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), - ] - ) - - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert group_id == "sg-older" - assert network.deleted_groups == ["sg-newer"] - assert network.created_groups == [] - - -def test_duplicate_a_port_still_attaches_is_kept_rather_than_deleted(sdk): +@pytest.mark.parametrize( + "existing_rules, configured_rules, expected_created, expected_deleted, expected_group_id", + [ + pytest.param( + None, + [SSH_FROM_SUBNET_RULE, EGRESS_ALL_V4_RULE], + [SSH_INGRESS_FROM_SUBNET, EGRESS_ALL_V4], + [], + f"sg-{GROUP_NAME}", + id="create-group", + ), + pytest.param( + [_stored_rule(**SSH_INGRESS), _stored_rule(**EGRESS_ALL_V4)], + [SSH, EGRESS_ALL_V4_RULE], + [], + [], + "sg-existing", + id="reuse-unchanged", + ), + pytest.param( + None, + [SSH, SSH, EGRESS_ALL_V4_RULE], + [SSH_INGRESS, EGRESS_ALL_V4], + [], + f"sg-{GROUP_NAME}", + id="dedupe-configured", + ), + pytest.param( + [ + _stored_rule(**SSH_INGRESS, rule_id=1), + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=8080, port_max=8080), + rule_id=2, + ), + ], + [SSH, EGRESS_ALL_V4_RULE], + [EGRESS_ALL_V4], + ["rule-2"], + "sg-existing", + id="delete-stale-rule", + ), + pytest.param( + [ + _stored_rule( + **_rule_fields("ingress", protocol="tcp", port_min=3389, port_max=3389), + rule_id=7, + ) + ], + [EGRESS_ALL_V4_RULE], + [EGRESS_ALL_V4], + ["rule-7"], + "sg-existing", + id="delete-rule-added-by-hand", + ), + ], +) +def test_rules_converge_to_the_configured_set( + sdk, existing_rules, configured_rules, expected_created, expected_deleted, expected_group_id +): """ - arrange: Two groups sharing the managed name, with runners attached to the newer one. - act: Reconcile. - assert: The attached group is adopted and the unused one deleted, so the charm converges - on the group protecting live runners and never asks OpenStack to delete a group a port - still uses. + arrange: A project whose group holds *existing_rules*, or no group at all when it is None. + act: Reconcile *configured_rules*. + assert: The group ends up holding exactly those rules — created once each, with anything + else removed — and the reconcile reports the group it kept or created. """ - network = _FakeNetwork( - [ - _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), - _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), - ], - ports=[_FakePort("sg-newer")], + groups = ( + [] + if existing_rules is None + else [_FakeGroup(GROUP_NAME, gid="sg-existing", rules=existing_rules)] ) - - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert group_id == "sg-newer" - assert network.deleted_groups == ["sg-older"] - - -def test_two_attached_duplicates_are_both_left_in_place(sdk): + network = _FakeNetwork(groups) + + group_id = _reconciler(sdk, network).reconcile(name=GROUP_NAME, rules=configured_rules) + + assert group_id == expected_group_id + assert network.created_rules == expected_created + assert network.deleted_rules == expected_deleted + + +@pytest.mark.parametrize( + "ports, delete_error, expected_group_id, expected_deleted, expected_error", + [ + pytest.param([], None, "sg-older", ["sg-newer"], None, id="no-ports-keep-oldest"), + pytest.param( + [_FakePort("sg-newer")], None, "sg-newer", ["sg-older"], None, id="attached-wins" + ), + pytest.param( + [_FakePort("sg-newer"), _FakePort("sg-older")], + None, + "sg-older", + [], + None, + id="both-attached-keep-both", + ), + pytest.param( + [], + openstack.exceptions.ConflictException("Security Group in use"), + "sg-older", + [], + None, + id="port-attaches-during-delete", + ), + pytest.param( + [], + openstack.exceptions.ForbiddenException("Policy does not allow this request"), + None, + None, + "Policy does not allow", + id="delete-refused-for-another-reason", + ), + ], +) +def test_duplicate_groups_converge_on_one_group( + sdk, ports, delete_error, expected_group_id, expected_deleted, expected_error +): """ - arrange: Two groups sharing the managed name, with a port attaching each. + arrange: Two groups sharing the managed name, with *ports* attaching them, and an OpenStack + whose delete raises *delete_error* when one is set. act: Reconcile. - assert: Neither is deleted — OpenStack would refuse — and the reconcile still succeeds on - the oldest, leaving the operator to move the other port. + assert: The charm keeps the group protecting live runners, or the oldest when none is + attached, deletes only duplicates no port uses, and tolerates a port attaching during + the delete — while any other delete failure still blocks. """ network = _FakeNetwork( [ _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), ], - ports=[_FakePort("sg-newer"), _FakePort("sg-older")], - ) - - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert group_id == "sg-older" - assert network.deleted_groups == [] - assert network.created_groups == [] - - -def test_a_port_attaching_during_the_reconcile_does_not_block(sdk): - """ - arrange: An unused duplicate, and an OpenStack that reports a conflict because a port - attached it between the port check and the delete. - act: Reconcile. - assert: The reconcile still succeeds on the survivor and the duplicate is left in place, - so a race with a runner launch cannot block the unit. - """ - - class _Conflicts(_FakeNetwork): - def delete_security_group(self, group_id): - raise openstack.exceptions.ConflictException("Security Group in use") - - network = _Conflicts( - [ - _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), - _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), - ] + ports=ports, ) + if delete_error is not None: - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + def fail(security_group): + raise delete_error - assert group_id == "sg-older" - assert network.deleted_groups == [] + setattr(network, "delete_security_group", fail) + if expected_error is not None: + with pytest.raises(SecurityGroupReconcileError) as exc_info: + _reconciler(sdk, network).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) + assert expected_error in str(exc_info.value) + return -def test_a_duplicate_delete_refused_for_another_reason_still_blocks(sdk): - """ - arrange: An OpenStack that refuses the duplicate delete with a non-conflict error, such as - a missing permission. - act: Reconcile. - assert: The failure blocks the unit, because it will not resolve on its own. - """ + group_id = _reconciler(sdk, network).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) - class _Forbidden(_FakeNetwork): - def delete_security_group(self, group_id): - raise openstack.exceptions.ForbiddenException("Policy does not allow this request") - - network = _Forbidden( - [ - _FakeGroup(GROUP_NAME, gid="sg-newer", created_at="2026-02-01T00:00:00Z"), - _FakeGroup(GROUP_NAME, gid="sg-older", created_at="2026-01-01T00:00:00Z"), - ] - ) - - with pytest.raises(SecurityGroupReconcileError) as exc_info: - _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) - - assert "Policy does not allow" in str(exc_info.value) + assert group_id == expected_group_id + assert network.deleted_groups == expected_deleted def test_ports_are_only_listed_when_the_name_is_duplicated(sdk): @@ -451,7 +363,7 @@ def test_ports_are_only_listed_when_the_name_is_duplicated(sdk): """ network = _FakeNetwork([_FakeGroup(GROUP_NAME, gid="sg-only")]) - _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, network).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) assert network.port_queries == [] @@ -472,7 +384,7 @@ def test_port_filter_is_applied_locally_too(sdk): ignore_port_filter=True, ) - group_id = _reconciler(sdk, network).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + group_id = _reconciler(sdk, network).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) assert group_id == "sg-older" assert network.deleted_groups == ["sg-newer"] @@ -484,7 +396,7 @@ def test_connection_is_closed_after_a_successful_reconcile(sdk): act: Reconcile. assert: The connection is closed, so repeated reconciles do not leak sessions. """ - _reconciler(sdk, _FakeNetwork()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, _FakeNetwork()).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) assert sdk.connection.closed is True @@ -496,7 +408,7 @@ def test_authentication_uses_the_provider_config(sdk): assert: The SDK is given those credentials, the configured region, and the public endpoint interface. """ - _reconciler(sdk, _FakeNetwork()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + _reconciler(sdk, _FakeNetwork()).reconcile(name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE]) assert sdk.auth == { "auth_url": "https://keystone.example.com:5000/v3", @@ -524,18 +436,8 @@ def connect(**_auth_fields): monkeypatch.setattr("openstack_security_group.openstack.connect", connect) with pytest.raises(SecurityGroupReconcileError) as exc_info: - SecurityGroupReconciler(_provider_config()).reconcile(GROUP_NAME, [EGRESS_ALL_V4_RULE]) + SecurityGroupReconciler(_provider_config()).reconcile( + name=GROUP_NAME, rules=[EGRESS_ALL_V4_RULE] + ) assert str(exc_info.value) == "keystone unreachable: connection refused" - - -def test_status_reason_is_bounded(): - """ - arrange: A failure with a message far longer than a status line. - act: Render it for the unit status. - assert: The status text is truncated, so Juju shows the start of the reason rather than - cutting it at an arbitrary point. - """ - exc = SecurityGroupReconcileError("x" * (MAX_STATUS_REASON_LENGTH * 2)) - - assert len(status_reason(exc)) == MAX_STATUS_REASON_LENGTH + len("...") diff --git a/docs/reference/charms.md b/docs/reference/charms.md index 4e23006c..2ee24330 100644 --- a/docs/reference/charms.md +++ b/docs/reference/charms.md @@ -86,6 +86,11 @@ OpenStack attaches the configured groups instead of the project's default securi the rules must allow the runner's outbound traffic to GitHub. When no egress rule is listed the charm allows all egress for that reason. +Nothing the charms provide needs an ingress rule. The GARM remote shell (`enable-shell`) rides +the agent's connection, which the runner opens to the GARM agent URL, and a `debug-ssh` session +is an outbound tmate client on the runner. Both are traffic the runner originates, so an +ingress-free group supports them. + A rule change reaches new runners on the next reconcile. A runner that already exists keeps the group it was launched with until GARM replaces it. From c683d6093acffb849568b8e92599ffd9794d2692 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 20 Oct 2026 10:23:19 +0000 Subject: [PATCH 10/26] test(e2e): assert the runner carries the managed security group The rest of the suite would pass if the group were silently dropped, because Nova would fall back to the project's default group and the runner would work exactly as before. After the runner is online, check the tenant directly: the group the configurator manages for the scale set admits no ingress, and the runner's port carries that group and no other. Sweep the run's garm-