Feature request - VPC flow log

[antonbabenko]

Inspired by terraform-community-modules/tf_aws_vpc#64 and https://github.com/GSA/terraform-vpc-flow-log

Some points to take into account when adding VPC flow log feature to this VPC module:

• Make creation of aws_flow_log conditional. Disabled by default.
• Make creation of aws_cloudwatch_log_group conditional. User should be able to provide existing log group.
• Limit resources in log_policy to only what is necessary.
• Make creation of aws_iam_role conditional. User should be able to provide existing IAM role.
• Do not create aws_cloudwatch_log_group and aws_iam_role resources, if aws_flow_log is disabled.
• Make changes to an existing complete-vpc example
• Update README

[bcenker]

I started to implement this, but wanted to get your thoughts on the following before I continue:

1. Do you think it would be better to have the user create an IAM role outside the scope of the VPC module and be forced to pass it in? Most cases I've seen have a single flowlogs role at the account level that is used by every flowlog in the account. Managing the IAM role in the VPC module could create an issue if the user wanted to destroy the VPC terraform that defined the role if that role was shared among other flowlogs in other VPC terraforms. Additionally, if I have 30 (or some arbitrarily large number of) VPCs terraformed with this module and they all shared an IAM role that I created in the one of them, I wouldn't want to have to remember which one I defined the role in if I wanted to modify it's permissions.

2. I am running into the same issue with referencing the conditionally created aws_cloudwatch_log_group resource that I ran into with when trying to use the ipv6_address_block attribute of a vpc that did not have ipv6 enabled. Namely, if the user specifies true that they want to create a log group, everything works as expected, but if the user specifies they want to use an existing log group, it is not possible (to my knowledge) to assign the log group name in the aws_flow_log resource based on this condition.
   i.e. this doesn't work (beware long variable names ahead):

log_group_name = "${var.vpc_flowlogs_create_new_cloudwatch_log_group ? aws_cloudwatch_log_group.this.name : var.vpc_flowlogs_use_existing_cloudwatch_log_group_name }"

Basically, if the referenced aws_cloudwatch_log_group resource is not created (which it would not be if the user specified var.vpc_flowlogs_create_new_cloudwatch_log_group as false), this ternary operation fatals when running terraform apply. This would mean, unless we can find a better way, the continued duplication of code for conditional resource creation.

3. I've seen in a couple cases where users want to log ACCEPT and REJECT traffic to different log groups, which means creating two flowlogs in a single VPC. This would likely not be feasible without modularizing the aws_flow_log. Although this feels like a corner case to me (since there are many ways to segment the ACCEPT and REJECT traffic from a single ALL traffic flowlog), I would be interested in knowing if you think it's worth offering this flexibility.

All that being said, the undesired duplication of code required to implement the conditionals has me wondering if you think it might be better to modularize the aws_flow_log resources outside of the VPC module, and then possibly consume that module within the VPC module (not sure how you feel about modules consuming modules, I'm not even sure how I feel about that). Please let me know what you think - I can continue to try to implement this using the same conditionally created resource method I used for the ipv6 feature (or maybe you have found a way around that which would be awesome) or we can look for other ways to get this done - I'll wait for you to let me know which way you'd like to proceed.

This was way longer than I expected - please let me know if anything needs further clarification.

Thanks!

[toringe]

Just a small comment on your no. 1. Our company's internal security compliance requires enabling of flow logs for all vpcs across all accounts. We have a single IAM Role that is created when new accounts are created, and this is the role used when adding flow logs for vpcs such that read access is granted to our global security office. So in our use-case, it would complicate things if terraform removed this role when destroying a vpc.

[antonbabenko]

🎉 v2.26.0 has been just released with VPC Flow Log supported.

Please let me know if there are any issues.

@toringe you should be able to use this module for your described use-case also. See examples for more details.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.