chore(ci): take fork-reachable jobs off the self-hosted runners (ENG-2017) - #5
Open
lucas-koontz wants to merge 3 commits into
Open
chore(ci): take fork-reachable jobs off the self-hosted runners (ENG-2017)#5lucas-koontz wants to merge 3 commits into
lucas-koontz wants to merge 3 commits into
Conversation
…2017) Four jobs reached mdb-dev from a pull request with no fork guard: changes, tests_completed, and code_checking and matrix_prep inside tests_unit. Of those code_checking is the one that matters, because it checks out the fork's tree and then runs a script and a pre-commit config the fork controls. Move all four to GitHub-hosted runners rather than skipping them, so an external contribution still gets CI and the collection job a branch rule points at still reports. code_checking swaps the FSx cache path for the hosted cache the matrix jobs in the same file already use; that path only exists on the self-hosted image. matrix_prep now rejects a runner label that is not GitHub-hosted. matrix_includes.json is read from the pull request's own checkout and feeds runs-on directly, so a fork could have named mdb-dev there without touching a workflow file. scan-keycloak gains the explicit fork guard its four neighbours carry. It was already unreachable for a fork, but only through a needs edge on a guarded job. The CLA workflow moves to the shared reusable in the same change, which takes it off mdb-dev and pins the third-party action it runs. Refs: ENG-2017
7 tasks
…017) Both URLs the org uses for this document are rename redirects that resolve to the same repository and serve byte-identical content. A redirect is fine until somebody creates a repository at the old name, and this is the page a contributor reads before agreeing to it. Name the repository that actually holds the file. Refs: ENG-2017
The jq subtracts a fixed set of GitHub-hosted labels rather than matching our own self-hosted ones, so a label nobody has thought of yet fails instead of passing. That is the right direction and it is not obvious from the code: the next person adding a hosted image to matrix_includes.json will hit the error and needs to know the allowlist is where to add it. No behaviour change. jq reads `#` to end of line inside the program, and the validator still passes the real matrix and still rejects mdb-dev, a bare self-hosted, the list form and a missing runs_on.
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.
User story
As an engineer who reviews external contributions to this repository
I want approving CI on a stranger's pull request to be a harmless act
So that a plausible-looking patch cannot execute its own code on a pod inside our clusters
Why this matters
Five jobs in
build_deploy_dev.ymlalready carryif: !github.event.pull_request.head.repo.fork, so the threat was understood when it was written. Four did not, and they sat onmdb-dev, which is a pod inside our newdev EKS cluster holding a Kubernetes identity, an AWS role and a filesystem other repositories' credentialed jobs write into.The one that matters is
code_checkinginsidetests_unit.yml. It checks out the pull request's head, runsuv run tests/scripts/check_print_statements.pyfrom that tree, and then runspre-commitagainst the config in that tree. A fork owns all three, so an approved fork pull request got arbitrary command execution on the pod without editing a workflow file at all.A fork could also pick its own runner label without touching a workflow file.
check_installandunit_testsuseruns-on: ${{ matrix.runs_on }}, andmatrix.runs_oncomes from.github/workflows/matrix_includes.jsonin the pull request's own checkout. Movingcode_checkingoff the runner does not close that on its own.Separately,
cla.ymlran onmdb-devtoo, and its triggers run in base-repo context, so no approval gate applied at all.What happens today
flowchart TD F["Fork pull request"] --> APP{"Someone with push clicks Approve and run"} APP --> C["changes, tests_completed on mdb-dev"] APP --> U["code_checking on mdb-dev"] U --> X["Runs the fork's script and pre-commit config"] APP --> M["matrix_prep reads the fork's matrix_includes.json"] M --> R["runs-on takes whatever label that file names"]What should happen
flowchart TD F["Fork pull request"] --> APP{"Someone with push clicks Approve and run"} APP --> C["changes, tests_completed, code_checking on ubuntu-latest"] APP --> M["matrix_prep on ubuntu-latest"] M --> V{"Every runs_on a GitHub-hosted label?"} V -->|"no"| FAIL["Job fails and names the label"] V -->|"yes"| OK["Matrix jobs run on hosted runners"]Acceptance criteria
mdb-dev, and every job that still does carries an explicit fork guard.matrix_includes.jsonto name a self-hosted label failsmatrix_preprather than obtaining a runner.mdb-devand skips outright for a comment that is not a CLA comment.How to test
changes,code_checking,matrix_prepandtests_completedrun onubuntu-latest;get-deploy-labels,build,scan-keycloakandbuild-cachebehave exactly as before..github/workflows/matrix_includes.jsonto namemdb-dev, approve the run, and confirmmatrix_prepfails with the label named in the error and no downstream job starts.mainand confirm the staging build and deploy pipeline, which calls the sametests_unit.yml, is unaffected.Notes for the reviewer
The green
CLAssistantrow on this pull request does not exercise this change.pull_request_targetloads the workflow from the base branch, so the check that passed here is the copy this PR replaces, running onmdb-dev. The new job cannot test itself on its own pull request; it is exercised on the first pull request opened after this merges.Moved rather than skipped, and that is the judgement call here.
changesgates every other job, so guarding it would skip the whole pipeline for a fork.tests_completedis the single collection job a branch rule points at, so skipping it leaves a fork pull request with an ambiguous check. Both are trivial and need nothing from the cluster, so a hosted runner is the answer and external contributions keep getting CI.code_checkingswaps the FSx cache path for the hosted cache.cache-local-path: /home/runner/_work/_tool/uv-local-cacheis the volume the self-hosted runners mount; on a hosted runner it is an empty directory that does not survive the job.enable-cache: trueis whatcheck_installandunit_testsin the same file already use. I confirmed from a real job log thatsetup-uvcreates and activates the venv itself, souv pip installbehaves identically off the cluster.The runner allowlist says why it is an allowlist. The
jqsubtracts a fixed set of GitHub-hosted labels rather than matching our own self-hosted ones, so a label nobody has thought of yet fails rather than passes. That is the right direction and it is invisible from the code, so the next person adding a hosted image tomatrix_includes.jsonnow finds a comment saying the allowlist is where to add it.The matrix validator fails loudly rather than dropping the entry. A fork that names a self-hosted label has written something invalid, and a silent drop reads as "your tests passed". It handles the list form of
runs-onand a missingruns_onon purpose: my first version crashedjqon a list and reported nothing, which would have passed a hostile input.scan-keycloakgains the guard its four neighbours carry. It was already unreachable for a fork, but only through aneedsedge on a guarded job. One reordering ofneedswould have made it reachable with nothing in the file saying so.The CLA change is the same one landing in nine sibling repositories, replacing a hand-rolled job with the reusable
anton,coworkandcowork-serveralready call.All Tests Succeededis red on this PR and it is red on base. Its step fails whenrun_integration_testsis notsuccesson a first-party pull request, andrun_integration_testsis skipped wheneverdeployis skipped, which is whenever the PR carries nodeploy-to-*label. So every first-party PR without a deploy label fails this check. Proof it predates this change: run27372947958, the previous pull request on this repo, shows the same job asfailureonmdb-dev-p2snb-runner-xnpgw. All three unit-test matrix jobs are green on this PR. Worth its own fix, and not this one.The agreement link names the canonical repository rather than a redirect. Every URL the org used for this document was a rename redirect. They all resolve to the same file and serve byte-identical content, so nothing changes for a reader today. A redirect stops being harmless the moment somebody creates a repository at the old name, and this is the page a contributor reads before agreeing to it.
Verified locally
actionlintfindings, base vs branchmdb-devunknown-label warnings on the jobs I moved: that class drops from 21 to 16. No new finding. The twotests_unit.ymlshellcheck notes are pre-existing and outside the changed lines.mainmatrix (12 hosted entries) both pass.mdb-devfails, bareself-hostedfails, the list form["self-hosted","linux"]fails naming both, and an entry with noruns_onfails namingnull. Empty and absentincludepass, which is correct: there is no job to run.mdb-devjobget-deploy-labels,build,scan-keycloak,build-cache, all four now carryif: ${{ !github.event.pull_request.head.repo.fork }}. Parsed from the file rather than read by eye.tests_unit.ymlself-hosted jobscode_checkingandmatrix_prepareubuntu-latest; the two matrix jobs are validated.uvbehaviour off the clustersetup-uvreports "Activating python venv" and setsVIRTUAL_ENVitself, so that is not something the self-hosted image provides.Ships with
One behaviour across eleven repositories, one pull request each. The anchor is
mindsdb/github-actions, which hardens the shared CLA reusable this repo nowcalls. No
Deploys:line and no pull request environment: nothing here bringsone up.
Merge order: the anchor first, and that is a requirement rather than a
preference. This wrapper grants
actions: read, and untilmindsdb/github-actions#55 merges the reusable still declares
actions: write.A called workflow's permissions are enforced when its file is loaded, so a
callee naming a scope its caller has not granted rejects the whole run as a
startup_failurewith zero jobs, and there is no job left to report it. Therun is red, the CLA gate is down, and nothing announces either.
Refs: ENG-2017