diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f4576f71..f65e85cfd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -580,6 +580,17 @@ jobs: terraform -chdir="$d" test done + # The post-deploy /health gate and the GCP uptime check must read the + # body's top-level "status", not just the 200 (#649). + - name: Test /health gate modules + if: matrix.cloud == 'gcp' + run: | + for d in terraform/modules/deployment-checks terraform/modules/monitoring/gcp; do + terraform -chdir="$d" init -backend=false -lockfile=readonly -input=false + terraform -chdir="$d" fmt -check -recursive + terraform -chdir="$d" test + done + - name: Terraform Init run: | cd terraform/environments/${{ matrix.cloud }} diff --git a/internal/server/health_contract_test.go b/internal/server/health_contract_test.go new file mode 100644 index 000000000..2dc252051 --- /dev/null +++ b/internal/server/health_contract_test.go @@ -0,0 +1,78 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// topLevelHealthStatus decodes only the top-level "status" field, the way the +// post-deploy check (jsondecode(body).status) and the GCP uptime check +// (JSON path $.status) read it. It deliberately does not decode into +// HealthStatus, so renaming the JSON tag breaks this test. +func topLevelHealthStatus(t *testing.T, body []byte) string { + t.Helper() + var doc map[string]json.RawMessage + require.NoError(t, json.Unmarshal(body, &doc), "/health body must be a JSON object: %s", body) + raw, ok := doc["status"] + require.True(t, ok, "/health body must carry a top-level \"status\" field: %s", body) + var status string + require.NoError(t, json.Unmarshal(raw, &status), "top-level \"status\" must be a string: %s", body) + return status +} + +// TestHealthBodyStatusIsTheVerdict pins the /health contract that +// terraform/modules/deployment-checks and the GCP uptime check in +// terraform/modules/monitoring/gcp gate on (#649): /health answers 200 even +// when degraded (liveness, #488), so the only verdict a caller can rely on is +// the top-level "status" field, which must read "healthy" only when the +// replica is fully healthy, on both the server mux and the Lambda router. +func TestHealthBodyStatusIsTheVerdict(t *testing.T) { + serveMux := func(t *testing.T, app *Application) (int, []byte) { + t.Helper() + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/health", nil) + w := httptest.NewRecorder() + CreateHTTPServer(app, 0).Handler.ServeHTTP(w, req) + return w.Code, w.Body.Bytes() + } + + t.Run("server mux healthy", func(t *testing.T) { + code, body := serveMux(t, &Application{ + Version: "test", + Config: &mockConfigStoreForHealth{}, + Auth: createHealthyAuthService(), + }) + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, "healthy", topLevelHealthStatus(t, body)) + }) + + t.Run("server mux degraded still answers 200", func(t *testing.T) { + // Auth store reachable, config store missing: a nested check still + // reads "healthy", which is why a substring match on the body is not + // a verdict. + code, body := serveMux(t, &Application{ + Version: "test", + Auth: createHealthyAuthService(), + }) + assert.Equal(t, http.StatusOK, code, "/health is the liveness contract and must stay 200") + assert.Equal(t, "degraded", topLevelHealthStatus(t, body)) + assert.Contains(t, string(body), `"healthy"`, + "a degraded body contains the string \"healthy\" in its nested checks; gates must read the top-level field") + }) + + t.Run("lambda router degraded still answers 200", func(t *testing.T) { + app := &Application{API: api.NewHandler(api.HandlerConfig{})} + event := `{"requestContext":{"http":{"method":"GET","path":"/health"}},"rawPath":"/health","headers":{}}` + resp, err := app.handleLambdaHTTPEvent(context.Background(), json.RawMessage(event)) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "degraded", topLevelHealthStatus(t, []byte(resp.Body))) + }) +} diff --git a/terraform/modules/deployment-checks/.terraform.lock.hcl b/terraform/modules/deployment-checks/.terraform.lock.hcl index b799f46f7..0c9a1e107 100644 --- a/terraform/modules/deployment-checks/.terraform.lock.hcl +++ b/terraform/modules/deployment-checks/.terraform.lock.hcl @@ -2,8 +2,11 @@ # Manual edits may be lost in future updates. provider "registry.terraform.io/hashicorp/http" { - version = "3.5.0" + version = "3.5.0" + constraints = "~> 3.4" hashes = [ + "h1:8bUoPwS4hahOvzCBj6b04ObLVFXCEmEN8T/5eOHmWOM=", + "h1:KsglDyFg9a9CIlOKxSSl8gDi+SAKfY+uvIZO1+SpeAc=", "h1:dl73+8wzQR++HFGoJgDqY3mj3pm14HUuH/CekVyOj5s=", "zh:047c5b4920751b13425efe0d011b3a23a3be97d02d9c0e3c60985521c9c456b7", "zh:157866f700470207561f6d032d344916b82268ecd0cf8174fb11c0674c8d0736", diff --git a/terraform/modules/deployment-checks/main.tf b/terraform/modules/deployment-checks/main.tf index 9aa47f31c..4f27b94cf 100644 --- a/terraform/modules/deployment-checks/main.tf +++ b/terraform/modules/deployment-checks/main.tf @@ -34,22 +34,40 @@ locals { # have scaled to zero after deployment. Terraform check-block data sources # are evaluated *after* all resources are applied, so this resource warms the # container before checks and bash tests run. +# +# /health is a liveness endpoint: it answers HTTP 200 even while required +# initialization is still pending or a dependency is down, and reports the +# verdict in the body's top-level "status" field ("healthy" / "degraded"). +# Waiting only for a 200 would hand the checks below a replica that is up but +# not initialized, so warmup waits for "status":"healthy" (#649). Without +# python3 the body cannot be parsed and warmup falls back to waiting for 200. resource "terraform_data" "warmup" { triggers_replace = [var.api_base_url] provisioner "local-exec" { command = <<-EOT + body_file=$(mktemp) + trap 'rm -f "$body_file"' EXIT for i in 1 2 3 4 5 6 7 8 9 10; do - status=$(curl -sk -o /dev/null -w "%%{http_code}" --connect-timeout 10 --max-time 30 "${var.api_base_url}/health" 2>/dev/null) || status="000" - if [ "$status" != "000" ] && [ "$status" != "502" ] && [ "$status" != "503" ]; then - echo "Warmup: service ready (HTTP $status) after attempt $i" - exit 0 + status=$(curl -sk -o "$body_file" -w "%%{http_code}" --connect-timeout 10 --max-time 30 "${var.api_base_url}/health" 2>/dev/null) || status="000" + if [ "$status" = "200" ]; then + if ! command -v python3 >/dev/null 2>&1; then + echo "Warmup: service answering (HTTP 200) after attempt $i; python3 not found, body status not checked" + exit 0 + fi + health=$(python3 -c 'import json,sys; s=json.load(open(sys.argv[1])).get("status"); print(s if isinstance(s,str) else "")' "$body_file" 2>/dev/null) || health="" + if [ "$health" = "healthy" ]; then + echo "Warmup: service healthy after attempt $i" + exit 0 + fi + echo "Warmup: attempt $i got HTTP 200 with status '$health', retrying in 5s..." + else + echo "Warmup: attempt $i got HTTP $status, retrying in 5s..." fi - echo "Warmup: attempt $i got HTTP $status, retrying in 5s..." sleep 5 done - echo "Warmup: service did not become ready after 10 attempts, continuing anyway" + echo "Warmup: service did not report healthy after 10 attempts, continuing anyway" EOT on_failure = continue @@ -79,6 +97,15 @@ check "health" { condition = data.http.health.status_code == 200 error_message = "Health check failed: ${var.api_base_url}/health returned ${data.http.health.status_code}" } + + # /health answers 200 by design even when every dependency is down (it is + # the liveness contract, #488), so the status code alone passes on a fully + # degraded deployment. The verdict is the body's top-level "status" field, + # which both the server mux and the Lambda API router emit (#649). + assert { + condition = try(jsondecode(data.http.health.response_body).status, "") == "healthy" + error_message = "Health check failed: ${var.api_base_url}/health reported status \"${try(tostring(jsondecode(data.http.health.response_body).status), "unparseable")}\", expected \"healthy\"" + } } # --- Security Headers --- diff --git a/terraform/modules/deployment-checks/tests/health.tftest.hcl b/terraform/modules/deployment-checks/tests/health.tftest.hcl new file mode 100644 index 000000000..6c7458653 --- /dev/null +++ b/terraform/modules/deployment-checks/tests/health.tftest.hcl @@ -0,0 +1,125 @@ +# The post-deploy health gate must fail on a degraded deployment (#649). +# /health answers HTTP 200 even when every dependency is down (liveness +# contract, #488); the verdict is the body's top-level "status" field. +# +# Every check block reads a mocked http data source. Only data.http.health is +# overridden per run; the others share the mock default (HTTP 200, "{}"), so +# the checks that expect a non-200 status always fail here and are listed in +# expect_failures. They are not what this file tests. +mock_provider "http" { + mock_data "http" { + defaults = { + status_code = 200 + response_body = "{}" + response_headers = {} + } + } +} + +variables { + api_base_url = "https://app.example.com" + provider_name = "gcp" +} + +run "healthy_body_passes" { + command = plan + + override_data { + target = data.http.health + values = { + status_code = 200 + response_body = "{\"timestamp\":\"2026-01-01T00:00:00Z\",\"checks\":{\"config_store\":{\"status\":\"healthy\"}},\"status\":\"healthy\",\"version\":\"test\"}" + } + } + + expect_failures = [ + check.security_headers, + check.auth_enforcement, + check.error_empty_body, + check.error_invalid_json, + check.method_post_summary, + check.content_type_xml, + check.content_type_missing, + check.request_size_limit, + check.frontend_serving, + ] +} + +# The case the issue describes: the database is unreachable, /health still +# answers 200, and the nested checks still contain the string "healthy". +run "degraded_body_with_200_fails" { + command = plan + + override_data { + target = data.http.health + values = { + status_code = 200 + response_body = "{\"timestamp\":\"2026-01-01T00:00:00Z\",\"checks\":{\"auth_store\":{\"status\":\"healthy\"},\"config_store\":{\"status\":\"unhealthy\",\"message\":\"config store unavailable\"}},\"status\":\"degraded\",\"version\":\"test\"}" + } + } + + expect_failures = [ + check.health, + check.security_headers, + check.auth_enforcement, + check.error_empty_body, + check.error_invalid_json, + check.method_post_summary, + check.content_type_xml, + check.content_type_missing, + check.request_size_limit, + check.frontend_serving, + ] +} + +# A 200 whose body is not the health JSON (e.g. an SPA index served by a +# catch-all route) must not pass either. +run "non_json_body_fails" { + command = plan + + override_data { + target = data.http.health + values = { + status_code = 200 + response_body = "healthy" + } + } + + expect_failures = [ + check.health, + check.security_headers, + check.auth_enforcement, + check.error_empty_body, + check.error_invalid_json, + check.method_post_summary, + check.content_type_xml, + check.content_type_missing, + check.request_size_limit, + check.frontend_serving, + ] +} + +run "non_200_fails" { + command = plan + + override_data { + target = data.http.health + values = { + status_code = 503 + response_body = "{\"status\":\"healthy\"}" + } + } + + expect_failures = [ + check.health, + check.security_headers, + check.auth_enforcement, + check.error_empty_body, + check.error_invalid_json, + check.method_post_summary, + check.content_type_xml, + check.content_type_missing, + check.request_size_limit, + check.frontend_serving, + ] +} diff --git a/terraform/modules/monitoring/gcp/main.tf b/terraform/modules/monitoring/gcp/main.tf index 2ab1b5396..ebc5f9d81 100644 --- a/terraform/modules/monitoring/gcp/main.tf +++ b/terraform/modules/monitoring/gcp/main.tf @@ -617,9 +617,23 @@ resource "google_monitoring_uptime_check_config" "service_health" { } } + # /health answers 200 by design even when every dependency is down (it is + # the liveness contract, #488), so the verdict has to come from the body. + # Match the top-level "status" field exactly: a CONTAINS_STRING "healthy" + # matcher also matches "unhealthy" and every nested check's + # {"status":"healthy"}, so it passed on a degraded replica (#649). The check + # stays on /health, not /ready (pointing an uptime monitor at /ready would + # flap the alert on every deploy); service_unavailable below only fires + # after 180s of failures, so a replica that reports "degraded" briefly while + # it initializes does not page. content_matchers { content = "healthy" - matcher = "CONTAINS_STRING" + matcher = "MATCHES_JSON_PATH" + + json_path_matcher { + json_path = "$.status" + json_matcher = "EXACT_MATCH" + } } } diff --git a/terraform/modules/monitoring/gcp/tests/uptime_check.tftest.hcl b/terraform/modules/monitoring/gcp/tests/uptime_check.tftest.hcl new file mode 100644 index 000000000..7f5052533 --- /dev/null +++ b/terraform/modules/monitoring/gcp/tests/uptime_check.tftest.hcl @@ -0,0 +1,39 @@ +# The uptime check must tell "process alive" from "serving traffic" (#649). +# /health answers HTTP 200 even when every dependency is down (liveness +# contract, #488), so the check has to match the body's top-level "status" +# exactly. A CONTAINS_STRING "healthy" matcher also matches "unhealthy" and +# every nested check's {"status":"healthy"}, so it passed on a degraded replica. +mock_provider "google" {} + +variables { + project_id = "example-project" + service_name = "cudly" + environment = "dev" + db_instance_id = "cudly-db" + service_url = "cudly.example.com" +} + +run "uptime_check_matches_top_level_status" { + command = plan + + assert { + condition = google_monitoring_uptime_check_config.service_health.http_check[0].path == "/health" + error_message = "The uptime check must stay on /health (liveness); /ready answers 503 during every rollout and would flap the alert." + } + + assert { + condition = length(google_monitoring_uptime_check_config.service_health.content_matchers) == 1 + error_message = "The uptime check needs exactly one content matcher on the /health body." + } + + assert { + condition = alltrue([ + for m in google_monitoring_uptime_check_config.service_health.content_matchers : + m.matcher == "MATCHES_JSON_PATH" && m.content == "healthy" && + length(m.json_path_matcher) == 1 && + m.json_path_matcher[0].json_path == "$.status" && + m.json_path_matcher[0].json_matcher == "EXACT_MATCH" + ]) + error_message = "The uptime check must exact-match \"healthy\" at JSON path $.status; a substring match on \"healthy\" also passes a degraded body." + } +}