diff --git a/.github/workflows/deploy-aws-fargate.yml b/.github/workflows/deploy-aws-fargate.yml index 68935718..86a2c6b6 100644 --- a/.github/workflows/deploy-aws-fargate.yml +++ b/.github/workflows/deploy-aws-fargate.yml @@ -140,6 +140,7 @@ jobs: cancel-in-progress: false outputs: alb_url: ${{ steps.outputs.outputs.alb_url }} + app_url: ${{ steps.outputs.outputs.app_url }} service_name: ${{ steps.outputs.outputs.service_name }} # Set at job level, not workflow level: workflow-level `env:` is resolved # before this job's `environment:` is in scope, so an environment-scoped @@ -223,8 +224,13 @@ jobs: id: outputs run: | cd terraform/environments/aws - echo "alb_url=$(terraform output -raw fargate_alb_dns_name 2>/dev/null | grep -v '::' || echo "")" >> "$GITHUB_OUTPUT" - echo "service_name=$(terraform output -raw fargate_service_name 2>/dev/null | grep -v '::' || echo "")" >> "$GITHUB_OUTPUT" + { + echo "alb_url=$(terraform output -raw fargate_alb_dns_name 2>/dev/null | grep -v '::' || echo "")" + # The URL that actually serves the app (https on the api- + # alias when HTTPS is enabled), for the test-deployment gates (#647). + echo "app_url=$(terraform output -raw fargate_app_url 2>/dev/null | grep -v '::' || echo "")" + echo "service_name=$(terraform output -raw fargate_service_name 2>/dev/null | grep -v '::' || echo "")" + } >> "$GITHUB_OUTPUT" - name: Save deployment info env: @@ -265,69 +271,66 @@ jobs: contents: read steps: - - name: Get ALB URL + # Gate on the URL that serves the app, not the bare ALB DNS name: with + # HTTPS enabled the port 80 listener answers 301 on every path, so an + # http:// gate would never reach a task (#647). fargate_app_url is + # https on the api- alias then (the name the listener certificate + # covers), otherwise http on the ALB DNS name. + - name: Get app URL id: get-url + env: + APP_URL: ${{ needs.deploy.outputs.app_url }} run: | - ALB_URL="${{ needs.deploy.outputs.alb_url }}" - if [ -z "$ALB_URL" ]; then - echo "Failed to get ALB URL from deploy outputs" + if [ -z "$APP_URL" ]; then + echo "Failed to get the app URL (fargate_app_url) from deploy outputs" exit 1 fi - # Ensure URL has scheme and no trailing slash - case "$ALB_URL" in - http://*|https://*) ;; # already has scheme - *) ALB_URL="http://$ALB_URL" ;; + case "$APP_URL" in + http://*|https://*) ;; + *) echo "fargate_app_url has no scheme: $APP_URL"; exit 1 ;; esac - ALB_URL="${ALB_URL%/}" - echo "url=$ALB_URL" >> "$GITHUB_OUTPUT" + APP_URL="${APP_URL%/}" + echo "Gating on $APP_URL" + echo "url=$APP_URL" >> "$GITHUB_OUTPUT" - name: Wait for deployment run: | echo "Waiting 60 seconds for ECS service to stabilize..." sleep 60 - - name: Test health endpoint - run: | - URL="${{ steps.get-url.outputs.url }}" - echo "Testing health endpoint: $URL/health" - - for i in {1..10}; do - # /health reports but never triggers the lazy DB init on the HTTP - # server; any /api/ request does. /health below is the gate. - curl -s -o /dev/null --max-time 60 "$URL/api/auth/check-admin" || true - # /health returns HTTP 200 even when degraded; the body is the verdict. - if RESPONSE=$(curl -fsS --max-time 10 "$URL/health"); then - echo "Response: $RESPONSE" - if jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then - echo "Health check passed!" - exit 0 - fi - fi - echo "Attempt $i failed, retrying in 10 seconds..." - sleep 10 - done - - echo "Health check failed after 10 attempts" - exit 1 - + # Needs the repo for scripts/wait-for-healthy.sh, used by both gates below. + - name: Checkout code + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + sparse-checkout: scripts/wait-for-healthy.sh + + # Fast fail gate: the server initializes its database in the background + # at startup, so no warm-up request is needed to make progress (#488). + # /ready answers 503 while the task that answers is still initializing, + # which the script counts as a failed attempt (#647). 30 x 10s covers the + # 300s ECS health check grace period, so a slow first boot that ECS + # still tolerates does not fail this gate. + - name: Test readiness endpoint + env: + APP_URL: ${{ steps.get-url.outputs.url }} + HEALTH_REQUIRED_STREAK: "1" + HEALTH_MAX_ATTEMPTS: "30" + HEALTH_INTERVAL_SECONDS: "10" + run: bash scripts/wait-for-healthy.sh "$APP_URL" + + # Rollout race (#513): during a rolling deploy the ALB can route a probe + # to a different task than the previous one, so one not-ready response + # must not fail the step. The script requires HEALTH_REQUIRED_STREAK + # consecutive ready responses within HEALTH_MAX_ATTEMPTS rounds and + # fails with the last body otherwise. - name: Run smoke tests - run: | - URL="${{ steps.get-url.outputs.url }}" - - echo "Running basic smoke tests..." - - for i in {1..3}; do - if RESPONSE=$(curl -fsS --max-time 10 "$URL/health") && - jq -e '.status == "healthy"' > /dev/null <<< "$RESPONSE"; then - echo "Health check $i: passed" - else - echo "Health check $i: failed: ${RESPONSE:-no response}" - exit 1 - fi - sleep 2 - done - - echo "All smoke tests passed!" + env: + APP_URL: ${{ steps.get-url.outputs.url }} + HEALTH_REQUIRED_STREAK: "3" + HEALTH_MAX_ATTEMPTS: "30" + HEALTH_INTERVAL_SECONDS: "5" + run: bash scripts/wait-for-healthy.sh "$APP_URL" # Summary summary: diff --git a/scripts/test-wait-for-healthy.sh b/scripts/test-wait-for-healthy.sh index 183869aa..89d8d371 100755 --- a/scripts/test-wait-for-healthy.sh +++ b/scripts/test-wait-for-healthy.sh @@ -20,21 +20,35 @@ mkdir -p "$WORK/bin" # H = HTTP 200 healthy # D = HTTP 200 but degraded body: what /health returned pre-#488, which the # pre-fix loop caught only by parsing the body -# U = HTTP 503 with the degraded body: what /ready returns, and what -# --fail-with-body still prints +# U = HTTP 503 with the degraded body: what /ready returns +# M = HTTP 301 to https://app.example/ready with an empty body: what an +# http:// URL gets from an HTTPS-only ALB listener (#647) # R = connection refused +# Models the real curl flags the callers use: -o FILE, -w with %{http_code} and +# %{redirect_url}, -f/--fail (exit 22 on >= 400, no body) and --fail-with-body +# (exit 22 on >= 400, body still written). Without either, curl exits 0 for any +# HTTP status, including 3xx and 5xx. cat > "$WORK/bin/curl" <<'STUB' #!/usr/bin/env bash fwb=0 fail=0 +out="" +wfmt="" +prev="" for a in "$@"; do - url="$a" - [ "$a" = "--fail-with-body" ] && fwb=1 - # -f/--fail, alone or inside a short cluster such as -fsS. + case "$prev" in + -o) out="$a"; prev=""; continue ;; + -w) wfmt="$a"; prev=""; continue ;; + esac case "$a" in - --*) [ "$a" = "--fail" ] && fail=1 ;; + -o|-w) prev="$a"; continue ;; + --fail-with-body) fwb=1 ;; + --fail) fail=1 ;; + --*) ;; + # -f inside a short cluster such as -fsS. -*f*) fail=1 ;; esac + url="$a" done # Real curl (7.76+) rejects -f/--fail together with --fail-with-body. if [ "$fwb" = 1 ] && [ "$fail" = 1 ]; then @@ -51,15 +65,33 @@ tokens=($SCENARIO) i=$n ((i >= ${#tokens[@]})) && i=$((${#tokens[@]} - 1)) echo $((n + 1)) > "$STATE" +degraded='{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' +location="" case "${tokens[$i]}" in - H) echo '{"status":"healthy","checks":{}}' ;; - D) echo '{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' ;; - # Real curl only prints the body on a 503 when --fail-with-body was passed; - # modeling that keeps the failure-diagnostics coverage honest. - U) [ "$fwb" = 1 ] && echo '{"status":"degraded","checks":{"auth_store":{"status":"unhealthy","message":"auth store not initialized"}}}' - echo "curl: (22) The requested URL returned error: 503" >&2; exit 22 ;; - R) echo "curl: (7) Failed to connect" >&2; exit 7 ;; + H) code=200; body='{"status":"healthy","checks":{}}' ;; + D) code=200; body="$degraded" ;; + U) code=503; body="$degraded" ;; + M) code=301; body=""; location="https://app.example/ready" ;; + R) echo "curl: (7) Failed to connect" >&2 + [ -n "$wfmt" ] && printf '000 ' + exit 7 ;; esac +if ((code >= 400)) && [ "$fail" = 1 ]; then + echo "curl: (22) The requested URL returned error: $code" >&2 + exit 22 +fi +if [ -n "$body" ]; then + if [ -n "$out" ]; then echo "$body" > "$out"; else echo "$body"; fi +fi +if [ -n "$wfmt" ]; then + w="${wfmt//'%{http_code}'/$code}" + printf '%s' "${w//'%{redirect_url}'/$location}" +fi +if ((code >= 400)) && [ "$fwb" = 1 ]; then + echo "curl: (22) The requested URL returned error: $code" >&2 + exit 22 +fi +exit 0 STUB chmod +x "$WORK/bin/curl" @@ -94,6 +126,12 @@ run_case "connection refused forever fails with curl error" 1 "R" "Failed to con run_case "curl error mid-streak resets it (H R H H U, streak 3, must not pass)" 1 "H R H H U" "Failed to connect" +# #647: an http:// URL in front of an HTTPS-only listener answers 301 on every +# path. Counting that as ready made the gate pass without reaching the app. +run_case "301 redirect forever fails and names the redirect target" 1 "M" "HTTP 301 redirect to https://app.example/ready" +run_case "301 mid-streak resets it (H H M H H U, must not pass)" 1 "H H M H H U" "auth store not initialized" +run_case "503 failure keeps the status and the body" 1 "U" "HTTP 503: {\"status\":\"degraded\"" + # run_env_case LABEL EXPECTED_EXIT SCENARIO OUTPUT_SUBSTRING [ENV=VAL ...] # Runs the SUT with ONLY the given env overrides (plus a zero sleep interval), # so the script's own defaults (streak 3, 30 attempts) are exercised. @@ -166,12 +204,19 @@ run_case "rollout race H U H H H passes under the readiness gate" 0 "H U H H H" # flag combinations that the installed curl rejects. Skipped when python3 is # missing; curl older than 7.76 has no --fail-with-body and would fail here. if command -v python3 > /dev/null; then - # Two servers: one always answers 200, the other 503 with a degraded body. + # Three servers: one always answers 200, one 503 with a degraded body, and one + # 301 to https on every path (an HTTPS-only ALB's port 80 listener, #647). python3 - > "$WORK/port" <<'PY' & import http.server, sys, threading class Handler(http.server.BaseHTTPRequestHandler): def do_GET(self): + if self.server.mode == "redirect": + self.send_response(301) + self.send_header("Location", "https://app.example.invalid" + self.path) + self.send_header("Content-Length", "0") + self.end_headers() + return code = 200 if self.server.mode == "ok" else 503 body = b'{"status":"degraded","checks":{"database":{"status":"unhealthy"}}}' if code == 503 else b'{"status":"healthy","checks":{}}' self.send_response(code) @@ -182,11 +227,11 @@ class Handler(http.server.BaseHTTPRequestHandler): pass servers = [] -for mode in ("ok", "unready"): +for mode in ("ok", "unready", "redirect"): s = http.server.HTTPServer(("127.0.0.1", 0), Handler) s.mode = mode servers.append(s) -print(servers[0].server_port, servers[1].server_port, flush=True) +print(servers[0].server_port, servers[1].server_port, servers[2].server_port, flush=True) for s in servers[1:]: threading.Thread(target=s.serve_forever, daemon=True).start() servers[0].serve_forever() @@ -194,7 +239,7 @@ PY SRV_PID=$! trap 'kill "$SRV_PID" 2>/dev/null || true; wait "$SRV_PID" 2>/dev/null || true; rm -rf "$WORK"' EXIT for _ in {1..50}; do [[ -s "$WORK/port" ]] && break; sleep 0.1; done - read -r OK_PORT UNREADY_PORT < "$WORK/port" + read -r OK_PORT UNREADY_PORT REDIRECT_PORT < "$WORK/port" out=$(HEALTH_INTERVAL_SECONDS=0 HEALTH_MAX_ATTEMPTS=3 HEALTH_REQUIRED_STREAK=3 \ "$SUT" "http://127.0.0.1:${OK_PORT}" 2>&1) && real_ok=0 || real_ok=$? @@ -215,6 +260,18 @@ PY echo "FAIL: real curl against a 503 /ready (exit $real_503)"; echo "$out" ((fail++)) || true fi + + # The reviewer's repro on #1025: a server answering 301 on every path must + # NOT pass, and the failure must name where it redirected to. + out=$(HEALTH_INTERVAL_SECONDS=0 HEALTH_MAX_ATTEMPTS=3 HEALTH_REQUIRED_STREAK=3 \ + "$SUT" "http://127.0.0.1:${REDIRECT_PORT}" 2>&1) && real_301=0 || real_301=$? + if [[ "$real_301" -eq 1 && "$out" == *"HTTP 301 redirect to https://app.example.invalid/ready"* ]]; then + echo "PASS: real curl rejects a 301 /ready and names the redirect target" + ((pass++)) || true + else + echo "FAIL: real curl against a 301 /ready (exit $real_301)"; echo "$out" + ((fail++)) || true + fi fi echo "passed=$pass failed=$fail" diff --git a/scripts/wait-for-healthy.sh b/scripts/wait-for-healthy.sh index 05b8fbbe..890acb2e 100755 --- a/scripts/wait-for-healthy.sh +++ b/scripts/wait-for-healthy.sh @@ -1,13 +1,12 @@ #!/usr/bin/env bash # wait-for-healthy.sh URL # -# Smoke gate for a just-deployed service. Polls URL/ready until curl -# --fail-with-body accepts the response (any status below 400) -# HEALTH_REQUIRED_STREAK times in a row. A not-ready or failed response inside -# the polling budget resets the streak instead of failing, because during a -# rollout a probe can hit a replica that is still initializing. If the budget -# (HEALTH_MAX_ATTEMPTS) runs out before the streak is reached, exits 1 and -# prints the last response (or curl error). +# Smoke gate for a just-deployed service. Polls URL/ready until it answers a +# 2xx status HEALTH_REQUIRED_STREAK times in a row. A not-ready or failed +# response inside the polling budget resets the streak instead of failing, +# because during a rollout a probe can hit a replica that is still +# initializing. If the budget (HEALTH_MAX_ATTEMPTS) runs out before the streak +# is reached, exits 1 and prints the last response (or curl error). # # /ready is the traffic-admission endpoint: it answers 503 until the replica it # answers for has completed initialization, so the gate reflects the same @@ -15,9 +14,13 @@ # degraded and is a liveness signal only. No warm-up request is needed: the # server initializes its database in the background at startup. # -# Requires curl 7.76+ for --fail-with-body, so a 503 still prints the JSON -# check detail that explains which dependency is not ready. Do not add -f: -# curl rejects -f/--fail together with --fail-with-body. +# Only 2xx counts as ready. A 3xx is NOT ready and is logged with its redirect +# target: an http:// URL in front of an HTTPS-only listener answers 301 on every +# path, and counting that as ready would make the gate pass without ever +# reaching the app (#647). Redirects are deliberately not followed; point the +# gate at the URL that serves the app instead. A non-2xx body is kept so a 503 +# still prints the JSON check detail that explains which dependency is not +# ready. # # Env (defaults suit the deploy workflows; tests shrink them): # HEALTH_REQUIRED_STREAK consecutive ready responses needed (3) @@ -47,22 +50,43 @@ fi streak=0 last="no response" +tmp="$(mktemp -d)" +trap 'rm -rf "$tmp"' EXIT + for ((attempt = 1; attempt <= MAX_ATTEMPTS; attempt++)); do - # /ready answers 503 until the replica has completed initialization; - # --fail-with-body keeps the JSON check detail so a failure says WHICH - # dependency is not ready. - if body=$(curl -sS --fail-with-body --max-time 10 "${URL}/ready" 2>&1); then - streak=$((streak + 1)) - echo "Readiness check ${attempt}: ready (${streak}/${REQUIRED_STREAK})" - if ((streak >= REQUIRED_STREAK)); then - echo "Service is ready." - exit 0 - fi + # curl exits 0 for any HTTP status here (no -f), so the status is judged + # below. stderr goes to its own file: curl prints its -w output even when the + # request fails, and mixing the two would garble the error message. + : > "$tmp/body" + if meta=$(curl -sS --max-time 10 -o "$tmp/body" -w '%{http_code} %{redirect_url}' "${URL}/ready" 2> "$tmp/err"); then + code="${meta%% *}" + redirect="${meta#* }" + [[ "$redirect" == "$meta" ]] && redirect="" + body="$(cat "$tmp/body")" + case "$code" in + 2??) + streak=$((streak + 1)) + echo "Readiness check ${attempt}: ready (${streak}/${REQUIRED_STREAK})" + if ((streak >= REQUIRED_STREAK)); then + echo "Service is ready." + exit 0 + fi + sleep "$INTERVAL" + continue + ;; + 3??) + last="HTTP ${code} redirect to ${redirect:-}; redirects are not followed, so point the gate at the URL that serves the app" + ;; + *) + last="HTTP ${code}: ${body:-}" + ;; + esac else - last="${body:-no response}" - streak=0 - echo "Readiness check ${attempt}: not ready yet, streak reset: ${last}" + err="$(cat "$tmp/err")" + last="${err:-no response}" fi + streak=0 + echo "Readiness check ${attempt}: not ready yet, streak reset: ${last}" sleep "$INTERVAL" done diff --git a/terraform/environments/aws/compute.tf b/terraform/environments/aws/compute.tf index e87f573f..b69fac13 100644 --- a/terraform/environments/aws/compute.tf +++ b/terraform/environments/aws/compute.tf @@ -197,8 +197,10 @@ module "compute_fargate" { : var.fargate_certificate_arn ) - # Health checks - health_check_path = "/health" + # Health checks. The ALB target group check admits traffic, so it uses the + # readiness endpoint: /ready answers 503 until the task has initialized + # (#647). The container liveness check stays on /health inside the module. + health_check_path = "/ready" # CORS allowed_origins = var.lambda_allowed_origins diff --git a/terraform/environments/aws/outputs.tf b/terraform/environments/aws/outputs.tf index 5908c2bd..b2b1d22b 100644 --- a/terraform/environments/aws/outputs.tf +++ b/terraform/environments/aws/outputs.tf @@ -128,6 +128,21 @@ output "fargate_api_url" { value = var.compute_platform == "fargate" ? module.compute_fargate[0].api_url : null } +# The URL that actually serves the app through the ALB, for the deploy smoke +# gate. With HTTPS enabled the port 80 listener only answers 301 to https, so +# the http:// ALB URL never reaches the app (#647); and https:// +# fails TLS verification because the listener's certificate is the wildcard for +# subdomain_zone_name. The api- alias in dns_records.tf is covered by that +# wildcard and points at the same ALB. +output "fargate_app_url" { + description = "URL that serves the Fargate app through the ALB: https on the api- alias when HTTPS is enabled, otherwise http on the ALB DNS name" + value = var.compute_platform != "fargate" ? null : ( + module.compute_fargate[0].api_https_url != null && length(aws_route53_record.fargate_alb_alias) > 0 + ? "https://${aws_route53_record.fargate_alb_alias[0].fqdn}" + : module.compute_fargate[0].api_url + ) +} + output "fargate_log_group_name" { description = "Fargate CloudWatch log group name" value = var.compute_platform == "fargate" ? module.compute_fargate[0].log_group_name : null diff --git a/terraform/modules/compute/aws/fargate/main.tf b/terraform/modules/compute/aws/fargate/main.tf index 2898d6dc..96778856 100644 --- a/terraform/modules/compute/aws/fargate/main.tf +++ b/terraform/modules/compute/aws/fargate/main.tf @@ -467,6 +467,10 @@ resource "aws_lb_target_group" "main" { vpc_id = var.vpc_id target_type = "ip" + # Traffic admission: var.health_check_path defaults to /ready, which answers + # 503 until this task has finished database initialization, so the ALB never + # routes to a cold task (#647). /health answers 200 even while degraded and + # stays the container liveness check below. health_check { enabled = true path = var.health_check_path @@ -670,6 +674,9 @@ resource "aws_ecs_task_definition" "main" { } } + # Liveness: a failing container health check makes ECS replace the task, + # so it must stay on /health (200 whenever the process is serving), not + # /ready (503 during initialization or a database outage). healthCheck = { command = ["CMD-SHELL", "curl -f http://localhost:8080/health || exit 1"] interval = 30 @@ -715,6 +722,12 @@ resource "aws_ecs_service" "main" { enable_execute_command = var.enable_execute_command + # ECS replaces a task its target group reports unhealthy. The ALB check is + # readiness (/ready), so without a grace period a task still connecting to + # the database or running migrations would be killed after + # unhealthy_threshold failed checks and never finish initializing. + health_check_grace_period_seconds = var.health_check_grace_period_seconds + deployment_maximum_percent = 200 deployment_minimum_healthy_percent = 100 diff --git a/terraform/modules/compute/aws/fargate/variables.tf b/terraform/modules/compute/aws/fargate/variables.tf index 643a4e30..d4486fa7 100644 --- a/terraform/modules/compute/aws/fargate/variables.tf +++ b/terraform/modules/compute/aws/fargate/variables.tf @@ -143,9 +143,20 @@ variable "certificate_arn" { } variable "health_check_path" { - description = "Health check path" + description = "ALB target group health check path. This check admits traffic, so it must be the readiness endpoint (/ready answers 503 until the task has finished initializing, #647); /health answers 200 even while degraded and is liveness only." type = string - default = "/health" + default = "/ready" +} + +variable "health_check_grace_period_seconds" { + description = "Seconds ECS ignores failing ALB health checks on a new task before replacing it. The ALB keeps an unready task out of rotation either way; this only stops ECS from killing a task whose database initialization (connect retries plus migrations) is still running." + type = number + default = 300 + + validation { + condition = var.health_check_grace_period_seconds >= 0 && var.health_check_grace_period_seconds <= 2147483647 && floor(var.health_check_grace_period_seconds) == var.health_check_grace_period_seconds + error_message = "health_check_grace_period_seconds must be a whole number between 0 and 2147483647." + } } variable "health_check_interval" { @@ -167,9 +178,9 @@ variable "healthy_threshold" { } variable "unhealthy_threshold" { - description = "Number of consecutive failed health checks" + description = "Consecutive failed ALB health checks before a target is marked unhealthy. The check is readiness (/ready), which also answers 503 while the database is unreachable, and ECS replaces targets the ALB marks unhealthy, so this is set high enough that a short database blip or a Multi-AZ failover does not cycle every task: with the default 30s interval, 6 failures is about 180s of continuous 503 before replacement starts." type = number - default = 3 + default = 6 } variable "log_retention_days" { diff --git a/terraform/modules/compute/aws/fargate_readiness_test.go b/terraform/modules/compute/aws/fargate_readiness_test.go new file mode 100644 index 00000000..33a846d3 --- /dev/null +++ b/terraform/modules/compute/aws/fargate_readiness_test.go @@ -0,0 +1,245 @@ +// Regression tests for issue #647: the Fargate ALB target group health check +// gated traffic on /health, which answers HTTP 200 even while the task is still +// initializing (degraded), so the ALB routed production traffic to cold tasks. +// PR #646 (#488) added /ready (503 until initialization completes) and moved +// every Azure traffic-admission check to it; these tests pin the AWS side of +// that contract: +// +// - the ALB target group check (traffic admission) uses /ready and only +// accepts HTTP 200; +// - the ECS container health check (liveness: failing it replaces the task) +// stays on /health; +// - the ECS service has a health check grace period, because ECS also +// replaces tasks the target group reports unhealthy, and a task still +// connecting to the database would otherwise be killed before it is ready; +// - the ALB tolerates a short database blip before marking targets +// unhealthy, since ECS replaces them once it does; +// - the deploy smoke gate polls /ready via scripts/wait-for-healthy.sh with +// no warm-up request, like deploy-azure.yml, on the URL that serves the +// app (an http:// ALB URL only gets a 301 when HTTPS is enabled). +package aws_test + +import ( + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// minHealthCheckGracePeriod is the floor for the ECS grace period default. The +// database connect retry budget alone is ~135s (internal/database +// connection.go), before migrations run. +const minHealthCheckGracePeriod = 180 + +// minTimeToUnhealthy is the floor, in seconds, for unhealthy_threshold x +// interval: how long /ready must keep failing before the ALB marks a target +// unhealthy and ECS replaces it. An RDS Multi-AZ failover typically takes +// 60-120s, so anything below this cycles every task on a routine failover. +const minTimeToUnhealthy = 150 + +// varDefault returns the integer default of a variable in variables.tf. +func varDefault(t *testing.T, vars, name string) int { + t.Helper() + v := hclBlock(vars, `variable\s+"`+name+`"`) + m := regexp.MustCompile(`\bdefault\s*=\s*(\d+)`).FindStringSubmatch(v) + if m == nil { + t.Fatalf(`variable %q must exist with a numeric default`, name) + } + n, _ := strconv.Atoi(m[1]) + return n +} + +// hclBlock returns the brace-balanced body of the first block whose header +// matches header (a regexp ending just before the opening brace), or "" when +// there is none. Braces inside "${...}" interpolations are balanced, so plain +// counting is enough for the files under test. +func hclBlock(content, header string) string { + loc := regexp.MustCompile(header + `\s*\{`).FindStringIndex(content) + if loc == nil { + return "" + } + depth := 0 + for i := loc[1] - 1; i < len(content); i++ { + switch content[i] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return content[loc[1]:i] + } + } + } + return "" +} + +func fargateFile(t *testing.T, name string) string { + t.Helper() + return stripCommentLines(readFile(t, filepath.Join("fargate", name))) +} + +func TestFargateALBHealthCheckUsesReadiness(t *testing.T) { + main := fargateFile(t, "main.tf") + tg := hclBlock(main, `resource\s+"aws_lb_target_group"\s+"main"`) + if tg == "" { + t.Fatal(`fargate/main.tf has no resource "aws_lb_target_group" "main"`) + } + hc := hclBlock(tg, `\bhealth_check`) + if hc == "" { + t.Fatal("aws_lb_target_group.main has no health_check block") + } + if !regexp.MustCompile(`\bpath\s*=\s*var\.health_check_path\b`).MatchString(hc) { + t.Error("aws_lb_target_group.main health_check.path must be var.health_check_path") + } + if !regexp.MustCompile(`\bmatcher\s*=\s*"200"`).MatchString(hc) { + t.Error(`aws_lb_target_group.main health_check.matcher must be "200": /ready signals not-ready with 503, and a wider matcher would admit it`) + } + + vars := fargateFile(t, "variables.tf") + v := hclBlock(vars, `variable\s+"health_check_path"`) + if !regexp.MustCompile(`\bdefault\s*=\s*"/ready"`).MatchString(v) { + t.Error(`variable "health_check_path" must default to "/ready" (#647): the ALB check admits traffic, and /health answers 200 while the task is still initializing`) + } + + env := stripCommentLines(readFile(t, filepath.Join(repoRoot(t), "terraform", "environments", "aws", "compute.tf"))) + mod := hclBlock(env, `module\s+"compute_fargate"`) + if mod == "" { + t.Fatal(`terraform/environments/aws/compute.tf has no module "compute_fargate"`) + } + if m := regexp.MustCompile(`\bhealth_check_path\s*=\s*"([^"]*)"`).FindStringSubmatch(mod); m != nil && m[1] != "/ready" { + t.Errorf(`module "compute_fargate" overrides health_check_path with %q; the ALB traffic-admission check must use "/ready" (#647)`, m[1]) + } +} + +func TestFargateContainerHealthCheckStaysOnLiveness(t *testing.T) { + main := fargateFile(t, "main.tf") + td := hclBlock(main, `resource\s+"aws_ecs_task_definition"\s+"main"`) + hc := hclBlock(td, `\bhealthCheck\s*=`) + if hc == "" { + t.Fatal("aws_ecs_task_definition.main has no container healthCheck") + } + if !strings.Contains(hc, "localhost:8080/health ") { + t.Error("the ECS container healthCheck must probe /health: it is liveness (failing it replaces the task)") + } + if strings.Contains(hc, "/ready") { + t.Error("the ECS container healthCheck must not probe /ready: a database outage would make ECS replace every task") + } +} + +func TestFargateServiceHasHealthCheckGracePeriod(t *testing.T) { + main := fargateFile(t, "main.tf") + svc := hclBlock(main, `resource\s+"aws_ecs_service"\s+"main"`) + if svc == "" { + t.Fatal(`fargate/main.tf has no resource "aws_ecs_service" "main"`) + } + if !regexp.MustCompile(`\bhealth_check_grace_period_seconds\s*=\s*var\.health_check_grace_period_seconds\b`).MatchString(svc) { + t.Error("aws_ecs_service.main must set health_check_grace_period_seconds = var.health_check_grace_period_seconds, or ECS replaces tasks that fail /ready while still initializing") + } + + if n := varDefault(t, fargateFile(t, "variables.tf"), "health_check_grace_period_seconds"); n < minHealthCheckGracePeriod { + t.Errorf("health_check_grace_period_seconds defaults to %d; it must be at least %d to cover database initialization", n, minHealthCheckGracePeriod) + } +} + +func TestFargateALBToleratesShortDatabaseBlip(t *testing.T) { + tg := hclBlock(fargateFile(t, "main.tf"), `resource\s+"aws_lb_target_group"\s+"main"`) + hc := hclBlock(tg, `\bhealth_check`) + for _, wire := range []string{`unhealthy_threshold\s*=\s*var\.unhealthy_threshold\b`, `interval\s*=\s*var\.health_check_interval\b`} { + if !regexp.MustCompile(`\b` + wire).MatchString(hc) { + t.Errorf("aws_lb_target_group.main health_check must set %s", wire) + } + } + + vars := fargateFile(t, "variables.tf") + threshold, interval := varDefault(t, vars, "unhealthy_threshold"), varDefault(t, vars, "health_check_interval") + + env := hclBlock(stripCommentLines(readFile(t, filepath.Join(repoRoot(t), "terraform", "environments", "aws", "compute.tf"))), `module\s+"compute_fargate"`) + for name, val := range map[string]*int{"unhealthy_threshold": &threshold, "health_check_interval": &interval} { + if m := regexp.MustCompile(`\b` + name + `\s*=\s*(\d+)`).FindStringSubmatch(env); m != nil { + *val, _ = strconv.Atoi(m[1]) + } + } + if threshold*interval < minTimeToUnhealthy { + t.Errorf("unhealthy_threshold (%d) x health_check_interval (%ds) = %ds of failing /ready before ECS replaces a task; it must be at least %ds so a short database blip or Multi-AZ failover does not cycle every task", threshold, interval, threshold*interval, minTimeToUnhealthy) + } + if threshold > 10 { + t.Errorf("unhealthy_threshold %d exceeds the ALB maximum of 10", threshold) + } +} + +func TestFargateDeploySmokeGateUsesReadiness(t *testing.T) { + wf := stripCommentLines(readFile(t, filepath.Join(repoRoot(t), ".github", "workflows", "deploy-aws-fargate.yml"))) + idx := strings.Index(wf, "\n test-deployment:") + if idx < 0 { + t.Fatal("deploy-aws-fargate.yml has no test-deployment job") + } + job := wf[idx+1:] + if end := regexp.MustCompile(`\n [A-Za-z0-9_-]+:\s*\n`).FindStringIndex(job[1:]); end != nil { + job = job[:end[0]+1] + } + + if n := strings.Count(job, `bash scripts/wait-for-healthy.sh "$APP_URL"`); n < 2 { + t.Errorf("test-deployment must gate on scripts/wait-for-healthy.sh for both the readiness and smoke steps (found %d), like deploy-azure.yml", n) + } + if !strings.Contains(job, "sparse-checkout: scripts/wait-for-healthy.sh") { + t.Error("test-deployment must check out scripts/wait-for-healthy.sh before running it") + } + if strings.Contains(job, "/api/auth/check-admin") { + t.Error("test-deployment must not send a warm-up request: the server initializes in the background (#488), and a warm-up only initializes whichever task the ALB picked") + } + if regexp.MustCompile(`/health\b`).MatchString(job) { + t.Error("test-deployment must not gate on /health: it answers 200 even while degraded; use /ready via scripts/wait-for-healthy.sh") + } + + // With HTTPS enabled the port 80 listener answers 301 on every path, so a + // gate on the http:// ALB URL never reaches a task. + if !strings.Contains(job, "needs.deploy.outputs.app_url") || strings.Contains(job, "needs.deploy.outputs.alb_url") { + t.Error("test-deployment must gate on needs.deploy.outputs.app_url (fargate_app_url), not the bare ALB DNS name: with HTTPS enabled http:// only answers 301") + } + if strings.Contains(job, `"http://$`) { + t.Error("test-deployment must not prepend http:// to the gate URL; fargate_app_url carries the scheme that serves the app") + } + if !regexp.MustCompile(`app_url=\$\(terraform output -raw fargate_app_url\b`).MatchString(wf) || + !regexp.MustCompile(`\bapp_url:\s*\$\{\{\s*steps\.outputs\.outputs\.app_url\s*\}\}`).MatchString(wf) { + t.Error("the deploy job must export app_url from `terraform output -raw fargate_app_url`") + } + + // The fast gate's budget must cover the ECS grace period, or a slow first + // boot that ECS still tolerates fails the deploy. + fastIdx := strings.Index(job, "name: Test readiness endpoint") + if fastIdx < 0 { + t.Fatal("test-deployment has no Test readiness endpoint step") + } + fast := job[fastIdx:] + attempts := regexp.MustCompile(`HEALTH_MAX_ATTEMPTS:\s*"(\d+)"`).FindStringSubmatch(fast) + interval := regexp.MustCompile(`HEALTH_INTERVAL_SECONDS:\s*"(\d+)"`).FindStringSubmatch(fast) + if attempts == nil || interval == nil { + t.Fatal("Test readiness endpoint must set HEALTH_MAX_ATTEMPTS and HEALTH_INTERVAL_SECONDS") + } + a, _ := strconv.Atoi(attempts[1]) + i, _ := strconv.Atoi(interval[1]) + grace := varDefault(t, fargateFile(t, "variables.tf"), "health_check_grace_period_seconds") + if a*i < grace { + t.Errorf("Test readiness endpoint polls for %d x %ds = %ds, shorter than the %ds ECS grace period", a, i, a*i, grace) + } +} + +func TestFargateAppURLOutputFollowsListener(t *testing.T) { + out := stripCommentLines(readFile(t, filepath.Join(repoRoot(t), "terraform", "environments", "aws", "outputs.tf"))) + o := hclBlock(out, `output\s+"fargate_app_url"`) + if o == "" { + t.Fatal(`terraform/environments/aws/outputs.tf has no output "fargate_app_url"`) + } + // https only when the module enabled its HTTPS listener, and on the alias the + // wildcard certificate covers (https:// fails TLS verification). + if !regexp.MustCompile(`module\.compute_fargate\[0\]\.api_https_url\s*!=\s*null`).MatchString(o) { + t.Error("fargate_app_url must switch to https exactly when the module's HTTPS listener exists (api_https_url != null)") + } + if !strings.Contains(o, `"https://${aws_route53_record.fargate_alb_alias[0].fqdn}"`) { + t.Error("fargate_app_url must use https on the api- alias (fargate_alb_alias), the name the listener certificate covers") + } + if !strings.Contains(o, "module.compute_fargate[0].api_url") { + t.Error("fargate_app_url must fall back to the module's http api_url when HTTPS is disabled") + } +}