From d9ca4a49924817c43b08c92def7602fb1a015bff Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 13:26:03 +0000 Subject: [PATCH 1/3] feat(garm): attach a configured OpenStack security group to runners (ISD-7035) --- charms/garm-configurator/charmcraft.yaml | 5 ++++ charms/garm-configurator/src/charm.py | 8 ++++-- charms/garm-configurator/src/charm_state.py | 9 ++++++ .../tests/unit/test_charm.py | 28 +++++++++++++++++++ charms/garm/src/charm.py | 1 + charms/garm/src/scaleset_reconciler.py | 10 +++++-- charms/garm/tests/unit/test_charm.py | 23 +++++++++++++++ .../tests/unit/test_scaleset_reconciler.py | 16 +++++++++++ docs/changelog.md | 1 + 9 files changed, 96 insertions(+), 5 deletions(-) diff --git a/charms/garm-configurator/charmcraft.yaml b/charms/garm-configurator/charmcraft.yaml index dd14772d..9d9647a0 100644 --- a/charms/garm-configurator/charmcraft.yaml +++ b/charms/garm-configurator/charmcraft.yaml @@ -52,6 +52,11 @@ config: type: string description: | OpenStack network name or ID to use for instances. + security-group: + type: string + description: | + OpenStack security group name or ID to attach to runner instances. When + unset, the provider attaches the project's default security group. github-app-id: type: int description: | diff --git a/charms/garm-configurator/src/charm.py b/charms/garm-configurator/src/charm.py index 99c779b2..46721b1f 100755 --- a/charms/garm-configurator/src/charm.py +++ b/charms/garm-configurator/src/charm.py @@ -133,9 +133,10 @@ def _configure_garm_relation(self, state: CharmState) -> 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. + image visibility, security group, 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. The optional ``org`` and ``repo`` fields are included only when set. When the image reference is present and this unit holds leadership, also @@ -161,6 +162,7 @@ def _configure_garm_relation(self, state: CharmState) -> None: "provider_name": state.provider_config.provider_name, "image_id": state.image or "", "image_visibility": state.scaleset_config.image_visibility or "", + "security_group": state.scaleset_config.security_group, "flavor": state.scaleset_config.flavor, "os_arch": state.scaleset_config.os_arch, "min_idle_runner": str(state.scaleset_config.min_idle_runner), diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index f8b987dd..115b12e2 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -24,6 +24,7 @@ OPENSTACK_PROJECT_DOMAIN_NAME_CONFIG_NAME = "openstack-project-domain-name" OPENSTACK_REGION_NAME_CONFIG_NAME = "openstack-region-name" OPENSTACK_NETWORK_CONFIG_NAME = "openstack-network" +SECURITY_GROUP_CONFIG_NAME = "security-group" GITHUB_APP_ID_CONFIG_NAME = "github-app-id" GITHUB_APP_INSTALLATION_ID_CONFIG_NAME = "github-app-installation-id" @@ -234,6 +235,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: OpenStack security group name or ID to attach to runners. """ name: str @@ -248,6 +250,7 @@ class ScalesetConfig(BaseModel): enable_shell: bool = False pre_install_scripts: str | None = None image_visibility: str | None = None + security_group: str = "" @classmethod def from_charm(cls, charm: ops.CharmBase) -> "ScalesetConfig": @@ -329,6 +332,7 @@ 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=_get_string_config(charm, SECURITY_GROUP_CONFIG_NAME), ) @@ -338,6 +342,11 @@ def _get_optional_string_config(charm: ops.CharmBase, key: str) -> str | None: return (str(value).strip() if value else "") or None +def _get_string_config(charm: ops.CharmBase, key: str) -> str: + """Return a stripped string configuration value, or "" when unset.""" + return _get_optional_string_config(charm, key) or "" + + def _ipv4_exclude_token_error(token: str) -> str | None: """Return the validation error category for an aproxy exclusion token.""" endpoints = token.split("-") diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 5fd075e4..9344bdc6 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -1042,3 +1042,31 @@ 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 ) + + +@pytest.mark.parametrize( + "configured, expected", + [("sg-runner", "sg-runner"), (" sg-runner ", "sg-runner"), (None, "")], + ids=["set", "stripped", "unset"], +) +def test_security_group_written_to_garm_configurator_relation(configured, expected): + """ + arrange: Valid config with security-group set, padded with whitespace, or unset. + act: Run config-changed with a garm-configurator relation present. + assert: The databag carries the stripped security group, or an empty string when unset so + the provider attaches the project's default group. + """ + ctx = Context(GarmConfiguratorCharm) + secret = _make_secret() + pk_secret = _make_private_key_secret() + config = _valid_config(secret, pk_secret) + if configured is not None: + config["security-group"] = configured + garm_relation = _make_garm_configurator_relation() + state = State(config=config, secrets=[secret, pk_secret], relations=[garm_relation]) + + out = ctx.run(ctx.on.config_changed(), state) + + assert out.get_relation(garm_relation.id).local_unit_data.get("security_group", "") == ( + expected + ) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index a2c0ed2e..6f4b111e 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=data.get("security_group", "").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..55b4ae4e 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: str = "" class ScalesetReconciler: @@ -1143,6 +1144,7 @@ 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") != desired_extra.get("security_groups") or (observed.template_id or 0) != template_id or observed.runner_prefix != runner_prefix(spec.name) ) @@ -1332,8 +1334,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 + security group a runner's port is attached to. Args: spec: The desired scaleset. @@ -1360,4 +1363,7 @@ def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: } if spec.image_visibility: extra_specs["image_visibility"] = spec.image_visibility + if spec.security_group: + # The provider's extra spec is a list; the configurator publishes one group. + extra_specs["security_groups"] = [spec.security_group] return extra_specs diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index e1a8f4df..956fb016 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1397,6 +1397,29 @@ def test_image_visibility_follows_the_configurator_databag( assert [spec.image_visibility for spec in specs] == [expected] +@pytest.mark.parametrize( + "databag_value, expected", + [("sg-runner", "sg-runner"), ("", ""), (None, "")], + ids=["set", "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 security_group, an empty value, or nothing — the + last being an older configurator that predates the option. + act: Run update-status. + assert: The scaleset spec carries the group, or none so the provider's default applies. + """ + data = {**_PROVIDER_UNIT_DATA, **_SCALESET_UNIT_DATA} + if databag_value is not None: + data["security_group"] = 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 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..b29d65b2 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="", ): 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=security_group, ) @@ -1338,6 +1340,20 @@ def test_create_sends_image_visibility_as_an_extra_spec(): assert params.extra_specs == {"image_visibility": "private"} +def test_create_sends_security_group_as_an_extra_spec(): + """ + arrange: A spec naming a security group locked down for runner traffic. + act: Reconcile a create. + assert: The group reaches the provider, which attaches it to the runner's port; without + it runners share the project's default group and can reach each other. + """ + client = FakeGarmClient(providers=["openstack-demo"], scalesets=[]) + _reconcile(client, [_spec(security_group="sg-runner")]) + + _, _, params = client.created[0] + assert params.extra_specs == {"security_groups": ["sg-runner"]} + + @pytest.mark.parametrize( "observed_extra, desired, expected_extra", [ diff --git a/docs/changelog.md b/docs/changelog.md index 56f84cc5..0ea9643f 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,7 @@ Each revision is versioned by the date of the revision. ## 2026-10-21 - `garm`: job steps now see `USER` and `LOGNAME` set to the account their UID resolves to, so `usermod -aG $USER` followed by `sg ` works on GARM runners. This works around the duplicate-UID account layout of the runner images; removal is tracked in [#438](https://github.com/canonical/github-runner-operators/issues/438). +- `garm-configurator`: add the optional `security-group` configuration, which the `garm` charm passes to the OpenStack provider as the scale set's `security_groups` extra spec. Runners in a project otherwise share its default security group, so they can reach each other and a compromised job can attack its peers; set `security-group` to a group that isolates them. The group applies to runners created after it is set; runners that already exist keep the group they were launched with. ## 2026-10-16 From 5fe1dd903375c322d270c6697377148084226504 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 13:28:11 +0000 Subject: [PATCH 2/3] test(garm): cover security group drift and trim the passthrough tests --- charms/garm-configurator/src/charm_state.py | 7 +---- .../tests/unit/test_charm.py | 10 +++---- charms/garm/tests/unit/test_charm.py | 4 +-- .../tests/unit/test_scaleset_reconciler.py | 28 +++++++++++++++++++ 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/charms/garm-configurator/src/charm_state.py b/charms/garm-configurator/src/charm_state.py index 115b12e2..ae500ab1 100644 --- a/charms/garm-configurator/src/charm_state.py +++ b/charms/garm-configurator/src/charm_state.py @@ -332,7 +332,7 @@ 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=_get_string_config(charm, SECURITY_GROUP_CONFIG_NAME), + security_group=str(charm.config.get(SECURITY_GROUP_CONFIG_NAME, "")).strip(), ) @@ -342,11 +342,6 @@ def _get_optional_string_config(charm: ops.CharmBase, key: str) -> str | None: return (str(value).strip() if value else "") or None -def _get_string_config(charm: ops.CharmBase, key: str) -> str: - """Return a stripped string configuration value, or "" when unset.""" - return _get_optional_string_config(charm, key) or "" - - def _ipv4_exclude_token_error(token: str) -> str | None: """Return the validation error category for an aproxy exclusion token.""" endpoints = token.split("-") diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 9344bdc6..4abe3956 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -1046,15 +1046,15 @@ def test_image_visibility_written_to_garm_configurator_relation(configured, expe @pytest.mark.parametrize( "configured, expected", - [("sg-runner", "sg-runner"), (" sg-runner ", "sg-runner"), (None, "")], - ids=["set", "stripped", "unset"], + [("sg-runner", "sg-runner"), (None, "")], + ids=["set", "unset"], ) def test_security_group_written_to_garm_configurator_relation(configured, expected): """ - arrange: Valid config with security-group set, padded with whitespace, or unset. + arrange: Valid config with security-group set or unset. act: Run config-changed with a garm-configurator relation present. - assert: The databag carries the stripped security group, or an empty string when unset so - the provider attaches the project's default group. + assert: The databag carries the group, or an empty string when unset so the provider + attaches the project's default group. """ ctx = Context(GarmConfiguratorCharm) secret = _make_secret() diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 956fb016..7db0122d 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1399,8 +1399,8 @@ def test_image_visibility_follows_the_configurator_databag( @pytest.mark.parametrize( "databag_value, expected", - [("sg-runner", "sg-runner"), ("", ""), (None, "")], - ids=["set", "empty", "absent"], + [("sg-runner", "sg-runner"), (None, "")], + ids=["set", "absent"], ) def test_security_group_follows_the_configurator_databag( ctx: Context, garm_api: _GarmApiMocks, databag_value: str | None, expected: str diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index b29d65b2..8170919e 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -1340,6 +1340,34 @@ def test_create_sends_image_visibility_as_an_extra_spec(): assert params.extra_specs == {"image_visibility": "private"} +@pytest.mark.parametrize( + "observed_extra, desired, expected_extra", + [ + ({"security_groups": ["sg-old"]}, "sg-runner", {"security_groups": ["sg-runner"]}), + ({"security_groups": ["sg-runner"]}, "", {}), + ], + ids=["changed", "cleared"], +) +def test_update_converges_security_group(observed_extra, desired, expected_extra): + """ + arrange: An existing scaleset carrying a different security group, or one the spec no + longer sets. + act: Reconcile the spec. + assert: One update brings the extra spec to the desired group, clearing it with an + explicit empty dict when unset — GARM round-trips the value as sent, so a key it + rewrote would update the scaleset on every reconcile instead. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(extra_specs=observed_extra)], + ) + _reconcile(client, [_spec(security_group=desired)]) + + assert len(client.updated) == 1 + _, params = client.updated[0] + assert params.extra_specs == expected_extra + + def test_create_sends_security_group_as_an_extra_spec(): """ arrange: A spec naming a security group locked down for runner traffic. From 226478e2c2ff55cd53151c8f7b92b0ec95d2f0f7 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 13:28:36 +0000 Subject: [PATCH 3/3] test(garm): fix the security group databag test docstring --- charms/garm/tests/unit/test_charm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 7db0122d..4a07cdb6 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1406,8 +1406,8 @@ def test_security_group_follows_the_configurator_databag( ctx: Context, garm_api: _GarmApiMocks, databag_value: str | None, expected: str ): """ - arrange: A configurator unit publishing security_group, an empty value, or nothing — the - last being an older configurator that predates the option. + arrange: A configurator unit publishing security_group, or nothing — the last being an + older configurator that predates the option. act: Run update-status. assert: The scaleset spec carries the group, or none so the provider's default applies. """