terraforming
terraforming copied to clipboard
Add security group individual rule descriptions
This addresses the concern from https://github.com/dtan4/terraforming/issues/437. This PR adds a description into each rule block in an SG.
ingress/egress {
from_port = ...
to_port = ...
protocol = ...
cidr_blocks = ...
}
is now
ingress/egress {
description = "<description>"
from_port = ...
to_port = ...
protocol = ...
cidr_blocks = ...
}
There is a limitation to this approach though. When creating a rule with multiple cidr blocks or security groups, 2 separate rules are created in the AWS console, but get processed by the API/terraforming as 1 rule with 2 cidr blocks / security groups.
If you were to (1) create a rule like described above, and (2) manually edit the description of one of those rules in the AWS console, the current way terraforming is structured (using in-line rules) would not be able to preserve the changed description.
As discussed in https://github.com/dtan4/terraforming/issues/262, using these in-line rules allows for mixing what are really multiple rules into one rule block. When using the separate aws_security_group_rule
resource, only one of cidr_blocks
, ipv6_cidr_blocks
, security_groups
and self
are allowed, which allows you to ensure that the separate descriptions for each are preserved.
This isn't a major issue as I don't think what I described impacts most use cases of terraforming, but be aware that it exists and that the ideal solution is to migrate to creating aws_security_group_rule
resources alongside the main aws_security_group
resource.
Tagging @nitrocode who wrote up the issue.
Coverage remained the same at 100.0% when pulling 85306f699cd263138ceb7eb2beed4834de821d2b on liamrahav:pr/sg-rule-descriptions into 67cb9299f283bc16bd70c197f25edc419bee280f on dtan4:master.
@dtan4 @waqark3389 @liamrahav
Nice job! LGTM!
ALL CHEER MERGE MERGE MERGE MERGE MERGE
Please merge 🙏
@liamrahav & @dtan4, this change would save us a lot of time with a large import we're currently doing, do you know if there are any blockers to merging it or when we could expect it to land? cc @arbabkhalil
@adzuci I don't have the ability to merge anything, sorry! Haven't looked at this in a while, but I suppose you can merge my branch locally for now if needed.