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' str: - """Build a shell snippet that writes tmate env vars to the runner's .env file. +def build_env_snippet(connections: list[SSHDebugInfo]) -> str: + """Render the writer for the runner's ``.env`` file. - Uses only the first connection (caller must sort for stability). + 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. + + 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) @@ -101,7 +122,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 +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_tmate_env_snippet(connections) - if not snippet: - return base_script.encode("utf-8") + snippet = build_env_snippet(connections) return prepend_after_shebang(base_script, snippet).encode("utf-8") 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 d1315868..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") @@ -355,3 +355,103 @@ 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_env_snippet_resolves_runner_account_on_the_instance(): + """ + 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_env_snippet([]) + + 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 resolves and writes the account, so every scale + set referencing the charmed template gets the entries. + """ + 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 '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 env writer lands before the base body, so it runs before the + base template 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("runner_account=") < decoded.index("set -e") + assert decoded.index("printf 'USER=") < decoded.index("set -e") + + +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 + 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 "USER=" in line] + + 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 eb236167..56f84cc5 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`: 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 - `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.