sec(ci): delete the GCP staging VPC peering before removing it from state (#330)

.github/workflows/ci.yml

@@ -1691,6 +1691,35 @@ jobs:
       - name: Run Cloud SQL scope self-tests
         run: bash scripts/test-cloud-sql-delete-scope.sh
 
+  # Assert that cleanup-staging.yml's destroy-gcp job deletes the Service
+  # Networking peering on the network its state names (the `network_name`
+  # output, not a hardcoded `cudly-staging-vpc`), and removes the peering from
+  # state only after that delete succeeded, with nothing swallowed (#330). The
+  # step it replaces removed the module-qualified address from state first and
+  # swallowed the delete's failure, which left a live peering no state
+  # tracked. The suite runs scripts/delete-owned-vpc-peering.sh end to end
+  # against stubbed terraform and gcloud, asserts the step order inside
+  # destroy-gcp (Cloud SQL, then peering, then destroy), and sweeps every
+  # workflow plus a recursive glob of scripts/ so the script stays the only
+  # place that runs `gcloud services vpc-peerings delete`.
+  # Fast (shell only), so it always runs.
+  vpc-peering-delete-scope:
+    name: VPC peering delete 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 VPC peering scope self-tests
+        run: bash scripts/test-vpc-peering-delete-scope.sh
+
   # Assert that every job applying a `compute_platform` writes the Terraform
   # state namespace that platform owns: `compute_platform=lambda` into
   # github-<env>/, `compute_platform=fargate` into github-fargate-<env>/. The
@@ -1870,6 +1899,7 @@ jobs:
       - ecr-delete-selection
       - rds-deletion-protection-scope
       - cloud-sql-delete-scope
+      - vpc-peering-delete-scope
       - aws-tfstate-platform-key
       - tfstate-concurrency-group
       - rollback-ecr-registry

.github/workflows/cleanup-staging.yml

@@ -405,14 +405,32 @@ jobs:
           # per the zero-expression-interpolation-in-run-blocks rule from #1641.
           PROJECT="$GCP_PROJECT_ID"
           ./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 \
-            --network=cudly-staging-vpc \
-            --project="$PROJECT" --quiet 2>/dev/null || echo "VPC peering already gone or not found"
+
+      # After the Cloud SQL step (the peering cannot go while the instance still
+      # uses it) and before `terraform destroy`. Deletes the peering on the
+      # network named by this state's `network_name` output, and removes it
+      # from state only after that delete succeeds. The Cloud SQL step used to do
+      # this inline: state rm first, delete on a hardcoded `cudly-staging-vpc`,
+      # and the delete's failure swallowed, which left a live peering untracked
+      # (#330). Rationale: the script's header.
+      #
+      # continue-on-error so a failed delete does not skip `Terraform Destroy`:
+      # the likely failure is GCP still releasing the peering just after the
+      # Cloud SQL delete, and skipping destroy would leave the whole staging
+      # stack running (and billing) instead of only the network. The script
+      # leaves the address in state when the delete fails, so the destroy below
+      # still tracks the peering. The failure is NOT swallowed: the last step
+      # fails the job when this step's outcome is failure.
+      - name: Delete the Service Networking peering this state owns
+        id: delete-peering
+        continue-on-error: true
+        env:
+          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"
+          ./scripts/delete-owned-vpc-peering.sh terraform/environments/gcp "$PROJECT"
 
       - name: Terraform Destroy
         env:
@@ -423,3 +441,13 @@ jobs:
           terraform destroy \
             -var-file="github-staging.tfvars" \
             -auto-approve
+
+      # Surfaces the peering step's failure, which continue-on-error turned
+      # into a passing step so that `Terraform Destroy` above still ran.
+      # !cancelled() rather than the implicit success(), so it still reports
+      # when the destroy failed too.
+      - name: Fail if the peering delete failed
+        if: ${{ !cancelled() && steps.delete-peering.outcome == 'failure' }}
+        run: |
+          echo "::error::Deleting the Service Networking peering failed; its address is still in Terraform state. Terraform Destroy ran anyway. See the 'Delete the Service Networking peering this state owns' step log, then re-run this job."
+          exit 1

scripts/delete-owned-vpc-peering.sh

@@ -0,0 +1,146 @@
+#!/usr/bin/env bash
+# delete-owned-vpc-peering.sh
+#
+# Deletes the Service Networking peering a Terraform state owns, then removes
+# it from that state. Nothing is removed from state unless the delete
+# succeeded.
+#
+# Usage: delete-owned-vpc-peering.sh TERRAFORM_STATE_DIR GCP_PROJECT_ID
+#
+# Runs before `terraform destroy` on the GCP staging state, after
+# delete-owned-cloud-sql-instance.sh: the peering cannot be removed while the
+# Cloud SQL instance still uses it, and Terraform's own destroy of
+# google_service_networking_connection fails on that "producer services still
+# using this connection" ordering, so the workflow deletes it directly.
+#
+# The step this replaces (#330) ran
+#
+#   terraform state rm module.networking.google_service_networking_connection.private_vpc_connection 2>/dev/null || true
+#   gcloud services vpc-peerings delete ... --network=cudly-staging-vpc ... 2>/dev/null || echo "..."
+#
+# which had two defects:
+#
+#   1. State was removed BEFORE the delete, and the delete's failure was
+#      swallowed. Unlike the root-level Cloud SQL addresses #1971 corrected,
+#      this address is module-qualified, so the state rm was real: a failed
+#      delete left a live peering that no state tracked any more, and the
+#      following `terraform destroy` then failed on google_compute_network
+#      with nothing left to reconcile against. Here the delete runs first, its
+#      exit status is not masked, and `terraform state rm` runs only after it
+#      returned 0.
+#   2. The network was the literal "cudly-staging-vpc" rather than the name the
+#      state publishes. A renamed network would have turned the delete into a
+#      "not found" that the `|| echo` reported as "already gone", while the
+#      state rm still ran. The network is now read from the state's
+#      `network_name` output (terraform/environments/gcp/outputs.tf ->
+#      module.networking.network_name -> google_compute_network.main.name),
+#      which is the network google_service_networking_connection
+#      .private_vpc_connection is attached to
+#      (terraform/modules/networking/gcp/main.tf).
+#
+# Whether there is anything to delete is decided by the state, not by gcloud:
+# the peering is deleted only while this state still tracks it. That makes a
+# re-run after a completed cleanup a no-op (the address is already gone from
+# state), and a re-run after a FAILED delete a retry (the address is still
+# there). The address is compared by exact equality against an unfiltered
+# `terraform state list`, by scripts/select-owned-name.sh, rather than with
+# `terraform state list ADDRESS`: that form exits 1 both when the address is
+# absent and when the state cannot be read (measured on Terraform 1.15.8), so
+# the two cannot be told apart without swallowing an error.
+#
+# `gcloud services vpc-peerings delete` waits for the operation unless `--async`
+# is passed, so its exit status IS the deletion confirmation, the same way
+# `gcloud sql instances delete` is in the Cloud SQL script. No `--async` here.
+#
+# Nothing is swallowed: no `2>/dev/null`, no `|| true`, no `|| echo`.
+#
+# Exit codes:
+#   0  completed, including the "peering no longer in state" case, which is
+#      what a re-run after a completed cleanup looks like
+#   1  the peering is in state but the network it belongs to cannot be read
+#      from a usable `network_name` output (state is left untouched)
+#   2  usage error (wrong arity, a state directory that does not exist, or a
+#      malformed project id)
+#   *  anything terraform, jq or the selector fails with, unmasked; a failed
+#      delete exits with gcloud's own status and leaves state untouched
+
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+PEERING_ADDR="module.networking.google_service_networking_connection.private_vpc_connection"
+
+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
+
+# Same project id shape check as delete-owned-cloud-sql-instance.sh: a
+# whitespace-only or malformed value would otherwise reach `gcloud --project=`.
+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 VPC peering cleanup" >&2
+  exit 2
+fi
+
+# Unfiltered on purpose: the equality test is the selector's. A failed
+# `terraform state list` fails the pipeline under pipefail, and so the step.
+IN_STATE="$(terraform -chdir="$STATE_DIR" state list \
+  | "${SCRIPT_DIR}/select-owned-name.sh" "$PEERING_ADDR")"
+
+if [[ -z "$IN_STATE" ]]; then
+  echo "State no longer tracks ${PEERING_ADDR}; nothing to delete, state left untouched."
+  exit 0
+fi
+
+OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)"
+# On its own line so a non-JSON `terraform output` fails here under `set -e`,
+# loudly, instead of reading as an absent output below.
+OUTPUTS_TYPE="$(jq -r 'type' <<<"$OUTPUTS_JSON")"
+if [[ "$OUTPUTS_TYPE" != "object" ]]; then
+  echo "error: 'terraform output -json' on '${STATE_DIR}' returned a ${OUTPUTS_TYPE}, not an object" >&2
+  exit 1
+fi
+
+if ! NETWORK="$(jq -er '.network_name.value' <<<"$OUTPUTS_JSON")"; then
+  echo "error: state '${STATE_DIR}' tracks ${PEERING_ADDR}, but its" >&2
+  echo "       'network_name' output is absent or null, so the network the peering" >&2
+  echo "       belongs to cannot be identified. Refusing to fall back to a hardcoded" >&2
+  echo "       network name. State left untouched." >&2
+  exit 1
+fi
+
+# RFC 1035, which is what google_compute_network accepts. An empty or
+# whitespace-bearing value is a broken output, not a network.
+if ! printf '%s' "$NETWORK" | grep -Eq '^[a-z]([-a-z0-9]{0,61}[a-z0-9])?$'; then
+  echo "error: state '${STATE_DIR}' publishes 'network_name' as '${NETWORK}', which" >&2
+  echo "       is not a VPC network name. Inspect" >&2
+  echo "       'terraform -chdir=${STATE_DIR} output -json' before destroying anything." >&2
+  echo "       State left untouched." >&2
+  exit 1
+fi
+
+echo "Deleting the Service Networking peering on network '${NETWORK}'..."
+if gcloud services vpc-peerings delete \
+  --service=servicenetworking.googleapis.com \
+  --network="$NETWORK" \
+  --project="$PROJECT" --quiet; then
+  echo "Service Networking peering on network '${NETWORK}' deleted."
+else
+  rc=$?
+  echo "error: deleting the Service Networking peering on network '${NETWORK}' failed" >&2
+  echo "       (exit ${rc}); ${PEERING_ADDR} is left in state so a re-run retries it." >&2
+  echo "       If the peering is confirmed gone already, remove the address by hand:" >&2
+  echo "       terraform -chdir=${STATE_DIR} state rm ${PEERING_ADDR}" >&2
+  exit "$rc"
+fi
+
+# Only after the delete above returned 0.
+terraform -chdir="$STATE_DIR" state rm "$PEERING_ADDR"

scripts/lib/code-scan-awk.sh

@@ -3,9 +3,10 @@
 #
 # 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, 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.
+# test-rds-deletion-protection-scope.sh, test-aws-tfstate-platform-key.sh,
+# test-cloud-sql-delete-scope.sh and test-vpc-peering-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 +55,7 @@
 # build_swept_scripts SCRIPTS_DIR
 #
 # Sets SWEPT_SCRIPTS to every `*.sh` anywhere under SCRIPTS_DIR, at any depth,
-# excluding the four guard suites themselves.
+# excluding the 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 +97,8 @@ 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 | test-cloud-sql-delete-scope.sh) continue ;;
+        test-aws-tfstate-platform-key.sh | test-cloud-sql-delete-scope.sh | \
+        test-vpc-peering-delete-scope.sh) continue ;;
     esac
     SWEPT_SCRIPTS+=("$candidate")
   done < <(find "$dir" -type f -name '*.sh' -print0 2>/dev/null | sort -z)