From a12f323c9e2ac8045836588961252b4229e9f2ba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 13:03:03 +0200 Subject: [PATCH 1/3] sec(ci): delete only the Cloud SQL instance the staging state owns before destroy The GCP staging cleanup step selected the Cloud SQL instance to delete with `gcloud sql instances list --filter="name:cudly-staging" | head -1`, deleted it with `|| true`, then ran three `terraform state rm` calls against root-level resource addresses regardless of the delete's outcome. The filter over-matched: substring `:` matched a read replica, an operator-named instance and a `backup-cudly-staging` name, and gcloud warns that `:` evaluation is changing such that this exact filter will match NOTHING on a future SDK. No `--filter` form is a stable equality test, so selection now lists instances unfiltered and compares them in the shell via the existing scripts/select-owned-name.sh (already used by the AWS ECR and RDS destroy paths). The three `state rm` addresses were also wrong: the resources live under `module.database`, not at root, so those commands have never removed anything (measured: a root-level address exits 1 "No matching objects found"). Correcting the addresses makes the removal real for the first time, so it now runs only after a successful delete, and neither the delete nor the state rm swallows a failure anymore. The step's body moves into scripts/delete-owned-cloud-sql-instance.sh so it can be exercised against stubbed gcloud/terraform in scripts/test-cloud-sql-delete-scope.sh (added in the next commit), mirroring the ECR and RDS scripts. Closes #1971 Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- .github/workflows/cleanup-staging.yml | 46 ++---- scripts/delete-owned-cloud-sql-instance.sh | 168 +++++++++++++++++++++ scripts/select-owned-name.sh | 34 +++-- 3 files changed, 204 insertions(+), 44 deletions(-) create mode 100755 scripts/delete-owned-cloud-sql-instance.sh diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 672147234..8d8a57cfa 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -382,7 +382,20 @@ jobs: cd terraform/environments/gcp terraform init -backend-config=/tmp/backend.tfbackend - - name: Delete Cloud SQL instance directly (avoids user/DB ordering deadlock) + # Runs before `terraform destroy`: destroying google_sql_user and + # google_sql_database through Terraform deadlocks on PostgreSQL object + # ownership, and github-staging.tfvars applies the instance with + # deletion_protection = true. Deletes only the instance THIS state owns, + # by exact name, then removes it from state. The + # `--filter="name:cudly-staging" | head -1` selection this step used to run + # also matched `cudly-staging-postgres-replica`, `backup-cudly-staging` and + # any operator-named `cudly-staging-*` instance, deleted with `|| true`, + # and then ran `terraform state rm` regardless (#1971). Rationale, the + # measured gcloud filter semantics and why nothing here is swallowed: the + # script's header. Same shape as the ECR and RDS steps in the AWS jobs + # above, which is how the guard reaches every sibling site (#1592 -> #1820 + # -> #1821 was a guard landing on one site and not the next). + - name: Delete the Cloud SQL instance this state owns env: TF_VAR_project_id: ${{ vars.GCP_PROJECT_ID }} GCP_PROJECT_ID: ${{ vars.GCP_PROJECT_ID }} @@ -390,37 +403,10 @@ jobs: # Routed via env: rather than interpolated into this script's source, # per the zero-expression-interpolation-in-run-blocks rule from #1641. PROJECT="$GCP_PROJECT_ID" - # Validate the shape, not merely non-emptiness: a whitespace-only or - # malformed value would otherwise reach `gcloud --project=`, match no - # instance, and silently skip the deletion this step exists to perform. - if ! printf '%s' "$PROJECT" | grep -Eq '^[a-z][a-z0-9-]{5,29}$'; then - echo "::error::vars.GCP_PROJECT_ID is unset or malformed ('$PROJECT'); refusing to run Cloud SQL cleanup" - exit 1 - fi - # Find and delete the staging Cloud SQL instance to avoid PostgreSQL dependency errors - INSTANCE=$(gcloud sql instances list --project="$PROJECT" \ - --filter="name:cudly-staging" --format="value(name)" 2>/dev/null | head -1) - if [ -n "$INSTANCE" ]; then - echo "Deleting Cloud SQL instance $INSTANCE..." - gcloud sql instances delete "$INSTANCE" --project="$PROJECT" --quiet || true - # Wait for deletion to complete (Service Networking Connection can't be removed until Cloud SQL is gone) - echo "Waiting for Cloud SQL instance to be fully deleted..." - for i in $(seq 1 30); do - if ! gcloud sql instances describe "$INSTANCE" --project="$PROJECT" --quiet 2>/dev/null; then - echo "Cloud SQL instance $INSTANCE deleted." - break - fi - echo "Still deleting... ($i/30)" - sleep 15 - done - fi - # Remove Cloud SQL resources from Terraform state so destroy doesn't try to delete them again - cd terraform/environments/gcp - terraform state rm google_sql_database_instance.main 2>/dev/null || true - terraform state rm google_sql_database.main 2>/dev/null || true - terraform state rm google_sql_user.main 2>/dev/null || true + ./scripts/delete-owned-cloud-sql-instance.sh terraform/environments/gcp "$PROJECT" # Remove the Service Networking Connection from state and delete directly # GCP needs extra time after Cloud SQL deletion to release VPC peering + cd terraform/environments/gcp terraform state rm module.networking.google_service_networking_connection.private_vpc_connection 2>/dev/null || true gcloud services vpc-peerings delete \ --service=servicenetworking.googleapis.com \ diff --git a/scripts/delete-owned-cloud-sql-instance.sh b/scripts/delete-owned-cloud-sql-instance.sh new file mode 100755 index 000000000..943d3526b --- /dev/null +++ b/scripts/delete-owned-cloud-sql-instance.sh @@ -0,0 +1,168 @@ +#!/usr/bin/env bash +# delete-owned-cloud-sql-instance.sh +# +# Deletes the one Cloud SQL instance a Terraform state owns, and nothing else, +# then removes the three resources that instance owns from state. +# +# Usage: delete-owned-cloud-sql-instance.sh TERRAFORM_STATE_DIR GCP_PROJECT_ID +# +# Run before `terraform destroy` on the GCP staging state: destroying +# google_sql_user and google_sql_database through Terraform deadlocks on +# PostgreSQL object ownership, and github-staging.tfvars applies the instance +# with database_deletion_protection = true, so the destroy fails on it anyway +# unless it is gone first. +# +# The state directory and project id are required arguments rather than +# constants because they are the identity of what gets deleted. Every caller +# happens to pass terraform/environments/gcp 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 instance in +# the project, by scripts/select-owned-name.sh -- the same selector +# force-delete-owned-ecr-repo.sh and disable-owned-rds-deletion-protection.sh +# use. The step this replaces selected by +# `gcloud sql instances list --filter="name:cudly-staging" ... | head -1` +# (#1971). Measured on Cloud SDK 456.0.0 with `gcloud config configurations +# list --filter=...` (a local-only evaluation, no API call, so no live account +# is needed to reproduce this): `name:cudly-staging` matched every hyphenated +# name sharing that substring, and gcloud printed "WARNING: --filter : +# operator evaluation is changing for consistency across Google APIs ... +# currently matches but will not match in the near future" -- per `gcloud +# topic filters`, the new `:` semantics are an anchored word match, under +# which this filter matches NOTHING, so a future SDK silently turns this +# selection into a no-op. `name=...` is case-folded (`CUDLY-STAGING-POSTGRES` +# matched too) and is also deprecated for the APIs where it behaves like `:`. +# No `--filter` form is a stable byte-equality test, so the listing below is +# passed UNFILTERED and the comparison happens in the shell, exactly as the +# ECR and RDS callers already do. +# +# The instance name is deterministic, not random-suffixed: +# terraform/modules/database/gcp/main.tf:36 sets +# name = "${var.service_name}-postgres", and service_name resolves to +# "${project_name}-${environment}" (terraform/environments/gcp/main.tf:91, +# github-staging.tfvars), so on staging it is the fixed string +# "cudly-staging-postgres". Equality still matters despite the fixed name: the +# same module creates "cudly-staging-postgres-replica" when +# enable_read_replica is set, every prefix of the real name also matches that +# sibling, and an operator-named "cudly-staging-*" instance or +# "backup-cudly-staging" would match a substring filter too. +# +# `gcloud sql instances delete` is synchronous (its `--async` flag is what +# opts out of waiting), so its exit status IS the deletion confirmation; no +# polling loop is needed, and none is run here. +# +# The three `terraform state rm` lines below are new in effect even though the +# step already ran three state rm commands: those addresses were root-level +# (`google_sql_database_instance.main`), but the resources live under +# `module.database` (terraform/environments/gcp/database.tf:5; +# scripts/gcp-import-dev-state.sh:273-288 imports them module-qualified). +# Measured on Terraform 1.14.7 against a state holding those three +# module-qualified resources: `terraform state rm +# google_sql_database_instance.main` exits 1 "No matching objects found" and +# leaves the state untouched; `terraform state rm +# module.database.google_sql_database_instance.main` removes it and exits 0. +# The old root-level addresses, combined with `2>/dev/null || true`, made +# every run of that step a silent no-op. Correcting the addresses makes the +# removal real for the first time, which is why it must run only after a +# successful delete and must not swallow a failure: an instance that is still +# present must stay in state, or a re-run would try to delete it again with no +# state entry to reconcile against, and `terraform destroy` would try to +# manage it too. +# +# Nothing is swallowed. The old step carried `|| true` on the delete and +# `2>/dev/null || true` on the state rm calls, so a failed delete or a failed +# state rm both reported success and the instance was left running (still +# billing) or the step moved on with the deletion undone. "Already gone" needs +# no swallowing: the selector prints nothing and this script exits 0 without +# touching state. +# +# Cites: #1971, #1592, #1820, #1821. +# +# Exit codes: +# 0 completed, including the "state already destroyed" and "instance +# already gone" cases, which are normal outcomes and not errors +# 1 the owned instance name cannot be resolved from a state that has +# outputs: the key is absent or null (state predates the output), or it +# is present and empty (state or module defect). Distinct messages, +# distinct remedies. +# 2 usage error (wrong arity, a state directory that does not exist, or a +# malformed project id) +# * anything gcloud, terraform, jq or the selector fails with, unmasked + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +if [[ $# -ne 2 ]]; then + echo "usage: $(basename "$0") TERRAFORM_STATE_DIR GCP_PROJECT_ID" >&2 + exit 2 +fi + +STATE_DIR="$1" +PROJECT="$2" + +if [[ ! -d "$STATE_DIR" ]]; then + echo "error: terraform state directory '${STATE_DIR}' does not exist" >&2 + exit 2 +fi + +# Validate the shape, not merely non-emptiness: a whitespace-only or malformed +# value would otherwise reach `gcloud --project=`, match no instance, and +# silently skip the deletion this script exists to perform. +if ! printf '%s' "$PROJECT" | grep -Eq '^[a-z][a-z0-9-]{5,29}$'; then + echo "error: GCP project id is unset or malformed ('${PROJECT}'); refusing to run Cloud SQL cleanup" >&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 Cloud SQL instance to delete." + exit 0 +fi + +if ! OWNED_INSTANCE="$(jq -er '.database_instance_name.value' <<<"$OUTPUTS_JSON")"; then + echo "error: state '${STATE_DIR}' has outputs, but 'database_instance_name' is" >&2 + echo " absent or null, so the instance this state owns cannot be identified." >&2 + echo " Fix: re-apply this state to publish the output, or delete that one" >&2 + echo " instance by hand, then re-run the destroy. Refusing to fall back to a" >&2 + echo " name pattern, which selects instances this state does not own." >&2 + exit 1 +fi + +case "$OWNED_INSTANCE" in + '' | *[![:graph:]]*) + echo "error: state '${STATE_DIR}' publishes 'database_instance_name', but it" >&2 + echo " resolved to '${OWNED_INSTANCE}', which is not an instance name." >&2 + echo " Unlike an absent output, re-applying will NOT fix this: the output is" >&2 + echo " there and empty. Inspect 'terraform -chdir=${STATE_DIR} output -json'" >&2 + echo " before destroying anything. Refusing to fall back to a name pattern." >&2 + exit 1 + ;; +esac + +echo "This state owns Cloud SQL instance '$OWNED_INSTANCE'" + +# Unfiltered on purpose: the equality test is the selector's, not gcloud's. +SELECTED="$(gcloud sql instances list --project="$PROJECT" --format='value(name)' \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_INSTANCE")" + +if [[ -z "$SELECTED" ]]; then + echo "Cloud SQL instance '$OWNED_INSTANCE' is not present in project '$PROJECT'; nothing to delete, state left untouched." + exit 0 +fi + +echo "Deleting Cloud SQL instance $SELECTED..." +gcloud sql instances delete "$SELECTED" --project="$PROJECT" --quiet +echo "Cloud SQL instance $SELECTED deleted." + +# Only after the delete above returned 0, and only the resources it removed. +# Module-qualified: the root-level addresses the workflow used to carry matched +# nothing (#1971). +for ADDR in \ + module.database.google_sql_database_instance.main \ + module.database.google_sql_database.main \ + module.database.google_sql_user.main; do + terraform -chdir="$STATE_DIR" state rm "$ADDR" +done diff --git a/scripts/select-owned-name.sh b/scripts/select-owned-name.sh index 3265e8b3f..49b0dd4db 100755 --- a/scripts/select-owned-name.sh +++ b/scripts/select-owned-name.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # select-owned-name.sh # -# Selects which AWS resources a destroy workflow may act destructively on. +# Selects which cloud resources a destroy workflow may act destructively on. # # Reads the account's resource 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 @@ -10,20 +10,25 @@ # # The owned name comes from `terraform output` on 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. Both current callers pass a -# name derived from `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 name has. +# a pattern someone hopes only matches that name. The two AWS callers pass a +# name derived from `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 name has. The GCP caller passes +# `${project_name}-${environment}-postgres` +# (terraform/modules/database/gcp/main.tf:36), a fixed name with no suffix, +# which every prefix shares with its `-replica` sibling. Equality is the only +# comparison that serves both shapes. # # The comparison is deliberately resource-agnostic, and shared rather than # copied per resource: two copies of it would have to be hardened in lockstep, # and a guard landing on one resource and not its sibling is precisely how #1592 -# became #1820 and then #1821. The callers are: +# became #1820 and then #1821 (and, on GCP, #1971). The callers are: # -# force-delete-owned-ecr-repo.sh `aws ecr delete-repository --force` -# disable-owned-rds-deletion-protection.sh `aws rds modify-db-instance -# --no-deletion-protection` +# force-delete-owned-ecr-repo.sh `aws ecr delete-repository --force` +# disable-owned-rds-deletion-protection.sh `aws rds modify-db-instance +# --no-deletion-protection` +# delete-owned-cloud-sql-instance.sh `gcloud sql instances delete` # # The filters this replaced were `contains(repositoryName,'cudly-dev')` and # `starts_with(DBInstanceIdentifier,'cudly-dev')` evaluated inside the destroy @@ -49,10 +54,11 @@ fi owned="$1" -# Neither an ECR repository name nor an RDS DBInstanceIdentifier may contain -# 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. +# Neither an ECR repository name, an RDS DBInstanceIdentifier, nor a Cloud SQL +# instance name (RFC 1035, `[a-z][a-z0-9-]*`) may contain 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 name must be non-empty and free of whitespace; got '${owned}'" >&2 From f61dfe8f02911da5f5570d9047b29c603e28f50b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 13:03:41 +0200 Subject: [PATCH 2/3] test(ci): guard the Cloud SQL delete selection in both directions Adds scripts/test-cloud-sql-delete-scope.sh, a third sibling to the ECR and RDS selection-scope suites (not an extension of either: different command, different stub, different terraform output key, and a state-write ordering assertion neither AWS suite needs). Asserts, in both directions: the owned instance is still selected out of a hostile listing where the wrong candidate sorts first (the issue's exact scenario), every near-miss name is refused, the destroy step and the shared script are wired together, nothing swallows a failure, and the full set of workflows and scripts is swept for any `gcloud sql instances delete` site that does not pipe through the selector. The behavioural section runs the script end to end against stubbed gcloud and terraform, so a filter reintroduced into the listing call, a failed delete that is not swallowed, and state removal happening only after a successful delete are all asserted as behaviour, not only as text. scripts/lib/code-scan-awk.sh gains the new suite to its exclusion list, since it carries the dangerous command and the selector as fixture data and would otherwise flag itself. ci.yml wires the new suite as an always-on job and adds it to ci-success.needs, since ci-success allowlists only its needs. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- .github/workflows/ci.yml | 41 ++ scripts/lib/code-scan-awk.sh | 10 +- scripts/test-cloud-sql-delete-scope.sh | 900 +++++++++++++++++++++++++ 3 files changed, 946 insertions(+), 5 deletions(-) create mode 100755 scripts/test-cloud-sql-delete-scope.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 51b08a039..ccfb79127 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1000,6 +1000,46 @@ jobs: - name: Run RDS scope self-tests run: bash scripts/test-rds-deletion-protection-scope.sh + # Assert that the Cloud SQL instance selector used by cleanup-staging.yml's + # destroy-gcp job deletes the instance the staging state owns and nothing + # else. deletion_protection = true on that instance means a selector that + # matches nothing leaves it behind and the destroy fails on it; a selector + # that over-matches deletes a database this state never owned. Both + # directions are asserted: every near-miss name is refused, and the owned + # instance is still selected out of a hostile listing first, since a + # selector matching nothing passes every refusal assertion while leaving + # the destroy broken. The suite also asserts the wiring, which is what + # #1592, #1820 and #1821 each escaped on other resources: the destroy step + # calls scripts/delete-owned-cloud-sql-instance.sh, that script lists + # instances with no `--filter` and deletes only what the selector yields, + # the three `terraform state rm` calls are module-qualified and run only + # after a successful delete, and nothing else under .github/workflows or + # scripts/ runs `gcloud sql instances delete` unguarded. That last claim is + # checked over a GLOB of both directories, not a list of known files, so a + # script added later is covered without anyone remembering to name it; the + # sweep is asserted to have opened a non-zero number of files first, since + # an empty swept set has no violations either. + # It additionally runs the script end to end against stubbed terraform and + # gcloud, so "a failed delete is not swallowed" and "state is removed only + # after a successful delete" are asserted as behaviour, not as text. + # Fast (shell only), so it always runs. + cloud-sql-delete-scope: + name: Cloud SQL delete selection scope + runs-on: ubuntu-latest + # Same shape as ecr-delete-selection above: this job checks out the tree and + # runs a shell script against it, so `contents: read` is all it needs. + permissions: + contents: read + + steps: + - name: Checkout code + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + + - name: Run Cloud SQL scope self-tests + run: bash scripts/test-cloud-sql-delete-scope.sh + # Assert that every job applying a `compute_platform` writes the Terraform # state namespace that platform owns: `compute_platform=lambda` into # github-/, `compute_platform=fargate` into github-fargate-/. The @@ -1076,6 +1116,7 @@ jobs: - gcp-secret-scope - ecr-delete-selection - rds-deletion-protection-scope + - cloud-sql-delete-scope - aws-tfstate-platform-key - azure-kv-access-policy if: always() diff --git a/scripts/lib/code-scan-awk.sh b/scripts/lib/code-scan-awk.sh index b39a18ee8..eedc142c1 100644 --- a/scripts/lib/code-scan-awk.sh +++ b/scripts/lib/code-scan-awk.sh @@ -3,9 +3,9 @@ # # Shared awk helper functions for the guard suites that scan workflow and shell # sources for what a step actually runs: test-ecr-delete-selection.sh, -# test-rds-deletion-protection-scope.sh and test-aws-tfstate-platform-key.sh. -# Sourced, not executed; it defines one variable, AWK_CODE_FUNCS, to be -# prepended to an awk program. +# test-rds-deletion-protection-scope.sh, test-aws-tfstate-platform-key.sh and +# test-cloud-sql-delete-scope.sh. Sourced, not executed; it defines one +# variable, AWK_CODE_FUNCS, to be prepended to an awk program. # # Shared rather than copied because these functions encode the rule that # separates code that RUNS a command from prose that only mentions it, and every @@ -54,7 +54,7 @@ # build_swept_scripts SCRIPTS_DIR # # Sets SWEPT_SCRIPTS to every `*.sh` anywhere under SCRIPTS_DIR, at any depth, -# excluding the three guard suites themselves. +# excluding the four guard suites themselves. # # Discovered rather than named file by file, in every suite, because naming the # scripts already known to be guarded is the same defect the suites exist to @@ -96,7 +96,7 @@ build_swept_scripts() { while IFS= read -r -d '' candidate; do case "$(basename "$candidate")" in test-rds-deletion-protection-scope.sh | test-ecr-delete-selection.sh | \ - test-aws-tfstate-platform-key.sh) continue ;; + test-aws-tfstate-platform-key.sh | test-cloud-sql-delete-scope.sh) continue ;; esac SWEPT_SCRIPTS+=("$candidate") done < <(find "$dir" -type f -name '*.sh' -print0 2>/dev/null | sort -z) diff --git a/scripts/test-cloud-sql-delete-scope.sh b/scripts/test-cloud-sql-delete-scope.sh new file mode 100755 index 000000000..2ce30276a --- /dev/null +++ b/scripts/test-cloud-sql-delete-scope.sh @@ -0,0 +1,900 @@ +#!/usr/bin/env bash +# test-cloud-sql-delete-scope.sh +# +# Asserts that the GCP staging destroy deletes the Cloud SQL instance each +# Terraform state owns, and nothing else, before removing it from state. +# +# Both directions matter, and the negative direction alone is worthless here: a +# selector that matches nothing passes every "no longer over-matches" assertion +# while silently leaving the owned instance behind, so `terraform destroy` +# fails on it (deletion_protection = true in github-staging.tfvars). A +# selector that over-matches deletes a Cloud SQL database this state never +# owned. So the owned instance is asserted to be selected out of a full, +# hostile listing BEFORE any absence is asserted (#1971). +# +# Exits 0 when all cases pass; exits 1 on any failure. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" +WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" + +# code_of() / invokes() / pipes_to_selector(), shared with +# test-ecr-delete-selection.sh and test-rds-deletion-protection-scope.sh so the +# rule separating code that RUNS a command from prose that mentions it has one +# definition rather than one that drifts per platform. +# shellcheck source=scripts/lib/code-scan-awk.sh +. "${SCRIPT_DIR}/lib/code-scan-awk.sh" + +SELECT="${SCRIPT_DIR}/select-owned-name.sh" +DELETE_SCRIPT="${SCRIPT_DIR}/delete-owned-cloud-sql-instance.sh" +DELETE_CMD_RE='gcloud[[:space:]]+sql[[:space:]]+instances[[:space:]]+delete' +OWNED="cudly-staging-postgres" # ${project_name}-${environment}-postgres, modules/database/gcp/main.tf:36 +STEP="Delete the Cloud SQL instance this state owns" + +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 FIRST: the owned instance is still selected ---------- +# +# Everything below this point asserts that some name is NOT selected, and +# every one of those assertions is satisfied by a selector that selects +# nothing at all. These run first and are counted, so the negative table can +# never be the only thing holding. +# +# The wrong candidate sorts FIRST, so `head -1` -- the pre-fix selection -- +# would have picked it (the issue's scenario). +LISTING=$( + cat <<'EOF' +cudly-staging-mirror +backup-cudly-staging +cudly-staging +cudly-staging-postgres +cudly-staging-postgres-replica +cudly-staging-prod-mirror +cudly-stagingx +cudly-dev-postgres +cudly-prod-postgres +CUDLY-STAGING-POSTGRES +EOF +) + +selected="$("$SELECT" "$OWNED" <<<"$LISTING" 2>/dev/null)" + +# Counted in the shell rather than with `grep -c`, which prints 0 and exits 1 on +# an empty selection: the exact case this assertion exists to catch, arriving +# as a non-zero exit that a `|| true` would then have to launder. +selected_count=0 +while IFS= read -r line; do + if [[ -n "$line" ]]; then + ((selected_count++)) || true + fi +done <<<"$selected" + +if [[ "$selected_count" -eq 1 ]]; then + echo "PASS: exactly one instance is selected out of the hostile project listing" + ((pass++)) || true +else + echo "FAIL: expected exactly 1 selected instance out of the project listing, got ${selected_count}" + echo " a selection of 0 leaves the owned instance behind and the destroy fails on" + echo " deletion_protection; a selection of >1 deletes a database this state does not own" + ((fail++)) || true +fi + +run_case "owned instance is selected out of the full project listing" \ + 0 "$OWNED" "$LISTING" "$OWNED" + +run_case "owned instance already gone selects nothing, exit 0" \ + 0 "" \ + "$(printf 'cudly-dev-postgres\ncudly-prod-postgres\n')" \ + "$OWNED" + +# The owned instance 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 gone" and leaves the protected instance +# behind for `terraform destroy` to trip over. +run_case_no_trailing_newline "owned instance on an unterminated final line is selected" \ + 0 "$OWNED" \ + "$(printf 'cudly-staging-mirror\n%s' "$OWNED")" \ + "$OWNED" + +# --- Negative direction: every near-miss is refused -------------------------- +# +# Each on its own line so a failure names the database that would have been +# deleted. The trailing comment is the filter that selects it, measured on +# Cloud SDK 456.0.0 (see the script's header for the full probe). +while IFS='|' read -r instance caught_by; do + [[ -n "$instance" ]] || continue + run_case "refused: ${instance} (selected by ${caught_by})" \ + 0 "" "$instance" "$OWNED" +done <<'EOF' +cudly-staging-mirror|name:cudly-staging (substring), sorts first so head -1 picked it +backup-cudly-staging|name:cudly-staging (substring) +cudly-staging|name:cudly-staging +cudly-staging-postgres-replica|name:cudly-staging, name:cudly-staging-postgres*, every prefix of the real name +cudly-staging-prod-mirror|name:cudly-staging +cudly-stagingx|name:cudly-staging +CUDLY-STAGING-POSTGRES|name=cudly-staging-postgres (gcloud = is case-folded, measured on SDK 456.0.0) +cudly-dev-postgres|no filter, regression guard +cudly-prod-postgres|no filter, regression guard +EOF + +# A name that differs from the owned one only by surrounding whitespace is a +# different instance, 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 every instance below. +run_case "owned name is not expanded as a glob pattern" \ + 0 "" \ + "$(printf 'cudly-staging-postgres\ncudly-staging-prod-mirror\n')" \ + 'cudly-staging-*' + +# A failed `terraform output` hands the selector an empty string. That must be +# a loud failure and not a silent empty selection, which looks identical to +# "the instance is already gone" and lets the destroy report success. +run_case "empty owned name exits 2" 2 "" "$LISTING" "" +run_case "whitespace-only owned name exits 2" 2 "" "$LISTING" " " + +# --- Terraform: the state actually publishes the instance name --------------- +# +# The script resolves the owned instance from `terraform output`, so the whole +# guard rests on that output existing and being the instance's real name. +# Delete the output and every case above stays green while the destroy fails +# at runtime on a state that cannot name what it owns. + +# assert_file_matches LABEL FILE AWK_CONDITION +assert_file_matches() { + local label="$1" + local file="$2" + local condition="$3" + + if [[ ! -f "$file" ]]; then + echo "FAIL: ${label} -- file not found at ${file}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" "$AWK_CODE_FUNCS"' + '"$condition"' + END { exit !(hits == 1) } + ' "$file"; then + echo "PASS: $label" + ((pass++)) || true + else + echo "FAIL: $label" + echo " in $(basename "$file") -- expected exactly one match" + ((fail++)) || true + fi +} + +# Matched across the whole `output` block rather than on one line: the value is +# on the line after the block header, so the two are correlated by remembering +# which block is open. `[{]` and `[}]` rather than bare braces, which start an +# interval expression in ERE -- CI's awk is mawk rather than the awk this was +# written on, the same reason the shared helpers avoid `\|` and `\$`. +assert_file_matches "the gcp environment publishes database_instance_name from the database module" \ + "${REPO_ROOT}/terraform/environments/gcp/outputs.tf" \ + 'code_of($0) ~ /^output[[:space:]]+"database_instance_name"[[:space:]]*[{]/ { in_block = 1; next } + in_block && code_of($0) ~ /^[}]/ { in_block = 0 } + in_block && code_of($0) ~ /value[[:space:]]*=[[:space:]]*module\.database\.instance_name[[:space:]]*$/ { hits++ }' + +assert_file_matches "the gcp database module publishes the real google_sql_database_instance name" \ + "${REPO_ROOT}/terraform/modules/database/gcp/outputs.tf" \ + 'code_of($0) ~ /^output[[:space:]]+"instance_name"[[:space:]]*[{]/ { in_block = 1; next } + in_block && code_of($0) ~ /^[}]/ { in_block = 0 } + in_block && code_of($0) ~ /value[[:space:]]*=[[:space:]]*google_sql_database_instance\.main\.name[[:space:]]*$/ { hits++ }' + +# --- Wiring: the destroy step routes through the shared script --------------- +# +# Every case above exercises the selector standalone. Revert the step to a +# `--filter="name:cudly-staging" | head -1` selection and all of them stay +# green while that step deletes whatever the filter matches. The recurrence +# mode that produced #1592, then #1820, then #1821 was exactly that: the guard +# landed on one resource or one platform and not its sibling. + +# assert_step_wiring WORKFLOW_FILE STEP_NAME EXPECTED_STEPS +# +# The state directory and the project argument are pinned rather than accepted +# as any argument because they decide which instance the call may delete. The +# regex ends at end-of-line, so a `|| true` appended to the call fails this +# too. +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\/delete-owned-cloud-sql-instance\.sh[[:space:]]+terraform\/environments\/gcp[[:space:]]+"[$]PROJECT"[[: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 script" + ((pass++)) || true + else + echo "FAIL: $(basename "$workflow") does not have exactly ${expected} step(s) named" + echo " '${step}' that each run" + echo " './scripts/delete-owned-cloud-sql-instance.sh terraform/environments/gcp \"\$PROJECT\"'" + echo " (call removed, state directory changed, project argument changed, a trailing" + echo " '|| true' appended, step renamed, or a step added/deleted)" + ((fail++)) || true + fi +} + +assert_step_wiring "${WORKFLOW_DIR}/cleanup-staging.yml" "$STEP" 1 + +# assert_script_wiring SCRIPT +# +# The other half: the script that step calls must still delete only what the +# exact-match selector yields, must read that name from `terraform output` +# rather than a filter, and must remove state only after a successful delete. +# Counts, each with a fixed expectation, so a second unguarded delete added +# beside the guarded one is caught, and so is a guard removed entirely: +# +# owned the instance name is read from `terraform output`, not +# hardcoded and not derived from a filter +# piped that name reaches the selector +# fed the SELECTED= assignment reads the selector's output, so +# the selector cannot be reduced to a no-op stage beside a +# delete driven by some other listing +# unfiltered/ the listing command carries no `--filter`; the filter is +# filtered not the guard, and a filter reintroduced here is the +# #1971 shape with a case-folded or deprecated operator +# deletes there is exactly one `gcloud sql instances delete` +# by_selected_var the instance it deletes is the one the selector yielded, +# not some other name that happened to be in scope +# state_rm the module-qualified `state rm` line appears exactly +# once in code, strictly AFTER the delete line +# root_addr no root-level `state rm google_sql...` address remains +# (the #1971 shape: an address that never matched anything) +# addr1/2/3 each of the three module-qualified addresses appears +# exactly once +assert_script_wiring() { + local script="$1" + + if [[ ! -f "$script" ]]; then + echo "FAIL: shared script not found at ${script}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" -v cmdre="$DELETE_CMD_RE" "$AWK_CODE_FUNCS"' + code_of($0) ~ /OWNED_INSTANCE=.*jq[[:space:]]+-er[[:space:]]+.*\.database_instance_name\.value/ { owned++ } + pipes_to_selector($0, "\"[$]OWNED_INSTANCE\"") { + piped++ + if (code_of(prev) ~ /SELECTED="?[$][(]gcloud[[:space:]]+sql[[:space:]]+instances[[:space:]]+list/) fed++ + } + code_of($0) ~ /gcloud[[:space:]]+sql[[:space:]]+instances[[:space:]]+list/ { + if (code_of($0) ~ ("--format=" SQ "value\\(name\\)" SQ)) { + if (code_of($0) !~ /--filter/) unfiltered++ + else filtered++ + } + } + invokes($0, cmdre) { deletes++; if (delete_line == 0) delete_line = FNR } + code_of($0) ~ /gcloud[[:space:]]+sql[[:space:]]+instances[[:space:]]+delete[[:space:]]+"[$]SELECTED"/ { by_selected_var++ } + code_of($0) ~ /terraform[[:space:]]+-chdir=.*[[:space:]]state[[:space:]]+rm[[:space:]]+"[$]ADDR"/ { + state_rm++ + if (state_rm_line == 0) state_rm_line = FNR + } + code_of($0) ~ /state[[:space:]]+rm[[:space:]]+google_sql/ { root_addr++ } + code_of($0) ~ /module\.database\.google_sql_database_instance\.main/ { addr1++ } + code_of($0) ~ /module\.database\.google_sql_database\.main/ { addr2++ } + code_of($0) ~ /module\.database\.google_sql_user\.main/ { addr3++ } + { prev = $0 } + END { + exit !(owned == 1 && piped == 1 && fed == 1 && unfiltered == 1 && filtered == 0 && \ + deletes == 1 && by_selected_var == 1 && state_rm == 1 && \ + (state_rm_line > delete_line) && root_addr == 0 && \ + addr1 == 1 && addr2 == 1 && addr3 == 1) + } + ' "$script"; then + echo "PASS: $(basename "$script") deletes only what the exact-match selector yields, unfiltered" + ((pass++)) || true + else + echo "FAIL: $(basename "$script") no longer reads the owned instance name from 'terraform" + echo " output', pipes it unfiltered to scripts/select-owned-name.sh, deletes exactly the" + echo " instance that pipeline yields, and removes exactly the three module-qualified" + echo " state addresses only after that delete succeeds -- expected one of each. This is" + echo " where the body lives, so a --filter or a root-level state address reintroduced" + echo " here is the #1971 shape, whatever the call site looks like" + ((fail++)) || true + fi +} + +assert_script_wiring "$DELETE_SCRIPT" + +# assert_nothing_swallowed SCRIPT +# +# The old step carried `2>/dev/null` on the listing, `|| true` on the delete, +# and `2>/dev/null || echo "..."` on the peering delete in the same step +# (#1971). Asserted on code_of() so the header, which quotes these forms to +# explain why they are gone, is not itself a violation. +assert_nothing_swallowed() { + local script="$1" + + if awk -v SQ="'" "$AWK_CODE_FUNCS"' + code_of($0) ~ /2>[[:space:]]*\/dev\/null/ { print " swallowed stderr: " FNR; bad++ } + code_of($0) ~ /[|][|][[:space:]]*true/ { print " swallowed exit status: " FNR; bad++ } + code_of($0) ~ /[|][|][[:space:]]*echo/ { print " swallowed exit status via echo: " FNR; bad++ } + END { exit !(bad == 0) } + ' "$script"; then + echo "PASS: $(basename "$script") swallows neither a failed listing, a failed delete, nor a failed state rm" + ((pass++)) || true + else + echo "FAIL: $(basename "$script") suppresses an error on the line(s) above. A partial or" + echo " failed delete must fail loudly (#1971), not report success and leave the" + echo " instance running while state is removed out from under it" + ((fail++)) || true + fi +} + +assert_nothing_swallowed "$DELETE_SCRIPT" + +# --- Behaviour: the script run end to end against stubbed gcloud and terraform +# +# Everything above is static. None of it can show that the pipeline actually +# deletes the right instance, that a failed call is really not swallowed, or +# that state is removed only after a successful delete -- a script can satisfy +# every text assertion and still do the wrong thing at runtime. +# +# `terraform` and `gcloud` are stubbed on PATH, and every invocation of either +# is logged to the SAME file, so the ordering assertion (state rm after +# delete) can be made from one call log rather than two that would have to be +# interleaved by wall-clock time. The gcloud stub exits 99 if any argument +# starts with `--filter`, so a filter reintroduced into the listing call fails +# as BEHAVIOUR, not only as text. +STUB_DIR="$(mktemp -d)" +STUB_STATE="$(mktemp -d)" +CALLS="$(mktemp)" +trap 'rm -rf "$STUB_DIR" "$STUB_STATE"; rm -f "$CALLS"' EXIT + +cat >"${STUB_DIR}/terraform" <<'EOF' +#!/usr/bin/env bash +echo "terraform $*" >>"$CALLS" +case "$2" in + output) + printf '%s' "$TF_OUTPUT_JSON" + ;; + state) + if [[ "$3" == "rm" ]]; then + [[ "${STATE_RM_FAILS:-0}" == "1" ]] && exit 3 + fi + ;; +esac +exit 0 +EOF + +cat >"${STUB_DIR}/gcloud" <<'EOF' +#!/usr/bin/env bash +echo "gcloud $*" >>"$CALLS" +case "$3" in + list) + for arg in "$@"; do + case "$arg" in + --filter*) + echo "stub: --filter is not the guard" >&2 + exit 99 + ;; + esac + done + [[ "${LIST_FAILS:-0}" == "1" ]] && { echo "list failed" >&2; exit 255; } + printf '%s\n' "$LISTING" + ;; + delete) + [[ "${DELETE_FAILS:-0}" == "1" ]] && { echo "delete failed" >&2; exit 254; } + ;; +esac +exit 0 +EOF +chmod +x "${STUB_DIR}/terraform" "${STUB_DIR}/gcloud" + +export CALLS + +# The hostile listing from the positive-direction section above, one per line, +# exactly what `gcloud sql instances list --format='value(name)'` prints. +STUB_LISTING="$LISTING" + +PROJECT_OK="cudly-staging" + +# run_script STATE_DIR PROJECT -> STUB_EXIT, STUB_ERR, and a call log in $CALLS +run_script() { + : >"$CALLS" + local errfile + errfile="$(mktemp)" + STUB_EXIT=0 + PATH="${STUB_DIR}:${PATH}" "$DELETE_SCRIPT" "$@" >/dev/null 2>"$errfile" || STUB_EXIT=$? + STUB_ERR="$(cat "$errfile")" + rm -f "$errfile" +} + +# assert_behaviour LABEL CONDITION_RESULT DETAIL +assert_behaviour() { + if [[ "$2" == "0" ]]; then + echo "PASS: $1" + ((pass++)) || true + else + echo "FAIL: $1" + echo " $3" + ((fail++)) || true + fi +} + +count_calls() { grep -c "$1" "$CALLS" 2>/dev/null || true; } + +export LISTING="$STUB_LISTING" + +# A state that is already destroyed is a normal outcome, not an error, and +# must not reach gcloud at all. +export TF_OUTPUT_JSON='{}' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a state with no outputs exits 0 without calling gcloud or state rm" \ + "$([[ "$STUB_EXIT" -eq 0 && "$(count_calls '^gcloud')" -eq 0 && "$(count_calls 'state rm')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" + +# A state that predates the output. Distinct from the empty case below, +# because the remedies differ: this one wants an apply. +export TF_OUTPUT_JSON='{"network_name":{"value":"cudly-staging-vpc"}}' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a state missing the output exits 1 without calling gcloud" \ + "$([[ "$STUB_EXIT" -eq 1 && "$(count_calls '^gcloud')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +assert_behaviour "behaviour: the missing-output error tells the operator to re-apply" \ + "$([[ "$STUB_ERR" == *"absent or null"* && "$STUB_ERR" == *"re-apply this state"* ]] && echo 0 || echo 1)" \ + "stderr: ${STUB_ERR}" + +# `jq -er` accepts an empty string, so without its own check this reaches gcloud. +export TF_OUTPUT_JSON='{"database_instance_name":{"value":""}}' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: an empty instance name exits 1 without calling gcloud" \ + "$([[ "$STUB_EXIT" -eq 1 && "$(count_calls '^gcloud')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +assert_behaviour "behaviour: the empty-name error is distinct and says an apply will not fix it" \ + "$([[ "$STUB_ERR" == *"will NOT fix this"* && "$STUB_ERR" != *"re-apply this state"* ]] && echo 0 || echo 1)" \ + "stderr: ${STUB_ERR}" + +# A null value takes the jq branch, not the empty branch. +export TF_OUTPUT_JSON='{"database_instance_name":{"value":null}}' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a null instance name exits 1 without calling gcloud" \ + "$([[ "$STUB_EXIT" -eq 1 && "$(count_calls '^gcloud')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" + +# Malformed project ids: validation precedes everything, including +# `terraform output`, so none of these reach terraform or gcloud. Each is a +# distinct malformation: empty, whitespace-only, wrong case, too short. +export TF_OUTPUT_JSON='{"database_instance_name":{"value":"cudly-staging-postgres"}}' +for bad_project in "" " " "Cudly-Staging" "cudly"; do + run_script "$STUB_STATE" "$bad_project" + assert_behaviour "behaviour: malformed project '${bad_project}' exits 2 without calling terraform or gcloud" \ + "$([[ "$STUB_EXIT" -eq 2 && ! -s "$CALLS" ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +done + +# The golden path, against the hostile listing (mirror sorts first). +export LISTING="$STUB_LISTING" +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: the golden path exits 0" \ + "$([[ "$STUB_EXIT" -eq 0 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" +assert_behaviour "behaviour: exactly one instance is deleted out of the hostile listing" \ + "$([[ "$(count_calls 'sql instances delete')" -eq 1 ]] && echo 0 || echo 1)" \ + "delete calls: $(count_calls 'sql instances delete')" +assert_behaviour "behaviour: the deleted instance is the one the state owns" \ + "$(grep -q 'sql instances delete cudly-staging-postgres --project=' "$CALLS" && echo 0 || echo 1)" \ + "calls: $(grep delete "$CALLS" || echo none)" +assert_behaviour "behaviour: exactly three state rm calls, each module-qualified" \ + "$([[ "$(count_calls 'state rm module\.database\.')" -eq 3 ]] && echo 0 || echo 1)" \ + "state rm calls: $(grep 'state rm' "$CALLS" || echo none)" +assert_behaviour "behaviour: the first state rm call comes after the delete call" \ + "$([[ "$(grep -n 'sql instances delete' "$CALLS" | head -1 | cut -d: -f1)" -lt \ + "$(grep -n 'state rm' "$CALLS" | head -1 | cut -d: -f1)" ]] && echo 0 || echo 1)" \ + "call log: $(cat -n "$CALLS")" + +# Asserted per neighbour so a failure names the database that would have been +# deleted. +while IFS= read -r neighbour; do + [[ -n "$neighbour" ]] || continue + assert_behaviour "behaviour: neighbour ${neighbour} is not deleted" \ + "$(grep -q -- "sql instances delete ${neighbour} " "$CALLS" && echo 1 || echo 0)" \ + "calls: $(grep delete "$CALLS" || echo none)" +done <<'EOF' +cudly-staging-mirror +cudly-staging-postgres-replica +cudly-staging-prod-mirror +backup-cudly-staging +cudly-stagingx +EOF + +# Re-running a cleanup after a completed one is normal, not an error. +export LISTING=$'cudly-dev-postgres\ncudly-prod-postgres' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: an instance already gone exits 0, deletes nothing, removes no state" \ + "$([[ "$STUB_EXIT" -eq 0 && "$(count_calls 'sql instances delete')" -eq 0 && "$(count_calls 'state rm')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" + +# The two defects #1971 found, as behaviour rather than as text. A failed +# delete used to be swallowed by `|| true` and the state rm lines ran anyway +# (root-level addresses that never matched, so this never showed up); here a +# failed delete must fail the step AND leave state untouched. +export LISTING="$STUB_LISTING" DELETE_FAILS=1 +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a failed delete fails the step and removes no state" \ + "$([[ "$STUB_EXIT" -ne 0 && "$(count_calls 'state rm')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +unset DELETE_FAILS + +export LIST_FAILS=1 +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a failed listing fails the step and deletes nothing" \ + "$([[ "$STUB_EXIT" -ne 0 && "$(count_calls 'sql instances delete')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +unset LIST_FAILS + +export STATE_RM_FAILS=1 +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: a failed state rm fails the step after exactly one delete" \ + "$([[ "$STUB_EXIT" -ne 0 && "$(count_calls 'sql instances delete')" -eq 1 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +unset STATE_RM_FAILS + +run_script +assert_behaviour "behaviour: no arguments exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + +run_script "$STUB_STATE" +assert_behaviour "behaviour: one argument exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + +run_script "$STUB_STATE" "$PROJECT_OK" "extra" +assert_behaviour "behaviour: three arguments exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + +run_script "${STUB_STATE}/does-not-exist" "$PROJECT_OK" +assert_behaviour "behaviour: a missing state directory exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + +# The assertions above name the one file and script they know about, so a NEW +# delete site in a new step, workflow or script is invisible to them -- which +# is how #1820 outlived #1592 and #1821 outlived both. This sweep is keyed on +# the dangerous call instead of on a name: everything anywhere in the swept +# set that runs `gcloud sql instances delete` must pipe through the selector, +# whatever it is called. +# +# scripts/ is GLOBBED via build_swept_scripts, not named file by file, for the +# same reason: a NEW script running the command without the selector must be +# caught even though nobody remembered to list it here. +# +# The four guard suites are excluded by name because they carry both the +# command and the selector as fixture data and in awk programs; sweeping them +# would report this file as a violation of itself. + +# 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. +# +# 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="'" -v cmdre="$DELETE_CMD_RE" "$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) + } + pipes_to_selector($0, "\"[$][A-Za-z_][A-Za-z0-9_]*\"") { has_selector = 1 } + invokes($0, cmdre) { has_delete = 1 } + END { finish(); if (total == 0) print "no `gcloud sql instances delete` 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 +} + +# The scripts/ half of the swept set is globbed, so a script added later is +# swept without anyone remembering to name it here. Rationale and the nullglob +# reasoning: build_swept_scripts in scripts/lib/code-scan-awk.sh. +build_swept_scripts "$SCRIPT_DIR" + +# "Found no violations" must not be reachable by looking at nothing, so the +# swept set is asserted non-empty and asserted to contain the one script that +# actually runs the command. Guarding the expansion too: under `set -u`, bash +# 3.2 treats "${arr[@]}" on an empty array as an unbound variable. +if [[ ${#SWEPT_SCRIPTS[@]} -eq 0 ]]; then + echo "FAIL: the scripts/ half of the swept set is empty" + echo " ${SCRIPT_DIR}/*.sh matched nothing, so the sweep below would report a" + echo " clean result for files it never opened" + ((fail++)) || true +else + echo "PASS: the swept set holds ${#SWEPT_SCRIPTS[@]} script(s) under scripts/" + ((pass++)) || true + + swept_has_guarded=0 + for swept_candidate in "${SWEPT_SCRIPTS[@]}"; do + [[ "$swept_candidate" == "$DELETE_SCRIPT" ]] && swept_has_guarded=1 + done + if [[ "$swept_has_guarded" -eq 1 ]]; then + echo "PASS: the swept set includes $(basename "$DELETE_SCRIPT"), the script that runs the command" + ((pass++)) || true + else + echo "FAIL: the swept set does not include $(basename "$DELETE_SCRIPT")" + echo " the sweep would then find no delete site at all and pass vacuously" + ((fail++)) || true + fi + + assert_sweep "every 'gcloud sql instances delete' site in .github/workflows and scripts/ pipes through the selector" \ + "$WORKFLOW_DIR" "" "${SWEPT_SCRIPTS[@]}" +fi + +# --- The sweep itself, in both directions, over fixtures --------------------- +# +# The sweep is the only assertion covering 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: a guard that +# fired on ci.yml's comment describing this assertion would police what may be +# written rather than what is run. +FIXTURE_DIR="$(mktemp -d)" +# Replaces the stub trap set above rather than adding to it, so it has to +# clean up both sets. A second `trap ... EXIT` silently discards the first. +trap 'rm -rf "$FIXTURE_DIR" "$STUB_DIR" "$STUB_STATE"; rm -f "$CALLS"' EXIT +mkdir -p "${FIXTURE_DIR}/prose" "${FIXTURE_DIR}/wired" "${FIXTURE_DIR}/unwired" "${FIXTURE_DIR}/scripts" "${FIXTURE_DIR}/misattrib" + +cat >"${FIXTURE_DIR}/prose/mentions.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `gcloud sql instances delete` step is wired + echo "would run gcloud sql instances delete if it were wired" + echo 'gcloud sql instances delete is named here too' +EOF + +cat >"${FIXTURE_DIR}/wired/deletes.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `gcloud sql instances delete` step is wired + echo "would run gcloud sql instances delete if it were wired" + + - name: Delete the Cloud SQL instance this state owns + run: | + SELECTED="$(gcloud sql instances list --project="$PROJECT" --format='value(name)' \ + | ./scripts/select-owned-name.sh "$OWNED_INSTANCE")" + gcloud sql instances delete "$SELECTED" --project="$PROJECT" --quiet +EOF + +# The #1971 shape: a substring filter and `head -1`, no selector, `|| true` on +# the delete. A commented-out selector line inside the `if` must not satisfy +# the wiring, which is why the selector match runs on the comment-stripped +# line. Body is the pre-fix step from origin/main (lines 385-416) verbatim, +# plus that one commented-out line. +cat >"${FIXTURE_DIR}/unwired/deletes.yml" <<'EOF' + - name: Delete Cloud SQL instance directly (avoids user/DB ordering deadlock) + env: + TF_VAR_project_id: ${{ vars.GCP_PROJECT_ID }} + GCP_PROJECT_ID: ${{ vars.GCP_PROJECT_ID }} + run: | + # Routed via env: rather than interpolated into this script's source, + # per the zero-expression-interpolation-in-run-blocks rule from #1641. + PROJECT="$GCP_PROJECT_ID" + # Validate the shape, not merely non-emptiness: a whitespace-only or + # malformed value would otherwise reach `gcloud --project=`, match no + # instance, and silently skip the deletion this step exists to perform. + if ! printf '%s' "$PROJECT" | grep -Eq '^[a-z][a-z0-9-]{5,29}$'; then + echo "::error::vars.GCP_PROJECT_ID is unset or malformed ('$PROJECT'); refusing to run Cloud SQL cleanup" + exit 1 + fi + # Find and delete the staging Cloud SQL instance to avoid PostgreSQL dependency errors + INSTANCE=$(gcloud sql instances list --project="$PROJECT" \ + --filter="name:cudly-staging" --format="value(name)" 2>/dev/null | head -1) + if [ -n "$INSTANCE" ]; then + # | ./scripts/select-owned-name.sh "$OWNED_INSTANCE" + echo "Deleting Cloud SQL instance $INSTANCE..." + gcloud sql instances delete "$INSTANCE" --project="$PROJECT" --quiet || true + # Wait for deletion to complete (Service Networking Connection can't be removed until Cloud SQL is gone) + echo "Waiting for Cloud SQL instance to be fully deleted..." + for i in $(seq 1 30); do + if ! gcloud sql instances describe "$INSTANCE" --project="$PROJECT" --quiet 2>/dev/null; then + echo "Cloud SQL instance $INSTANCE deleted." + break + fi + echo "Still deleting... ($i/30)" + sleep 15 + done + fi +EOF + +assert_sweep "a step that only mentions the command is not a delete site" \ + "${FIXTURE_DIR}/prose" 'no `gcloud sql instances delete` 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 "Delete Cloud SQL instance directly (avoids user/DB ordering deadlock)"' + +assert_sweep "a directory holding no workflow file is reported, not passed" \ + "${FIXTURE_DIR}/empty-does-not-exist" 'no workflow files found under' + +# The body 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. Both directions, over a file swept by name the way the real one +# is. The `prose` dir supplies the workflow half and contributes no delete site. +cat >"${FIXTURE_DIR}/scripts/wired.sh" <<'EOF' +SELECTED="$(gcloud sql instances list --project="$PROJECT" --format='value(name)' \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_INSTANCE")" +gcloud sql instances delete "$SELECTED" --project="$PROJECT" --quiet +EOF + +cat >"${FIXTURE_DIR}/scripts/unwired.sh" <<'EOF' +INSTANCE=$(gcloud sql instances list --project="$PROJECT" \ + --filter="name:cudly-staging" --format="value(name)" 2>/dev/null | head -1) +gcloud sql instances delete "$INSTANCE" --project="$PROJECT" --quiet || true +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 running 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 swept next, and every fixture above +# sweeps one file at a time, so none of them can catch it. +cat >"${FIXTURE_DIR}/misattrib/a-unwired.yml" <<'EOF' + - name: Delete Cloud SQL instance + run: | + gcloud sql instances delete "$INSTANCE" --project="$PROJECT" --quiet +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 "Delete Cloud SQL instance"' + +echo +echo "passed: ${pass}, failed: ${fail}" +[[ "$fail" -eq 0 ]] From 46027497a689400bcd9374cfced3f772ac5b4479 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 8 Sep 2026 13:39:39 +0200 Subject: [PATCH 3/3] fix(ci): tighten the Cloud SQL delete stub and the fail-open outputs guard Two findings from adversarial review of #2085. The gcloud stub in test-cloud-sql-delete-scope.sh denylisted `--filter` specifically, so `--limit=1` (or `--page-size`, `--sort-by`, `--uri`, `--flags-file`) walked straight past it the same way `head -1` used to, and real gcloud 456 honours all of those. Converted the stub's `list` branch to an allowlist of the exact argument vector the script sends, so any of those flags now fails the suite as behaviour instead of passing unnoticed. `if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]` is fail-open: a jq failure (non-JSON on stdout from a broken `terraform output`) substitutes an empty string, and `[[ "" -eq 0 ]]` evaluates true, so the script reports "already destroyed" and exits 0 without ever calling gcloud. Moved the jq call to its own assignment so `set -e` surfaces its exit status instead of letting the `eq` comparison mask it. The same line existed verbatim in the ECR and RDS sibling scripts; fixed all three rather than only the one this PR touches, since fixing one implies the other two were considered and judged fine. Added a suite case asserting a terraform stub that prints non-JSON fails loudly with no gcloud call. Also fixes a pre-existing git-secrets false positive in disable-owned-rds-deletion-protection.sh: its output-state table separator row was a 60+ character run of only `-` and `|`, which trips the generic 40-character secret-shaped-string pattern once this file is staged again for any reason. Replaced the `|` column dividers with `+` on that one row, which breaks the run without changing the table's readability. Co-Authored-By: claude-flow Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC --- scripts/delete-owned-cloud-sql-instance.sh | 9 +++- .../disable-owned-rds-deletion-protection.sh | 11 ++++- scripts/force-delete-owned-ecr-repo.sh | 9 +++- scripts/test-cloud-sql-delete-scope.sh | 42 ++++++++++++++----- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/scripts/delete-owned-cloud-sql-instance.sh b/scripts/delete-owned-cloud-sql-instance.sh index 943d3526b..914846d78 100755 --- a/scripts/delete-owned-cloud-sql-instance.sh +++ b/scripts/delete-owned-cloud-sql-instance.sh @@ -117,7 +117,14 @@ if ! printf '%s' "$PROJECT" | grep -Eq '^[a-z][a-z0-9-]{5,29}$'; then fi OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)" -if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]; then +# On its own line, not inlined into the `if` test: a jq failure inside +# `"$(jq ...)"` there would substitute an empty string, and `[[ "" -eq 0 ]]` +# evaluates true, treating a broken `terraform output` (non-JSON on stdout) +# the same as zero outputs -- a silent "already destroyed" exit. Assigned to +# its own variable, the failing command substitution's exit status is the +# assignment statement's own exit status, so `set -e` catches it here. +OUTPUTS_LENGTH="$(jq -r 'length' <<<"$OUTPUTS_JSON")" +if [[ "$OUTPUTS_LENGTH" -eq 0 ]]; then echo "State has no outputs; the stack is already destroyed and there is no Cloud SQL instance to delete." exit 0 fi diff --git a/scripts/disable-owned-rds-deletion-protection.sh b/scripts/disable-owned-rds-deletion-protection.sh index 2ee1daf99..c5463ba0c 100755 --- a/scripts/disable-owned-rds-deletion-protection.sh +++ b/scripts/disable-owned-rds-deletion-protection.sh @@ -45,7 +45,7 @@ # branch below is driven by the payload: # # state | -json | jq length | jq -er .value | handled as -# -------------------|---------------|-----------|---------------|------------ +# -------------------+---------------+-----------+---------------+------------ # no state file | {} | 0 | exit 1 | skip, exit 0 # state, no outputs | {} | 0 | exit 1 | skip, exit 0 # key absent | {...} w/o key | >=1 | exit 1 | exit 1, remedy @@ -97,7 +97,14 @@ if [[ ! -d "$STATE_DIR" ]]; then fi OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)" -if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]; then +# On its own line, not inlined into the `if` test: a jq failure inside +# `"$(jq ...)"` there would substitute an empty string, and `[[ "" -eq 0 ]]` +# evaluates true, treating a broken `terraform output` (non-JSON on stdout) +# the same as zero outputs -- a silent "already destroyed" exit. Assigned to +# its own variable, the failing command substitution's exit status is the +# assignment statement's own exit status, so `set -e` catches it here. +OUTPUTS_LENGTH="$(jq -r 'length' <<<"$OUTPUTS_JSON")" +if [[ "$OUTPUTS_LENGTH" -eq 0 ]]; then echo "State has no outputs; the stack is already destroyed and there is no RDS instance to unprotect." exit 0 fi diff --git a/scripts/force-delete-owned-ecr-repo.sh b/scripts/force-delete-owned-ecr-repo.sh index 2148cf2d7..e13e13c08 100755 --- a/scripts/force-delete-owned-ecr-repo.sh +++ b/scripts/force-delete-owned-ecr-repo.sh @@ -67,7 +67,14 @@ if [[ ! -d "$STATE_DIR" ]]; then fi OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)" -if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]; then +# On its own line, not inlined into the `if` test: a jq failure inside +# `"$(jq ...)"` there would substitute an empty string, and `[[ "" -eq 0 ]]` +# evaluates true, treating a broken `terraform output` (non-JSON on stdout) +# the same as zero outputs -- a silent "already destroyed" exit. Assigned to +# its own variable, the failing command substitution's exit status is the +# assignment statement's own exit status, so `set -e` catches it here. +OUTPUTS_LENGTH="$(jq -r 'length' <<<"$OUTPUTS_JSON")" +if [[ "$OUTPUTS_LENGTH" -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 diff --git a/scripts/test-cloud-sql-delete-scope.sh b/scripts/test-cloud-sql-delete-scope.sh index 2ce30276a..5b7523fb8 100755 --- a/scripts/test-cloud-sql-delete-scope.sh +++ b/scripts/test-cloud-sql-delete-scope.sh @@ -410,9 +410,13 @@ assert_nothing_swallowed "$DELETE_SCRIPT" # `terraform` and `gcloud` are stubbed on PATH, and every invocation of either # is logged to the SAME file, so the ordering assertion (state rm after # delete) can be made from one call log rather than two that would have to be -# interleaved by wall-clock time. The gcloud stub exits 99 if any argument -# starts with `--filter`, so a filter reintroduced into the listing call fails -# as BEHAVIOUR, not only as text. +# interleaved by wall-clock time. The gcloud stub's `list` branch is an +# ALLOWLIST of the exact argument vector the script is supposed to send, not a +# denylist of `--filter`: a denylist of one forbidden flag still lets +# `--limit=1`, `--page-size`, `--sort-by`, `--uri` or `--flags-file` through, +# and real gcloud 456 honours every one of those the same way `head -1` used +# to, so a filter OR any of those reintroduced into the listing call fails as +# BEHAVIOUR, not only as text. STUB_DIR="$(mktemp -d)" STUB_STATE="$(mktemp -d)" CALLS="$(mktemp)" @@ -439,14 +443,17 @@ cat >"${STUB_DIR}/gcloud" <<'EOF' echo "gcloud $*" >>"$CALLS" case "$3" in list) - for arg in "$@"; do - case "$arg" in - --filter*) - echo "stub: --filter is not the guard" >&2 - exit 99 - ;; - esac - done + # Allowlist, not a denylist: a denylist of `--filter` alone still lets + # `--limit=1`, `--page-size`, `--sort-by`, `--uri` or `--flags-file` + # through, and real gcloud 456 honours every one of those, so any of them + # walks straight past the selector the same way `head -1` used to. + # Require the exact argument vector this script is supposed to send and + # nothing else. + if [[ "$#" -ne 5 || "$1" != "sql" || "$2" != "instances" || "$3" != "list" || \ + "$4" != --project=* || "$5" != "--format=value(name)" ]]; then + echo "stub: unexpected 'gcloud sql instances list' invocation: $*" >&2 + exit 99 + fi [[ "${LIST_FAILS:-0}" == "1" ]] && { echo "list failed" >&2; exit 255; } printf '%s\n' "$LISTING" ;; @@ -501,6 +508,19 @@ assert_behaviour "behaviour: a state with no outputs exits 0 without calling gcl "$([[ "$STUB_EXIT" -eq 0 && "$(count_calls '^gcloud')" -eq 0 && "$(count_calls 'state rm')" -eq 0 ]] && echo 0 || echo 1)" \ "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" +# `jq -r 'length' <<<"$OUTPUTS_JSON"` fails on non-JSON, and inlined into +# `if [[ "$(jq ...)" -eq 0 ]]` a failure there substitutes an empty string, +# which `-eq 0` accepts as true -- a broken `terraform output` (a warning +# printed to stdout ahead of the JSON, say) would then read as "no outputs, +# already destroyed" and exit 0 without ever reaching gcloud. Asserted as +# behaviour: a `terraform output -json` that prints garbage must fail the +# step loudly, not silently skip the deletion. +export TF_OUTPUT_JSON='not valid json' +run_script "$STUB_STATE" "$PROJECT_OK" +assert_behaviour "behaviour: terraform output printing non-JSON fails loudly without calling gcloud" \ + "$([[ "$STUB_EXIT" -ne 0 && "$(count_calls '^gcloud')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$CALLS")" + # A state that predates the output. Distinct from the empty case below, # because the remedies differ: this one wants an apply. export TF_OUTPUT_JSON='{"network_name":{"value":"cudly-staging-vpc"}}'