feat(cloud): add VIP managed machines S3 configuration #303
Conversation
Add Terraform configuration for syncing VIP machines from S3.
WalkthroughThis PR adds VIP (Managed Machines) S3 integration to the Basilica API infrastructure. New Terraform variables, IAM policies, and environment variables configure the API service to read VIP machine configurations from an S3 bucket with configurable polling intervals. An additional ECS service output exposes the task role name. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/cloud/compute.tfscripts/cloud/modules/ecs-service/outputs.tfscripts/cloud/terraform.tfvars.examplescripts/cloud/variables.tf
🧰 Additional context used
🪛 Gitleaks (8.30.0)
scripts/cloud/compute.tf
[high] 413-413: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
scripts/cloud/modules/ecs-service/outputs.tf (1)
26-29: LGTM!The new output follows the established pattern and correctly exposes the task role name, enabling IAM policy attachments in the parent module.
scripts/cloud/variables.tf (1)
198-224: LGTM!The VIP variable definitions are well-structured with appropriate types, sensible defaults, and clear descriptions. The 60-second default polling interval provides reasonable freshness without excessive S3 API calls.
scripts/cloud/compute.tf (1)
410-415: LGTM!The VIP environment variable configuration correctly follows the nested naming convention and includes appropriate fallback logic for the S3 region. The
tostring()conversion for the polling interval ensures proper type handling.scripts/cloud/terraform.tfvars.example (1)
119-151: LGTM!The VIP configuration documentation is well-structured and provides clear guidance with:
- Descriptive section headers
- Example values for all variables
- Detailed CSV format specification including all required columns
- Helpful inline notes
This documentation will help users configure VIP machine synchronization correctly.
| resource "aws_iam_role_policy" "api_vip_s3_access" { | ||
| count = var.vip_s3_bucket != "" ? 1 : 0 | ||
| name = "${local.name_prefix}-api-vip-s3-access" | ||
| role = module.basilica_api_service.task_role_name | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetObject", | ||
| "s3:ListBucket" | ||
| ] | ||
| Resource = [ | ||
| "arn:aws:s3:::${var.vip_s3_bucket}", | ||
| "arn:aws:s3:::${var.vip_s3_bucket}/${var.vip_s3_key}" | ||
| ] | ||
| } | ||
| ] | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refine IAM policy to follow least privilege principle.
The current policy structure has several issues:
-
Incorrect resource mapping:
s3:ListBucketoperates on the bucket resource, whiles3:GetObjectoperates on object resources. Both actions shouldn't share the same Resource array. -
Missing wildcard for object access: Line 464 should specify objects under the key path, not just the exact key (especially if it's a prefix or could be empty).
-
Overly broad when
vip_s3_keyis empty: Ifvip_s3_keydefaults to"", line 464 becomesarn:aws:s3:::bucket/, which grants access to all objects.
🔎 Proposed IAM policy fix
resource "aws_iam_role_policy" "api_vip_s3_access" {
count = var.vip_s3_bucket != "" ? 1 : 0
name = "${local.name_prefix}-api-vip-s3-access"
role = module.basilica_api_service.task_role_name
policy = jsonencode({
Version = "2012-10-17"
Statement = [
+ {
+ Effect = "Allow"
+ Action = [
+ "s3:ListBucket"
+ ]
+ Resource = [
+ "arn:aws:s3:::${var.vip_s3_bucket}"
+ ]
+ },
{
Effect = "Allow"
Action = [
- "s3:GetObject",
- "s3:ListBucket"
+ "s3:GetObject"
]
Resource = [
- "arn:aws:s3:::${var.vip_s3_bucket}",
- "arn:aws:s3:::${var.vip_s3_bucket}/${var.vip_s3_key}"
+ var.vip_s3_key != "" ? "arn:aws:s3:::${var.vip_s3_bucket}/${var.vip_s3_key}" : "arn:aws:s3:::${var.vip_s3_bucket}/*"
]
}
]
})
}This separates ListBucket (bucket-level) from GetObject (object-level) permissions and handles the empty vip_s3_key case explicitly.
🤖 Prompt for AI Agents
In scripts/cloud/compute.tf around lines 448-469, the IAM policy mixes bucket-
and object-level ARNs and lacks a wildcard for object prefixes; update the
policy to grant s3:ListBucket only to the bucket ARN
("arn:aws:s3:::${var.vip_s3_bucket}") and grant s3:GetObject to object ARNs with
a wildcard for the key prefix (use "arn:aws:s3:::${var.vip_s3_bucket}/*" when
var.vip_s3_key is empty, otherwise
"arn:aws:s3:::${var.vip_s3_bucket}/${var.vip_s3_key}*"); implement this by
splitting the Statement into two entries (one for ListBucket with the bucket
ARN, one for GetObject with the conditional/wildcard object ARN) so least
privilege and correct resource mapping are enforced.
Adds Terraform configuration for syncing VIP (managed machines) from S3.
New variables:
vip_s3_bucket/vip_s3_key- S3 location of machines CSVvip_s3_region- Optional region overridevip_poll_interval_secs- Sync frequencyWhen
vip_s3_bucketis set, an IAM policy is attached to grant the API service S3 read access.Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.