feat(o11y): add o11y terraform module for VM - #5341
Conversation
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
hassaansaleem28
changed the title
|
Contribution validation failed:
|
|
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:
Summary by CodeRabbit
WalkthroughAdds an optional VictoriaMetrics module for ECS Fargate. The module provisions encrypted EFS storage, security groups, CloudWatch logging, IAM permissions, an ARM64 task definition, and a single ECS service. Live infrastructure supplies configuration and exposes selected resource identifiers. ChangesVictoriaMetrics observability
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Contribution validation failed:
|
4 similar comments
|
Contribution validation failed:
|
|
Contribution validation failed:
|
|
Contribution validation failed:
|
|
Contribution validation failed:
|
![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
🤖 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 `@infrastructure/live/main.tf`:
- Around line 253-271: The observability toggle currently owns retained EFS
storage through the conditional observability module, allowing Terraform to
delete it when disabled. In infrastructure/live/main.tf lines 253-271, keep the
module toggle limited to ECS runtime resources and move EFS ownership into
persistent storage configuration; in
infrastructure/modules/observability/main.tf lines 117-123, stop declaring or
managing the retained metrics EFS there, and add an explicit decommission path
for intentional removal.
In `@infrastructure/modules/observability/main.tf`:
- Around line 248-270: Update the aws_ecs_service.vm resource to register tasks
through Cloud Map using service_registries, configure the required private DNS
namespace/service resources, and output the resulting stable VictoriaMetrics DNS
name for clients. Preserve the existing Fargate networking and service settings;
use an internal NLB only if the current client requirements cannot use Cloud
Map.
- Around line 117-119: In infrastructure/modules/kms/main.tf, add a KMS policy
statement that grants EFS the necessary encrypt and decrypt permissions scoped
to the EFS service using a kms:ViaService condition for
elasticfilesystem.<region>.amazonaws.com. This will allow the
aws_efs_file_system resource in infrastructure/modules/observability/main.tf to
successfully use var.kms_key_arn without AccessDenied errors. Update any KMS
policy test coverage to include verification of the new EFS scoped condition
alongside the existing CloudWatch Logs condition.
- Around line 78-86: Update aws_security_group_rule.vm_egress_https to remove
the unrestricted 0.0.0.0/0 HTTPS egress; restrict outbound access to approved
AWS service endpoint ranges required by the task, or remove this rule and use
the approved egress proxy configuration.
- Around line 238-245: Update the ECS volume’s EFS configuration in the
observability module to require IAM authorization by setting
authorization_config.iam to ENABLED. Add an EFS access point, a task role
limited to elasticfilesystem:ClientMount and elasticfilesystem:ClientWrite, and
a file-system policy requiring TLS, the access point, and that task role; wire
these resources into the existing vm-data efs_volume_configuration.
In `@infrastructure/modules/observability/variables.tf`:
- Around line 55-58: Update the vm_image variable definition to validate that
the value references a SHA-256 image digest and document that the image must be
a linux/arm64 artifact, matching the ECS task’s ARM64 runtime contract. Keep the
existing string type and description scope while adding validation that rejects
non-digest image references.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: affcc8dd-bca3-4410-a58b-0dd543129005
📒 Files selected for processing (5)
infrastructure/live/main.tfinfrastructure/live/variables.tfinfrastructure/modules/observability/main.tfinfrastructure/modules/observability/outputs.tfinfrastructure/modules/observability/variables.tf
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
|
Contribution validation failed:
|
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 15 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@infrastructure/modules/observability/README.md`:
- Line 67: Add the missing trailing newline after the END_TF_DOCS marker in the
README, then rerun the pre-commit end-of-file-fixer to verify the file remains
unchanged.
- Line 52: Update the subnet_ids variable description in the source Terraform
variable definition to document both private and public subnet usage when NAT
gateways are disabled, then regenerate
infrastructure/modules/observability/README.md so its input table matches the
updated description.
In `@infrastructure/modules/observability/tests/unit.tftest.hcl`:
- Around line 111-118: Update the test_task_mounts_encrypted_efs_volume
assertion to also verify that the vm-data volume’s EFS file system ID is
aws_efs_file_system.vm.id. Target the volume identified by its vm-data name,
while preserving the existing transit_encryption == "ENABLED" validation.
- Around line 66-72: Update the test_efs_ingress_from_vm_only assertion to
require source_security_group_id == aws_security_group.vm.id and to_port ==
2049, while preserving the existing from_port and ingress checks. Keep the
assertion focused on the aws_security_group_rule.efs_from_vm production rule.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1238403e-527b-4431-878a-321bdb97dc76
📒 Files selected for processing (3)
infrastructure/live/README.mdinfrastructure/modules/observability/README.mdinfrastructure/modules/observability/tests/unit.tftest.hcl
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/o11y #5341 +/- ##
=============================================
Coverage 98.84% 98.84%
=============================================
Files 541 543 +2
Lines 17165 17235 +70
Branches 2464 2472 +8
=============================================
+ Hits 16966 17036 +70
Misses 99 99
Partials 100 100
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 9 unresolved issues from previous reviews.
Re-trigger cubic
|
Contribution validation failed:
|
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
|
Contribution validation failed:
|
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
|
Contribution validation failed:
|
|
Contribution validation failed:
|
|
please use "re-request review" button to notify users |
ah, my bad. |
|
no issues! |
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
great work, LGTM! I tested it in LocalStack, and it works.
I only have one blocking request, rest can be addressed when you want.
Also, I think you need to address the false positive semgrep findings.
| observability_vm_image = trimspace(trimprefix( | ||
| one([for line in split("\n", file("${path.root}/../../docker/victoriametrics/Dockerfile")) : line if startswith(line, "FROM ")]), | ||
| "FROM " | ||
| )) |
There was a problem hiding this comment.
can it be something like this?
observability_vm_image = regex("(?m)^FROM (victoriametrics/victoria-metrics:\\S+)", file("${path.root}/../../docker/victoriametrics/Dockerfile"))[0]| @@ -0,0 +1 @@ | |||
| .terraform.lock.hcl | |||
There was a problem hiding this comment.
I think we should commit this lockfile in this setup well.
| environment = var.environment | ||
| kms_key_arn = module.kms.key_arn | ||
| project_name = var.project_name | ||
| subnet_ids = var.enable_nat_gateway ? module.networking.private_subnet_ids : module.networking.public_subnet_ids |
There was a problem hiding this comment.
I don't think VictoriaMetrics container should run in public subnet. This is risky. Why do we need to do this?
There was a problem hiding this comment.
I think I understand now, this is required for staging as it has no NAT.
I originally disabled NAT for staging to reduce costs. But since we have AWS's support for credits and all, I think we can afford a NAT for staging.
If @arkid15r agrees, can you remove enable_nat_gateway and enable it for both staging and production? It'll require some minor refactor in networking module, but it should be straightforward. Just use private subnet where there's currently a conditional expression based on this variable.
Not only this, it reduces backend and frontend task/container attack surface as they no longer get a public IP.
There was a problem hiding this comment.
It's fine if you want to do it later in a different PR, but please add a TODO comment for now if you do so.
| common_tags = { Environment = "test", Project = "nest" } | ||
| environment = "test" | ||
| project_name = "nest" | ||
| vm_image = "victoriametrics/victoria-metrics:v1.145.0@sha256:c014fb5a711d38cb24fd0673197592cd1394bb903dbb16aea565620c9c8a3d70" |
There was a problem hiding this comment.
can we use the same pattern here?
| image: victoriametrics/victoria-metrics:v1.145.0@sha256:c014fb5a711d38cb24fd0673197592cd1394bb903dbb16aea565620c9c8a3d70 | ||
| build: | ||
| context: ../../docker/victoriametrics | ||
| dockerfile: Dockerfile |
There was a problem hiding this comment.
Just pointing out that it runs as root but ECS task runs as non-root. Maybe something to keep in mind, no action required for this PR.
| variable "vm_retention_period" { | ||
| description = "The VictoriaMetrics data retention period (e.g., 12, 5y)." | ||
| type = string | ||
| default = "12" | ||
| } |
There was a problem hiding this comment.
12? please update to make it more explicit that it's 12 months. Both description and default.
| } | ||
|
|
||
| resource "aws_security_group_rule" "vm_ingest_from_apps" { | ||
| for_each = toset(var.app_security_group_ids) |
There was a problem hiding this comment.
this breaks on an empty state apply. This is localstack logs but same will happen in prod/staging:
Plan: 21 to add, 0 to change, 0 to destroy.
╷
│ Error: Invalid for_each argument
│
│ on ../../modules/observability/main.tf line 67, in resource "aws_security_group_rule" "vm_ingest_from_apps":
│ 67: for_each = toset(var.app_security_group_ids)
│ ├────────────────
│ │ var.app_security_group_ids is list of string with 3 elements
│
│ The "for_each" set includes values derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the
│ instances of this resource.
│
│ When working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time
│ results.
│
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge.
╵
╷
│ Error: Invalid for_each argument
│
│ on ../../modules/observability/main.tf line 130, in resource "aws_efs_mount_target" "vm":
│ 130: for_each = toset(var.subnet_ids)
│ ├────────────────
│ │ var.subnet_ids is list of string with 2 elements
│
│ The "for_each" set includes values derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the
│ instances of this resource.
│
│ When working with unknown values in for_each, it's better to use a map value where the keys are defined statically in your configuration and where only the values contain apply-time
│ results.
│
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a second time to fully converge.
╵
make: *** [infrastructure-apply-observability] Error 1
It is because for_each needs to know its keys at plan time. But since we're passing the values from resources that haven't been created yet, those values are "known after apply". Since length of list is known at plan time, you should use count = length(var.app_security_group_ids) instead.
Example:
Signed-off-by: hassaansaleem28 <iamhassaans@gmail.com>
8953fc5
|
Contribution validation failed:
|
2 similar comments
|
Contribution validation failed:
|
|
Contribution validation failed:
|
|
❌ The last analysis has failed. |
2 participants
Proposed change
Resolves #5327
Add terraform module for observability.
/datavolume so metrics survive task restartslive/behindenable_observability--terraform planshows no changes until turned ondocker/victoriametrics/Dockerfileso dependabot tracks it and local/prod stay in syncterraform testsuite -- 16 unit tests (mocked) + an integration test applied against LocalStackChecklist