Infra reorg#27
Conversation
Deployment configs now live under infra/, organized per target. Dockerfiles and docker-compose.yml intentionally stay at the repo root (shared by CI, build scripts, and the macOS app). Deploy with: fly deploy -c infra/fly/fly.toml Co-Authored-By: OpenClaude <[email protected]>
infra/aws/ provisions a production-leaning single-node deployment: - t4g.small (AL2023 arm64) running the published ghcr.io/gitlawb/node image + postgres:16 via docker compose - separate encrypted EBS data volume (prevent_destroy) with daily DLM snapshots; survives instance replacement - postgres password via random_password -> SSM SecureString, fetched at boot by instance profile (never in user-data); optional secrets (operator key, Pinata JWT, S3 secret) follow the same path - SSM Session Manager access (no SSH by default), IMDSv2 required, metrics port closed unless explicitly opened - SSM command document for image upgrades (user-data runs once) Verified: terraform fmt/init/validate clean; rendered compose passes docker compose config; rendered user-data passes bash -n. Co-Authored-By: OpenClaude <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AWS Terraform infra for a single EC2 gitlawb node, Docker Compose/user-data bootstrap, SSM-stored secrets and DLM snapshots, supporting docs, and a Fly.io Dockerfile path fix. ChangesDeployment documentation
AWS Terraform infrastructure
Sequence Diagram(s)sequenceDiagram
participant EC2 as EC2 instance
participant UserData as user-data script
participant Docker as Docker/Compose
participant SSM as SSM Parameter Store
participant EBS as EBS volume
EC2->>UserData: cloud-init executes
UserData->>Docker: install Docker + Compose
UserData->>EBS: discover unmounted disk
UserData->>EBS: format ext4 if needed
UserData->>EBS: mount to /mnt/data
UserData->>SSM: fetch postgres_password and optional secrets
UserData->>UserData: write /opt/gitlawb/.env and compose.yaml
UserData->>Docker: docker compose pull
UserData->>Docker: docker compose up -d --remove-orphans
Docker-->>EC2: Postgres + node running
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
infra/aws/main.tf (1)
165-172: ⚡ Quick winGuard against opening SSH without a key pair.
When
ssh_ingress_cidris set butssh_key_nameis null, SSH gets exposed but isn’t actually usable. Add a plan-time precondition so this misconfiguration fails fast.🔒 Suggested guardrail
resource "aws_instance" "node" { ami = nonsensitive(data.aws_ssm_parameter.al2023_arm64.value) instance_type = var.instance_type subnet_id = local.subnet_id vpc_security_group_ids = [aws_security_group.node.id] iam_instance_profile = aws_iam_instance_profile.node.name key_name = var.ssh_key_name user_data = local.user_data tags = merge(local.common_tags, { Name = var.name_prefix }) + lifecycle { + precondition { + condition = var.ssh_ingress_cidr == null || var.ssh_key_name != null + error_message = "ssh_key_name must be set when ssh_ingress_cidr is configured." + } + ignore_changes = [ami] + } - - lifecycle { - # New AL2023 AMI releases shouldn't force-replace the instance; replace - # deliberately (taint) for OS upgrades. user_data changes also only apply - # at first boot — use the upgrade SSM command for image bumps. - ignore_changes = [ami] - } }Also applies to: 251-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/aws/main.tf` around lines 165 - 172, Add a plan-time precondition to the resource that creates the SSH rule (the block containing the for_each using var.ssh_ingress_cidr) so Terraform fails if ssh_ingress_cidr is set but ssh_key_name is null; implement a precondition like: condition = var.ssh_ingress_cidr == null || var.ssh_key_name != null and a clear error_message, referencing var.ssh_ingress_cidr and var.ssh_key_name to guard the aws_security_group_rule (or the resource that contains the shown content) from exposing unusable SSH access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/aws/outputs.tf`:
- Around line 6-9: The output "api_url" currently reconstructs the endpoint from
aws_eip.node.public_ip and var.gitlawb_port and ignores the module override
var.public_url/local.public_url; change the output "api_url" value to return the
module's public_url when provided (i.e., use var.public_url or local.public_url
if non-empty) and only fall back to building
"http://${aws_eip.node.public_ip}:${var.gitlawb_port}" when public_url is unset,
so the output honors the module contract.
In `@infra/aws/README.md`:
- Around line 7-15: Add a language tag to the fenced code block that begins with
"Elastic IP ──► EC2 t4g.small (Amazon Linux 2023, arm64)" by changing the
opening ``` to ```text so the block is explicitly marked as plain text (this
fixes the MD040 markdownlint warning).
In `@infra/aws/user-data.sh.tftpl`:
- Around line 3-4: Remove command tracing around secret handling: change the
global "set -euxo pipefail" to "set -euo pipefail" (remove the -x) and/or
explicitly disable tracing just around the SSM fetch and .env rendering blocks
by inserting "set +x" before those sections and "set -x" after them; target the
existing "set -euxo pipefail" invocation and the secret-handling block (the SSM
fetch/.env rendering region referenced in the diff) so secrets are never written
to /var/log/gitlawb-bootstrap.log.
- Around line 14-16: The curl download of Docker Compose (the command using
COMPOSE_VERSION and uname -m) lacks retry and timeout controls; update that curl
invocation to add robust flags such as --fail, --retry with a nonzero count,
--retry-connrefused, --retry-delay (or --retry-max-time) and a global --max-time
to bound total time, so transient network failures won't break first-boot
provisioning and the download will time out deterministically.
In `@infra/aws/variables.tf`:
- Around line 51-55: The snapshot_retain_count variable allows zero/negative
values; add a Terraform validation block to enforce a lower bound (e.g.,
var.snapshot_retain_count >= 1) so invalid values are rejected at plan time;
update the variable "snapshot_retain_count" to include validation { condition =
var.snapshot_retain_count >= 1 description = "snapshot_retain_count must be >=
1" } to prevent failures when aws_dlm_lifecycle_policy is applied.
In `@infra/README.md`:
- Around line 5-10: The fenced code block showing the infra directory tree is
missing a language tag (MD040); update the triple-backtick block that contains
the tree (the infra/ ├── fly/ ... └── aws/ lines) to use a language tag of
"text" (i.e. change ``` to ```text) so the markdownlint rule is satisfied.
---
Nitpick comments:
In `@infra/aws/main.tf`:
- Around line 165-172: Add a plan-time precondition to the resource that creates
the SSH rule (the block containing the for_each using var.ssh_ingress_cidr) so
Terraform fails if ssh_ingress_cidr is set but ssh_key_name is null; implement a
precondition like: condition = var.ssh_ingress_cidr == null || var.ssh_key_name
!= null and a clear error_message, referencing var.ssh_ingress_cidr and
var.ssh_key_name to guard the aws_security_group_rule (or the resource that
contains the shown content) from exposing unusable SSH access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baa07535-3f9e-447a-8a18-3397b0454bad
📒 Files selected for processing (13)
docs/RUN-A-NODE.mdinfra/README.mdinfra/aws/.gitignoreinfra/aws/.terraform.lock.hclinfra/aws/README.mdinfra/aws/compose.yaml.tftplinfra/aws/main.tfinfra/aws/outputs.tfinfra/aws/terraform.tfvars.exampleinfra/aws/user-data.sh.tftplinfra/aws/variables.tfinfra/aws/versions.tfinfra/fly/fly.toml
- api_url output now honors the public_url override (local.public_url) - disable xtrace around SSM secret fetch + .env write so secret values never reach the bootstrap log - add retry/timeout flags to the docker compose download - validate snapshot_retain_count (1-1000) at plan time - add language tags to fenced tree blocks (MD040) Co-Authored-By: OpenClaude <[email protected]>
|
@coderabbitai please review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== infra/fly/fly.toml ==="
cat infra/fly/fly.toml
echo ""
echo "=== infra/README.md ==="
cat infra/README.md
echo ""
echo "=== infra/aws/README.md ==="
cat infra/aws/README.md
echo ""
echo "=== infra/aws/main.tf ==="
cat infra/aws/main.tf
echo ""
echo "=== infra/aws/variables.tf ==="
cat infra/aws/variables.tf
echo ""
echo "=== infra/aws/outputs.tf ==="
cat infra/aws/outputs.tf
echo ""
echo "=== infra/aws/user-data.sh.tftpl ==="
cat infra/aws/user-data.sh.tftpl
echo ""
echo "=== infra/aws/compose.yaml.tftpl ==="
cat infra/aws/compose.yaml.tftpl
echo ""
echo "=== infra/aws/versions.tf ==="
cat infra/aws/versions.tf
echo ""
echo "=== infra/aws/terraform.tfvars.example ==="
cat infra/aws/terraform.tfvars.example
echo ""
echo "=== infra/aws/.gitignore ==="
cat infra/aws/.gitignore
echo ""
echo "=== infra/aws/.terraform.lock.hcl ==="
cat infra/aws/.terraform.lock.hcl
echo ""
echo "=== docs/RUN-A-NODE.md ==="
cat docs/RUN-A-NODE.mdLength of output: 42487
PR Review: Infra ReorgOverall this is a solid, well-structured Terraform deployment with good security practices (SSM secrets, IMDSv2, encrypted EBS, 🔴 Critical / High1. Security Group VPC mismatch when
|
- security group now derives its VPC from the selected subnet, so subnet_id overrides into non-default VPCs work - s3_access_key_id moves to SSM SecureString (same path as the secret key) instead of being embedded in user-data and state - ignore user_data drift on the instance: it only runs at first boot, so re-rendering caused pointless stop/starts; README documents the -replace workflow for config changes - DLM snapshot targeting is stack-specific (Snapshot + Name tags), not any Snapshot=true volume in the account - comment in fly.toml that it targets the shared test instance - user-data compose up gains --remove-orphans (consistent with the upgrade SSM document) - node service gets an HTTP /health healthcheck in compose Co-Authored-By: OpenClaude <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
infra/aws/main.tf (1)
75-81: 💤 Low valueConsider supporting a customer-managed KMS key (optional).
The static analysis hint flags that this SSM parameter uses the AWS-managed
aws/ssmkey rather than a customer-managed CMK. For production deployments requiring stricter key rotation policies or cross-account audit trails, adding an optionalkms_key_idvariable (defaulting tonullfor AWS-managed) would provide flexibility:key_id = var.ssm_kms_key_id # null → AWS-managed defaultThis is acceptable as-is for most use cases, especially given the explicit acknowledgment at lines 94-96.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/aws/main.tf` around lines 75 - 81, The aws_ssm_parameter resource aws_ssm_parameter.s3_access_key currently stores a SecureString using the AWS-managed key; add optional support for a customer-managed KMS key by introducing a new variable (e.g., var.ssm_kms_key_id with default null) and set the resource's key_id attribute to that variable (key_id = var.ssm_kms_key_id) so null continues to use AWS-managed aws/ssm while allowing a CMK when provided; update any docs/variables.tf accordingly and keep the existing count/name/type/value/tags logic in aws_ssm_parameter.s3_access_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@infra/aws/main.tf`:
- Around line 75-81: The aws_ssm_parameter resource
aws_ssm_parameter.s3_access_key currently stores a SecureString using the
AWS-managed key; add optional support for a customer-managed KMS key by
introducing a new variable (e.g., var.ssm_kms_key_id with default null) and set
the resource's key_id attribute to that variable (key_id = var.ssm_kms_key_id)
so null continues to use AWS-managed aws/ssm while allowing a CMK when provided;
update any docs/variables.tf accordingly and keep the existing
count/name/type/value/tags logic in aws_ssm_parameter.s3_access_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a7be83-4f60-46cc-94bb-d03978ff2f00
📒 Files selected for processing (6)
infra/aws/README.mdinfra/aws/compose.yaml.tftplinfra/aws/main.tfinfra/aws/terraform.tfvars.exampleinfra/aws/user-data.sh.tftplinfra/fly/fly.toml
✅ Files skipped from review due to trivial changes (3)
- infra/fly/fly.toml
- infra/aws/terraform.tfvars.example
- infra/aws/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- infra/aws/compose.yaml.tftpl
- infra/aws/user-data.sh.tftpl
Add ssm_kms_key_id (default null = AWS-managed aws/ssm key). When set, all five SecureString parameters are encrypted with the CMK and the instance role gains kms:Decrypt scoped to that key — without it the boot-time secret fetch would fail. Co-Authored-By: OpenClaude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/aws/main.tf`:
- Around line 134-145: The KMS Decrypt statement currently grants broad access
whenever var.ssm_kms_key_id != null; update the IAM statement that builds
Statement (the block referencing local.secret_param_arns and
data.aws_kms_key.ssm[0].arn) to add a Condition restricting kms:Decrypt to SSM
usage by including kms:ViaService = "ssm.<region>.amazonaws.com" (use the
module/stack region value) and an EncryptionContext condition using the
PARAMETER_ARN key (kms:EncryptionContext:PARAMETER_ARN) limited to
local.secret_param_arns so decrypt can only be performed for the SSM Parameter
ARNs you manage rather than the entire CMK.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b68064eb-b00c-4b6d-8dc2-86878ea5e6cf
📒 Files selected for processing (4)
infra/aws/README.mdinfra/aws/main.tfinfra/aws/terraform.tfvars.exampleinfra/aws/variables.tf
✅ Files skipped from review due to trivial changes (1)
- infra/aws/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- infra/aws/terraform.tfvars.example
- infra/aws/variables.tf
The grant on the customer-managed key was unconditioned, allowing the instance role to decrypt any ciphertext under the same CMK. Restrict it with kms:ViaService = ssm.<region>.amazonaws.com and kms:EncryptionContext:PARAMETER_ARN limited to the stack's parameter ARNs. Co-Authored-By: OpenClaude <[email protected]>
1 participant
move fly files under infra directory
Summary by CodeRabbit
Documentation
New Features
Chores