AWS network ACLs swallow errors on read

[ctiwald]

I was coding the fix for #1798 and said to myself, "Huh, I better fix this for network ACLs, too", except network ACLs didn't appear to suffer the problem. Poking around, it turns out the read function for those resources is dropping errors on the floor. These two lines don't work, so ingress and egress rules for network ACLs never actually update on read:

https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_network_acl.go#L162-L163

I'm going to propose a fix for this code, but it'll expose the problem in #1765, #1798, and #1177, just for a different resource. My fix is going to include a proposed solution -- detecting the absence of the default rules on creation, and erroring, telling the user that they must include the default rules in their configuration file. Furthermore, if the reading of rules discovers rules that aren't already recorded (i.e. generating a new hash for the rule), I'm also going to error, instructing the user to include the rules and that they can later remove them if they so desire -- a strategy that does in fact work around the problem.

If this isn't the way you all want to go, that's fine, but I'm a little stumped by the correct behavior here. I do believe if we implement #1765 for network ACLs it will be surprising behavior to users of AWS who've grown accustomed to the default behavior. If you all prefer, I can implement #1765 for network ACLs.

[ctiwald]

Hmmm... under further investigation here, it looks like the default rules put into place by AWS can neither be configured by the user nor can they be removed. In other words, I'm not going to force the user to do anything. The code's just going to skip these "special" rules, which are identifiable by their unique rule number.

[catsby]

Should this issue be closed then?

[ctiwald]

Nah. This is still totally an issue. It's just getting a different solution. I'll have a PR for it today. Working on the acceptance tests now.

[catsby]

oh hey, looks like we can close this, thanks!

[ghost]

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.