diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e36f247c4..d6122da87 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -717,6 +717,25 @@ 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. + ecr-delete-selection: + name: ECR delete selection scope + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + + - name: Run selector self-tests + run: bash scripts/test-select-ecr-repos-to-delete.sh + # Summary job - all checks must pass ci-success: name: CI Success @@ -732,6 +751,7 @@ jobs: - azure-role-parity - aws-iam-parity - gcp-secret-scope + - ecr-delete-selection if: always() steps: diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index a496befa0..db02bc79a 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -128,15 +128,43 @@ 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. - name: Force-delete ECR repo run: | - for REPO in $(aws ecr describe-repositories \ - --query "repositories[?contains(repositoryName,'cudly-dev')].repositoryName" \ - --output text 2>/dev/null); do - 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 run: | diff --git a/scripts/select-ecr-repos-to-delete.sh b/scripts/select-ecr-repos-to-delete.sh new file mode 100755 index 000000000..21b9d2de1 --- /dev/null +++ b/scripts/select-ecr-repos-to-delete.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# select-ecr-repos-to-delete.sh +# +# Selects which ECR repositories a destroy workflow may force-delete. +# +# Reads the account's repository names on stdin, one per line, and prints back +# only the ones that are byte-for-byte identical to the owned name passed as the +# single argument. Nothing else is ever printed, so the caller can pipe the +# output straight into `aws ecr delete-repository --force`. +# +# The owned name comes from `terraform output -raw ecr_repository_name`, i.e. +# from the state the destroy is about to tear down, so it is the name this +# environment actually created rather than a pattern someone hopes only matches +# that name. The repository is `local.stack_name` +# (terraform/environments/aws/main.tf), which carries a random suffix, so no +# literal list can be hardcoded here and no prefix describes it uniquely: +# `cudly-dev--backup` shares every prefix the real repository has. +# +# The previous filter was `contains(repositoryName,'cudly-dev')` evaluated +# inside the destroy workflow, which also selected `backup-cudly-dev` and +# `cudly-dev-prod-mirror` and force-deleted every image in them. A +# `starts_with`/`case cudly-dev*` allow-list, the shape cleanup-staging.yml +# uses, still selects `cudly-dev-prod-mirror`; equality is the only comparison +# that does not. +# +# Exit codes: +# 0 selection completed (an empty selection is normal -- the repository may +# already be gone, and the caller must not treat that as an error, so it +# is reported on stderr rather than through the exit code) +# 2 usage error, including an empty or whitespace-bearing owned name, which +# is what a failed `terraform output` looks like. Never degrades into +# "select nothing" or "select everything". + +set -euo pipefail + +if [[ $# -ne 1 ]]; then + echo "usage: $(basename "$0") OWNED_REPOSITORY_NAME < repository-names" >&2 + echo " reads candidate repository names on stdin, one per line" >&2 + exit 2 +fi + +owned="$1" + +# ECR repository names contain no whitespace, so anything that does is not a +# name this script was handed on purpose -- most likely an empty or +# warning-polluted `terraform output`. Refuse rather than guess. +case "$owned" in + '' | *[![:graph:]]*) + echo "error: owned repository name must be non-empty and free of whitespace; got '${owned}'" >&2 + exit 2 + ;; +esac + +found=0 + +# `|| [[ -n "$candidate" ]]` so a final line with no trailing newline is still +# compared. `read` returns non-zero on such a line, which would otherwise drop +# the one entry the caller is looking for and report an empty selection. +while IFS= read -r candidate || [[ -n "$candidate" ]]; do + # Quoted right-hand side: [[ ]] would otherwise treat it as a glob pattern, + # which is the same class of over-matching this script exists to remove. + if [[ "$candidate" == "$owned" ]]; then + printf '%s\n' "$candidate" + found=1 + fi +done + +# An empty selection is a normal outcome (exit 0 above), but it is also what a +# listing taken from the wrong region or account looks like. Say which name was +# looked for so the two are distinguishable in the caller's log. +if [[ "$found" -eq 0 ]]; then + echo "note: '${owned}' is not present in the listing on stdin; nothing to delete (already deleted, or the listing came from a different region or account)" >&2 +fi diff --git a/scripts/test-select-ecr-repos-to-delete.sh b/scripts/test-select-ecr-repos-to-delete.sh new file mode 100755 index 000000000..236ae4c64 --- /dev/null +++ b/scripts/test-select-ecr-repos-to-delete.sh @@ -0,0 +1,224 @@ +#!/usr/bin/env bash +# test-select-ecr-repos-to-delete.sh +# +# Exercises select-ecr-repos-to-delete.sh in BOTH directions over a table of +# real and adversarial repository names. Both directions matter because the +# consumer force-deletes what this selector prints: a selector that matches +# nothing passes every "no longer over-matches" assertion while silently +# leaving the dev repository behind, and a selector that over-matches deletes +# images it never owned. +# +# Exits 0 when all cases pass; exits 1 on any failure. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SELECT="${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" + +# The repository terraform/environments/aws creates for the dev stack: +# local.stack_name = "${project_name}-${environment}-${random_id.suffix.hex}". +OWNED="cudly-dev-1a2b3c4d" + +# A plausible listing of `aws ecr describe-repositories` in the target account. +# Every entry other than $OWNED is a repository this workflow does not own; the +# annotated ones are selected by the filters that were considered and rejected. +ACCOUNT_LISTING=$( + cat <<'EOF' +cudly +cudly-dev +cudly-dev-1a2b3c4d +cudly-dev-1a2b3c4d-backup +cudly-dev-prod-mirror +backup-cudly-dev +prod-cudly-dev-archive +my-cudly-dev-clone +cudly-staging-9f8e7d6c +cudly-prod-0badc0de +EOF +) + +pass=0 +fail=0 + +# assert_case LABEL EXPECTED_EXIT EXPECTED_STDOUT ACTUAL_EXIT ACTUAL_STDOUT +assert_case() { + local label="$1" + local expected_exit="$2" + local expected_out="$3" + local actual_exit="$4" + local actual_out="$5" + + if [[ "$actual_exit" -eq "$expected_exit" && "$actual_out" == "$expected_out" ]]; then + echo "PASS: $label" + ((pass++)) || true + else + echo "FAIL: $label" + echo " expected exit $expected_exit, got $actual_exit" + echo " expected stdout: '${expected_out}'" + echo " actual stdout: '${actual_out}'" + ((fail++)) || true + fi +} + +# run_case LABEL EXPECTED_EXIT EXPECTED_STDOUT STDIN [ARGS...] +run_case() { + local label="$1" + local expected_exit="$2" + local expected_out="$3" + local stdin_data="$4" + shift 4 + + local actual_out actual_exit=0 + actual_out="$("$SELECT" "$@" <<<"$stdin_data" 2>/dev/null)" || actual_exit=$? + + assert_case "$label" "$expected_exit" "$expected_out" "$actual_exit" "$actual_out" +} + +# run_case_no_trailing_newline LABEL EXPECTED_EXIT EXPECTED_STDOUT STDIN [ARGS...] +# +# Same assertions, but stdin has no trailing newline. `<<<` always appends one, +# so run_case structurally cannot reach the final-unterminated-line path where +# `read` returns non-zero with the line already in the variable. +run_case_no_trailing_newline() { + local label="$1" + local expected_exit="$2" + local expected_out="$3" + local stdin_data="$4" + shift 4 + + local actual_out actual_exit=0 + actual_out="$(printf '%s' "$stdin_data" | "$SELECT" "$@" 2>/dev/null)" || actual_exit=$? + + assert_case "$label" "$expected_exit" "$expected_out" "$actual_exit" "$actual_out" +} + +# --- Positive direction: the repository that SHOULD be destroyed is selected -- +# +# Asserted against the full account listing, not against a listing containing +# only the owned name, so a selector that drops entries under pressure from the +# neighbouring names is caught here. +run_case "owned repo is selected out of the full account listing" \ + 0 "$OWNED" "$ACCOUNT_LISTING" "$OWNED" + +run_case "selection is not tied to one suffix (different random_id)" \ + 0 "cudly-dev-deadbeef" \ + "$(printf 'cudly\ncudly-dev-deadbeef\ncudly-dev-deadbeef-backup\n')" \ + "cudly-dev-deadbeef" + +run_case "owned repo already deleted selects nothing, exit 0" \ + 0 "" \ + "$(printf 'cudly\ncudly-staging-9f8e7d6c\n')" \ + "$OWNED" + +run_case "empty account listing selects nothing, exit 0" \ + 0 "" "" "$OWNED" + +# The owned repository arriving as the last line of an unterminated stream is +# still selected. A plain `while read` drops it and reports an empty selection, +# which reads as "already deleted" and leaves the repository behind for +# `terraform destroy` to trip over. +run_case_no_trailing_newline "owned repo on an unterminated final line is selected" \ + 0 "$OWNED" \ + "$(printf 'cudly\n%s' "$OWNED")" \ + "$OWNED" + +run_case_no_trailing_newline "owned repo as the only, unterminated line is selected" \ + 0 "$OWNED" "$OWNED" "$OWNED" + +# --- Negative direction: every non-owned name is excluded ------------------- +# +# Each is asserted on its own so a failure names the repository that would have +# been force-deleted. The trailing comment on each is the filter that selects +# it: `contains` is what shipped, `starts_with`/`case cudly-dev*` is the +# allow-list shape cleanup-staging.yml uses, and `cudly-dev-1a2b3c4d*` is the +# tightest prefix that still describes the real repository. +while IFS='|' read -r repo caught_by; do + [[ -n "$repo" ]] || continue + run_case "excluded: ${repo} (selected by ${caught_by})" \ + 0 "" "$repo" "$OWNED" +done <<'EOF' +cudly-dev|contains +cudly-dev-prod-mirror|contains, starts_with cudly-dev +cudly-dev-1a2b3c4d-backup|contains, starts_with cudly-dev, prefix of the real repo +backup-cudly-dev|contains +prod-cudly-dev-archive|contains +my-cudly-dev-clone|contains +cudly|prefix of every name here +cudly-staging-9f8e7d6c|no filter, regression guard +cudly-prod-0badc0de|no filter, regression guard +CUDLY-DEV-1A2B3C4D|a case-folding comparison +EOF + +# A name that differs from the owned one only by surrounding whitespace is a +# different repository, and comparing it as equal would delete the wrong one. +run_case "excluded: owned name with a leading space" \ + 0 "" " ${OWNED}" "$OWNED" + +# The owned name is compared literally, not as a glob. An unquoted right-hand +# side in [[ ]] would make this select both repositories. +run_case "owned name is not expanded as a glob pattern" \ + 0 "" \ + "$(printf 'cudly-dev-1a2b3c4d\ncudly-dev-prod-mirror\n')" \ + 'cudly-dev-*' + +# --- 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 +# loud failure and not a silent empty selection, which would look identical to +# "the repository is already gone" and let the destroy report success. +run_case "empty owned name exits 2" 2 "" "$ACCOUNT_LISTING" "" +run_case "whitespace-only owned name exits 2" 2 "" "$ACCOUNT_LISTING" " " +run_case "owned name containing a space exits 2" 2 "" "$ACCOUNT_LISTING" "cudly dev" +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 ---- +# +# 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. +# +# 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. +# +# `[|]` 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 + } + 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 + +echo +echo "passed: ${pass}, failed: ${fail}" +[[ "$fail" -eq 0 ]]