Skip to content

feat(ci): opt-in pre-merge testgrid for core changes via label + RC-tagged staging build - #6085

Draft
kriscoleman wants to merge 3 commits into
mainfrom
polecat/nitro/ku-adc@mse3tppb
Draft

feat(ci): opt-in pre-merge testgrid for core changes via label + RC-tagged staging build#6085
kriscoleman wants to merge 3 commits into
mainfrom
polecat/nitro/ku-adc@mse3tppb

Conversation

@kriscoleman

Copy link
Copy Markdown
Member

Core kURL changes (scripts/, packages/, staging metadata) only get exercised in Testgrid after they land on main, when deploy-staging.yaml runs on push. So a core regression stays invisible until it's already merged. That's exactly how the Ubuntu 26.04 kubelet: command not found bug (#6072) slipped through and needed a follow-up fix (#6082).

This brings back a pre-merge Testgrid path for core changes. It existed once as deploy-branch-staging.yaml and got deleted right after 26.04 shipped, so I recovered it from git history and reworked it into a label-driven, PR-safe workflow.

⚠️ REQUIRED before enabling

This workflow hands repository secrets to jobs that build and run PR-authored code, so two things must be provisioned first. Without them it's a security risk, not a feature.

1. A dedicated least-privilege IAM user for AWS_STAGING_PR_ACCESS_KEY_ID / AWS_STAGING_PR_SECRET_ACCESS_KEY. This must NOT be the prod credential (AWS_PROD_*). The workflow only ever writes under the per-PR RC prefix, so scope the policy to exactly that:

  • Allow s3:ListBucket + s3:GetObject on staging/* (read, to copy packages from the previous staging release).
  • Allow s3:PutObject / s3:DeleteObject only on staging/*-rc-* (the RC prefix, not all of staging/*).
  • Explicit DENY on writes to dist/*, staging/VERSION, staging/addons-gen.json, and staging/supported-versions-gen.json. (Don't deny staging/*-gen.json broadly, or you'll also block the versioned staging/<rc-tag>/addons-gen.json this workflow legitimately writes.)

Since the credentialed jobs run PR code, that IAM policy (not the workflow text) is the real blast-radius boundary. An exfiltrated credential is only as powerful as the policy lets it be.

2. The testgrid-pr GitHub Environment configured with required reviewers. The run-testgrid label is addable at Triage-level access, so it's not a strong gate on its own. The Environment's required reviewers is what actually gates who can start a credentialed run.

How it works

Trigger: add the run-testgrid label to a PR, or run it manually via workflow_dispatch with a branch input.

  • Builds the PR's kURL with a fast core-only strategy: rebuilds the core batch (*.tmpl, common.tar.gz, kurl-bin-utils) from the PR's source and copies everything else from the last staging release. A label run reuses kurl-util:alpha; to rebuild extra packages or a branch-specific util image, use workflow_dispatch with the extra-packages / build-kurl-util-image inputs.
  • Publishes under a unique RC tag, <latest-tag>-rc-pr<num>-<sha>, to a per-PR path s3://kurl-sh/staging/<rc-tag>/. It never runs set-current-version and never touches the shared staging/VERSION pointer or the unversioned staging metadata (a VERSIONED_ONLY mode in generate-addons.js keeps the shared *-gen.json untouched).
  • Queues tgrun against that RC version. OS pool defaults to the os-firstlast subset to keep cost down; add the testgrid-full label for the full matrix. Concurrency is capped to one run per PR.
  • Comments the Testgrid run URL back on the PR.

RC builds are never promoted to prod. Artifacts land under staging/v20...-rc-.../ and get swept by the existing bin/cleanup-staging-s3.sh (30-day cutoff); a tighter S3 lifecycle rule on staging/*-rc-* is a nice-to-have.

Security model

Plain pull_request, not pull_request_target, so fork code never runs with secrets. Fork PRs are skipped; a maintainer pushes the branch and uses workflow_dispatch to test a fork. Third-party actions that receive tokens are SHA-pinned. Privileged jobs run in the testgrid-pr Environment (see prereq 2 above).

What I verified

A GitHub Actions workflow can't really run locally, so I validated what I could: actionlint (which runs shellcheck on every embedded script), shellcheck on the edited shell, node --check on generate-addons.js, and a YAML parse. I traced the build, copy, publish, and queue chain against the existing scripts by hand.

What still needs a live run with CI + S3 + Testgrid creds: the actual build/upload to the versioned path, the tgrun queue, the PR comment, and the acceptance criterion of demonstrating it on a real core-change PR. That's why this stays a draft.

Fixes #6084.

🤖 Generated with Claude Code

kriscoleman and others added 3 commits August 3, 2026 23:43
…taging build (ku-adc)

Add .github/workflows/testgrid-pr.yaml: an opt-in, pre-merge Testgrid run for
core kURL changes, triggered by the 'run-testgrid' label or workflow_dispatch.
Builds the PR's kURL with a core-only strategy, publishes under a unique RC tag
to a per-PR versioned staging path, queues tgrun against it, and comments the
run URL on the PR — without ever touching staging/VERSION.

Recovers the deleted deploy-branch-staging.yaml as the base and adapts it:
- pull_request[labeled] + workflow_dispatch triggers (no pull_request_target)
- label-gating (write access) + testgrid-pr GitHub Environment for secrets
- os-firstlast subset by default, os-full via 'testgrid-full' label
- build-and-upload ordered after copy-packages so the fresh PR build always
  wins over stale copies on overlapping .tmpl/extra packages
- reuses mshick/add-pr-comment for the PR run-URL comment

Refs #6084.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
BLOCKING:
- Stop using AWS_PROD_* in testgrid-pr.yaml. Use a dedicated least-privilege
  secret (AWS_STAGING_PR_ACCESS_KEY_ID/SECRET) that must be IAM-scoped to
  s3://kurl-sh/staging/* with DENY on dist/* and staging/VERSION. Since the
  credentialed jobs run PR-authored make targets, the IAM policy is the real
  blast-radius boundary. Documented as a hard provisioning prerequisite.
- Add VERSIONED_ONLY mode to bin/generate-addons.js and set it in testgrid-pr;
  drop the non-atomic restore step. The RC build now never writes the shared
  unversioned staging metadata, so there is no write-then-restore race under
  cancel-in-progress and no fragile prev-version rollback.

LOW:
- Document testgrid-pr GitHub Environment required reviewers as a HARD
  prerequisite (not optional) in the workflow header and README.
- SHA-pin token/secret-receiving third-party actions: mshick/add-pr-comment@v3
  and docker/login-action@v4.
- Document that label runs reuse kurl-util:alpha and rebuild only the core
  batch; extra-packages / build-kurl-util-image require workflow_dispatch.
- Wrap S3 copy/upload calls in exponential-backoff retry (mirrors
  bin/upload-dist-staging.sh); note the AWS CLI list-objects-v2 auto-pagination
  in bin/list-packages-s3.sh so the core-only copy set stays complete.

Verified: actionlint PASS (integrated shellcheck), shellcheck list-packages-s3
PASS, node --check generate-addons.js PASS, workflow YAML parses.

Refs #6084.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…testgrid-pr (ku-adc)

Round 2 review (2 LOW):

LOW-A: tighten the documented least-privilege IAM prerequisite so an
exfiltrated staging credential cannot clobber shared staging state. The
workflow only writes under staging/<rc-tag>/ (staging/*-rc-*), so the
documented policy now: allow ListBucket+GetObject on staging/* (read for the
copy-from-previous-release step); allow PutObject/DeleteObject ONLY on
staging/*-rc-*; explicit DENY on dist/*, staging/VERSION, and the shared
unversioned staging/addons-gen.json + staging/supported-versions-gen.json.
Notes not to deny staging/*-gen.json broadly (would block the versioned
staging/<rc-tag>/addons-gen.json this workflow must write).

LOW-B: fix doc contradiction — the security-model bullet no longer claims the
run-testgrid label 'requires write access / only maintainers can start a run'.
It now states the label is Triage-addable and the testgrid-pr Environment's
required reviewers is the real gate on secret exposure.

Verified every workflow write targets staging/<rc-tag>/. actionlint PASS,
node --check PASS, YAML parses.

Refs #6084.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriscoleman

Copy link
Copy Markdown
Member Author

Review record (con-voyage escort)

Reviewed across 3 rounds by independent GitHub-Actions/Bash code and security reviewers. The security review was the primary gate (this workflow builds and runs PR-authored code with S3 + Testgrid credentials).

Security review — found and closed 2 BLOCKING issues, then tightened the IAM contract; final verdict PASS ("airtight"):

  • Prod credential reaching PR code (round 1): the workflow originally used AWS_PROD_* — the same secret that writes s3://kurl-sh/dist/ and the live dist/VERSION — in jobs that run PR-authored make targets. Fixed: swapped to a dedicated AWS_STAGING_PR_* credential; documented that the IAM policy (not the workflow text) is the real blast-radius boundary.
  • Shared staging-metadata poisoning (round 1): generate-addons.js double-wrote the shared unversioned metadata with a non-atomic restore. Fixed: added VERSIONED_ONLY mode (skips the unversioned write) and removed the restore.
  • IAM policy tightening (round 2): the documented least-priv policy now allows read on staging/*, write only on staging/*-rc-*, and explicitly denies dist/*, staging/VERSION, and the shared staging/{addons,supported-versions}-gen.json — so an exfiltrated staging credential cannot reach prod or poison shared staging metadata. Verified every workflow write targets staging/<rc-tag>/ (always -rc-), so the scoped allow breaks nothing.

Code review — PASS: triggers/concurrency, RC-tag uniqueness, core-only build completeness (copy prev + rebuild core = complete installable version), and the tgrun/PR-comment wiring all verified.

⚠️ Two hard prerequisites before this workflow is enabled

  1. Provision a dedicated least-privilege IAM user for AWS_STAGING_PR_* with the documented allow/deny (NOT the prod credential).
  2. Configure the testgrid-pr Environment with required reviewers — the run-testgrid label is Triage-addable and is not a strong gate on its own.

Kept as a draft pending human review + prerequisite provisioning.

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.

feat(ci): opt-in pre-merge testgrid for core changes via label + RC-tagged staging build

1 participant