Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ terraform.rc
backend/lambda-deploy.zip
backend/lambda-placeholder.zip
backend/placeholder.js
backend/build/
response.json
logs.txt

Expand Down
95 changes: 95 additions & 0 deletions .tasks/task-deploy-infra/2026-08-30-144740-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# Dev infrastructure deployment: state backend reconciliation, variable wiring, and conditional CloudFront/WAF/DNS (v1 review)

This change gets the full Taskly Terraform stack to apply cleanly on a fresh AWS account that is missing several account-level capabilities. It reconciles the S3 state backend to the real bucket, wires every required root-module variable through the thin `dev` wrapper (including sensitive `documentdb_master_password`/`jwt_signing_key` that had no home before), and makes three resources conditional so an unverified/unprovisioned account still converges: CloudFront distributions (account not CloudFront-verified), the WAF→API-Gateway association (WAFv2 cannot attach to an HTTP API v2 stage), and Route 53 failover records (no hosted zone). A new `scripts/build-lambda.sh` produces the four Lambda zips as one full-backend bundle and prunes runtime-provided/unused `node_modules` to fit Lambda's 250 MB unzipped limit. The orchestrator independently confirmed `terraform plan` is idempotent, 219 resources applied, and `GET /api/health` returns 200 with `database:connected`.

Watch for: (1) the Lambda bundle relies on the `nodejs20.x` runtime providing all `@aws-sdk/*` packages — including `@aws-sdk/s3-request-presigner`, imported at module-load time by `routes/upload.js` (confirmed; a future runtime that stops bundling the SDK would break cold start). (2) Disabling CloudFront and the WAF association narrows the intended security architecture — the uploads path loses the CDN and the API runs with an unattached WebACL, i.e. no active WAF protection (confirmed; documented, but worth a conscious sign-off). (3) A fresh apply on another account still defaults `enable_cloudfront = true` and will fail on a CloudFront-unverified account until the operator sets the flag (confirmed).

**Verdict**: APPROVED

## High-level view

The conditional-resource work is correct. CloudFront distributions and their bucket policies are gated with `count`, the self-references were updated to `[0]`, and the outputs use `one(...)` with graceful fallbacks — `uploads_distribution_domain_name` degrades to the uploads S3 regional domain so the Lambda's `CDN_DOMAIN` stays valid, while ID/ARN outputs return `""`. The WAF association is gated behind a flag defaulting to `false` (WAFv2 regional WebACLs genuinely cannot attach to API Gateway v2), and DNS failover records are gated on a non-empty `hosted_zone_id`. All guards reference resources that exist, and the root module declares every variable the dev wrapper passes through.

The Lambda bundle — one identical full-backend zip for all four functions — resolves every handler path regardless of which zip loads, and the only `sharp`/`@img` consumer (`image-processor.js`) is not one of the four deployed functions, so pruning those is safe. Pruning `@aws-sdk`/`@smithy` is safe *today* because `nodejs20.x` bundles AWS SDK v3 and the deployed health path proved the full `server.js` module graph (which statically imports the SDK via `routes/upload.js` and `config/aws.js`) loads. This is a runtime-version-coupled assumption, not an intrinsic property of the code.

The security posture change should be an explicit decision, not a silent default: with CloudFront off, uploads are served directly from S3 regional domains and the frontend has no CDN/OAC edge; with the WAF association off, the created WebACL protects nothing. Both are documented, and the WebACL-still-created design keeps the resource ready to attach once the API is fronted by CloudFront.

Secret handling is clean: the two secrets are declared `sensitive`, live only in a gitignored `secret.auto.tfvars` that is not tracked, and no real values appear in committed tfvars or the deployment report.

<details>
<summary>Issues (4)</summary>

1. **AWS SDK runtime coupling** — the Lambda zips omit `@aws-sdk`/`@smithy` and depend on `nodejs20.x` providing them; `routes/upload.js` imports `@aws-sdk/s3-request-presigner` at load time, so a runtime upgrade that stops bundling the SDK would fail cold start. Pin the runtime or add the SDK packages to the bundle before moving off `nodejs20.x`.
2. **WAF provides no active protection** — with `enable_api_gateway_association = false` (the default) the WebACL exists but is attached to nothing, so the API has no WAF in front of it. Confirm this is acceptable for dev and track fronting the API with CloudFront (CLOUDFRONT-scope WebACL) to restore protection.
3. **CloudFront-off weakens the upload/edge path** — uploads are served from the S3 regional domain and the frontend has no OAC/CDN edge. Fine as a documented temporary state; re-enable once the account is CloudFront-verified.
4. **Fresh-apply default trips on unverified accounts** — `enable_cloudfront` defaults to `true` in both root and dev, so a fresh apply on another CloudFront-unverified account fails until the operator sets it false. Document this as a required pre-apply input for new accounts.

</details>

<details>
<summary>Details</summary>

### Conditional CloudFront: count guards, self-references, and output fallbacks

Both distributions and their two `aws_s3_bucket_policy` resources are gated with `count = var.enable_distributions ? 1 : 0`, and the bucket policies' `AWS:SourceArn` self-references were updated to `aws_cloudfront_distribution.frontend[0].arn` / `uploads[0].arn`. The outputs switched to `one(...) != null ? ... : ""`, avoiding an index into an empty list. The load-bearing one is `uploads_distribution_domain_name`, which falls back to `var.uploads_bucket_regional_domain_name` — that value feeds `module.lambda`'s `cdn_domain` / `CDN_DOMAIN`, so the application still gets a resolvable domain instead of `""`. When disabled, the root `cloudfront_frontend_url` output is `""` and the frontend bucket has no OAC policy — the frontend is not edge-served in this state. That is the intended trade-off for an unverified account, and the default remains `true` so verified accounts get the full topology.

### WAF WebACL created but unattached on HTTP API v2

The `aws_wafv2_web_acl_association.api_gateway` resource is gated behind `var.enable_api_gateway_association` (default `false`), and the root `module "waf"` block does not override it. WAFv2 regional WebACLs cannot associate with API Gateway v2 (HTTP API) stages, and Taskly's API is `aws_apigatewayv2_api`. The net posture is that the API runs with no WAF actively in front of it — the created-but-unattached WebACL is inert until the API is fronted by CloudFront (CLOUDFRONT scope) or migrated to a REST API. This is a security reduction versus the stated architecture, so it should be an explicit sign-off rather than an implicit default.

### DNS failover gated on hosted zone

`aws_route53_record.api_primary` and `api_secondary` are gated with `count = var.hosted_zone_id != "" ? 1 : 0` (default `""`); the rest of the disaster-recovery module still applies without them, and the guard references only `var.hosted_zone_id`, so there is no dependency on a disabled resource. Supplying a real zone later brings the records back with no other change.

### Lambda bundle: one full-backend zip, pruned to fit 250 MB

```
build-lambda.sh
backend/ ──────────────────────► api-handler.zip ──┬─► taskly-dev-api
(full source + prod node_modules, ├─► achievement-processor (identical copy)
minus @aws-sdk/@smithy/@img/sharp/core-js) ├─► notification-processor (identical copy)
└─► email-processor (identical copy)
```

One identical full-backend bundle to all four functions means every handler path (`index.handler`, `lambda/processors/*.handler`) resolves no matter which zip loads. Pruning `sharp`/`@img` is confirmed safe: the only importer is `lambda/processors/image-processor.js`, which is not one of the four deployed handlers and is not transitively imported by `server.js` or `lambda/handler.js`.

Pruning `@aws-sdk`/`@smithy` is the load-bearing assumption. `server.js` statically imports `routes/upload.js`, which statically imports `@aws-sdk/s3-request-presigner` and `@aws-sdk/client-s3`; `config/aws.js`, `utils/secrets.js`, `services/emailService.js`, and the processors import other `@aws-sdk/client-*` packages. These are top-level ESM imports resolved at cold-start module load, so a missing one would throw `ERR_MODULE_NOT_FOUND` before `/api/health` could return. The observed 200 proves the full module graph loaded and the `nodejs20.x` runtime provided all of them — the prune is verified safe on the current runtime. The residual risk is version coupling: it depends on the runtime bundling AWS SDK v3, which AWS has signaled will not hold on future runtimes. The runtime pin and the prune list are now coupled and must move together.

The script fetches the DocumentDB global TLS bundle into the archive root, is idempotent, and only zips entries that exist (missing optional dirs warn rather than fail). No `*.zip` is tracked in git.

### Backend and variable wiring

Every pass-through the dev wrapper sends into `module.taskly` (`documentdb_master_password`, `jwt_signing_key`, `ses_domain`, `hosted_zone_id`, `domain_name`, `documentdb_instance_class`, `documentdb_instance_count`, `vpc_cidr`, `enable_cloudfront`) is declared in the root module — verified by name, so no pass-through errors on an undeclared variable. The previously-undeclared tuning variables (`api_handler_memory`, throttling/WAF/log/budget knobs) are now declared in the dev `variables.tf` to silence undeclared-variable warnings; the file's own note states the root does not yet consume several of them, so they are inert placeholders, not functional wiring — worth knowing that setting e.g. `api_handler_memory` in tfvars currently has no effect. The dev `outputs.tf` re-exports nine root outputs, all of which exist in the root. The backend bucket rename matches the real bucket; the lock table was created out-of-band.

### S3 replication and monitoring ordering fixes

Two apply-time races were fixed with explicit edges. The uploads replication config now `depends_on = [aws_s3_bucket_versioning.uploads_replica]` (versioning must be enabled on the destination before a replication config referencing it can be created) and adds `delete_marker_replication { status = "Enabled" }` (required by the V2 replication schema S3 selects once a `filter {}` is present). Both referenced resources exist in the module. The monitoring module now consumes `module.lambda.api_handler_log_group_name` — sourced from `aws_cloudwatch_log_group.api_handler.name` — instead of an interpolated `/aws/lambda/${name}` string, building a real dependency edge so metric filters are not created before the log group exists. This is a genuine ordering fix, not cosmetic.

### Secret-leak check

The two secrets are declared `sensitive`, `documentdb_endpoint` outputs are `sensitive`, and the new `infrastructure/.gitignore` excludes `*.auto.tfvars`, `environments/*/secret.auto.tfvars`, state files, and plan output. `git ls-files` shows no tracked `secret.auto.tfvars`, and the deployment report redacts the password.

</details>

<details>
<summary>File map</summary>

- `infrastructure/environments/dev/backend.tf` — state bucket reconciled to the real account-suffixed bucket.
- `infrastructure/environments/dev/main.tf` — pass secrets, DNS/email, sizing, and `enable_cloudfront` into `module.taskly`.
- `infrastructure/environments/dev/variables.tf` — declare required secrets, DNS/sizing, and previously-undeclared tuning vars.
- `infrastructure/environments/dev/outputs.tf` — re-export nine root outputs for consumers.
- `infrastructure/environments/dev/terraform.tfvars` — set `enable_cloudfront = false` with rationale.
- `infrastructure/main.tf` — wire `enable_distributions` and switch monitoring to the lambda log-group output.
- `infrastructure/variables.tf` — root `enable_cloudfront` flag (default true).
- `infrastructure/modules/cloudfront/{main,outputs,variables}.tf` — conditional distributions/policies, `one(...)` outputs with S3 fallback, `enable_distributions` var.
- `infrastructure/modules/waf/{main,variables}.tf` — conditional API Gateway association, default false.
- `infrastructure/modules/disaster-recovery/dns-failover.tf` — DNS records gated on non-empty hosted zone.
- `infrastructure/modules/disaster-recovery/main.tf` — replication `depends_on` versioning + `delete_marker_replication`.
- `infrastructure/modules/lambda/outputs.tf` — new `api_handler_log_group_name` output.
- `infrastructure/.gitignore` — exclude state, secrets, plans.
- `scripts/build-lambda.sh` — build four full-backend Lambda zips, prune SDK/native modules, fetch TLS bundle.
- `.tasks/task-deploy-infra/**` — task state and deployment report.

Full diff: `git diff main -- infrastructure/ scripts/build-lambda.sh`

</details>
Loading
Loading