provider/aws: Remove default egress rule from Security Group on creation

[catsby]

Security Groups in a VPC receive a default egress rule to permit all. This causes some issues though, as it's implicit, and causes errors if people try to add a duplicate rule. It also triggers a new plan if you do specify an egress rule, as we interpret that as needing to remove the old one.

This PR changes two things:

• by default, if you specify an egress rule, we delete the default ALLOW ALL rule
• you must now specify an egress rule when creating a Security Group inside a VPC reneged

Refs #1177
Fixes #1169, #1631
Probably fixes #1299

[catsby]

len(remove) == 0 && len(os.List()) == 0 may be redundant, but I wasn't sure if len(os.List()) == 0 was enough. Should it be sufficient to check for nil here?

[phinze]

Probably technically redundant, but I feel like it doesn't hurt to be extra explicit/conservative.

[catsby]

Note: some acceptance tests in SG now fail, but I can fix those if we think this is the right direction to take

[phinze]

Will this work if a user specifies the same egress rule in their config?

Probably an AccTest would answer one way or another.

[catsby]

yes, it deletes the default, then immediately creates a new one

[phinze]

Oh right, I forgot these lists end up getting executed serially, not in batches. Works for me!

[phinze]

I was picturing a wholly separate API request to specifically nuke the implicit rule, letting the remaining code do its thing unchanged, but if we can get this rule-list strategy tested and make it work I'm cool with it.

[catsby]

I've updated this, but am stuck on something.

The enforcement of an egress rule is, as it stands, only enforceable if the configuration of the Security Group (SG) sets the vpc_id attribute. If you do not, then the SG config has a nil value, and sent to AWS. If your AWS environment has a default VPC, the SG is created in that VPC, and gets the default egress rule applied.

We still revoke it just fine, but we failed to enforce the restriction. I can see confusion / problems from this, as users are probably used to but also maybe unaware of the default rule.

[ctiwald]

This is also contributing to the -1 protocol problem in #1177. Egress rules get the default, but local config doesn't have it, so there's always something found on plan.

[ctiwald]

Note this will also become a decision point for network ACLs, but the need isn't apparent because the read function for network ACLs is broken ( #1808 )

[phinze]

Whitespace funkies here.

[phinze]

LGTM except for whitespace nits

[catsby]

@phinze ah, thanks; formatting issue after rebase ಠ_ಠ

Fixed, merging this

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