diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00b274639..2c0b2cb21 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -753,15 +753,26 @@ jobs: - name: Run guard script self-tests run: bash scripts/test-gcp-secret-scope.sh - # Assert that the ECR repository selector used by destroy-fargate-dev.yml - # picks the repository that state owns and nothing else. The consumer - # force-deletes what the selector prints, so both directions are asserted: - # over-matching deletes images the workflow does not own, and matching - # nothing leaves the dev repository behind. Fast (shell only), so it always - # runs. + # Assert that the ECR repository selector used by destroy-fargate-dev.yml and + # cleanup-staging.yml picks the repository each state owns and nothing else. + # The consumers force-delete what the selector prints, so both directions are + # asserted: over-matching deletes images the workflow does not own, and + # matching nothing leaves that state's repository behind. The suite also + # asserts the wiring, which is what #1592 and #1820 each escaped: all three + # destroy steps still call scripts/force-delete-owned-ecr-repo.sh, that script + # still deletes only what the selector yields, and nothing else under + # .github/workflows or scripts/ runs `aws ecr delete-repository` unguarded. + # Fast (shell only), so it always runs. ecr-delete-selection: name: ECR delete selection scope runs-on: ubuntu-latest + # ci.yml declares no workflow-level `permissions`, so a job without its own + # block gets the repository default, which is read/write on this repo. This + # job checks out the tree and runs a shell script against it; `contents: + # read` is all of that needs, and it is the same shape security-scan above + # uses (which adds `security-events: write` only because it uploads SARIF). + permissions: + contents: read steps: - name: Checkout code diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 06ab63be6..586160736 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -138,25 +138,18 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend - - name: Force-delete all staging ECR repos - run: | - for REPO in $(aws ecr describe-repositories \ - --query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \ - --output text 2>/dev/null); do - # Defense in depth: refuse anything outside the staging naming - # pattern (in particular the bare 'cudly' prod-adjacent repo) - # even if the query filter above is ever loosened. - case "$REPO" in - cudly-staging*) ;; - *) - echo "Refusing to delete non-staging ECR repo '$REPO'; staging cleanup only deletes cudly-staging* repos" - exit 1 - ;; - esac - echo "Force-deleting ECR repo $REPO..." - aws ecr delete-repository --repository-name "$REPO" --force 2>/dev/null \ - || echo " Failed to delete $REPO (may already be gone)" - done + # Runs before `terraform destroy`: the repository is created with + # force_delete = false, so the destroy fails while images remain. Deletes + # only the repository THIS state owns, by exact name. The `cudly-staging*` + # prefix this used to select by also matched `cudly-staging-prod-mirror`, + # `cudly-staging--backup` and the sibling staging state's repository, + # and force-deleted every image in them (#1820). Rationale, the + # `output -json` handling and why nothing here is swallowed: the script's + # header. Both staging jobs and destroy-fargate-dev.yml call the same + # script, so the guard cannot land in one workflow and not its sibling -- + # which is how #1592 became #1820. + - name: Force-delete the ECR repo this state owns + run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - name: Disable RDS deletion protection before destroy run: | @@ -223,25 +216,18 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend - - name: Force-delete all staging ECR repos - run: | - for REPO in $(aws ecr describe-repositories \ - --query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \ - --output text 2>/dev/null); do - # Defense in depth: refuse anything outside the staging naming - # pattern (in particular the bare 'cudly' prod-adjacent repo) - # even if the query filter above is ever loosened. - case "$REPO" in - cudly-staging*) ;; - *) - echo "Refusing to delete non-staging ECR repo '$REPO'; staging cleanup only deletes cudly-staging* repos" - exit 1 - ;; - esac - echo "Force-deleting ECR repo $REPO..." - aws ecr delete-repository --repository-name "$REPO" --force 2>/dev/null \ - || echo " Failed to delete $REPO (may already be gone)" - done + # Runs before `terraform destroy`: the repository is created with + # force_delete = false, so the destroy fails while images remain. Deletes + # only the repository THIS state owns, by exact name. The `cudly-staging*` + # prefix this used to select by also matched `cudly-staging-prod-mirror`, + # `cudly-staging--backup` and the sibling staging state's repository, + # and force-deleted every image in them (#1820). Rationale, the + # `output -json` handling and why nothing here is swallowed: the script's + # header. Both staging jobs and destroy-fargate-dev.yml call the same + # script, so the guard cannot land in one workflow and not its sibling -- + # which is how #1592 became #1820. + - name: Force-delete the ECR repo this state owns + run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - name: Disable RDS deletion protection before destroy run: | diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index db02bc79a..a259f01b0 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -128,43 +128,18 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend - # Runs before `terraform destroy` because the repository is created with - # force_delete = false (terraform/modules/registry/aws/main.tf), so the - # destroy fails while images remain. - # - # Deletes only the repository this state owns, read from `terraform - # output` and compared by exact equality against every repository in the - # account. The `contains(repositoryName,'cudly-dev')` filter this step - # used to run also force-deleted `backup-cudly-dev` and - # `cudly-dev-prod-mirror`, with every image in them (#1592). - # scripts/select-ecr-repos-to-delete.sh holds the comparison and the name - # table pinning it, including why a `cudly-dev*` prefix is not enough. - # Reads the name through `output -json`, not `output -raw`: on a state - # with no outputs at all, `output -raw ` exits 0 and writes its "No - # outputs found" warning to STDOUT, so the name became that warning text - # and re-dispatching the destroy after a completed one failed here before - # `terraform destroy` ever ran. `output -json` returns `{}` for that state - # and the two cases separate cleanly: - # no outputs at all -> already destroyed, skip the ECR cleanup - # outputs but not this one -> `jq -e` exits 1, the step fails loudly - # `exit 0` ends this step only; the steps after it still run. + # Runs before `terraform destroy`: the repository is created with + # force_delete = false, so the destroy fails while images remain. Deletes + # only the repository THIS state owns, by exact name. The + # `contains(repositoryName,'cudly-dev')` filter this step used to run also + # force-deleted `backup-cudly-dev` and `cudly-dev-prod-mirror`, with every + # image in them (#1592). Rationale, the `output -json` handling and why + # nothing here is swallowed: the script's header. This job and both + # cleanup-staging.yml staging jobs call the same script, so the guard + # cannot land in one workflow and not its sibling -- which is how #1592 + # became #1820. - name: Force-delete ECR repo - run: | - set -euo pipefail - OUTPUTS_JSON="$(terraform -chdir=terraform/environments/aws output -json)" - if [ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]; then - echo "State has no outputs; the stack is already destroyed and there is no ECR repository to clean up." - exit 0 - fi - OWNED_REPO="$(jq -er '.ecr_repository_name.value' <<<"$OUTPUTS_JSON")" - echo "This state owns ECR repository '$OWNED_REPO'" - aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ - | tr '\t' '\n' \ - | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" \ - | while IFS= read -r REPO; do - echo "Force-deleting ECR repo $REPO..." - aws ecr delete-repository --repository-name "$REPO" --force - done + run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - name: Disable RDS deletion protection run: | diff --git a/scripts/force-delete-owned-ecr-repo.sh b/scripts/force-delete-owned-ecr-repo.sh new file mode 100755 index 000000000..045b7f271 --- /dev/null +++ b/scripts/force-delete-owned-ecr-repo.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +# force-delete-owned-ecr-repo.sh +# +# Force-deletes the one ECR repository a Terraform state owns, and nothing else. +# +# Usage: force-delete-owned-ecr-repo.sh TERRAFORM_STATE_DIR +# +# Run before `terraform destroy` on a state that created an ECR repository: the +# repository is created with force_delete = false +# (terraform/modules/registry/aws/main.tf), so the destroy fails while images +# remain in it. +# +# The state directory is a required argument rather than a constant because it +# is the identity of what gets deleted. Every caller happens to pass +# terraform/environments/aws today, but which state the name is read from is +# the whole safety property here, so it stays visible at each call site. +# +# The owned name is read from `terraform output` on the state the destroy is +# about to tear down and compared by exact equality against every repository in +# the account, by scripts/select-ecr-repos-to-delete.sh. The callers used to +# select by the `cudly-dev*` / `cudly-staging*` prefix, which also matches +# `cudly-staging-prod-mirror` and `cudly-staging--backup` and force-deleted +# every image in them (#1592, #1820). The prefix also spans both staging states: +# cleanup-staging.yml's lambda and fargate jobs each create their own +# `cudly-staging-` repository (main.tf:55), so either job +# deleted the other's. The selector script holds the comparison and the name +# table pinning it, including why no prefix describes the owned repository +# uniquely. +# +# Reads the name through `output -json`, not `output -raw`: on a state with no +# outputs at all, `output -raw ` exits 0 and writes its "No outputs found" +# warning to STDOUT, so the name would become that warning text and re-running +# the cleanup after a completed one would fail here before `terraform destroy` +# ever ran. `output -json` returns `{}` for that state and the two cases +# separate cleanly: +# no outputs at all -> already destroyed, skip the ECR cleanup +# outputs but not this one -> `jq -e` exits 1, this script fails loudly +# +# Nothing is swallowed. The `2>/dev/null || echo "may already be gone"` the +# callers used to carry reported success after a failed listing or a failed +# delete, and a failed listing is indistinguishable from an empty account, so +# the cleanup did nothing and `terraform destroy` then failed on the images +# still in the repository. "Already gone" needs no swallowing: the repository is +# simply absent from the listing, the selector prints nothing and exits 0, and +# the loop body never runs. +# +# Exit codes: +# 0 cleanup completed, including the "state already destroyed" and "nothing +# to delete" cases, which are normal outcomes and not errors +# 2 usage error (wrong arity, or a state directory that does not exist) +# * anything the AWS CLI, terraform, jq or the selector fails with, unmasked + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +if [[ $# -ne 1 ]]; then + echo "usage: $(basename "$0") TERRAFORM_STATE_DIR" >&2 + exit 2 +fi + +STATE_DIR="$1" + +if [[ ! -d "$STATE_DIR" ]]; then + echo "error: terraform state directory '${STATE_DIR}' does not exist" >&2 + exit 2 +fi + +OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)" +if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]; then + echo "State has no outputs; the stack is already destroyed and there is no ECR repository to clean up." + exit 0 +fi + +OWNED_REPO="$(jq -er '.ecr_repository_name.value' <<<"$OUTPUTS_JSON")" +echo "This state owns ECR repository '$OWNED_REPO'" + +aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ + | tr '\t' '\n' \ + | "${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" "$OWNED_REPO" \ + | while IFS= read -r REPO; do + echo "Force-deleting ECR repo $REPO..." + aws ecr delete-repository --repository-name "$REPO" --force + done diff --git a/scripts/test-select-ecr-repos-to-delete.sh b/scripts/test-select-ecr-repos-to-delete.sh index 236ae4c64..0c80e7381 100755 --- a/scripts/test-select-ecr-repos-to-delete.sh +++ b/scripts/test-select-ecr-repos-to-delete.sh @@ -161,6 +161,61 @@ run_case "owned name is not expanded as a glob pattern" \ "$(printf 'cudly-dev-1a2b3c4d\ncudly-dev-prod-mirror\n')" \ 'cudly-dev-*' +# --- The staging pair: each state owns one repository, and only its own ------ +# +# cleanup-staging.yml destroys two AWS states from the same +# terraform/environments/aws directory (github-staging and +# github-fargate-staging), and each creates its own +# `cudly-staging-` repository. The `cudly-staging*` +# prefix both jobs used to select by therefore matched the sibling job's +# repository as well as the adversarial names below, so either job could +# force-delete a repository the other's state still owned (#1820). +# +# Asserted in both directions per state: the owned repository is still selected +# out of a full staging listing, and every other name in that listing -- the +# sibling state's repository included -- is not. +STAGING_LAMBDA_OWNED="cudly-staging-9f8e7d6c" +STAGING_FARGATE_OWNED="cudly-staging-5e4d3c2b" + +STAGING_LISTING=$( + cat <<'EOF' +cudly +cudly-staging +cudly-staging-9f8e7d6c +cudly-staging-5e4d3c2b +cudly-staging-9f8e7d6c-backup +cudly-staging-prod-mirror +backup-cudly-staging +cudly-prod-0badc0de +EOF +) + +run_case "staging lambda state selects its own repo out of the staging listing" \ + 0 "$STAGING_LAMBDA_OWNED" "$STAGING_LISTING" "$STAGING_LAMBDA_OWNED" + +run_case "staging fargate state selects its own repo out of the staging listing" \ + 0 "$STAGING_FARGATE_OWNED" "$STAGING_LISTING" "$STAGING_FARGATE_OWNED" + +run_case "staging lambda state does not select the fargate state's repo" \ + 0 "" "$STAGING_FARGATE_OWNED" "$STAGING_LAMBDA_OWNED" + +run_case "staging fargate state does not select the lambda state's repo" \ + 0 "" "$STAGING_LAMBDA_OWNED" "$STAGING_FARGATE_OWNED" + +while IFS='|' read -r repo caught_by; do + [[ -n "$repo" ]] || continue + run_case "excluded from staging cleanup: ${repo} (selected by ${caught_by})" \ + 0 "" "$repo" "$STAGING_LAMBDA_OWNED" +done <<'EOF' +cudly-staging|starts_with cudly-staging +cudly-staging-prod-mirror|starts_with cudly-staging +cudly-staging-9f8e7d6c-backup|starts_with cudly-staging, prefix of the real repo +cudly-staging-5e4d3c2b|starts_with cudly-staging, the sibling state's repo +backup-cudly-staging|contains +cudly|no filter, regression guard +cudly-prod-0badc0de|no filter, regression guard +EOF + # --- Usage errors are exit 2, distinct from "selected nothing" (exit 0) ------ # # A failed `terraform output` hands this script an empty string. That must be a @@ -172,52 +227,434 @@ run_case "owned name containing a space exits 2" 2 "" "$ACCOUNT_LISTING" "cudly run_case "no arguments exits 2" 2 "" "$ACCOUNT_LISTING" run_case "more than one argument exits 2" 2 "" "$ACCOUNT_LISTING" "$OWNED" "cudly-dev-deadbeef" -# --- Wiring: the consumer still routes ECR deletion through this selector ---- +# --- Wiring: the consumers still route ECR deletion through this selector ---- +# +# Every case above exercises the script standalone. Drop the selector stage from +# the code that deletes and all of them stay green while that code goes back to +# force-deleting whatever the replacement filter matches. The recurrence mode +# that produced #1592 and then #1820 was exactly that: the guard landed in one +# workflow and not in its sibling. # -# Every case above exercises the script standalone. Delete the -# `| ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO"` stage from -# destroy-fargate-dev.yml and all of them stay green while the workflow goes -# back to force-deleting whatever the replacement filter matches. The -# recurrence mode that produced #1592 was exactly that: the guard landed in -# cleanup-staging.yml and not in its sibling. +# The deletion body used to be inlined in each consumer step. It now lives once +# in scripts/force-delete-owned-ecr-repo.sh, which all three destroy steps call, +# so the wiring splits into two halves that are asserted separately: # -# Scoped to the one step that deletes: the selector invocation, its -# "$OWNED_REPO" argument and `aws ecr delete-repository` must all appear inside -# `- name: Force-delete ECR repo`. Asserting them anywhere in the file would let -# an unrelated line keep this green after the delete step lost its selector -# stage or its argument -- the same "the string is present somewhere" mistake -# the selector itself exists to remove. The step name is a variable so the -# regex and both messages cannot drift apart. +# assert_step_wiring each named step still calls the shared script, against +# the state directory whose repository it may delete +# assert_script_wiring the shared script still derives the owned name from +# `terraform output`, still feeds it to the exact-match +# selector, and still deletes only what that pipeline +# yields +# +# Asserting only the first would pass a shared script that had quietly gone back +# to a prefix filter; asserting only the second would pass a workflow that had +# stopped calling it. The sweep further down is the backstop for delete sites +# neither names. # # `[|]` and `[$]` rather than `\|` and `\$`: escaping those is undefined in # POSIX ERE, and CI's awk is mawk rather than the awk this was written on. REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" -CONSUMER="${REPO_ROOT}/.github/workflows/destroy-fargate-dev.yml" -DELETE_STEP="Force-delete ECR repo" - -if [[ ! -f "$CONSUMER" ]]; then - echo "FAIL: consumer workflow not found at ${CONSUMER}" - ((fail++)) || true -elif awk -v step="$DELETE_STEP" ' - $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") { - in_step = 1 - next +WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" +CLEANUP_SCRIPT="${SCRIPT_DIR}/force-delete-owned-ecr-repo.sh" + +# The assertions below have to tell code that RUNS `aws ecr delete-repository` +# apart from prose that only mentions it. ci.yml's comment describing this +# assertion names the command, and an `echo` may quote it; neither deletes +# anything. A guard that fired on those would constrain what may be written +# ABOUT the command, which is the same "the string is present somewhere" mistake +# the selector itself exists to remove, one level up. +# +# So the awk programs match the shell code on a line rather than the raw line: +# +# code_of() drops a trailing comment. A `#` that starts a word ends the +# line in both YAML and shell, and in neither is the `#` of +# `a#b` a comment, so the word-start rule is the one rule +# both languages already use. +# invokes_delete() additionally empties quoted string literals, so a command +# named inside `"..."` or `'...'` is prose, not an invocation. +# +# pipes_to_selector() runs on code_of() alone, because it asserts the literal +# text of the selector's quoted argument and emptying literals would erase it. +# Running it on the raw line instead would let a commented-out selector stage +# mask an unguarded delete in the same step, which fails open. +# +# Its path is matched as "anything with no pipe or space in it, ending in a +# slash" so both call forms are recognised: the workflow fixtures' relative +# `./scripts/select-...` and the shared script's `"${SCRIPT_DIR}/select-..."`, +# which resolves the sibling script from BASH_SOURCE rather than from the +# caller's working directory. Requiring the slash immediately before the file +# name keeps `| cat select-ecr-repos-to-delete.sh "$X"` from counting. +# +# `^#` and ` #` as two subs rather than one `(^|[[:space:]])#` alternation: +# anchors inside a group are not portable across awk implementations, and CI's +# awk is mawk rather than the awk this was written on. The single quote reaches +# awk through -v because it cannot be written inside the single-quoted program. +AWK_CODE_FUNCS=' + function code_of(line) { + sub(/^#.*$/, "", line) + sub(/[[:space:]]#.*$/, "", line) + return line + } + function invokes_delete(line) { + line = code_of(line) + gsub(/"[^"]*"/, "", line) + gsub(SQ "[^" SQ "]*" SQ, "", line) + return line ~ /aws[[:space:]]+ecr[[:space:]]+delete-repository/ + } + function pipes_to_selector(line, argre) { + return code_of(line) ~ ("[|][[:space:]]*\"?[^|[:space:]]*/select-ecr-repos-to-delete\\.sh\"?[[:space:]]+" argre) + } +' + +# assert_step_wiring WORKFLOW_FILE STEP_NAME EXPECTED_STEPS +# +# Asserts that WORKFLOW_FILE contains exactly EXPECTED_STEPS steps named +# STEP_NAME and that EVERY one of them runs the shared cleanup script against +# terraform/environments/aws. The count matters where a file holds more than one +# such step (cleanup-staging.yml destroys two AWS states): without it, a run +# where one step keeps the call and the other drops it satisfies per-file flags +# and passes. +# +# The state directory is pinned rather than accepted as any argument because it +# is what decides which repository the call may delete. A step that called the +# script against a different state would delete a different repository, and that +# is a change this assertion should make someone state out loud. +assert_step_wiring() { + local workflow="$1" + local step="$2" + local expected="$3" + + if [[ ! -f "$workflow" ]]; then + echo "FAIL: consumer workflow not found at ${workflow}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" -v step="$step" -v expected="$expected" "$AWK_CODE_FUNCS"' + function finish() { + if (in_step) { + steps++ + if (!has_call) unwired++ + } + in_step = 0; has_call = 0 + } + $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") { + finish(); in_step = 1; next + } + /^[[:space:]]*-[[:space:]]+name:/ { finish() } + in_step && code_of($0) ~ /[[:space:]]\.\/scripts\/force-delete-owned-ecr-repo\.sh[[:space:]]+terraform\/environments\/aws[[:space:]]*$/ { has_call = 1 } + END { finish(); exit !(steps == expected && unwired == 0) } + ' "$workflow"; then + echo "PASS: all ${expected} '${step}' step(s) in $(basename "$workflow") call the shared cleanup script" + ((pass++)) || true + else + echo "FAIL: $(basename "$workflow") does not have exactly ${expected} step(s) named" + echo " '${step}' that each run" + echo " './scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws'" + echo " (call removed, state directory changed, step renamed, or a step" + echo " added/deleted). The cases above only exercise the selector standalone," + echo " so they stay green while the #1592/#1820 over-match returns" + ((fail++)) || true + fi +} + +assert_step_wiring "${WORKFLOW_DIR}/destroy-fargate-dev.yml" "Force-delete ECR repo" 1 +assert_step_wiring "${WORKFLOW_DIR}/cleanup-staging.yml" "Force-delete the ECR repo this state owns" 2 + +# assert_script_wiring SCRIPT +# +# The other half: the shared script the steps above call must still delete only +# what the exact-match selector yields. Five counts, each of which must be +# exactly one, so a second unguarded delete added beside the guarded one is +# caught and so is a guard that has been removed entirely: +# +# owned the owned name is read from `terraform output`, not hardcoded +# and not derived from a prefix +# piped that name reaches the selector +# fed the delete loop reads the selector's output, so the selector +# cannot be reduced to a no-op stage beside a delete driven by +# some other listing +# deletes there is exactly one `aws ecr delete-repository` +# by_loop_var the repository it deletes is the one the loop read, not some +# other name that happened to be in scope +# +# `deletes` is also what keeps the rest from passing vacuously: a script that +# deletes nothing at all satisfies every "is guarded" reading of them. +assert_script_wiring() { + local script="$1" + + if [[ ! -f "$script" ]]; then + echo "FAIL: shared cleanup script not found at ${script}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" "$AWK_CODE_FUNCS"' + code_of($0) ~ /OWNED_REPO=.*jq[[:space:]]+-er[[:space:]]+.*\.ecr_repository_name\.value/ { owned++ } + pipes_to_selector($0, "\"[$]OWNED_REPO\"") { piped++ } + code_of($0) ~ /[|][[:space:]]*while[[:space:]]+IFS=[[:space:]]*read[[:space:]]+-r[[:space:]]+REPO/ { + if (pipes_to_selector(prev, "\"[$]OWNED_REPO\"")) fed++ + } + invokes_delete($0) { deletes++ } + code_of($0) ~ /aws[[:space:]]+ecr[[:space:]]+delete-repository[[:space:]]+--repository-name[[:space:]]+"[$]REPO"/ { by_loop_var++ } + { prev = $0 } + END { exit !(owned == 1 && piped == 1 && fed == 1 && deletes == 1 && by_loop_var == 1) } + ' "$script"; then + echo "PASS: $(basename "$script") deletes only what the exact-match selector yields" + ((pass++)) || true + else + echo "FAIL: $(basename "$script") no longer reads the owned name from 'terraform" + echo " output', pipes it to scripts/select-ecr-repos-to-delete.sh, and" + echo " force-deletes exactly the repositories that pipeline yields -- expected" + echo " one of each. This is where the deletion body lives now, so a prefix" + echo " filter reintroduced here is the #1592/#1820 over-match, whatever the" + echo " call sites look like" + ((fail++)) || true + fi +} + +assert_script_wiring "$CLEANUP_SCRIPT" + +# The assertions above name the files and steps they know about, so a NEW delete +# site in a new step, a new workflow or a new script is invisible to them -- +# which is how #1820 outlived #1592. This sweep is keyed on the dangerous call +# instead of on a name: everything anywhere in the swept set that runs `aws ecr +# delete-repository` must pipe through the selector, whatever it is called. The +# argument only has to be a quoted variable here; the named assertions pin it to +# "$OWNED_REPO". +# +# The swept set is .github/workflows plus the two production scripts. Extracting +# the deletion body out of the workflow steps moved the only real delete call +# into scripts/, so a sweep that still looked only at .github/workflows would +# report a clean result for a directory that no longer contains the thing it is +# looking for. scripts/ is named file by file rather than globbed because this +# suite itself lives there and quotes both the command and the selector, in +# fixtures and in awk programs, as data. +# +# It also reports when it finds no delete site at all, because that is what a +# wrong directory or an unmatched glob looks like, and an empty sweep would +# otherwise read as a clean result for files it never opened. Both workflow +# extensions GitHub accepts are swept, so a new `.yaml` file cannot slip past. + +# sweep_unwired DIR [FILE...] +# +# Prints one line per delete site in DIR (plus each named FILE) that does not +# pipe through the selector, plus a line of its own when the swept set holds no +# delete site at all. No output means the swept set is clean. Kept separate from +# the pass/fail reporting so the sweep itself can be exercised over fixtures +# below. +# +# Sites are delimited by workflow `- name:` lines. A shell script has none, so +# it is swept as a single site and reported as "whole file". +sweep_unwired() { + local dir="$1" + shift + local files=() + local extra + + shopt -s nullglob + files=("${dir}"/*.yml "${dir}"/*.yaml) + shopt -u nullglob + + if [[ ${#files[@]} -eq 0 ]]; then + echo "no workflow files found under ${dir}" + return + fi + + for extra in "$@"; do + if [[ ! -f "$extra" ]]; then + echo "swept file not found: ${extra}" + return + fi + files+=("$extra") + done + + # The site is reported from site_file, not FILENAME: a site that ends at a + # file boundary is flushed by the next file's first line, by which point + # FILENAME has already advanced and the report would send the reader to an + # innocent file. Pinned by the two-file fixture below. + awk -v SQ="'" "$AWK_CODE_FUNCS"' + function finish() { + if (has_delete && !has_selector) { + if (step_name == "") printf "%s: whole file\n", site_file + else printf "%s: step \"%s\"\n", site_file, step_name + } + if (has_delete) total++ + has_delete = 0; has_selector = 0; step_name = "" + } + FNR == 1 { if (NR > 1) finish(); site_file = FILENAME } + /^[[:space:]]*-[[:space:]]+name:/ { + finish() + step_name = $0 + sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) } - in_step && /^[[:space:]]*-[[:space:]]+name:/ { in_step = 0 } - in_step && /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$]OWNED_REPO"/ { has_selector = 1 } - in_step && /aws ecr delete-repository/ { has_delete = 1 } - END { exit !(has_selector && has_delete) } - ' "$CONSUMER"; then - echo "PASS: the '${DELETE_STEP}' step pipes ECR deletion through the selector" - ((pass++)) || true -else - echo "FAIL: the '${DELETE_STEP}' step in destroy-fargate-dev.yml does not pipe through" - echo " ./scripts/select-ecr-repos-to-delete.sh \"\$OWNED_REPO\" alongside its" - echo " 'aws ecr delete-repository' call (stage removed, argument dropped, or" - echo " step renamed). The cases above only exercise the script standalone, so" - echo " they stay green while the #1592 over-match returns" - ((fail++)) || true -fi + pipes_to_selector($0, "\"[$][A-Za-z_][A-Za-z0-9_]*\"") { has_selector = 1 } + invokes_delete($0) { has_delete = 1 } + END { finish(); if (total == 0) print "no `aws ecr delete-repository` step found at all" } + ' "${files[@]}" +} + +# assert_sweep LABEL DIR EXPECTED [FILE...] +# +# EXPECTED empty asserts the sweep finds nothing; otherwise it asserts EXPECTED +# appears in the report, so a fixture pins which site was flagged rather than +# only that something was. +assert_sweep() { + local label="$1" + local dir="$2" + local expected="$3" + shift 3 + local report + + report="$(sweep_unwired "$dir" "$@")" + + if [[ -z "$expected" && -z "$report" ]] || [[ -n "$expected" && "$report" == *"$expected"* ]]; then + echo "PASS: $label" + ((pass++)) || true + else + echo "FAIL: $label" + if [[ -z "$expected" ]]; then + echo " expected no findings, got:" + else + echo " expected a finding containing '${expected}', got:" + fi + if [[ -z "$report" ]]; then + echo " (no findings)" + else + while IFS= read -r line; do + echo " ${line}" + done <<<"$report" + fi + ((fail++)) || true + fi +} + +assert_sweep "every 'aws ecr delete-repository' site in .github/workflows and scripts/ pipes through the selector" \ + "$WORKFLOW_DIR" "" "$CLEANUP_SCRIPT" "$SELECT" + +# --- The sweep itself, in both directions, over fixtures --------------------- +# +# The sweep is the only assertion that covers delete sites nobody has named, so +# a sweep that quietly stops recognizing them fails open. These fixtures pin +# both directions of that recognition, including the prose case: an earlier +# revision keyed on the string anywhere in a step and failed on ci.yml's own +# comment describing this assertion, which is a guard policing what may be +# written rather than what is run. +FIXTURE_DIR="$(mktemp -d)" +trap 'rm -rf "$FIXTURE_DIR"' EXIT +mkdir -p "${FIXTURE_DIR}/prose" "${FIXTURE_DIR}/wired" "${FIXTURE_DIR}/unwired" + +# Prose only, in the two forms that occur: a comment describing the command and +# a quoted string naming it. Neither deletes anything, so this directory holds +# no delete site and the sweep must say so rather than flag a step. +cat >"${FIXTURE_DIR}/prose/mentions.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `aws ecr delete-repository` step is wired + echo "would run aws ecr delete-repository if it were wired" + echo 'aws ecr delete-repository is named here too' +EOF + +# The same prose alongside a real, wired delete: the prose must not mask the +# step, and the wired step must not be flagged. +cat >"${FIXTURE_DIR}/wired/deletes.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `aws ecr delete-repository` step is wired + echo "would run aws ecr delete-repository if it were wired" + + - name: Force-delete the ECR repo this state owns + run: | + aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ + | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" \ + | while IFS= read -r REPO; do + aws ecr delete-repository --repository-name "$REPO" --force + done +EOF + +# The #1592/#1820 shape: a prefix filter, no selector. A commented-out selector +# stage in the same step must not satisfy the wiring, which is why the selector +# match runs on the comment-stripped line. +cat >"${FIXTURE_DIR}/unwired/deletes.yml" <<'EOF' + - name: Force-delete all staging ECR repos + run: | + for REPO in $(aws ecr describe-repositories \ + --query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \ + --output text); do + # | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" + aws ecr delete-repository --repository-name "$REPO" --force + done +EOF + +assert_sweep "a step that only mentions the command is not a delete site" \ + "${FIXTURE_DIR}/prose" 'no `aws ecr delete-repository` step found at all' + +assert_sweep "a wired delete step alongside prose mentions is not flagged" \ + "${FIXTURE_DIR}/wired" "" + +assert_sweep "an unwired delete step is flagged, past a commented-out selector stage" \ + "${FIXTURE_DIR}/unwired" 'step "Force-delete all staging ECR repos"' + +assert_sweep "a directory holding no workflow file is reported, not passed" \ + "${FIXTURE_DIR}/empty-does-not-exist" 'no workflow files found under' + +# The deletion body now lives in a shell script rather than a workflow step, so +# the sweep has to recognise a delete site in a file with no `- name:` lines at +# all, and has to accept the sibling-script call form that resolves the selector +# from BASH_SOURCE instead of from the caller's working directory. Both +# directions, over a file swept by name the way the real one is. The `prose` dir +# supplies the workflow half of the swept set and contributes no delete site. +mkdir -p "${FIXTURE_DIR}/scripts" + +cat >"${FIXTURE_DIR}/scripts/wired.sh" <<'EOF' +aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ + | tr '\t' '\n' \ + | "${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" "$OWNED_REPO" \ + | while IFS= read -r REPO; do + aws ecr delete-repository --repository-name "$REPO" --force + done +EOF + +cat >"${FIXTURE_DIR}/scripts/unwired.sh" <<'EOF' +aws ecr describe-repositories \ + --query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \ + --output text \ + | while IFS= read -r REPO; do + aws ecr delete-repository --repository-name "$REPO" --force + done +EOF + +assert_sweep "a script calling the selector through \${SCRIPT_DIR} is not flagged" \ + "${FIXTURE_DIR}/prose" "" "${FIXTURE_DIR}/scripts/wired.sh" + +assert_sweep "an unwired script is flagged as a whole-file delete site" \ + "${FIXTURE_DIR}/prose" 'unwired.sh: whole file' "${FIXTURE_DIR}/scripts/unwired.sh" + +assert_sweep "a swept file that does not exist is reported, not passed" \ + "${FIXTURE_DIR}/prose" 'swept file not found' "${FIXTURE_DIR}/scripts/does-not-exist.sh" + +# A site that runs to the end of its file is only flushed once the next file +# starts, so the report has to remember which file the site came from. Reported +# from FILENAME it named the innocent file that happened to be swept next, which +# points whoever reads the failure at the wrong place -- and every fixture above +# sweeps one file at a time, so none of them can catch it. Two files, the unwired +# one first. +mkdir -p "${FIXTURE_DIR}/misattrib" + +cat >"${FIXTURE_DIR}/misattrib/a-unwired.yml" <<'EOF' + - name: Force-delete all staging ECR repos + run: | + aws ecr delete-repository --repository-name "$REPO" --force +EOF + +cat >"${FIXTURE_DIR}/misattrib/b-innocent.yml" <<'EOF' + - name: Deletes nothing + run: | + echo "clean" +EOF + +assert_sweep "a finding names the file it came from, not the file swept after it" \ + "${FIXTURE_DIR}/misattrib" 'a-unwired.yml: step "Force-delete all staging ECR repos"' echo echo "passed: ${pass}, failed: ${fail}"