terraforming icon indicating copy to clipboard operation
terraforming copied to clipboard

Add security group individual rule descriptions

Open liamrahav opened this issue 5 years ago • 7 comments

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 avatar Jun 26 '19 18:06 liamrahav

Tagging @nitrocode who wrote up the issue.

liamrahav avatar Jun 26 '19 18:06 liamrahav

Coverage Status

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

coveralls avatar Jun 26 '19 18:06 coveralls

@dtan4 @waqark3389 @liamrahav

Nice job! LGTM!

nitrocode avatar Jan 21 '20 20:01 nitrocode

ALL CHEER MERGE MERGE MERGE MERGE MERGE

waqarkhan3389 avatar Jan 25 '20 23:01 waqarkhan3389

Please merge 🙏

skwp avatar Oct 07 '21 15:10 skwp

@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 avatar Oct 13 '21 16:10 adzuci

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

liamrahav avatar Oct 14 '21 13:10 liamrahav