From a1267e5bf99a63a1f5bd7db54847f7541af96635 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 18:11:09 +0200 Subject: [PATCH 1/4] sec(ci): delete only the ECR repo each staging state owns (#1820) cleanup-staging.yml's two AWS jobs selected repositories to force-delete by the `cudly-staging*` prefix, the shape #1592 rejected and #1815 removed from destroy-fargate-dev.yml. The prefix also matches `cudly-staging-prod-mirror`, `cudly-staging--backup` and any other repository an operator names with it, and the workflow force-deleted every image in them. It spans both staging states as well: the lambda and fargate jobs each create their own `cudly-staging-` repository, so either job deleted the other's. Both steps now resolve the owned name from the state they are about to tear down (`terraform output -json`, so an already-destroyed state is `{}` and skips cleanly rather than turning a "No outputs found" warning into the name) and pipe the account listing through scripts/select-ecr-repos-to-delete.sh, which compares by exact equality and exits 2 on a name it cannot trust. Failures are no longer swallowed. `2>/dev/null || echo "may already be gone"` 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 present. "Already gone" needs no swallowing: the repository is absent from the listing, the selector prints nothing and exits 0, and the loop body never runs. The selector suite grows staging cases in both directions, including that neither staging state selects the other's repository, and its wiring assertion becomes a reusable per-step check with an expected step count (a file with two delete steps passes per-file flags when only one keeps the selector). A sweep keyed on `aws ecr delete-repository` rather than on step names covers the sites nobody has named yet, which is how #1820 outlived #1592. Closes #1820 --- .github/workflows/ci.yml | 14 +- .github/workflows/cleanup-staging.yml | 132 ++++++++++---- scripts/test-select-ecr-repos-to-delete.sh | 203 +++++++++++++++++---- 3 files changed, 275 insertions(+), 74 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00b274639..e40526bf4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -753,12 +753,14 @@ 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 every `aws ecr delete-repository` step in .github/workflows still + # pipes through the selector, which is what #1592 and #1820 each escaped. + # Fast (shell only), so it always runs. ecr-delete-selection: name: ECR delete selection scope runs-on: ubuntu-latest diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 06ab63be6..e667d7322 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -138,25 +138,55 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend - - name: Force-delete all staging ECR repos + # 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 job's state owns, read from `terraform + # output` and compared by exact equality against every repository in the + # account. This step used to select by the `cudly-staging*` prefix, which + # also matches `cudly-staging-prod-mirror` and `cudly-staging--backup` + # and force-deleted every image in them (#1820). The prefix also spans + # both staging states: the lambda and fargate jobs each create their own + # `cudly-staging-` repository (main.tf:55), so + # either job deleted the other's. scripts/select-ecr-repos-to-delete.sh + # 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-dispatching 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, the step fails loudly + # `exit 0` ends this step only; the steps after it still run. + # + # Nothing is swallowed here. The `2>/dev/null || echo "may already be + # gone"` this step 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. + - name: Force-delete the ECR repo this state owns 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 + 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 - name: Disable RDS deletion protection before destroy run: | @@ -223,25 +253,55 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend - - name: Force-delete all staging ECR repos + # 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 job's state owns, read from `terraform + # output` and compared by exact equality against every repository in the + # account. This step used to select by the `cudly-staging*` prefix, which + # also matches `cudly-staging-prod-mirror` and `cudly-staging--backup` + # and force-deleted every image in them (#1820). The prefix also spans + # both staging states: the lambda and fargate jobs each create their own + # `cudly-staging-` repository (main.tf:55), so + # either job deleted the other's. scripts/select-ecr-repos-to-delete.sh + # 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-dispatching 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, the step fails loudly + # `exit 0` ends this step only; the steps after it still run. + # + # Nothing is swallowed here. The `2>/dev/null || echo "may already be + # gone"` this step 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. + - name: Force-delete the ECR repo this state owns 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 + 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 - name: Disable RDS deletion protection before destroy run: | diff --git a/scripts/test-select-ecr-repos-to-delete.sh b/scripts/test-select-ecr-repos-to-delete.sh index 236ae4c64..53ddb6eaa 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,50 +227,134 @@ 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. 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. +# `| ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO"` stage from a +# consumer and all of them stay green while that workflow 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. # -# 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. +# Scoped to the steps that delete: the selector invocation, its "$OWNED_REPO" +# argument and `aws ecr delete-repository` must all appear inside one and the +# same step. Asserting them anywhere in the file would let an unrelated line +# keep this green after a delete step lost its selector stage or its argument +# -- the same "the string is present somewhere" mistake the selector itself +# exists to remove. Step names are variables so the regexes and the messages +# cannot drift apart. # # `[|]` 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" +WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" + +# 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 pipes through the selector alongside its +# `aws ecr delete-repository` call. 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 selector and the other drops it satisfies +# per-file flags and passes. +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 [[ ! -f "$CONSUMER" ]]; then - echo "FAIL: consumer workflow not found at ${CONSUMER}" + if awk -v step="$step" -v expected="$expected" ' + function finish() { + if (in_step) { + steps++ + if (!(has_selector && has_delete)) unwired++ + } + in_step = 0; has_selector = 0; has_delete = 0 + } + $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") { + finish(); in_step = 1; next + } + /^[[:space:]]*-[[:space:]]+name:/ { finish() } + 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 { finish(); exit !(steps == expected && unwired == 0) } + ' "$workflow"; then + echo "PASS: all ${expected} '${step}' step(s) in $(basename "$workflow") pipe ECR deletion through the selector" + ((pass++)) || true + else + echo "FAIL: $(basename "$workflow") does not have exactly ${expected} step(s) named" + echo " '${step}' that each pipe ./scripts/select-ecr-repos-to-delete.sh" + echo " \"\$OWNED_REPO\" alongside their 'aws ecr delete-repository' call (stage" + echo " removed, argument dropped, step renamed, or a step added/deleted). The" + echo " cases above only exercise the script standalone, so they stay green" + echo " 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 + +# The assertions above name the steps they know about, so a NEW delete site in +# a new step or a new workflow is invisible to them -- which is how #1820 +# outlived #1592. This sweep is keyed on the dangerous call instead of on a +# name: every step anywhere in .github/workflows that runs `aws ecr +# delete-repository` must pipe through the selector, whatever it or its +# workflow is called. The argument only has to be a quoted variable here; the +# named assertions pin it to "$OWNED_REPO". +# +# It also fails when it finds no delete step at all, because that is what a +# wrong WORKFLOW_DIR or an unmatched glob looks like, and an empty sweep would +# otherwise report a clean result for files it never read. Both workflow +# extensions GitHub accepts are swept, so a new `.yaml` file cannot slip past. +shopt -s nullglob +WORKFLOW_FILES=("${WORKFLOW_DIR}"/*.yml "${WORKFLOW_DIR}"/*.yaml) +shopt -u nullglob + +if [[ ${#WORKFLOW_FILES[@]} -eq 0 ]]; then + echo "FAIL: no workflow files found under ${WORKFLOW_DIR}" ((fail++)) || true -elif awk -v step="$DELETE_STEP" ' - $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") { - in_step = 1 - next + echo + echo "passed: ${pass}, failed: ${fail}" + exit 1 +fi + +unwired_report="$( + awk ' + function finish() { + if (has_delete && !has_selector) { + printf "%s: step \"%s\"\n", FILENAME, step_name + } + if (has_delete) total++ + has_delete = 0; has_selector = 0; 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" + FNR == 1 && NR > 1 { finish() } + /^[[:space:]]*-[[:space:]]+name:/ { + finish() + step_name = $0 + sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) + } + /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$][A-Za-z_][A-Za-z0-9_]*"/ { has_selector = 1 } + /aws ecr delete-repository/ { has_delete = 1 } + END { finish(); if (total == 0) print "no `aws ecr delete-repository` step found at all" } + ' "${WORKFLOW_FILES[@]}" +)" + +if [[ -z "$unwired_report" ]]; then + echo "PASS: every 'aws ecr delete-repository' step in .github/workflows pipes 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" + echo "FAIL: an 'aws ecr delete-repository' step does not pipe through" + echo " ./scripts/select-ecr-repos-to-delete.sh, so it deletes whatever its own" + echo " filter matches:" + while IFS= read -r line; do + echo " ${line}" + done <<<"$unwired_report" ((fail++)) || true fi From 9dd505c1979efb57c1768801b9ca2b44439c2074 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 19:26:50 +0200 Subject: [PATCH 2/4] test(ci): match ECR delete invocations, not prose, in the wiring sweep (#1820) The sweep that asserts every `aws ecr delete-repository` step across .github/workflows pipes through scripts/select-ecr-repos-to-delete.sh keyed on the command string appearing anywhere in a step. That flagged ci.yml's own self-tests step, whose comment names the command while describing the assertion, so the guard policed what may be written about the command rather than what is run. Both wiring assertions now match the shell code on a line: code_of() drops a trailing comment at a word-start `#`, the one rule YAML and shell already share. invokes_delete() additionally empties quoted string literals, so a command named inside "..." or '...' is prose. The selector match runs on code_of() alone, since it asserts the literal text of the "$OWNED_REPO" argument and emptying literals would erase it. Running it on the raw line would let a commented-out selector stage mask an unguarded delete in the same step, which fails open. The sweep body moves into sweep_unwired(), so the recognition it depends on is itself exercised over fixtures in both directions: prose alone is not a delete site, a wired delete beside prose is not flagged, and an unwired delete is flagged past a commented-out selector stage. A directory holding no workflow file is reported rather than passed, since an empty file set satisfies every negative predicate and would otherwise read as clean for files never opened. 41 passed, 0 failed. --- scripts/test-select-ecr-repos-to-delete.sh | 210 +++++++++++++++++---- 1 file changed, 173 insertions(+), 37 deletions(-) diff --git a/scripts/test-select-ecr-repos-to-delete.sh b/scripts/test-select-ecr-repos-to-delete.sh index 53ddb6eaa..695d3faf7 100755 --- a/scripts/test-select-ecr-repos-to-delete.sh +++ b/scripts/test-select-ecr-repos-to-delete.sh @@ -249,6 +249,45 @@ run_case "more than one argument exits 2" 2 "" "$ACCOUNT_LISTING" "$OWNED" "cudl REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" +# Both wiring assertions below have to tell a step that RUNS `aws ecr +# delete-repository` apart from one that only mentions it. ci.yml's own comment +# describing this assertion names the command in prose, 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. +# +# The selector match runs on code_of() alone, because it asserts the literal +# text of the `"$OWNED_REPO"` 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. +# +# `^#` 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/ + } +' + # assert_step_wiring WORKFLOW_FILE STEP_NAME EXPECTED_STEPS # # Asserts that WORKFLOW_FILE contains exactly EXPECTED_STEPS steps named @@ -268,7 +307,7 @@ assert_step_wiring() { return fi - if awk -v step="$step" -v expected="$expected" ' + if awk -v SQ="'" -v step="$step" -v expected="$expected" "$AWK_CODE_FUNCS"' function finish() { if (in_step) { steps++ @@ -280,8 +319,8 @@ assert_step_wiring() { finish(); in_step = 1; next } /^[[:space:]]*-[[:space:]]+name:/ { finish() } - 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 } + in_step && code_of($0) ~ /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$]OWNED_REPO"/ { has_selector = 1 } + in_step && invokes_delete($0) { has_delete = 1 } END { finish(); exit !(steps == expected && unwired == 0) } ' "$workflow"; then echo "PASS: all ${expected} '${step}' step(s) in $(basename "$workflow") pipe ECR deletion through the selector" @@ -308,24 +347,31 @@ assert_step_wiring "${WORKFLOW_DIR}/cleanup-staging.yml" "Force-delete the ECR r # workflow is called. The argument only has to be a quoted variable here; the # named assertions pin it to "$OWNED_REPO". # -# It also fails when it finds no delete step at all, because that is what a -# wrong WORKFLOW_DIR or an unmatched glob looks like, and an empty sweep would -# otherwise report a clean result for files it never read. Both workflow +# It also reports when it finds no delete step 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. -shopt -s nullglob -WORKFLOW_FILES=("${WORKFLOW_DIR}"/*.yml "${WORKFLOW_DIR}"/*.yaml) -shopt -u nullglob - -if [[ ${#WORKFLOW_FILES[@]} -eq 0 ]]; then - echo "FAIL: no workflow files found under ${WORKFLOW_DIR}" - ((fail++)) || true - echo - echo "passed: ${pass}, failed: ${fail}" - exit 1 -fi - -unwired_report="$( - awk ' + +# sweep_unwired DIR +# +# Prints one line per delete step in DIR that does not pipe through the +# selector, plus a line of its own when DIR holds no delete step at all. No +# output means the directory is clean. Kept separate from the pass/fail +# reporting so the sweep itself can be exercised over fixtures below. +sweep_unwired() { + local dir="$1" + local files=() + + 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 + + awk -v SQ="'" "$AWK_CODE_FUNCS"' function finish() { if (has_delete && !has_selector) { printf "%s: step \"%s\"\n", FILENAME, step_name @@ -339,24 +385,114 @@ unwired_report="$( step_name = $0 sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) } - /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$][A-Za-z_][A-Za-z0-9_]*"/ { has_selector = 1 } - /aws ecr delete-repository/ { has_delete = 1 } + code_of($0) ~ /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$][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" } - ' "${WORKFLOW_FILES[@]}" -)" - -if [[ -z "$unwired_report" ]]; then - echo "PASS: every 'aws ecr delete-repository' step in .github/workflows pipes through the selector" - ((pass++)) || true -else - echo "FAIL: an 'aws ecr delete-repository' step does not pipe through" - echo " ./scripts/select-ecr-repos-to-delete.sh, so it deletes whatever its own" - echo " filter matches:" - while IFS= read -r line; do - echo " ${line}" - done <<<"$unwired_report" - ((fail++)) || true -fi + ' "${files[@]}" +} + +# assert_sweep LABEL DIR EXPECTED +# +# EXPECTED empty asserts the sweep finds nothing; otherwise it asserts EXPECTED +# appears in the report, so a fixture pins which step was flagged rather than +# only that something was. +assert_sweep() { + local label="$1" + local dir="$2" + local expected="$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' step in .github/workflows pipes through the selector" \ + "$WORKFLOW_DIR" "" + +# --- 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' echo echo "passed: ${pass}, failed: ${fail}" From 45996c179a304c9e8bfb5228d442d05ea0f1552d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 22:40:35 +0200 Subject: [PATCH 3/4] ci: scope the ecr-delete-selection job to contents: read (#1820) ci.yml declares no workflow-level `permissions`, so every job without its own block is minted a GITHUB_TOKEN at the repository default, which this repo has set to read/write. The job checks out the tree and runs a shell script over it; `contents: read` covers that and nothing more. Matches the shape security-scan already uses in this file, which adds `security-events: write` on top only because it uploads SARIF. --- .github/workflows/ci.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e40526bf4..d2a5fa9ff 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -764,6 +764,13 @@ jobs: 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 From 94625c17cf61c064e578f415275e3f9b61172633 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 22:42:06 +0200 Subject: [PATCH 4/4] refactor(ci): delete the owned ECR repo from one script all three destroy steps call (#1820) destroy-fargate-dev.yml and both cleanup-staging.yml staging jobs carried byte-identical copies of the same 15-line cleanup body and a near-identical 30-line comment above it, so every change to how the repository is chosen had to land three times. Landing it in one workflow and not its sibling is literally how #1592 became #1820; three copies is that failure mode with more places to forget. The body moves to scripts/force-delete-owned-ecr-repo.sh and each step becomes a one-line call. Behaviour is unchanged: same `output -json` handling, same exact-equality selection through scripts/select-ecr-repos-to-delete.sh, same absence of error swallowing. The state directory stays an explicit argument at each call site rather than a constant inside the script, because which state the owned name is read from is what decides which repository may be deleted. The wiring assertions in scripts/test-select-ecr-repos-to-delete.sh previously matched the inline selector pipeline inside each named step. That text no longer lives there, so they are re-pointed at where the behaviour now is and split in two: assert_step_wiring each named step still calls the shared script against terraform/environments/aws assert_script_wiring the shared script still reads the owned name from `terraform output`, still feeds it to the selector, and still deletes only what that pipeline yields Asserting only the first would pass a shared script that had gone back to a prefix filter; only the second would pass a workflow that had stopped calling it. Both counts are exact, so a step or a delete added beside the guarded one is caught rather than absorbed. The unnamed-site sweep now covers the two production scripts alongside .github/workflows. Moving the delete out of the workflow steps moved it out of the sweep's reach, and a sweep that kept looking only at .github/workflows would have reported a clean result for a directory that no longer contains the call it looks for. scripts/ is swept file by file rather than globbed because this suite lives there and quotes both the command and the selector as fixture data. Proven to bite, against a copy of scripts/ + .github/workflows/ so no tracked file was mutated (the suite derives its root from BASH_SOURCE). Control: exit 0, 46/0. Each mutation exits 1 with the targeted FAIL: removing either staging call site or the dev one (44 or 45 passed, 1 failed); reverting a staging site to a `starts_with` prefix match (2 failed, the sweep naming the step); dropping the selector stage from the shared script; and keeping the selector call but moving it out of the pipeline so the delete loop reads the raw listing (2 failed each). --- .github/workflows/ci.yml | 6 +- .github/workflows/cleanup-staging.yml | 118 ++------- .github/workflows/destroy-fargate-dev.yml | 47 +--- scripts/force-delete-owned-ecr-repo.sh | 84 +++++++ scripts/test-select-ecr-repos-to-delete.sh | 272 ++++++++++++++++----- 5 files changed, 338 insertions(+), 189 deletions(-) create mode 100755 scripts/force-delete-owned-ecr-repo.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2a5fa9ff..2c0b2cb21 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -758,8 +758,10 @@ jobs: # 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 every `aws ecr delete-repository` step in .github/workflows still - # pipes through the selector, which is what #1592 and #1820 each escaped. + # 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 diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index e667d7322..586160736 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -138,55 +138,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 job's state owns, read from `terraform - # output` and compared by exact equality against every repository in the - # account. This step used to select by the `cudly-staging*` prefix, which - # also matches `cudly-staging-prod-mirror` and `cudly-staging--backup` - # and force-deleted every image in them (#1820). The prefix also spans - # both staging states: the lambda and fargate jobs each create their own - # `cudly-staging-` repository (main.tf:55), so - # either job deleted the other's. scripts/select-ecr-repos-to-delete.sh - # 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-dispatching 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, the step fails loudly - # `exit 0` ends this step only; the steps after it still run. - # - # Nothing is swallowed here. The `2>/dev/null || echo "may already be - # gone"` this step 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. + # 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: | - 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 before destroy run: | @@ -253,55 +216,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 job's state owns, read from `terraform - # output` and compared by exact equality against every repository in the - # account. This step used to select by the `cudly-staging*` prefix, which - # also matches `cudly-staging-prod-mirror` and `cudly-staging--backup` - # and force-deleted every image in them (#1820). The prefix also spans - # both staging states: the lambda and fargate jobs each create their own - # `cudly-staging-` repository (main.tf:55), so - # either job deleted the other's. scripts/select-ecr-repos-to-delete.sh - # 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-dispatching 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, the step fails loudly - # `exit 0` ends this step only; the steps after it still run. - # - # Nothing is swallowed here. The `2>/dev/null || echo "may already be - # gone"` this step 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. + # 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: | - 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 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 695d3faf7..0c80e7381 100755 --- a/scripts/test-select-ecr-repos-to-delete.sh +++ b/scripts/test-select-ecr-repos-to-delete.sh @@ -229,32 +229,40 @@ run_case "more than one argument exits 2" 2 "" "$ACCOUNT_LISTING" "$OWNED" "cudl # --- Wiring: the consumers still route ECR deletion through this selector ---- # -# Every case above exercises the script standalone. Delete the -# `| ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO"` stage from a -# consumer and all of them stay green while that workflow goes back to +# 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. # -# Scoped to the steps that delete: the selector invocation, its "$OWNED_REPO" -# argument and `aws ecr delete-repository` must all appear inside one and the -# same step. Asserting them anywhere in the file would let an unrelated line -# keep this green after a delete step lost its selector stage or its argument -# -- the same "the string is present somewhere" mistake the selector itself -# exists to remove. Step names are variables so the regexes and the messages -# cannot drift apart. +# 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: +# +# 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)" WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" - -# Both wiring assertions below have to tell a step that RUNS `aws ecr -# delete-repository` apart from one that only mentions it. ci.yml's own comment -# describing this assertion names the command in prose, 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. +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: # @@ -265,11 +273,18 @@ WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" # invokes_delete() additionally empties quoted string literals, so a command # named inside `"..."` or `'...'` is prose, not an invocation. # -# The selector match runs on code_of() alone, because it asserts the literal -# text of the `"$OWNED_REPO"` argument and emptying literals would erase it. +# 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 @@ -286,16 +301,24 @@ AWK_CODE_FUNCS=' 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 pipes through the selector alongside its -# `aws ecr delete-repository` call. 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 selector and the other drops it satisfies -# per-file flags and passes. +# 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" @@ -311,27 +334,26 @@ assert_step_wiring() { function finish() { if (in_step) { steps++ - if (!(has_selector && has_delete)) unwired++ + if (!has_call) unwired++ } - in_step = 0; has_selector = 0; has_delete = 0 + 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\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$]OWNED_REPO"/ { has_selector = 1 } - in_step && invokes_delete($0) { has_delete = 1 } + 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") pipe ECR deletion through the selector" + 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 pipe ./scripts/select-ecr-repos-to-delete.sh" - echo " \"\$OWNED_REPO\" alongside their 'aws ecr delete-repository' call (stage" - echo " removed, argument dropped, step renamed, or a step added/deleted). The" - echo " cases above only exercise the script standalone, so they stay green" - echo " while the #1592/#1820 over-match returns" + 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 } @@ -339,28 +361,96 @@ assert_step_wiring() { 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 -# The assertions above name the steps they know about, so a NEW delete site in -# a new step or a new workflow is invisible to them -- which is how #1820 -# outlived #1592. This sweep is keyed on the dangerous call instead of on a -# name: every step anywhere in .github/workflows that runs `aws ecr -# delete-repository` must pipe through the selector, whatever it or its -# workflow is called. The argument only has to be a quoted variable here; the -# named assertions pin it to "$OWNED_REPO". +# 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 step at all, because that is what a +# 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 +# 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. # -# Prints one line per delete step in DIR that does not pipe through the -# selector, plus a line of its own when DIR holds no delete step at all. No -# output means the directory 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) @@ -371,38 +461,52 @@ sweep_unwired() { 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) { - printf "%s: step \"%s\"\n", FILENAME, step_name + 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 && NR > 1 { finish() } + FNR == 1 { if (NR > 1) finish(); site_file = FILENAME } /^[[:space:]]*-[[:space:]]+name:/ { finish() step_name = $0 sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) } - code_of($0) ~ /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$][A-Za-z_][A-Za-z0-9_]*"/ { has_selector = 1 } + 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 +# 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 step was flagged rather than +# 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")" + report="$(sweep_unwired "$dir" "$@")" if [[ -z "$expected" && -z "$report" ]] || [[ -n "$expected" && "$report" == *"$expected"* ]]; then echo "PASS: $label" @@ -425,8 +529,8 @@ assert_sweep() { fi } -assert_sweep "every 'aws ecr delete-repository' step in .github/workflows pipes through the selector" \ - "$WORKFLOW_DIR" "" +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 --------------------- # @@ -494,6 +598,64 @@ assert_sweep "an unwired delete step is flagged, past a commented-out selector s 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}" [[ "$fail" -eq 0 ]]