feat(ha-hot-hot/aws): VPC flow logs, matching single-vm/unlimited-scale#52
Open
dmchaledev wants to merge 1 commit into
Open
feat(ha-hot-hot/aws): VPC flow logs, matching single-vm/unlimited-scale#52dmchaledev wants to merge 1 commit into
dmchaledev wants to merge 1 commit into
Conversation
SECURITY-DEFAULTS.md claims "VPC Flow Logs ... enabled by default (enable_flow_logs = true)" for every module, but ha-hot-hot/aws had no enable_flow_logs variable, no aws_flow_log resource, and no flow-log IAM role — tracked in #47 as the AWS-side half of a larger cross-module gap. Ports the existing pattern from unlimited-scale/aws verbatim: aws_cloudwatch_log_group.flow_logs, aws_iam_role.flow_logs, aws_iam_role_policy.flow_logs, aws_flow_log.vpc, all gated on enable_flow_logs (default true). Adds flow_log_group_name output and forwards the variable/output through asm-aws-ha and sat-aws-ha. Adds terraform test coverage: default-on assertion in tests/basic.tftest.hcl, plus enabled/disabled plan checks in tests/feature_flags.tftest.hcl. No existing resources changed; enable_flow_logs=true is additive only. Existing deployments get one new CloudWatch log group, IAM role, and flow log on their next apply. The Azure NSG flow-log side of #47 (single-vm/azure, ha-hot-hot/azure, unlimited-scale/azure — no Azure module has this at all) is out of scope here per #47's own recommendation to split AWS and Azure work. Verified locally: terraform validate and terraform test (8/8 passing, including the 2 new flow-log runs) on ha-hot-hot/aws, asm-aws-ha, and sat-aws-ha. tflint/checkov/trivy could not run in this sandbox (network policy blocks the release-artifact downloads); the change is a verbatim copy of the pattern already tflint/checkov-clean in unlimited-scale/aws, and no new checkov suppression is expected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRS3XkFnAjqmeaK5v9Vv5q
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SECURITY-DEFAULTS.mdstates unconditionally that "VPC Flow Logs ... are enabled by default (enable_flow_logs = true)". That's true forsingle-vm/awsandunlimited-scale/aws, butha-hot-hot/awshad noenable_flow_logsvariable, noaws_flow_logresource, and no flow-log IAM role at all — soasm-aws-haandsat-aws-hacustomers got no network flow visibility despite the docs promising it.This gap (and the parallel Azure-side gap — no Azure module has NSG flow logs today) is tracked in #47, which explicitly recommends splitting the fix into an AWS PR (this one) and a separate Azure PR, since Azure needs a destination-resource decision first (Log Analytics Workspace vs. Storage Account) that AWS doesn't.
Changes
Ports the existing pattern from
modules/unlimited-scale/aws/main.tfverbatim intomodules/ha-hot-hot/aws:aws_cloudwatch_log_group.flow_logs,aws_iam_role.flow_logs,aws_iam_role_policy.flow_logs,aws_flow_log.vpc— all gated oncount = var.enable_flow_logs ? 1 : 0enable_flow_logsvariable (bool, defaulttrue), matching the single-vm/unlimited-scale conventionflow_log_group_nameoutputasm-aws-haandsat-aws-ha(variable + output)terraform testcoverage: a default-on assertion added totests/basic.tftest.hcl, plus enabled/disabled plan checks added totests/feature_flags.tftest.hcl(mirrors the existingkms_*/redis_*flag-test pattern in the same file)No existing resources are modified.
enable_flow_logsdefaults totrue, so existingha-hot-hot/aws,asm-aws-ha, andsat-aws-hadeployments will see one newaws_cloudwatch_log_group, one newaws_iam_role, one newaws_iam_role_policy, and one newaws_flow_logon their nextterraform apply— no replacements, no downtime.Out of scope
The Azure side of #47 (
single-vm/azure,ha-hot-hot/azure,unlimited-scale/azure— none have NSG/Network Watcher flow logs) is left for a follow-up PR, per #47's own recommendation.Test plan
terraform validatepasses onmodules/ha-hot-hot/aws,modules/asm-aws-ha,modules/sat-aws-haterraform test— 8/8 passing onha-hot-hot/aws, including the 2 new flow-log runs (flow_logs_enabled_by_default,flow_logs_disabled_creates_nothing)tflint/checkov/trivy— not runnable in this sandbox (network policy blocks the release-artifact downloads for these tools); the added resources are a verbatim copy of the pattern already clean under all three inunlimited-scale/aws, and the existing global.checkov.yamlskip-checks (CKV_AWS_338,CKV_AWS_158) already cover this resource shape, so no new suppression is expected. Will confirm once CI runs.Closes the AWS half of #47.
Generated by Claude Code