Add security group individual rule descriptions

liamrahav commented 5 years ago

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.

liamrahav commented 5 years ago

Tagging @nitrocode who wrote up the issue.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 85306f699cd263138ceb7eb2beed4834de821d2b on liamrahav:pr/sg-rule-descriptions into 67cb9299f283bc16bd70c197f25edc419bee280f on dtan4:master.

nitrocode commented 4 years ago

@dtan4 @waqark3389 @liamrahav

Nice job! LGTM!

waqarkhan3389 commented 4 years ago

ALL CHEER MERGE MERGE MERGE MERGE MERGE

skwp commented 3 years ago

Please merge 🙏

adzuci commented 2 years ago

@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

liamrahav commented 2 years ago

@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.

dtan4 / terraforming

Add security group individual rule descriptions #475