From 37f29db9a74f9a4e8e67c52b240998faf7edadb5 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 04:04:13 +0000 Subject: [PATCH 1/6] fix(garm): set USER/LOGNAME in the runner .env to the UID account GARM starts the runner as `runner`, which the runner images create as a duplicate-UID alias of `ubuntu` (useradd --non-unique --uid "$(id -u ubuntu)", home /home/ubuntu, /home/runner -> /home/ubuntu). Job steps therefore read $USER=runner, while every lookup keyed on the UID - getpwuid(1000), and with it sg, newgrp, chown, and sbuild's schroot user - resolves to `ubuntu`, the first entry for that UID in /etc/passwd. A step that passes $USER to one of those tools acted on a different account than the one the tool checked. Concretely, canonical/toolchains-ci does: sudo usermod -aG sbuild $USER # adds runner sg sbuild -c 'mk-sbuild ...' # checks ubuntu -> password prompt and fails with "Password: Invalid password." because the ubuntu account has a locked password. This affects any workflow using $USER with a UID-based tool, not just sbuild. The runner applies /.env to its own environment before any job starts (actions/runner src/Runner.Listener/Program.cs, LoadAndSetEnv), so writing USER and LOGNAME there makes $USER agree with the passwd entry the UID resolves to, and steps inherit it. The snippet is injected into github_linux_charmed, which scale sets without runner options reference directly; scale sets with runner options copy that template as the base for their own, so both get the variables. Values are written once per boot and are stripped before being rewritten, so a reused instance cannot accumulate duplicate keys. --- charms/garm/src/garm_template.py | 47 ++++++++++--- charms/garm/src/runner_paths.py | 8 +++ charms/garm/tests/unit/test_garm_template.py | 72 ++++++++++++++++++++ docs/changelog.md | 4 ++ 4 files changed, 122 insertions(+), 9 deletions(-) diff --git a/charms/garm/src/garm_template.py b/charms/garm/src/garm_template.py index 84b1a7d6..40de5eab 100644 --- a/charms/garm/src/garm_template.py +++ b/charms/garm/src/garm_template.py @@ -9,7 +9,7 @@ from charm_state import SSHDebugInfo from garm_api import GarmApiError, GarmAuthenticatedClient -from runner_paths import RUNNER_ENV_PATH, prepend_after_shebang +from runner_paths import RUNNER_ENV_PATH, RUNNER_UID_USER, prepend_after_shebang logger = logging.getLogger(__name__) @@ -28,10 +28,11 @@ def apply_charmed_template( """Create or update the charmed runner install template and return its id. Derives ``github_linux_charmed`` from the built-in ``github_linux`` base by - prepending shell snippets. The debug-ssh snippet (tmate env vars) is added - only when connections are present; with none the charmed template is still - maintained as a snippet-free copy of the base so scalesets can reference it - unconditionally (future snippets attach the same way). + prepending shell snippets, so scalesets can reference it unconditionally. + The user-env snippet (``USER``/``LOGNAME`` matching the account the runner + UID resolves to) is always present; the debug-ssh snippet (tmate env vars) is + added only when connections are present. Scale sets with runner options + derive their own template from this one, so both snippets travel with them. Args: client: Authenticated GARM API client. @@ -59,6 +60,35 @@ def apply_charmed_template( return _sync_charmed_template(client, charmed, patched_data, len(connections)) +def build_runner_env_snippet() -> str: + """Build a shell snippet that aligns ``$USER``/``$LOGNAME`` with the UID. + + GARM starts the runner as ``runner``, which the runner images create as a + duplicate-UID alias of ``ubuntu``. Every lookup keyed on the UID — + ``getpwuid``, and with it ``sg``, ``newgrp`` and sbuild's schroot user — + resolves to ``ubuntu``, so a job that read ``$USER`` acted on a different + account than the one those tools check. The runner applies ``.env`` to its + own environment before any job starts, so steps inherit these values and + ``$USER`` matches the passwd entry the UID resolves to. + + Returns: + A shell snippet string (no shebang) writing the two variables to the + runner ``.env`` file. + """ + lines = [ + f"mkdir -p $(dirname {RUNNER_ENV_PATH})", + f"touch {RUNNER_ENV_PATH}", + # Idempotent on reused instances: drop any previous value before writing. + f"sed -i '/^USER=/d;/^LOGNAME=/d' {RUNNER_ENV_PATH}", + f'cat >> {RUNNER_ENV_PATH} << "EOF"', + f"USER={RUNNER_UID_USER}", + f"LOGNAME={RUNNER_UID_USER}", + "EOF", + "", + ] + return "\n".join(lines) + + def build_tmate_env_snippet(connections: list[SSHDebugInfo]) -> str: """Build a shell snippet that writes tmate env vars to the runner's .env file. @@ -101,7 +131,8 @@ def _build_charmed_template_data( Args: client: Authenticated GARM API client. base_id: ID of the base template to fetch. - connections: Debug-ssh connections used to build the tmate snippet. + connections: Debug-ssh connections used to build the tmate snippet; may + be empty. Returns: Patched shell script as bytes. @@ -119,9 +150,7 @@ def _build_charmed_template_data( # GARM returns the template body as a base64-encoded string. base_script = base64.b64decode(base_template.data).decode("utf-8") - snippet = build_tmate_env_snippet(connections) - if not snippet: - return base_script.encode("utf-8") + snippet = build_runner_env_snippet() + build_tmate_env_snippet(connections) return prepend_after_shebang(base_script, snippet).encode("utf-8") diff --git a/charms/garm/src/runner_paths.py b/charms/garm/src/runner_paths.py index 1507388c..c0487823 100644 --- a/charms/garm/src/runner_paths.py +++ b/charms/garm/src/runner_paths.py @@ -17,6 +17,14 @@ PRE_JOB_HOOK_PATH = f"{RUNNER_HOME}/pre-job.sh" RUNNER_ENV_PATH = f"{RUNNER_HOME}/.env" +# The account that owns the runner's UID. The runner images create `runner` as a +# duplicate-UID alias of `ubuntu`, so `getpwuid(uid)` — and with it `sg`, +# `newgrp`, `chown`, and sbuild's schroot user — resolves to `ubuntu`, the first +# entry for that UID in /etc/passwd, while `$USER` said `runner`. Job `.env` sets +# `USER`/`LOGNAME` to this value so both agree. See +# canonical/self-hosted-github-runner-images images/standard/templates/_common.j2. +RUNNER_UID_USER = "ubuntu" + def prepend_after_shebang(script: str, snippet: str) -> str: """Insert *snippet* immediately after the shebang line of *script*. diff --git a/charms/garm/tests/unit/test_garm_template.py b/charms/garm/tests/unit/test_garm_template.py index d1315868..6c5a63e4 100644 --- a/charms/garm/tests/unit/test_garm_template.py +++ b/charms/garm/tests/unit/test_garm_template.py @@ -355,3 +355,75 @@ def test_apply_charmed_template_raises_when_charmed_has_no_id(): garm_template.CharmedTemplateError, match=garm_template.GARM_CHARMED_TEMPLATE_NAME ): garm_template.apply_charmed_template(client, []) + + +def test_runner_env_snippet_matches_uid_resolved_account(): + """ + arrange: No inputs. + act: Call build_runner_env_snippet(). + assert: The snippet writes USER and LOGNAME to the runner .env, set to the + account the runner UID resolves to, and strips stale values first. + """ + snippet = garm_template.build_runner_env_snippet() + + assert f"touch {garm_template.RUNNER_ENV_PATH}" in snippet + assert f"sed -i '/^USER=/d;/^LOGNAME=/d' {garm_template.RUNNER_ENV_PATH}" in snippet + assert f"USER={garm_template.RUNNER_UID_USER}" in snippet + assert f"LOGNAME={garm_template.RUNNER_UID_USER}" in snippet + assert garm_template.RUNNER_UID_USER == "ubuntu" + + +def test_build_charmed_template_data_sets_user_env_without_connections(): + """ + arrange: A base script and no debug-ssh connections. + act: Call _build_charmed_template_data(). + assert: The rendered script still carries the USER/LOGNAME block, so every + scale set referencing the charmed template gets it. + """ + client = MagicMock() + template = MagicMock() + template.data = base64.b64encode(b"#!/bin/bash\nset -e\necho hello\n").decode() + client.get_template.return_value = template + + decoded = garm_template._build_charmed_template_data(client, 1, []).decode() + + assert decoded.startswith("#!/bin/bash\n") + assert f"USER={garm_template.RUNNER_UID_USER}" in decoded + assert f"LOGNAME={garm_template.RUNNER_UID_USER}" in decoded + + +def test_build_charmed_template_data_user_env_precedes_base_body(): + """ + arrange: A base script whose body would abort on error. + act: Call _build_charmed_template_data() with no connections. + assert: The user-env block lands before the base body, so it is written even + though the base template later enables set -e. + """ + client = MagicMock() + template = MagicMock() + template.data = base64.b64encode(b"#!/bin/bash\nset -e\necho body\n").decode() + client.get_template.return_value = template + + decoded = garm_template._build_charmed_template_data(client, 1, []).decode() + + assert decoded.index("USER=") < decoded.index("set -e") + assert decoded.index("LOGNAME=") < decoded.index("set -e") + + +def test_build_charmed_template_data_sets_user_env_once(): + """ + arrange: A base script and one debug-ssh connection. + act: Call _build_charmed_template_data(). + assert: Exactly one USER= assignment is written, and the tmate variables are + still present alongside it. + """ + client = MagicMock() + template = MagicMock() + template.data = base64.b64encode(b"#!/bin/bash\necho hello\n").decode() + client.get_template.return_value = template + + decoded = garm_template._build_charmed_template_data(client, 1, [_CONN]).decode() + written = [line for line in decoded.splitlines() if line.startswith("USER=")] + + assert written == [f"USER={garm_template.RUNNER_UID_USER}"] + assert f"TMATE_SERVER_HOST={_CONN.host}" in decoded diff --git a/docs/changelog.md b/docs/changelog.md index eb236167..0d9f6964 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-21 + +- `garm`: set `USER` and `LOGNAME` in the runner `.env` so job steps see the account the runner UID resolves to (`ubuntu`) rather than the duplicate-UID alias (`runner`) that GARM starts the runner as. Tools that resolve the caller by UID — `sg`, `newgrp`, `chown`, and sbuild's schroot user — already reported `ubuntu`, so a step doing `usermod -aG $USER` added the alias while `sg ` checked `ubuntu` and fell back to a password prompt. The values are written by the `github_linux_charmed` template, which scale sets without runner options reference directly and scale sets with runner options derive their template from. + ## 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. From de3dd9c029c3d8abce9fb05c68a8e51f75ccc5a5 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 05:26:13 +0000 Subject: [PATCH 2/6] fix(garm): align job USER/LOGNAME with the runner UID account GARM starts the runner as `runner`, which the runner images create as a duplicate-UID alias of `ubuntu`. Job steps therefore read $USER=runner while every lookup keyed on the UID - getpwuid(1000), and with it sg, newgrp, chown and sbuild's schroot user - resolves to `ubuntu`. A step that hands $USER to one of those tools acts on a different account than the tool checks, e.g. toolchains-ci's `usermod -aG sbuild $USER` followed by `sg sbuild`, which fails with "Password: Invalid password.". The runner applies /.env to its own environment before any job starts (actions/runner Program.cs, LoadAndSetEnv), so writing USER and LOGNAME there makes $USER agree with the passwd entry the UID resolves to. The account is resolved on the instance rather than hardcoded, so the snippet stays correct if the image gives the account its own UID. One writer now renders the whole .env block: the account pair is written with printf because the value must expand on the instance, and the tmate variables keep their quoted heredoc so relation-provided values are not expanded. Removal is tracked in #438. --- charms/garm/src/garm_template.py | 91 +++++++++----------- charms/garm/src/runner_paths.py | 8 -- charms/garm/src/runner_template.py | 2 +- charms/garm/tests/unit/test_garm_template.py | 78 +++++++++++------ docs/changelog.md | 2 +- 5 files changed, 96 insertions(+), 85 deletions(-) diff --git a/charms/garm/src/garm_template.py b/charms/garm/src/garm_template.py index 40de5eab..fbfebdf3 100644 --- a/charms/garm/src/garm_template.py +++ b/charms/garm/src/garm_template.py @@ -9,7 +9,7 @@ from charm_state import SSHDebugInfo from garm_api import GarmApiError, GarmAuthenticatedClient -from runner_paths import RUNNER_ENV_PATH, RUNNER_UID_USER, prepend_after_shebang +from runner_paths import RUNNER_ENV_PATH, RUNNER_USER, prepend_after_shebang logger = logging.getLogger(__name__) @@ -60,64 +60,55 @@ def apply_charmed_template( return _sync_charmed_template(client, charmed, patched_data, len(connections)) -def build_runner_env_snippet() -> str: - """Build a shell snippet that aligns ``$USER``/``$LOGNAME`` with the UID. +def build_env_snippet(connections: list[SSHDebugInfo]) -> str: + """Render the writer for the runner's ``.env`` file. - GARM starts the runner as ``runner``, which the runner images create as a - duplicate-UID alias of ``ubuntu``. Every lookup keyed on the UID — - ``getpwuid``, and with it ``sg``, ``newgrp`` and sbuild's schroot user — - resolves to ``ubuntu``, so a job that read ``$USER`` acted on a different - account than the one those tools check. The runner applies ``.env`` to its - own environment before any job starts, so steps inherit these values and - ``$USER`` matches the passwd entry the UID resolves to. - - Returns: - A shell snippet string (no shebang) writing the two variables to the - runner ``.env`` file. - """ - lines = [ - f"mkdir -p $(dirname {RUNNER_ENV_PATH})", - f"touch {RUNNER_ENV_PATH}", - # Idempotent on reused instances: drop any previous value before writing. - f"sed -i '/^USER=/d;/^LOGNAME=/d' {RUNNER_ENV_PATH}", - f'cat >> {RUNNER_ENV_PATH} << "EOF"', - f"USER={RUNNER_UID_USER}", - f"LOGNAME={RUNNER_UID_USER}", - "EOF", - "", - ] - return "\n".join(lines) + The GitHub Actions runner applies ``.env`` to its own environment before any + job starts, so steps inherit these values. + The first two entries are the account the runner UID resolves to, because + the runner images create ``runner`` as a duplicate-UID alias of ``ubuntu``: + every lookup keyed on the UID — ``getpwuid``, and with it ``sg``, ``newgrp``, + ``chown`` and sbuild's schroot user — resolves to ``ubuntu``, so a step that + read ``$USER`` acted on a different account than those tools check. Resolving + on the instance keeps the snippet correct if the account layout changes: it + yields ``ubuntu`` on the current images and ``runner`` on an image that gives + the account its own UID, where the override is a no-op. -def build_tmate_env_snippet(connections: list[SSHDebugInfo]) -> str: - """Build a shell snippet that writes tmate env vars to the runner's .env file. - - Uses only the first connection (caller must sort for stability). + The tmate variables are appended only when debug-ssh connections exist and + use a quoted heredoc, so relation-provided values are written verbatim. Args: - connections: List of SSHDebugInfo from the debug-ssh relation. + connections: Debug-ssh connections; empty means no tmate variables. Returns: - A shell snippet string (no shebang) to be prepended to the base template, - or an empty string when there are no connections. + A shell snippet string (no shebang) writing the entries to ``.env``. """ - if not connections: - return "" - conn = connections[0] - # Global env write: injected into the shared github_linux_charmed template, so - # these tmate vars apply to every scaleset (a separate .env append from the - # per-scaleset one in runner_template.render_pre_job_hooks). - runner_env = RUNNER_ENV_PATH + # TODO(#438): drop the USER/LOGNAME entries once the runner account layout is + # settled; the alias, and with it this override, should disappear. lines = [ - f"mkdir -p $(dirname {runner_env})", - f'cat >> {runner_env} << "EOF"', - f"TMATE_SERVER_HOST={conn.host}", - f"TMATE_SERVER_PORT={conn.port}", - f"TMATE_SERVER_RSA_FINGERPRINT={conn.rsa_fingerprint}", - f"TMATE_SERVER_ED25519_FINGERPRINT={conn.ed25519_fingerprint}", - "EOF", - "", + f"mkdir -p $(dirname {RUNNER_ENV_PATH})", + # `id -u` works whoever runs the template (cloud-init runs it as root). + f'runner_account="$(getent passwd "$(id -u {RUNNER_USER})" | cut -d: -f1)"', + # printf rather than a heredoc: the value must expand on the instance, + # while the guard avoids blanking USER when the lookup fails. + f"[ -n \"$runner_account\" ] && printf 'USER=%s\\nLOGNAME=%s\\n' " + f'"$runner_account" "$runner_account" >> {RUNNER_ENV_PATH}', ] + if connections: + conn = connections[0] + # Global env write: injected into the shared github_linux_charmed + # template, so these apply to every scaleset (a separate .env append + # from the per-scaleset one in runner_template.render_pre_job_hooks). + lines += [ + f'cat >> {RUNNER_ENV_PATH} << "EOF"', + f"TMATE_SERVER_HOST={conn.host}", + f"TMATE_SERVER_PORT={conn.port}", + f"TMATE_SERVER_RSA_FINGERPRINT={conn.rsa_fingerprint}", + f"TMATE_SERVER_ED25519_FINGERPRINT={conn.ed25519_fingerprint}", + "EOF", + ] + lines.append("") return "\n".join(lines) @@ -150,7 +141,7 @@ def _build_charmed_template_data( # GARM returns the template body as a base64-encoded string. base_script = base64.b64decode(base_template.data).decode("utf-8") - snippet = build_runner_env_snippet() + build_tmate_env_snippet(connections) + snippet = build_env_snippet(connections) return prepend_after_shebang(base_script, snippet).encode("utf-8") diff --git a/charms/garm/src/runner_paths.py b/charms/garm/src/runner_paths.py index c0487823..1507388c 100644 --- a/charms/garm/src/runner_paths.py +++ b/charms/garm/src/runner_paths.py @@ -17,14 +17,6 @@ PRE_JOB_HOOK_PATH = f"{RUNNER_HOME}/pre-job.sh" RUNNER_ENV_PATH = f"{RUNNER_HOME}/.env" -# The account that owns the runner's UID. The runner images create `runner` as a -# duplicate-UID alias of `ubuntu`, so `getpwuid(uid)` — and with it `sg`, -# `newgrp`, `chown`, and sbuild's schroot user — resolves to `ubuntu`, the first -# entry for that UID in /etc/passwd, while `$USER` said `runner`. Job `.env` sets -# `USER`/`LOGNAME` to this value so both agree. See -# canonical/self-hosted-github-runner-images images/standard/templates/_common.j2. -RUNNER_UID_USER = "ubuntu" - def prepend_after_shebang(script: str, snippet: str) -> str: """Insert *snippet* immediately after the shebang line of *script*. diff --git a/charms/garm/src/runner_template.py b/charms/garm/src/runner_template.py index e3bcb93f..80c71d04 100644 --- a/charms/garm/src/runner_template.py +++ b/charms/garm/src/runner_template.py @@ -151,7 +151,7 @@ def render_pre_job_hooks(config: RunnerConfig) -> str: env_delim = _heredoc_delimiter("\n".join(env_entries), "GARM_CHARM_ENV") # Per-scaleset env write: these vars come from the scaleset's RunnerConfig, so # they land in the per-scaleset template (a separate .env append from the - # global tmate one in garm_template.build_tmate_env_snippet). + # global one in garm_template.build_env_snippet). return _PRE_JOB_HOOKS_TEMPLATE.render( runner_home=RUNNER_HOME, hook_path=PRE_JOB_HOOK_PATH, diff --git a/charms/garm/tests/unit/test_garm_template.py b/charms/garm/tests/unit/test_garm_template.py index 6c5a63e4..7c85ad56 100644 --- a/charms/garm/tests/unit/test_garm_template.py +++ b/charms/garm/tests/unit/test_garm_template.py @@ -18,7 +18,7 @@ def test_ssh_debug_info_env_vars(): """ arrange: One SSHDebugInfo connection. - act: Call build_tmate_env_snippet() and prepend_after_shebang(). + act: Call build_env_snippet() and prepend_after_shebang(). assert: The patched script starts with the shebang, then contains all four TMATE_SERVER_* variables with correct values. """ @@ -30,7 +30,7 @@ def test_ssh_debug_info_env_vars(): ) base_script = "#!/bin/bash\necho 'hello'\n" - snippet = garm_template.build_tmate_env_snippet([conn]) + snippet = garm_template.build_env_snippet([conn]) patched = garm_template.prepend_after_shebang(base_script, snippet) assert patched.startswith("#!/bin/bash\n") @@ -357,28 +357,54 @@ def test_apply_charmed_template_raises_when_charmed_has_no_id(): garm_template.apply_charmed_template(client, []) -def test_runner_env_snippet_matches_uid_resolved_account(): +def test_env_snippet_resolves_runner_account_on_the_instance(): """ - arrange: No inputs. - act: Call build_runner_env_snippet(). - assert: The snippet writes USER and LOGNAME to the runner .env, set to the - account the runner UID resolves to, and strips stale values first. + arrange: No debug-ssh connections. + act: Call build_env_snippet(). + assert: The account is derived on the instance from the runner UID and + written as USER and LOGNAME, with no account name baked into the + template. """ - snippet = garm_template.build_runner_env_snippet() + snippet = garm_template.build_env_snippet([]) - assert f"touch {garm_template.RUNNER_ENV_PATH}" in snippet - assert f"sed -i '/^USER=/d;/^LOGNAME=/d' {garm_template.RUNNER_ENV_PATH}" in snippet - assert f"USER={garm_template.RUNNER_UID_USER}" in snippet - assert f"LOGNAME={garm_template.RUNNER_UID_USER}" in snippet - assert garm_template.RUNNER_UID_USER == "ubuntu" + assert 'getent passwd "$(id -u runner)"' in snippet + assert "printf 'USER=%s\\nLOGNAME=%s\\n'" in snippet + assert "$runner_account" in snippet + assert "USER=ubuntu" not in snippet + assert "USER=runner" not in snippet + + +def test_env_snippet_guards_an_empty_lookup(): + """ + arrange: No debug-ssh connections. + act: Call build_env_snippet(). + assert: The write is guarded, so a failed lookup cannot blank USER in .env. + """ + snippet = garm_template.build_env_snippet([]) + + assert '[ -n "$runner_account" ]' in snippet + + +def test_env_snippet_writes_tmate_vars_verbatim(): + """ + arrange: One debug-ssh connection. + act: Call build_env_snippet(). + assert: The tmate variables go through a quoted heredoc, so values from the + relation are not shell-expanded. + """ + snippet = garm_template.build_env_snippet([_CONN]) + + assert '<< "EOF"' in snippet + assert f"TMATE_SERVER_HOST={_CONN.host}" in snippet + assert f"TMATE_SERVER_PORT={_CONN.port}" in snippet def test_build_charmed_template_data_sets_user_env_without_connections(): """ arrange: A base script and no debug-ssh connections. act: Call _build_charmed_template_data(). - assert: The rendered script still carries the USER/LOGNAME block, so every - scale set referencing the charmed template gets it. + assert: The rendered script resolves and writes the account, so every scale + set referencing the charmed template gets the entries. """ client = MagicMock() template = MagicMock() @@ -388,16 +414,17 @@ def test_build_charmed_template_data_sets_user_env_without_connections(): decoded = garm_template._build_charmed_template_data(client, 1, []).decode() assert decoded.startswith("#!/bin/bash\n") - assert f"USER={garm_template.RUNNER_UID_USER}" in decoded - assert f"LOGNAME={garm_template.RUNNER_UID_USER}" in decoded + assert 'getent passwd "$(id -u runner)"' in decoded + assert "printf 'USER=%s\\nLOGNAME=%s\\n'" in decoded + assert "TMATE_SERVER_HOST" not in decoded def test_build_charmed_template_data_user_env_precedes_base_body(): """ arrange: A base script whose body would abort on error. act: Call _build_charmed_template_data() with no connections. - assert: The user-env block lands before the base body, so it is written even - though the base template later enables set -e. + assert: The env writer lands before the base body, so it runs before the + base template enables set -e. """ client = MagicMock() template = MagicMock() @@ -406,15 +433,15 @@ def test_build_charmed_template_data_user_env_precedes_base_body(): decoded = garm_template._build_charmed_template_data(client, 1, []).decode() - assert decoded.index("USER=") < decoded.index("set -e") - assert decoded.index("LOGNAME=") < decoded.index("set -e") + assert decoded.index("runner_account=") < decoded.index("set -e") + assert decoded.index("printf 'USER=") < decoded.index("set -e") -def test_build_charmed_template_data_sets_user_env_once(): +def test_build_charmed_template_data_writes_user_env_once(): """ arrange: A base script and one debug-ssh connection. act: Call _build_charmed_template_data(). - assert: Exactly one USER= assignment is written, and the tmate variables are + assert: Exactly one USER assignment is written, and the tmate variables are still present alongside it. """ client = MagicMock() @@ -423,7 +450,8 @@ def test_build_charmed_template_data_sets_user_env_once(): client.get_template.return_value = template decoded = garm_template._build_charmed_template_data(client, 1, [_CONN]).decode() - written = [line for line in decoded.splitlines() if line.startswith("USER=")] + written = [line for line in decoded.splitlines() if "USER=" in line] - assert written == [f"USER={garm_template.RUNNER_UID_USER}"] + assert len(written) == 1 + assert "printf" in written[0] assert f"TMATE_SERVER_HOST={_CONN.host}" in decoded diff --git a/docs/changelog.md b/docs/changelog.md index 0d9f6964..56f84cc5 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-21 -- `garm`: set `USER` and `LOGNAME` in the runner `.env` so job steps see the account the runner UID resolves to (`ubuntu`) rather than the duplicate-UID alias (`runner`) that GARM starts the runner as. Tools that resolve the caller by UID — `sg`, `newgrp`, `chown`, and sbuild's schroot user — already reported `ubuntu`, so a step doing `usermod -aG $USER` added the alias while `sg ` checked `ubuntu` and fell back to a password prompt. The values are written by the `github_linux_charmed` template, which scale sets without runner options reference directly and scale sets with runner options derive their template from. +- `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). ## 2026-10-16 From 279f859b66bacf9dcadf0a50b846169146d88686 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 06:27:33 +0000 Subject: [PATCH 3/6] docs: clarify UID fix in template --- charms/garm/src/garm_template.py | 2 + task.md | 96 ++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 task.md diff --git a/charms/garm/src/garm_template.py b/charms/garm/src/garm_template.py index fbfebdf3..b95d0401 100644 --- a/charms/garm/src/garm_template.py +++ b/charms/garm/src/garm_template.py @@ -66,6 +66,8 @@ def build_env_snippet(connections: list[SSHDebugInfo]) -> str: The GitHub Actions runner applies ``.env`` to its own environment before any job starts, so steps inherit these values. + # 2026-10-21: once the account setup decisions are finalized, this workaround + # can be removed. See #438. The first two entries are the account the runner UID resolves to, because the runner images create ``runner`` as a duplicate-UID alias of ``ubuntu``: every lookup keyed on the UID — ``getpwuid``, and with it ``sg``, ``newgrp``, diff --git a/task.md b/task.md new file mode 100644 index 00000000..c654cbff --- /dev/null +++ b/task.md @@ -0,0 +1,96 @@ +Here is a complete, actionable implementation plan for adding automated, per-scaleset OpenStack security group management to the `garm-configurator` and `garm` charms. It is structured so that agent developers can pick up individual tasks and work independently. + +--- + +## 🎯 Objective + +Enable the `garm-configurator` charm to reconcile a dedicated, locked-down OpenStack security group for each GitHub scaleset it manages. The charm must create or update the security group to match operator-defined rules, then pass its identifier to the `garm` charm via the existing `garm-configurator` relation. The `garm` charm must consume that identifier and ensure it reaches the `garm-provider-openstack` as the `security_groups` entry in the provider’s `extra_specs` schema. + +This keeps responsibility for network‑isolation policy with the configuration layer (the `garm-configurator`) and preserves the `garm` charm as a workload executor that simply forwards configuration to the GARM server and OpenStack provider. + +--- + +## 🗺️ Work Breakdown Structure + +### Phase 1 — Foundation & Contracts (Pre‑Implementation) + +| Task | Description | Owner | Deliverable | +|------|-------------|-------|-------------| +| **1.1 Define relation‑data contract** | Add a new key `security_group_id` to the `garm-configurator` relation databag. Document it alongside existing keys (`name`, `provider_name`, `image_id`, etc.) in `charms/garm-configurator/src/charm_state.py` and `charms/garm/src/charm_state.py`. | Configurator & GARM charm maintainers | Updated `RUNNER_OPTION_DATABAG_KEYS` or equivalent constant, plus a short design note in `docs/reference/charms.md`. | +| **1.2 Validate OpenStack RBAC** | Confirm that the OpenStack credentials passed to `garm-configurator` (via `openstack-password` secret and related options) already include permissions to create, update, and delete security groups and rules. If not, document the required role (`member` with `create_security_group`, `create_security_group_rule`, `delete_security_group`, `delete_security_group_rule`). | Platform / infra team | A short RFC or issue comment listing the exact OpenStack policy actions. | +| **1.3 Confirm `extra_specs` plumbing** | Verify that the `garm` charm already passes a JSON‑encoded `extra_specs` object to the GARM API when creating a scaleset. If it does not, add an internal function to construct `extra_specs` from relation data. The `garm-provider-openstack` schema already accepts `security_groups` as an array of strings. | GARM charm maintainers | A code comment or unit test proving `extra_specs` can carry `security_groups`. | + +**Why first?** These contracts must be agreed upon before parallel work begins on either charm. They also surface any missing OpenStack permissions early, avoiding blocked integration tests later. + +--- + +### Phase 2 — `garm-configurator` Implementation + +| Task | Description | Owner | Deliverable | +|------|-------------|-------|-------------| +| **2.1 Add charm configuration options** | Extend `charms/garm-configurator/charmcraft.yaml` with a new option, e.g. `security-group-rules` (type: `string`, JSON‑encoded list of rule objects) and optionally `manage-security-group` (type: `boolean`, default `true`). Document the rule schema in the option description. | Configurator maintainer | Updated `charmcraft.yaml` with new options and validation notes. | +| **2.2 Parse and validate rule configuration** | In `charms/garm-configurator/src/charm_state.py`, add a Pydantic model (e.g. `SecurityGroupRule`) and a validator that parses the `security-group-rules` option. Reject malformed rules with `CharmConfigInvalidError` so the charm enters `BlockedStatus`. | Configurator maintainer | New pydantic model, validation unit tests. | +| **2.3 Implement OpenStack security‑group reconciler** | Create a new module (e.g. `charms/garm-configurator/src/openstack_security_group.py`) with a class `SecurityGroupReconciler`. It must: 1) authenticate using the existing `ProviderConfig` credentials; 2) search for a security group by a deterministic name (e.g. `garm-`); 3) create it if missing; 4) diff and reconcile rules (add missing, remove extra); 5) return the security group ID. Use the OpenStack SDK (`openstacksdk`) or `gophercloud`‑style HTTP calls consistent with the charm’s existing dependencies. | Configurator maintainer | A testable reconciler class with idempotent `reconcile()` method. | +| **2.4 Wire reconciler into `_reconcile`** | In `charms/garm-configurator/src/charm.py`, call the reconciler during `_reconcile` (after `CharmState` validation, before `_configure_garm_relation`). If reconciliation fails, set `BlockedStatus` with a clear message; if it succeeds, store the resulting ID on the `CharmState` or pass it directly to `_configure_garm_relation`. | Configurator maintainer | Updated `_reconcile` logic with error handling. | +| **2.5 Publish `security_group_id` to relation** | In `_configure_garm_relation`, add `"security_group_id": ` to the `basic_data` dictionary that is written to `relation.data[self.unit]`. Ensure the key is omitted when no security group is managed (backwards compatibility). | Configurator maintainer | Relation databag now includes `security_group_id` when applicable. | +| **2.6 Lifecycle‑safe deletion** | When the charm is removed or the scaleset is deleted, optionally delete the security group **only if** it is not referenced by any other resource. Implement a conservative default: do **not** delete the group automatically unless `manage-security-group` is explicitly set to `true` and a separate `delete-security-group-on-remove` option is enabled. | Configurator maintainer | Safe cleanup logic, documented behaviour. | + +**Integration point:** The relation databag currently writes keys such as `name`, `provider_name`, `image_id`, `flavor`, `os_arch`, `min_idle_runner`, `max_runner`, `labels`, `runner_group`, `enable_shell`, and `pre_install_scripts`. Adding `security_group_id` alongside these is a minimal, non‑breaking change. + +--- + +### Phase 3 — `garm` Charm Implementation + +| Task | Description | Owner | Deliverable | +|------|-------------|-------|-------------| +| **3.1 Parse `security_group_id` from relation** | In `charms/garm/src/charm_state.py`, add `security_group_id` to the dataclass or mapping that represents a configurator’s scaleset specification. Treat an absent key as `None` so existing deployments continue to work. | GARM charm maintainer | Updated `ScalesetSpec` (or equivalent) with `security_group_id: str \| None`. | +| **3.2 Build `extra_specs` for the provider** | In `charms/garm/src/scaleset_reconciler.py` (or a new helper), construct an `extra_specs` dictionary for the `garm-provider-openstack`. When `security_group_id` is present, include `{"security_groups": []}`. Pass this dictionary to the GARM API call that creates or updates the scaleset (the `CreateScaleSetParams` model already supports provider‑specific extra specs). | GARM charm maintainer | `extra_specs` populated with `security_groups` when the relation provides an ID. | +| **3.3 Add unit tests** | Extend `charms/garm/tests/unit/` with tests that: 1) a relation databag containing `security_group_id` results in `extra_specs["security_groups"]` being set; 2) an absent key leaves `extra_specs` unchanged. Use the existing pytest‑operator / ops‑testing patterns. | GARM charm maintainer | Passing unit tests covering the new path. | + +**Existing code context:** The GARM charm already builds a `ScalesetSpec` dataclass with fields like `name`, `provider_name`, `image`, `flavor`, etc.. Adding `security_group_id` follows the same pattern. The reconciler already calls `self._client.list_providers()` and `self._client.list_scalesets()`, so the API client is in place to accept `extra_specs` in the create/update payload. + +--- + +### Phase 4 — Testing Strategy + +| Test Level | Scope | Tools | Deliverable | +|------------|-------|-------|-------------| +| **Unit (configurator)** | Rule parsing, validation errors, reconciler idempotency (create → update → no‑op), relation databag contains `security_group_id`. | `pytest`, `ops.testing`, mocked OpenStack SDK. | ≥ 90 % coverage of new modules. | +| **Unit (GARM charm)** | Relation parsing, `extra_specs` construction, backward compatibility when key is absent. | `pytest`, `ops.testing`. | New test cases in `charms/garm/tests/unit/`. | +| **Integration** | Deploy both charms to a test OpenStack tenant. Configure a scaleset with a locked‑down security group rule (e.g. deny all ingress except GARM callback). Verify that the runner VM is attached to the created security group (via OpenStack CLI: `openstack server show -f json \| jq '.security_groups'`). Destroy the scaleset and confirm the security group is either left in place (default) or deleted (if opted in). | `pytest-operator`, `tox -e integration` (as per repository `CONTRIBUTING.md`). | A CI‑ready integration test that proves end‑to‑end isolation. | +| **Regression** | Ensure that deployments **without** the new config option continue to work exactly as before (no security group created, no `security_group_id` in relation). | Existing unit + integration suites. | All pre‑existing tests pass unchanged. | + +--- + +### Phase 5 — Documentation & Rollout + +| Task | Description | Owner | +|------|-------------|-------| +| **5.1 Update `charmcraft.yaml` descriptions** | Clearly describe the new `security-group-rules` option, its JSON schema, and the isolation benefit. | Configurator maintainer | +| **5.2 Update reference docs** | Add a section to `docs/reference/charms.md` explaining the new relation‑databag key and the security‑group lifecycle. Mention the required OpenStack RBAC. | Documentation maintainer | +| **5.3 Provide an example configuration** | Add a YAML snippet to the tutorial (`docs/tutorial/garm.md`) showing a minimal locked‑down rule set (e.g. allow ingress only from the GARM charm’s IP on port 8080). | Documentation maintainer | +| **5.4 Release notes** | Add a `CHANGELOG.md` entry under “New features” describing automated security‑group reconciliation. Note any permission changes required for existing deployments. | Release manager | +| **5.5 Staged rollout** | Merge behind a feature flag (`manage-security-group` default `false`) so that existing users are unaffected until they explicitly opt in. After one release cycle, consider changing the default to `true` if adoption is smooth. | Platform team | + +--- + +## ⚠️ Risks & Mitigations + +| Risk | Impact | Mitigation | +|------|--------|------------| +| OpenStack credentials lack `create_security_group_rule` permission. | Configurator enters `BlockedStatus`; runners cannot be isolated. | Pre‑flight validation (Task 1.2) and a clear error message directing operators to add the policy. | +| Security‑group rule conflicts with manually attached groups via `extra_specs`. | Unexpected network behaviour. | Document that the charm‑managed group is **additive**; operators should not duplicate rules in manual `extra_specs`. Provide a `security-group-rules` option that merges rather than replaces. | +| Accidental deletion of a security group still in use. | Running runners lose network access. | Default to **no deletion**; require an explicit `delete-security-group-on-remove` boolean. Before deletion, query OpenStack for ports using the group and refuse if any exist. | +| Backward compatibility broken for existing relation consumers. | Existing `garm` deployments fail to parse the new key. | Omit `security_group_id` when absent; the `garm` charm treats a missing key as `None`. Add a regression test. | + +--- + +## 📦 Deliverables Summary + +- **Configurator charm:** New `security-group-rules` (and optional `manage-security-group`) config options; a `SecurityGroupReconciler` module; relation‑databag key `security_group_id`; safe lifecycle handling. +- **GARM charm:** Parsing of `security_group_id`; `extra_specs` construction with `security_groups`; unit tests. +- **Tests:** Unit tests for both charms; an integration test that proves a runner is attached to the expected security group. +- **Docs:** Updated `charmcraft.yaml`, reference documentation, tutorial example, and release notes. +- **Rollout:** Feature flagged with a conservative default, staged promotion through `edge` → `candidate`. + +This plan allows agents to work in parallel on the configurator and GARM charm once the Phase 1 contracts are agreed, and it gives each task a concrete, verifiable deliverable. From f71a678435c3a137097aca0048a8a30e2de2b5f1 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 06:39:08 +0000 Subject: [PATCH 4/6] test(e2e): assert the job identity matches the runner UID The e2e job runs on a GARM-spawned runner, the only place the duplicate-UID account layout shows up at job level. It printed whoami but never compared it with $USER, so the toolchains-ci failure ("Password: Invalid password.") went unnoticed even though the disagreeing value was in the log. Adds three steps: - assert $USER and LOGNAME equal the account the UID resolves to - assert usermod -aG "$USER" then sg succeeds, the pattern from the report; the step is a new process whose groups predate the usermod, so sg must resolve the caller from the UID - report the runner uid/gid entry counts so #438 can tell when the .env override is no longer needed (non-gating, independent of the invoking account) --- .github/workflows/garm_e2e_test_run.yaml | 53 ++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/.github/workflows/garm_e2e_test_run.yaml b/.github/workflows/garm_e2e_test_run.yaml index 1cf66574..bd19af1e 100644 --- a/.github/workflows/garm_e2e_test_run.yaml +++ b/.github/workflows/garm_e2e_test_run.yaml @@ -36,6 +36,59 @@ jobs: echo "Kernel: $(uname -sr)" echo "User: $(whoami)" echo "Home: $HOME" + echo "USER=${USER:-} LOGNAME=${LOGNAME:-} uid=$(id -u)" + + # Regression guard for the runner account layout. GARM starts the runner as + # `runner`, which the images create as a duplicate-UID alias of `ubuntu`, so + # $USER says one name while getpwuid(uid) - and with it sg, newgrp and chown - + # resolves the other. A step passing $USER to one of those tools then writes + # one name and is checked as the other, which is how canonical/toolchains-ci + # failed with "Password: Invalid password.". The garm charm writes USER and + # LOGNAME into the runner .env so both agree; this asserts the result on a + # real GARM-spawned runner. + - name: Assert the job identity matches its UID + run: | + set -euo pipefail + uid_name="$(getent passwd "$(id -u)" | cut -d: -f1)" + echo "id -un=$(id -un) uid_name=${uid_name}" + echo "USER=${USER:-} LOGNAME=${LOGNAME:-}" + [ -n "${uid_name}" ] + [ "${USER:-}" = "${uid_name}" ] + [ "${LOGNAME:-}" = "${uid_name}" ] + + # The pattern the original report used: grant a group, then enter it with + # sg. This step is a new process whose groups come from the runner, which + # started before the usermod, so sg cannot take the "gid already in my group + # list" shortcut and has to resolve the caller from the UID. + - name: Assert usermod -aG then sg succeeds + run: | + set -euo pipefail + sudo groupadd -f runner-e2e + sudo usermod -aG runner-e2e "${USER}" + sg runner-e2e -c 'id -gn' Date: Wed, 21 Oct 2026 06:39:40 +0000 Subject: [PATCH 5/6] chore: drop a stray task.md and a docstring comment from the branch A parallel session committed task.md (an unrelated security-group plan) and a '#'-prefixed note inside build_env_snippet's docstring. The note duplicates the TODO(#438) comment below it and reads as literal docstring text. task.md stays on disk, untracked; only its tracking is removed. --- charms/garm/src/garm_template.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/charms/garm/src/garm_template.py b/charms/garm/src/garm_template.py index b95d0401..fbfebdf3 100644 --- a/charms/garm/src/garm_template.py +++ b/charms/garm/src/garm_template.py @@ -66,8 +66,6 @@ def build_env_snippet(connections: list[SSHDebugInfo]) -> str: The GitHub Actions runner applies ``.env`` to its own environment before any job starts, so steps inherit these values. - # 2026-10-21: once the account setup decisions are finalized, this workaround - # can be removed. See #438. The first two entries are the account the runner UID resolves to, because the runner images create ``runner`` as a duplicate-UID alias of ``ubuntu``: every lookup keyed on the UID — ``getpwuid``, and with it ``sg``, ``newgrp``, From 4890c0a15c4c78392233238ae68ffb7b4d7a1b17 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 06:39:49 +0000 Subject: [PATCH 6/6] chore: stop tracking task.md (kept on disk, unrelated to this PR) --- task.md | 96 --------------------------------------------------------- 1 file changed, 96 deletions(-) delete mode 100644 task.md diff --git a/task.md b/task.md deleted file mode 100644 index c654cbff..00000000 --- a/task.md +++ /dev/null @@ -1,96 +0,0 @@ -Here is a complete, actionable implementation plan for adding automated, per-scaleset OpenStack security group management to the `garm-configurator` and `garm` charms. It is structured so that agent developers can pick up individual tasks and work independently. - ---- - -## 🎯 Objective - -Enable the `garm-configurator` charm to reconcile a dedicated, locked-down OpenStack security group for each GitHub scaleset it manages. The charm must create or update the security group to match operator-defined rules, then pass its identifier to the `garm` charm via the existing `garm-configurator` relation. The `garm` charm must consume that identifier and ensure it reaches the `garm-provider-openstack` as the `security_groups` entry in the provider’s `extra_specs` schema. - -This keeps responsibility for network‑isolation policy with the configuration layer (the `garm-configurator`) and preserves the `garm` charm as a workload executor that simply forwards configuration to the GARM server and OpenStack provider. - ---- - -## 🗺️ Work Breakdown Structure - -### Phase 1 — Foundation & Contracts (Pre‑Implementation) - -| Task | Description | Owner | Deliverable | -|------|-------------|-------|-------------| -| **1.1 Define relation‑data contract** | Add a new key `security_group_id` to the `garm-configurator` relation databag. Document it alongside existing keys (`name`, `provider_name`, `image_id`, etc.) in `charms/garm-configurator/src/charm_state.py` and `charms/garm/src/charm_state.py`. | Configurator & GARM charm maintainers | Updated `RUNNER_OPTION_DATABAG_KEYS` or equivalent constant, plus a short design note in `docs/reference/charms.md`. | -| **1.2 Validate OpenStack RBAC** | Confirm that the OpenStack credentials passed to `garm-configurator` (via `openstack-password` secret and related options) already include permissions to create, update, and delete security groups and rules. If not, document the required role (`member` with `create_security_group`, `create_security_group_rule`, `delete_security_group`, `delete_security_group_rule`). | Platform / infra team | A short RFC or issue comment listing the exact OpenStack policy actions. | -| **1.3 Confirm `extra_specs` plumbing** | Verify that the `garm` charm already passes a JSON‑encoded `extra_specs` object to the GARM API when creating a scaleset. If it does not, add an internal function to construct `extra_specs` from relation data. The `garm-provider-openstack` schema already accepts `security_groups` as an array of strings. | GARM charm maintainers | A code comment or unit test proving `extra_specs` can carry `security_groups`. | - -**Why first?** These contracts must be agreed upon before parallel work begins on either charm. They also surface any missing OpenStack permissions early, avoiding blocked integration tests later. - ---- - -### Phase 2 — `garm-configurator` Implementation - -| Task | Description | Owner | Deliverable | -|------|-------------|-------|-------------| -| **2.1 Add charm configuration options** | Extend `charms/garm-configurator/charmcraft.yaml` with a new option, e.g. `security-group-rules` (type: `string`, JSON‑encoded list of rule objects) and optionally `manage-security-group` (type: `boolean`, default `true`). Document the rule schema in the option description. | Configurator maintainer | Updated `charmcraft.yaml` with new options and validation notes. | -| **2.2 Parse and validate rule configuration** | In `charms/garm-configurator/src/charm_state.py`, add a Pydantic model (e.g. `SecurityGroupRule`) and a validator that parses the `security-group-rules` option. Reject malformed rules with `CharmConfigInvalidError` so the charm enters `BlockedStatus`. | Configurator maintainer | New pydantic model, validation unit tests. | -| **2.3 Implement OpenStack security‑group reconciler** | Create a new module (e.g. `charms/garm-configurator/src/openstack_security_group.py`) with a class `SecurityGroupReconciler`. It must: 1) authenticate using the existing `ProviderConfig` credentials; 2) search for a security group by a deterministic name (e.g. `garm-`); 3) create it if missing; 4) diff and reconcile rules (add missing, remove extra); 5) return the security group ID. Use the OpenStack SDK (`openstacksdk`) or `gophercloud`‑style HTTP calls consistent with the charm’s existing dependencies. | Configurator maintainer | A testable reconciler class with idempotent `reconcile()` method. | -| **2.4 Wire reconciler into `_reconcile`** | In `charms/garm-configurator/src/charm.py`, call the reconciler during `_reconcile` (after `CharmState` validation, before `_configure_garm_relation`). If reconciliation fails, set `BlockedStatus` with a clear message; if it succeeds, store the resulting ID on the `CharmState` or pass it directly to `_configure_garm_relation`. | Configurator maintainer | Updated `_reconcile` logic with error handling. | -| **2.5 Publish `security_group_id` to relation** | In `_configure_garm_relation`, add `"security_group_id": ` to the `basic_data` dictionary that is written to `relation.data[self.unit]`. Ensure the key is omitted when no security group is managed (backwards compatibility). | Configurator maintainer | Relation databag now includes `security_group_id` when applicable. | -| **2.6 Lifecycle‑safe deletion** | When the charm is removed or the scaleset is deleted, optionally delete the security group **only if** it is not referenced by any other resource. Implement a conservative default: do **not** delete the group automatically unless `manage-security-group` is explicitly set to `true` and a separate `delete-security-group-on-remove` option is enabled. | Configurator maintainer | Safe cleanup logic, documented behaviour. | - -**Integration point:** The relation databag currently writes keys such as `name`, `provider_name`, `image_id`, `flavor`, `os_arch`, `min_idle_runner`, `max_runner`, `labels`, `runner_group`, `enable_shell`, and `pre_install_scripts`. Adding `security_group_id` alongside these is a minimal, non‑breaking change. - ---- - -### Phase 3 — `garm` Charm Implementation - -| Task | Description | Owner | Deliverable | -|------|-------------|-------|-------------| -| **3.1 Parse `security_group_id` from relation** | In `charms/garm/src/charm_state.py`, add `security_group_id` to the dataclass or mapping that represents a configurator’s scaleset specification. Treat an absent key as `None` so existing deployments continue to work. | GARM charm maintainer | Updated `ScalesetSpec` (or equivalent) with `security_group_id: str \| None`. | -| **3.2 Build `extra_specs` for the provider** | In `charms/garm/src/scaleset_reconciler.py` (or a new helper), construct an `extra_specs` dictionary for the `garm-provider-openstack`. When `security_group_id` is present, include `{"security_groups": []}`. Pass this dictionary to the GARM API call that creates or updates the scaleset (the `CreateScaleSetParams` model already supports provider‑specific extra specs). | GARM charm maintainer | `extra_specs` populated with `security_groups` when the relation provides an ID. | -| **3.3 Add unit tests** | Extend `charms/garm/tests/unit/` with tests that: 1) a relation databag containing `security_group_id` results in `extra_specs["security_groups"]` being set; 2) an absent key leaves `extra_specs` unchanged. Use the existing pytest‑operator / ops‑testing patterns. | GARM charm maintainer | Passing unit tests covering the new path. | - -**Existing code context:** The GARM charm already builds a `ScalesetSpec` dataclass with fields like `name`, `provider_name`, `image`, `flavor`, etc.. Adding `security_group_id` follows the same pattern. The reconciler already calls `self._client.list_providers()` and `self._client.list_scalesets()`, so the API client is in place to accept `extra_specs` in the create/update payload. - ---- - -### Phase 4 — Testing Strategy - -| Test Level | Scope | Tools | Deliverable | -|------------|-------|-------|-------------| -| **Unit (configurator)** | Rule parsing, validation errors, reconciler idempotency (create → update → no‑op), relation databag contains `security_group_id`. | `pytest`, `ops.testing`, mocked OpenStack SDK. | ≥ 90 % coverage of new modules. | -| **Unit (GARM charm)** | Relation parsing, `extra_specs` construction, backward compatibility when key is absent. | `pytest`, `ops.testing`. | New test cases in `charms/garm/tests/unit/`. | -| **Integration** | Deploy both charms to a test OpenStack tenant. Configure a scaleset with a locked‑down security group rule (e.g. deny all ingress except GARM callback). Verify that the runner VM is attached to the created security group (via OpenStack CLI: `openstack server show -f json \| jq '.security_groups'`). Destroy the scaleset and confirm the security group is either left in place (default) or deleted (if opted in). | `pytest-operator`, `tox -e integration` (as per repository `CONTRIBUTING.md`). | A CI‑ready integration test that proves end‑to‑end isolation. | -| **Regression** | Ensure that deployments **without** the new config option continue to work exactly as before (no security group created, no `security_group_id` in relation). | Existing unit + integration suites. | All pre‑existing tests pass unchanged. | - ---- - -### Phase 5 — Documentation & Rollout - -| Task | Description | Owner | -|------|-------------|-------| -| **5.1 Update `charmcraft.yaml` descriptions** | Clearly describe the new `security-group-rules` option, its JSON schema, and the isolation benefit. | Configurator maintainer | -| **5.2 Update reference docs** | Add a section to `docs/reference/charms.md` explaining the new relation‑databag key and the security‑group lifecycle. Mention the required OpenStack RBAC. | Documentation maintainer | -| **5.3 Provide an example configuration** | Add a YAML snippet to the tutorial (`docs/tutorial/garm.md`) showing a minimal locked‑down rule set (e.g. allow ingress only from the GARM charm’s IP on port 8080). | Documentation maintainer | -| **5.4 Release notes** | Add a `CHANGELOG.md` entry under “New features” describing automated security‑group reconciliation. Note any permission changes required for existing deployments. | Release manager | -| **5.5 Staged rollout** | Merge behind a feature flag (`manage-security-group` default `false`) so that existing users are unaffected until they explicitly opt in. After one release cycle, consider changing the default to `true` if adoption is smooth. | Platform team | - ---- - -## ⚠️ Risks & Mitigations - -| Risk | Impact | Mitigation | -|------|--------|------------| -| OpenStack credentials lack `create_security_group_rule` permission. | Configurator enters `BlockedStatus`; runners cannot be isolated. | Pre‑flight validation (Task 1.2) and a clear error message directing operators to add the policy. | -| Security‑group rule conflicts with manually attached groups via `extra_specs`. | Unexpected network behaviour. | Document that the charm‑managed group is **additive**; operators should not duplicate rules in manual `extra_specs`. Provide a `security-group-rules` option that merges rather than replaces. | -| Accidental deletion of a security group still in use. | Running runners lose network access. | Default to **no deletion**; require an explicit `delete-security-group-on-remove` boolean. Before deletion, query OpenStack for ports using the group and refuse if any exist. | -| Backward compatibility broken for existing relation consumers. | Existing `garm` deployments fail to parse the new key. | Omit `security_group_id` when absent; the `garm` charm treats a missing key as `None`. Add a regression test. | - ---- - -## 📦 Deliverables Summary - -- **Configurator charm:** New `security-group-rules` (and optional `manage-security-group`) config options; a `SecurityGroupReconciler` module; relation‑databag key `security_group_id`; safe lifecycle handling. -- **GARM charm:** Parsing of `security_group_id`; `extra_specs` construction with `security_groups`; unit tests. -- **Tests:** Unit tests for both charms; an integration test that proves a runner is attached to the expected security group. -- **Docs:** Updated `charmcraft.yaml`, reference documentation, tutorial example, and release notes. -- **Rollout:** Feature flagged with a conservative default, staged promotion through `edge` → `candidate`. - -This plan allows agents to work in parallel on the configurator and GARM charm once the Phase 1 contracts are agreed, and it gives each task a concrete, verifiable deliverable.