Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A partial fix the "-1" protocol problem in AWS security groups #1798

Merged
merged 4 commits into from
May 7, 2015

Conversation

ctiwald
Copy link
Contributor

@ctiwald ctiwald commented May 5, 2015

This addresses, but does not completely close, #1177, and probably a few other issues if you go rooting around.

The core problem is this -- every time a provider discards or appends additional configuration data not captured by the terraform config file on API read, it'll result in a hashing error. It crops up twice for AWS security groups. First, protocol = "-1" rules don't record ports in AWS, meaning resources applied with ports and that protocol will always be out of hash-sync when reading the rules back down. Second, egress rules get a default 0.0.0.0/0 rule that isn't necessarily present in the local config file. That behavior is discussed in #1765.

This patch solves the first problem by forcing the user to explicitly declare to_port = 0 and from_port = 0 if passing protocol = -1. The logic is confined to expandIPPerms and is pretty straightforward. The patch also includes a slew of new testing, but notably not egress tests because they cannot pass.

There is no fix for the egress problem in this PR, but I did discover a (crappy) workaround. If users declare an '0.0.0.0/0' egress rule and runs terraform apply, AWS will error, claiming the rule is a duplicate, but record everything with the proper hash. Run terraform apply again, and terraform will run without error. Users can then remove the '0.0.0.0/0' egress rule, if that is their intent, and be completely in sync.

Ingress and egress rules given a "-1" protocol don't have ports when
Read out of AWS. This results in hashing problems, as a local
config file might contain port declarations AWS can't ever return.

Rather than making ports optional fields, which carries with it a huge
headache trying to distinguish between zero-value attributes (e.g.
'to_port = 0') and attributes that are simply omitted, simply force the
user to opt-in when using the "-1" protocol. If they choose to use it,
they must now specify "0" for both to_port and from_port. Any other
configuration will error.
These only test ingress rules as egress rules are broken by the
default "0.0.0.0/0" rule Amazon includes with every egressed security
group.
@ctiwald
Copy link
Contributor Author

ctiwald commented May 5, 2015

This problem also afflicts network ACLs, as documented in #1808, the problem is just hidden because the read function on those resources swallows errors. I'm going to implement code like this for network ACLs, but the long term fix will really depend on the adjudication of #1765. If we don't remove the defaults, we'll need to clue the user into adding the rules to their config files to keep the TF config consistent with the ingress / egress rules.

@catsby
Copy link
Contributor

catsby commented May 5, 2015

#1765 has been merged

@ctiwald
Copy link
Contributor Author

ctiwald commented May 7, 2015

Fixed this problem for ACLs too over in #1843.

@catsby catsby merged commit 2526379 into hashicorp:master May 7, 2015
@catsby
Copy link
Contributor

catsby commented May 7, 2015

Thanks @ctiwald ! Had to make a small tweak after structure_test got some updates:

Overall, great work!

minamijoyo added a commit to minamijoyo/terraform that referenced this pull request Dec 4, 2016
In the case of aws_security_group, if protocol = -1 and a port number
other than 0 is specified, an error occurs during terraform apply. This
is a workaround for AWS to ignore port numbers with protocol = -1.
(refs: hashicorp#1798)

However, aws_security_group_rule does not have this check and behavior
is inconsistent.
aws_security_group_rule should have the same check.
@ghost
Copy link

ghost commented May 2, 2020

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.

@ghost ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants