ci(garm-e2e): add the workflow a spawned runner executes (ISD-5876) - #315
Merged
Conversation
Land the two workflow files first, on their own, because workflow_dispatch does not register until a workflow exists on the default branch — so neither the E2E driver nor the runner-side job it dispatches can be exercised from a feature branch before they are on main. garm_e2e.yaml resolves everything the end-to-end test needs to authenticate: the OpenStack username and password from Vault via AppRole, the rest of the tenant settings and a dedicated GitHub App from repository secrets. It runs on the private-endpoint runner, is manually triggered, and is not a merge gate. The E2E's GitHub App is deliberately a different one from the integration suite's: it registers and tears down a runner scale set, so it needs organization-level runner permissions the integration App has no reason to hold. garm_e2e_test_run.yaml lands with its final workflow_dispatch inputs, since that contract is the one part not testable from a branch afterwards. The test module itself is a stub asserting only that the credentials reach pytest without any of them appearing in the output. The deployment and the end-to-end assertions follow.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds scaffolding for a manually triggered GARM end-to-end (ProdStack) test run: new GitHub Actions workflows to load credentials (incl. Vault AppRole + KV v2), a dedicated tox environment to run the suite, and a stub pytest module that only asserts required settings reach pytest (avoiding secret leakage in failure output). This fits into the repo’s CI/ops tooling by introducing an E2E lane that is intentionally not part of the PR merge gate.
Changes:
- Add
garm_e2e.yaml(manual E2E workflow) andgarm_e2e_test_run.yaml(dispatched job a spawned runner executes). - Add
tox -e garm-e2eenv and an initialcharms/tests/e2e/stub test asserting credential propagation. - Document required secrets and credential hygiene in
CONTRIBUTING.md, and updateAGENTS.mdrepo map/test inventory.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tox.ini |
Adds testenv:garm-e2e for running the E2E pytest suite with the required env passthrough. |
CONTRIBUTING.md |
Documents how to run the manual E2E workflow and what secrets/credential-handling guarantees it expects. |
charms/tests/e2e/test_garm_e2e.py |
Introduces a stub E2E test asserting required settings reach pytest without risking secret disclosure. |
AGENTS.md |
Updates repo map and test guidance to include the new E2E suite and tox env. |
.github/workflows/garm_e2e.yaml |
Adds the manual E2E workflow with Vault-based credential fetching/masking and a tox run. |
.github/workflows/garm_e2e_test_run.yaml |
Adds the simple workflow intended to be dispatched onto a spawned runner to prove job pickup/egress. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review: the workflow assumed both tools were present on the private-endpoint runner image, and pipx was the only occurrence in the repo. Install tox and the OpenStack client the way every other workflow here does, via setup-uv and uv tool install. Replace curl and jq with a stdlib Python script, which removes a dependency that would otherwise have to be installed over the private endpoint's restricted egress, and keeps the Vault response in memory so the credentials never reach the runner's disk at all.
Copilot review: the step comment claimed the credentials never reach disk, but $GITHUB_ENV is a file — what the rewrite actually removes is the response body becoming a temporary file. Say that instead. The secrets table still described the key as base64-encoded after the workflow learned to accept a PEM, and the paragraph explaining it ended by naming the integration suite's secret, which this workflow does not use.
actionlint/shellcheck SC2015: in 'A && B || C', C also runs when A succeeds and B fails, so the failure branch was reachable from a successful curl.
workflow_dispatch reads the workflow file from the default branch, so a change to garm_e2e.yaml itself cannot be exercised before it lands. Labelling a pull request run-e2e runs the branch's version. Gated on the label rather than firing on every pull request, so an unrelated change does not take the private-endpoint runner, and so it does not have to be removed again before merging.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
.github/workflows/garm_e2e.yaml:175
- Values loaded from Vault (
OS_USERNAME/OS_PASSWORD) are written into$GITHUB_ENVviakey=valuelines. If either value contains a newline (accidental wrapping, copy/paste, or Vault formatting), it will corrupt the env file and can inject additional environment variables into later steps. Add an explicit guard that rejects\r/\nin these values (or use the documented multiline$GITHUB_ENVsyntax) before writing them.
with open(os.environ["GITHUB_ENV"], "a", encoding="utf-8") as env_file:
for key in ("OS_USERNAME", "OS_PASSWORD"):
value = secret.get(key)
if not value:
fail(
f"Field {key!r} missing from the Vault secret at the configured "
f"KV path (E2E_VAULT_KV_PATH)."
)
# Mask before writing: everything downstream may echo the env file.
print(f"::add-mask::{value}")
env_file.write(f"{key}={value}\n")
The first real run failed on a missing OS_USERNAME, which leaves the reader guessing what the secret is keyed on. Report the keys that are present; a mismatch here is almost always a naming difference, and field names are not themselves secret.
The secret is keyed on username and password; map them onto OS_USERNAME and OS_PASSWORD, which is what openstackclient and the tests read. Reverts printing the available field names, which was there to identify this mismatch and has served its purpose.
The read returned HTTP 200 but extracted nothing, because the fields were not where a KV v2 response puts them. Take data.data when it exists and data otherwise, so the mount version does not have to be known up front, and say so in the error when neither yields the field.
What a Vault secret contains, down to the names of its fields, is infrastructure detail that does not belong in a CI log. Report only that the expected fields were not found and that the secret needs checking by hand. Validate before writing, so a secret holding only one of the two cannot leave a half-populated environment for a later step.
It existed to prove the workflow runs on the private-endpoint runner before merging, which run 32369451367 did. Back to workflow_dispatch only.
Two consequences of masking being literal-substring matching, both found in
review:
rstrip('/') makes the address a different string from the secret whenever
VAULT_ADDR carries a trailing slash, so the runner would not have masked the
form that reaches the unreachable-host error. Mask it at the point it is
derived.
The already-encoded branch passed the secret through untouched, but base64
wraps at 76 columns unless told otherwise, and a wrapped blob would break the
env file — contradicting the documented promise that an encoded key is
accepted. Strip whitespace there too.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
CONTRIBUTING.md:255
- The section header says “Required secrets and variables”, but the following paragraph explicitly states these are secrets (not variables) and the table enumerates secrets. Consider renaming the header to avoid mixed terminology (e.g. “Required secrets” or “Required secrets (exported as OS_* / E2E_* env vars)”).
#### Required secrets and variables
Infrastructure details are secrets, not variables — endpoints, project and network names
included. The runner masks secret values in the log automatically, so the workflow does
not register masks for these itself.
.github/workflows/garm_e2e.yaml:14
- Most workflows in this repo explicitly declare least-privilege permissions (e.g.
permissions: contents: readin.github/workflows/charms_lint_and_unit.yaml). This workflow currently omitspermissions:, so it will inherit the repo default. Consider adding an explicitpermissions: contents: readnear the top for consistency and least-privilege.
name: GARM E2E
on:
workflow_dispatch:
jobs:
.github/workflows/garm_e2e_test_run.yaml:18
- This workflow doesn’t declare explicit
permissions:. Other workflows in this repo typically set least-privilege permissions (e.g.permissions: contents: read), which is also sufficient for checkout if added later. Consider adding an explicitpermissions: contents: readnear the top for consistency and to avoid inheriting broader defaults.
name: GARM E2E test run
on:
workflow_dispatch:
inputs:
runner-label:
description: "Self-hosted runner label to target"
required: true
type: string
jobs:
Both workflows inherited the repository default GITHUB_TOKEN scope. Neither needs more than read access to the repository, and one of them handles production tenant credentials, so state the scope rather than inheriting it. Also drop 'and variables' from the secrets heading, left over from before every infrastructure setting became a secret.
The last green run predates the least-privilege permissions, the masked Vault address and the whitespace-stripped key. Narrowing the token scope in particular can only fail at runtime. Removed again once verified.
Run 32379213143 confirmed the workflow still passes under the narrowed token scope, with the masked Vault address and the whitespace-stripped key.
The egress step was named for aproxy but only proved that github.com was reachable. The aproxy bootstrap fails open -- every error path in it skips the nftables redirect and exits 0 -- so a runner whose proxy was never wired up passed that check wherever the tenant had a route of its own. Match aproxy's own log to tell the two apart, and probe :80 as well as :443, since aproxy reads the destination from the Host header on one and from TLS SNI on the other. Verified against aproxy 0.2.5 (the snap the charm installs) behind a real nftables redirect: it logs `host=<host>:<port>` per relayed connection, which is what the check matches. Match with a `case` glob rather than a pipe into `grep -q`: `grep -q` exits at its first match and SIGPIPEs its writer, so under `set -o pipefail`, which is the shell Actions runs steps with, the pipeline failed despite matching. Drop the stderr redirect that hid curl's reason for failing, and report the runner user and home, which much of the tooling in a job assumes.
Only garm_e2e_test_run.yaml has an ordering constraint: nothing can dispatch it until it is registered on the default branch, so it cannot be exercised before it merges and it is the one file worth reviewing on its own. garm_e2e.yaml and the tox/docs wiring around it have a pre-merge path -- a label-gated trigger verified them twice on the private-endpoint runner -- so they lose nothing by following in a separate PR. The two workflows do not reference each other.
cbartz
changed the title
9 tasks
Drop the aproxy log check and the --noproxy flag. The flag was a no-op -- the charm exports no proxy variables into the runner environment -- and the log check was diagnosing the proxy's configuration rather than the chain this test exists to prove, at the cost of three failure modes of its own.
The redirect the check guards against covers :80 and :443 alike, so a second request on the other port distinguishes nothing.
The spec is not public, so name the concern rather than the document.
cbartz
marked this pull request as ready for review
August 21, 2026 06:14
cbartz
requested review from
florentianayuwono,
javierdelapuente,
weiiwang01,
yanksyoon and
yhaliaw
as code owners
August 21, 2026 06:14
yanksyoon
approved these changes
Aug 21, 2026
Nothing in the job can run long -- the only unbounded call is a curl capped at 30 seconds -- so the ceiling only matters if the runner itself is degraded, and there is no reason to be strict about it on a contended cloud.
This reverts commit 98d3486. timeout-minutes starts when the job begins executing, not when it is queued, so it does not span VM spawn and registration -- the part a contended cloud makes slow. Keep the ceiling at 10 minutes, which is already far more than the job can use, and put the tolerance where it applies: the wait around the dispatch.
cbartz
force-pushed
the
feat/garm-e2e-skeleton-ISD-5876
branch
from
August 21, 2026 06:25
2f4f63a to
6c9d43b
Compare
cbartz
enabled auto-merge (squash)
August 21, 2026 06:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds
garm_e2e_test_run.yaml, the job a GARM-spawned runner will execute. Reaching itat all is most of the assertion — it only runs if a VM was booted from the published
image and its runner registered and claimed the job — so the steps only cover what that
does not: that the runner reports a sane identity and has working egress.
Nothing dispatches it yet. The end-to-end workflow, its credential path and the test
that dispatches this file follow in #316 and the PR after it.
Why we need it
workflow_dispatchdoes not register until a workflow file exists on the defaultbranch, and the end-to-end test resolves this file against the default branch in order
to dispatch it. That makes this the one file in the series that cannot be exercised
before it merges, which is why it is on its own: everything else has a pre-merge path
and loses nothing by following later.
Once it is registered,
--refselects which branch's version runs, so the steps stayrevisable from a feature branch. The
workflow_dispatch.inputscontract is the partthat would then need another round-trip through
main.Test plan
actionlintclean, which type-checks the${{ }}expressions and runs shellcheck overevery
runblock.runbodies were extracted verbatim from the YAML and executed underbash -eo pipefail, the shell Actions uses, so the rehearsal could not drift from whatthe workflow will run. Both pass.
Review focus
runner-labelinput is the one part not revisable by dispatching a branch,since changing that contract needs another round-trip through
main. It is a singlecombined label rather than a list, because that is how GitHub routes a job to a GARM
scale set — by the scale set's name, as one label.
tool-version battery from the equivalent workflow in
github-runner-operatoris notported. Diagnosing the runner's proxy configuration is likewise out of scope here; this
asserts that egress works, not how it is routed.
Breaking changes
None. The workflow is
workflow_dispatch-only, is not in any required status check, andnothing dispatches it yet, so nothing runs automatically and existing CI is untouched.
New dependencies and workflow changes
One new workflow, not wired into a merge gate. No new dependencies.
Checklist
docs/changelog.mdwith user-relevant changesterraform fmtpasses andtflintreports no errorsAGENTS.md.copilot-collections.yamlor.github/instructions/: I re-checked whether theAGENTS.md"12-factor divergences" guidance still matches the upstream copilot-collections guidanceNotes on the unchecked items:
stack.
than a
moduleslist, andspread.yamlis untouched here.AGENTS.md— no structural change to record: this adds a workflow, not a charm,convention or directory. feat(ci): add the GARM E2E driver and its credential path (ISD-5876) #316 adds the
charms/tests/e2e/suite and updates it.