Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Add security group individual rule descriptions #475

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liamrahav
Copy link

This addresses the concern from #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 #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
Copy link
Author

Tagging @nitrocode who wrote up the issue.

@coveralls
Copy link

Coverage Status

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

@nitrocode
Copy link

@dtan4 @waqark3389 @liamrahav

Nice job! LGTM!

@waqarkhan3389
Copy link

ALL CHEER
MERGE MERGE MERGE MERGE MERGE

@skwp
Copy link

skwp commented Oct 7, 2021

Please merge 🙏

@adzuci
Copy link

adzuci commented Oct 13, 2021

@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
Copy link
Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants