Repository navigation
fix(iac): require PostgreSQL 15+ in every environment's version input - #1255
Conversation
The migrations need PostgreSQL 15 (000134 uses UNIQUE NULLS NOT DISTINCT), but the environment inputs still accepted older majors: Azure's postgres_version allowed 11-16, and AWS database_engine_version and GCP database_version had no validation. A custom deployment on an older engine would plan and apply, then fail at migration time. Validate all three at plan time: Azure "15"/"16", GCP POSTGRES_15/POSTGRES_16, and AWS "<major>" or "<major>.<minor>" with a major of 15 or more (try() makes a malformed value a validation failure rather than an evaluation error). Every shipped tfvars pins 16, so no deployed environment changes. Add a mocked tftest per environment (old major rejected, 15 and 16 accepted), run by a new "Test environment inputs" step in the terraform-validate CI job, and state the minimum in docs/DEPLOYMENT.md. Closes LeanerCloud#1233 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…5 tftest The combined-tree git-secrets run flagged terraform/environments/aws/tests/postgres_version.tftest.hcl:10, a bare account_id = "123456789012" in the mocked aws_caller_identity. The repo allows that placeholder only on AccountID lines in _test.go files, so add an anchored whole-line entry for this one mock line (bracketed digits so the entry does not match itself), like the other terraform/ entries. Refs LeanerCloud#1233 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…S tftest The combined-tree git-secrets run flagged terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl:10, a bare account_id = "123456789012" in the mocked aws_caller_identity. Add an anchored whole-line .gitallowed entry for that one line (bracketed digits so the entry does not match itself), placed before the other terraform/ entries so it does not collide with LeanerCloud#1255's entry for its own tftest. Refs LeanerCloud#324 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Brings the branch up to date with main so the PR is mergeable again. Conflicted files: - .gitallowed: main (LeanerCloud#1151) renamed the two terraform/profiles/README.md entries from database_password to admin_password to follow the README, and the branch added the postgres_version tftest account-ID entry next to them. Main's README entries are kept (they match the merged README) and the branch's tftest entry follows them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's LeanerCloud#1147 moved every run-block ${{ }} interpolation in ci.yml into env, giving terraform-validate a job-level CLOUD. The step this branch adds still interpolated matrix.cloud straight into its run block, which the merge brought back. Use the quoted CLOUD env like the neighbouring steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged current main into the branch and added one follow-up commit. New head: 2212023.
Checks:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB18. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Correction to my review above. The verdict is unchanged. Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB18. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
…rst, read the dev Function URL from state (#1312) * chore(iac): derive the CFN issuer host, create the GCP signing key first, read the dev Function URL from state Three low findings from the 2026-09-02 audit. A13b-014: the aws-target CloudFormation template took the trust policy's condition-key host as a second parameter, OIDCIssuerHost, that the operator had to keep equal to OIDCIssuerURL minus https://. The host is now derived with !Select [1, !Split ["https://", !Ref OIDCIssuerURL]], the same value as the Terraform module's local.oidc_condition_host, and the server no longer passes the parameter in the bundle's params JSON or deploy script (CFN would reject an undeclared parameter). A13c-024: the Cloud Run module's OIDC signing key now has create_before_destroy, matching its AWS twin, so a change that forces a new key cannot destroy the live one first. destroy_scheduled_duration becomes an input whose default keeps every deployed key unchanged (the provider marks it ForceNew). prevent_destroy cannot be an input (Terraform requires a literal) and staging cleanup runs terraform destroy, so the redundant `prevent_destroy = false` line is replaced by a comment saying so. A13c-017: github-dev.tfvars no longer pins the dev Lambda Function URL host. The deploy and rollback workflows read the previous apply's lambda_function_url output (scripts/read-lambda-function-url.sh) and pass it as TF_VAR_lambda_deployed_function_url; compute.tf appends its origin to the CORS allowlist. A post-apply step warns when the apply changed the URL. Refs #324 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(iac): allowlist the mocked caller-identity account ID in the CORS tftest The combined-tree git-secrets run flagged terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl:10, a bare account_id = "123456789012" in the mocked aws_caller_identity. Add an anchored whole-line .gitallowed entry for that one line (bracketed digits so the entry does not match itself), placed before the other terraform/ entries so it does not collide with #1255's entry for its own tftest. Refs #324 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Merge origin/main into fix/issue-324 Bring the branch up to date with main. Conflicted files: - .github/workflows/ci.yml: this branch appended a "Run Function URL reader self-tests" step to the aws-tfstate-platform-key job; main (#1112) added a new rollback-ecr-registry job right after that job. Kept both: the step stays at the end of aws-tfstate-platform-key and main's job follows it unchanged. rollback.yml merged cleanly: main's rewritten "Verify image exists" steps and this branch's TF_VAR_lambda_deployed_function_url export touch different steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Merge origin/main into fix/issue-324 Two textual conflicts, both resolved by keeping both sides: - ci.yml: main added an init-backend.sh self-test step to the same job this branch appends the Function URL reader self-tests to, plus a new tfstate-concurrency-group job after it. Both steps stay in the job, and main's job follows. - cloud-run/.terraform.lock.hcl: main's "~> 5.45" constraint with the union of both sides' h1: hashes for google 5.45.2. #1256 (merged on main) added OIDCIssuerHost to the standalone generator; the next commit drops it again to match this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(scripts): drop OIDCIssuerHost from the standalone IaC generator #1256 added OIDCIssuerHost to iacData because aws-cfn-deploy.sh.tmpl passed it at the time. This branch removes the parameter from the CloudFormation template and the deploy script (the host is derived from OIDCIssuerURL), so the field is unused again and the drift test that compares iacData with federationIaCData fails until it goes. The bundle test now checks that the rendered deploy script passes OIDCIssuerURL and never OIDCIssuerHost, since CloudFormation rejects an override for an undeclared parameter. The field walker's control uses OIDCIssuerURL instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(iac): note the key ring order and the URL reader's failure mode Review follow-ups on #1312. create_before_destroy on the signing key also applies to the key ring it depends on, and the Function URL reader treats any terraform output failure as "no URL". Both are intended, but neither was written down where the code is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(api): pass t to federationHandler in the CFN bundle test #1223 on main made federationHandler take *testing.T so it can register AssertExpectations on its mock. This branch's CFN bundle parameter test still called it with no arguments, so internal/api stopped compiling after the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Merge origin/main into fix/issue-324 Picks up #1467, which also edits scripts/generate-federation-iac.go, so the generator tests are re-run on the merged tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Root cause
The migrations need PostgreSQL 15+ (#1110, migration 000134, uses
UNIQUE NULLS NOT DISTINCT), but the Terraform environment inputs still accepted older majors:terraform/environments/azure/variables.tfpostgres_version: validation allowed11-16.terraform/environments/aws/variables.tfdatabase_engine_version: no validation.terraform/environments/gcp/variables.tfdatabase_version: no validation.So a custom or self-hosted deployment on an older engine would plan and apply cleanly, then fail at migration time.
Fix
Validate each input at plan time:
postgres_version:contains(["15", "16"], ...).database_version:contains(["POSTGRES_15", "POSTGRES_16"], ...).database_engine_version:try(tonumber(regex("^([0-9]+)(\\.[0-9]+)?$", v)[0]) >= 15, false). This accepts the major-only pin ("16") and the"16.6"form theterraform/profiles/aws/*.tfvars.examplefiles use.try()makes a malformed value such as"sixteen"fail validation cleanly instead of failing with atonumber()evaluation error. (Terraform 1.10's&&does not short-circuit, so the issue's plaintonumber(split(".", v)[0]) >= 15would error instead of fail.)Each variable's description and error message now name the 15+ requirement.
docs/DEPLOYMENT.md→ Prerequisites states the minimum and names the input for each cloud.Every shipped tfvars (
github-{dev,staging,prod}.tfvars,dev.tfvars.example, profiles) pins 16, so no deployed environment's plan changes.Deploy gating: the AWS check takes effect on the next AWS plan. The Azure and GCP changes only take effect at their next deploy, which is gated on the paused Azure/GCP deploys. Until those resume, they are covered only by CI (
terraform validateplus the new tftests below).Tests
A new
terraform/environments/{aws,azure,gcp}/tests/postgres_version.tftest.hclper environment:14,14.12andsixteen; accepts15,16and16.6.14; accepts15and16.POSTGRES_14; acceptsPOSTGRES_15andPOSTGRES_16.Every provider is mocked (
mock_provider), so nothing reaches a cloud API. Each run usescommand = planwithplan_options { target = [module.database] }.module.databaseis the input's only consumer, and targeting it keeps the variable's validation in the graph without planning the whole environment against mock data. An untargeted mocked plan fails on unrelated mock-data gaps (empty AZ lists, non-12-digit mock account IDs, an unknown-at-plancount). I also checked that targeting an unrelated resource (random_id.suffix) drops the validation and gives "Missing expected failure", which is why the tests target the consumer. Fixtures used: AWS mocksaws_caller_identity/aws_availability_zoneswith the placeholder123456789012that the fargate tftest already uses. GCP sets a 64-zero placeholdercredential_encryption_key, because the auto-generated one is unknown at plan and the secrets module uses it in acount.CI did not run any environment-level tftests before this change. A new "Test environment inputs" step in the
terraform-validatejob (afterTerraform Validate) runsterraform testinterraform/environments/<cloud>when atests/dir exists.Verification
All of this is local, mocked-provider tftest evidence (Terraform v1.10.5, the CI pin). No
terraform plan/applywas run against a real backend or with real credentials. Everything ran withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.Commands, run on temp copies of
terraform/(after= this branch,before=origin/mainplus the new test files) so.terraform/never touched the worktree:Fail-before / pass-after (
terraform test):rejects_postgres_14fail: "Missing expected failure", rest skippedrejects_postgres_14fail: "Missing expected failure", rest skippedrejects_postgres_14fail: "Missing expected failure", rest skippedAlso checked:
beforecopy, aws 3/3, azure 2/2 and gcp 2/2 passed. A failing reject run therefore comes from the missing validation, not from the harness.>= 16, Azure["16"], GCP["POSTGRES_16"]):accepts_postgres_15fails with "Invalid value for variable" in all three environments, so the accept runs do catch an over-tight floor.Combined-tree verification. This branch was tested together with six other queued branches on a local, never-pushed tree:
queue/platform@77d116c6= origin/maina14dc8f2plus these heads, merged in this order:Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration, andgocyclo -over 10is silent on the changed non-test files. These ran atb61a9c30, which differs from77d116c6only by one.gitallowedline on fix/issue-1233.-tags integration -count=1):cmd/cleanup-lambda,internal/analytics,internal/auth,internal/database/postgres/migrationsandinternal/purchase.internal/apipasses, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass:TestDeleteManagedPlanByExecutionStatusandTestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).TESTCONTAINERS_RYUK_DISABLED=trueto avoid a testcontainers reaper race under load.scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
terraform/modules/database/azurepostgres_version,terraform/modules/database/gcpdatabase_version,terraform/modules/database/awsengine_version) are still unvalidated. The Azure module's description still lists "11, 12, 13, 14, 15, 16". The issue targets the environment inputs, which are the only callers in this repo. Adding the same floor to the modules would be a small follow-up.docs/DEPLOYMENT.mdstill says "Aurora Serverless v2 PostgreSQL 16.4" for AWS, while the env provisions anaws_db_instancepinned to major 16. This is pre-existing doc drift and is left alone.Maintainer: please mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/this-quarter,impact/few,effort/xs,type/bug.Closes #1233
🤖 Generated with Claude Code