diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9ce64107..f0d0e1e33 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1052,7 +1052,8 @@ jobs: done # Each database module must default to deletion protection and honor - # an explicit opt-out (#293). Skips a cloud until it has a tests dir. + # an explicit opt-out (#293); AWS's multi_az must default off and reach + # aws_db_instance.main (#719). Skips a cloud until it has a tests dir. - name: Test database module env: CLOUD: ${{ matrix.cloud }} diff --git a/.github/workflows/deploy-aws-lambda.yml b/.github/workflows/deploy-aws-lambda.yml index e323325b3..20b5e457d 100644 --- a/.github/workflows/deploy-aws-lambda.yml +++ b/.github/workflows/deploy-aws-lambda.yml @@ -53,6 +53,7 @@ on: - 'terraform/modules/compute/aws/lambda/**' - '!terraform/modules/compute/aws/lambda/tests/**' - 'terraform/modules/database/aws/**' + - '!terraform/modules/database/aws/tests/**' - 'terraform/modules/secrets/aws/**' - 'terraform/modules/networking/aws/**' - 'terraform/environments/aws/**' diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= diff --git a/terraform/environments/aws/database.tf b/terraform/environments/aws/database.tf index 56cce70b2..b72e872c1 100644 --- a/terraform/environments/aws/database.tf +++ b/terraform/environments/aws/database.tf @@ -32,6 +32,9 @@ module "database" { allocated_storage = var.database_allocated_storage max_allocated_storage = var.database_max_allocated_storage + # High availability + multi_az = var.database_multi_az + # Networking vpc_id = module.networking.vpc_id vpc_cidr = var.vpc_cidr diff --git a/terraform/environments/aws/database_ha_guard_test.go b/terraform/environments/aws/database_ha_guard_test.go new file mode 100644 index 000000000..4134bbe40 --- /dev/null +++ b/terraform/environments/aws/database_ha_guard_test.go @@ -0,0 +1,104 @@ +// Regression test for issue #719: the AWS prod database was a single-AZ +// burstable db.t4g.micro and the database module could not express Multi-AZ, +// while GCP (REGIONAL) and Azure (ZoneRedundant) prod were already HA. The +// module half is covered by terraform/modules/database/aws/tests; this pins +// the environment wiring and the prod values. +package aws_test + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// tfvarsValue returns the right-hand side of a top-level `name = value` +// assignment in a tfvars file, ignoring comment lines and trailing comments. +func tfvarsValue(t *testing.T, path, name string) (string, bool) { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + re := regexp.MustCompile(`^` + regexp.QuoteMeta(name) + `\s*=\s*(.*)$`) + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if i := strings.Index(line, "#"); i >= 0 { + line = strings.TrimSpace(line[:i]) + } + if m := re.FindStringSubmatch(line); m != nil { + return strings.Trim(strings.TrimSpace(m[1]), `"`), true + } + } + return "", false +} + +func TestAWSProdDatabaseIsHighlyAvailable(t *testing.T) { + const prod = "github-prod.tfvars" + + if v, ok := tfvarsValue(t, prod, "database_multi_az"); !ok || v != "true" { + t.Errorf("%s: database_multi_az = %q (set: %v), want true: prod RDS needs a standby in a second AZ", prod, v, ok) + } + + class, ok := tfvarsValue(t, prod, "database_instance_class") + if !ok { + t.Fatalf("%s: database_instance_class not set, so prod falls back to the burstable db.t4g.micro default", prod) + } + if regexp.MustCompile(`^db\.t\d`).MatchString(class) { + t.Errorf("%s: database_instance_class = %q is burstable (CPU credits); prod needs a non-burstable class", prod, class) + } +} + +// topLevelBlock returns the text of the top-level block that starts with +// header, up to its closing brace in column 0 (terraform fmt layout), or "" +// when the header is absent. +func topLevelBlock(data []byte, header *regexp.Regexp) string { + loc := header.FindIndex(data) + if loc == nil { + return "" + } + rest := string(data[loc[0]:]) + if end := strings.Index(rest, "\n}"); end >= 0 { + return rest[:end+2] + } + return rest +} + +func TestAWSDatabaseMultiAZIsWired(t *testing.T) { + cases := []struct { + file string + block *regexp.Regexp // when set, re must match inside this top-level block + re *regexp.Regexp + what string + }{ + { + file: "variables.tf", + re: regexp.MustCompile(`variable\s+"database_multi_az"\s*\{[^}]*type\s*=\s*bool[^}]*default\s*=\s*false`), + what: `variable "database_multi_az" (bool, default false)`, + }, + { + file: "database.tf", + re: regexp.MustCompile(`(?m)^\s*multi_az\s*=\s*var\.database_multi_az\s*$`), + what: "multi_az = var.database_multi_az on module \"database\"", + }, + { + file: filepath.Join("..", "..", "modules", "database", "aws", "main.tf"), + block: regexp.MustCompile(`(?m)^resource\s+"aws_db_instance"\s+"main"\s*\{`), + re: regexp.MustCompile(`(?m)^\s*multi_az\s*=\s*var\.multi_az\s*$`), + what: `multi_az = var.multi_az on aws_db_instance.main`, + }, + } + for _, c := range cases { + data, err := os.ReadFile(c.file) + if err != nil { + t.Fatalf("reading %s: %v", c.file, err) + } + if c.block != nil { + data = []byte(topLevelBlock(data, c.block)) + } + if !c.re.Match(data) { + t.Errorf("%s: missing %s", c.file, c.what) + } + } +} diff --git a/terraform/environments/aws/github-prod.tfvars b/terraform/environments/aws/github-prod.tfvars index 6f15a1cde..9f16cc351 100644 --- a/terraform/environments/aws/github-prod.tfvars +++ b/terraform/environments/aws/github-prod.tfvars @@ -20,9 +20,11 @@ enable_docker_build = true # Build image via Terraform build module (no separate # Lambda Configuration # lambda_reserved_concurrency caps concurrent execution environments, each of # which holds its own DB pool (#718): 20 x db_max_connections (3) = 60 fits -# database_max_connections (80, db.t4g.micro) minus the 10 reserved -# connections. Reserving 20 needs an account concurrency quota of at least 120 -# (Lambda keeps 100 unreserved). +# database_max_connections (default 80) minus the 10 reserved connections. +# 80 is the db.t4g.micro figure and a conservative floor for db.m7g.large +# (#719); raise it only after checking SHOW max_connections on the instance. +# Reserving 20 needs an account concurrency quota of at least 120 (Lambda +# keeps 100 unreserved). lambda_memory_size = 1024 lambda_timeout = 300 lambda_reserved_concurrency = 20 @@ -50,10 +52,15 @@ db_min_connections = 1 # ============================================== # Database (RDS PostgreSQL) # ============================================== +# Non-burstable and Multi-AZ (#719): 2 vCPU / 8 GiB with a standby in a second +# AZ, matching GCP prod (db-custom-2-7680, REGIONAL) and Azure prod +# (GP_Standard_D2s_v3, ZoneRedundant). database_name = "cudly" database_username = "cudly" database_engine_version = "16" +database_instance_class = "db.m7g.large" +database_multi_az = true database_backup_retention_days = 30 database_deletion_protection = true database_skip_final_snapshot = false diff --git a/terraform/environments/aws/outputs.tf b/terraform/environments/aws/outputs.tf index 93dc5ca3e..a7b480c3b 100644 --- a/terraform/environments/aws/outputs.tf +++ b/terraform/environments/aws/outputs.tf @@ -207,6 +207,7 @@ output "deployment_info" { nat_enabled = false # Using IPv6 dual-stack, no NAT Gateway needed vpc_endpoints = false # Not needed with IPv6 db_instance_class = var.database_instance_class + db_multi_az = var.database_multi_az db_storage_gb = var.database_allocated_storage } } diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index 015e60436..e7bbc5bc4 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -135,11 +135,17 @@ variable "database_engine_version" { } variable "database_instance_class" { - description = "RDS instance class" + description = "RDS instance class. Burstable db.t* classes throttle once their CPU credits run out under sustained load, so use a non-burstable class for production." type = string default = "db.t4g.micro" } +variable "database_multi_az" { + description = "Provision the RDS instance as Multi-AZ (standby in a second AZ, automatic failover). Roughly doubles the instance cost, so it is off by default; enable it for production." + type = bool + default = false +} + # Connection budget (#718). Every Lambda execution environment and every # Fargate task opens its own pgx pool of up to db_max_connections, so the # number of those that can run at once, times db_max_connections, must fit in diff --git a/terraform/modules/database/aws/main.tf b/terraform/modules/database/aws/main.tf index c9307ce3e..c9f73a4ca 100644 --- a/terraform/modules/database/aws/main.tf +++ b/terraform/modules/database/aws/main.tf @@ -129,6 +129,10 @@ resource "aws_db_instance" "main" { auto_minor_version_upgrade = true instance_class = var.instance_class + # Multi-AZ standby in a second AZ with synchronous replication and automatic + # failover (#719). Opt-in: it roughly doubles the instance cost. + multi_az = var.multi_az + db_name = var.database_name username = var.master_username password = local.db_password diff --git a/terraform/modules/database/aws/tests/multi_az.tftest.hcl b/terraform/modules/database/aws/tests/multi_az.tftest.hcl new file mode 100644 index 000000000..8a1fd0237 --- /dev/null +++ b/terraform/modules/database/aws/tests/multi_az.tftest.hcl @@ -0,0 +1,37 @@ +# Multi-AZ support (#719). The module could not express a standby replica, so +# prod on AWS had no HA while GCP (REGIONAL) and Azure (ZoneRedundant) did. +# The environment wiring is pinned by TestAWSDatabaseMultiAZIsWired in +# terraform/environments/aws. +mock_provider "aws" {} +mock_provider "random" {} + +variables { + stack_name = "cudly-test" + permissions_boundary_arn = "arn:aws:iam::123456789012:policy/cudly-boundary" + vpc_id = "vpc-0123456789abcdef0" + vpc_cidr = "10.0.0.0/16" + private_subnet_ids = ["subnet-0aaaaaaaaaaaaaaaa", "subnet-0bbbbbbbbbbbbbbbb"] + admin_email = "admin@example.com" +} + +run "single_az_by_default" { + command = plan + + assert { + condition = aws_db_instance.main.multi_az == false + error_message = "multi_az must default to false so dev/staging keep their single-AZ cost profile." + } +} + +run "multi_az_reaches_the_instance" { + command = plan + + variables { + multi_az = true + } + + assert { + condition = aws_db_instance.main.multi_az == true + error_message = "multi_az = true must provision a Multi-AZ standby on aws_db_instance.main." + } +} diff --git a/terraform/modules/database/aws/variables.tf b/terraform/modules/database/aws/variables.tf index 9f5f6ad1d..9f0ab686a 100644 --- a/terraform/modules/database/aws/variables.tf +++ b/terraform/modules/database/aws/variables.tf @@ -61,11 +61,17 @@ variable "engine_version" { } variable "instance_class" { - description = "RDS instance class (e.g., db.t4g.micro, db.t4g.small)" + description = "RDS instance class (e.g., db.t4g.micro, db.m7g.large). Burstable db.t* classes run on CPU credits and throttle once those are spent under sustained load, so use a non-burstable class for production." type = string default = "db.t4g.micro" } +variable "multi_az" { + description = "Provision a Multi-AZ standby with automatic failover. Roughly doubles the instance cost; enable for production. The DB subnet group must span at least two AZs." + type = bool + default = false +} + variable "allocated_storage" { description = "Initial allocated storage in GB" type = number diff --git a/terraform/profiles/aws/prod.tfvars.example b/terraform/profiles/aws/prod.tfvars.example index b8e9980e9..ff3b327b0 100644 --- a/terraform/profiles/aws/prod.tfvars.example +++ b/terraform/profiles/aws/prod.tfvars.example @@ -31,6 +31,8 @@ admin_email = "admin@example.com" database_name = "cudly" database_username = "cudly" database_engine_version = "16" # major only: a full minor pin breaks on RDS auto-upgrade (see variables.tf) +database_instance_class = "db.m7g.large" # Non-burstable (#719) +database_multi_az = true # Standby in a second AZ (#719) database_backup_retention_days = 30 # Longer retention for prod database_deletion_protection = true # Protect against deletion database_skip_final_snapshot = false