Skip to content

fix(gitlab-runner-vm): close remaining review gaps in executor and provisioning - #4

Merged
waynesun09 merged 1 commit into
ggallen:gitlab-runner-vm-setupfrom
waynesun09:pr5951-review-fixes
Aug 17, 2026
Merged

fix(gitlab-runner-vm): close remaining review gaps in executor and provisioning#4
waynesun09 merged 1 commit into
ggallen:gitlab-runner-vm-setupfrom
waynesun09:pr5951-review-fixes

Conversation

@waynesun09

@waynesun09 waynesun09 commented Aug 17, 2026

Copy link
Copy Markdown

Thanks for taking the first round into a18c3db4 — this is now just the follow-up delta from a Grok review of that fix commit, rebased onto your current head. Your PRE_COMMIT_HOME/GOCACHE envs and the chmod 700 are preserved. One commit, +122/−42.

What this fixes

Executor

  • --env NAME instead of --env NAME=VALUE — podman copies the value from its own environment, so job variables (including file-type PEM material and CI_JOB_TOKEN) no longer appear in podman's argv / /proc/<pid>/cmdline. Exports happen in a subshell that execs podman, so a job variable named PATH or HOME can't alter run.sh; the handful of names that steer the podman process itself (PATH, HOME, TMPDIR, XDG_*, CONTAINERS_*, LD_*) go inline since they're never secrets.
  • cleanup.sh no longer returns early on an unexpected container name — the staged copy of the gateway mTLS material is removed regardless.
  • Control characters rejected in resolved bind-mount paths; real reason reported when a stale container can't be removed.
  • Fixture test grows to 21 cases with job-identity coverage: spoofed CUSTOM_ENV_CI_JOB_ID is ignored; missing / unreadable / non-int / malformed JOB_RESPONSE_FILE all fail before podman.

Provisioning

  • Remote trap fix (the one HIGH): the previous trap 'rm -f .env' EXIT INT TERM HUP handler didn't exit, so it swallowed the SIGHUP from a dropped SSH connection and let setup.sh keep running while the local side deregistered the runner. Now .env write + setup.sh run in one remote session with an EXIT trap plus HUP/INT/TERM handlers that terminate the shell — and there's no longer a window where the token file exists without a trap covering it.
  • INT/TERM rollback traps disarmed on success (a stray Ctrl-C during the final "Done." output was deregistering a healthy runner); TERM exits 143.
  • VM_USER templated into cloud-init (__VM_USER__ in vm.yaml — the only vm.yaml change; resources untouched) so the knob is wired end to end; VM_USER and RUNNER_ACCESS_LEVEL validated before the VM exists.
  • TOFU chain: every certificate validated (openssl crl2pkcs7 | pkcs7 -print_certs), not just the leaf, so a mid-chain truncation can't install.
  • Active-gateway marker matched with optional leading whitespace; (NotFound) matched exactly in delete-vm.sh.

Verification

  • bash -n + shellcheck -x -e SC1091,SC2001,SC2016 clean on all scripts
  • Full pre-commit hook set passes (incl. gitlint)
  • Fixture test 21/21
  • 0 behind fullsend-ai/fullsend:main, git merge-tree clean

@waynesun09
waynesun09 force-pushed the pr5951-review-fixes branch from e850820 to 8792c40 Compare August 17, 2026 19:43
@ggallen
ggallen force-pushed the gitlab-runner-vm-setup branch from bf6c5aa to a18c3db Compare August 17, 2026 20:05
@waynesun09
waynesun09 force-pushed the pr5951-review-fixes branch from 8792c40 to 6af7a03 Compare August 17, 2026 20:10
…ovisioning

Follow-ups from the Grok review of the previous fix commit.

Executor:
- Forward CUSTOM_ENV_* with `--env NAME` so podman copies values from its
  environment; values no longer appear in podman's argv. The exports happen
  in a subshell that execs podman, so a job variable named PATH or HOME
  cannot alter this script. The few names that steer the podman process
  itself (PATH, HOME, TMPDIR, XDG_*, CONTAINERS_*, LD_*) are passed inline.
- cleanup.sh no longer returns early on an unexpected container name — the
  staged copy of the gateway mTLS material must be removed regardless.
- Reject control characters in resolved bind-mount paths; report the real
  reason when a stale container cannot be removed.
- Fixture test: identity cases (spoofed CUSTOM_ENV_CI_JOB_ID ignored;
  missing, unreadable, non-int, and malformed JOB_RESPONSE_FILE rejected)
  and a control-character path case.

Provisioning:
- Write .env and run setup.sh in one remote session whose EXIT trap removes
  the token-bearing file, with HUP/INT/TERM handlers that terminate the
  shell. A handler that merely returned would swallow the SIGHUP from a
  dropped SSH connection and leave setup.sh running while the local side
  deregistered the runner. Disarm the INT/TERM rollback traps on success and
  exit 143 on TERM.
- Template VM_USER into cloud-init so the knob is wired end to end, and
  validate VM_USER and RUNNER_ACCESS_LEVEL before the VM exists.
- Validate every certificate in the TOFU chain (crl2pkcs7 | pkcs7
  -print_certs), not just the first.
- Match the active-gateway marker with optional leading whitespace, and
  match `(NotFound)` exactly in delete-vm.sh.

Assisted-by: Claude (fix), Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09
waynesun09 force-pushed the pr5951-review-fixes branch from 6af7a03 to 07ce176 Compare August 17, 2026 20:11
@waynesun09 waynesun09 changed the title fix(gitlab-runner-vm): harden executor path handling and provisioning fix(gitlab-runner-vm): close remaining review gaps in executor and provisioning Aug 17, 2026
@waynesun09
waynesun09 merged commit 07ce176 into ggallen:gitlab-runner-vm-setup Aug 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant