Skip to content

Commit

Permalink
refactor revokeRulesForType to be revokeAllNetworkACLEntries
Browse files Browse the repository at this point in the history 
Refactor method to delete all network ACL entries, regardless of type. The
previous implementation was under the assumption that we may only eliminate some
rule types and possibly not others, so the split was necessary.

We're now removing them all, so the logic isn't necessary

Several doc and test cleanups are here as well
  • Loading branch information
@catsby
catsby committed Apr 15, 2016
1 parent c8e29f1 commit 7abb13a
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 41 deletions.
38 changes: 18 additions & 20 deletions builtin/providers/aws/resource_aws_default_network_acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const awsDefaultAclRuleNumber = 32767
func resourceAwsDefaultNetworkAcl() *schema.Resource {
return &schema.Resource{
Create: resourceAwsDefaultNetworkAclCreate,
// We reuse aws_network_acl's read method, the operations are the same
// We reuse aws_network_acl's read method, the operations are the same
Read: resourceAwsNetworkAclRead,
Delete: resourceAwsDefaultNetworkAclDelete,
Update: resourceAwsDefaultNetworkAclUpdate,
Expand All @@ -34,14 +34,19 @@ func resourceAwsDefaultNetworkAcl() *schema.Resource {
ForceNew: true,
Computed: false,
},
// subnet_id is a deprecated value in aws_network_acl, so we don't support
// using it here. We do re-use aws_network_acl's READ method which will
// attempt to set this value, so we include it here
"subnet_id": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
// We want explicit managment of Subnets here, so we do not allow them to be
// computed. Instead, an empty config will enforce just that; removal of the
// any Subnets that have been assigned to the Default Network ACL. Because we
// can't actually remove them, this will be a continual plan
// can't actually remove them, this will be a continual plan until the
// Subnets are themselves destroyed or reassigned to a different Network
// ACL
"subnet_ids": &schema.Schema{
Type: schema.TypeSet,
Optional: true,
Expand Down Expand Up @@ -143,14 +148,11 @@ func resourceAwsDefaultNetworkAcl() *schema.Resource {

func resourceAwsDefaultNetworkAclCreate(d *schema.ResourceData, meta interface{}) error {
d.SetId(d.Get("default_network_acl_id").(string))
log.Printf("[DEBUG] Revoking ingress rules for Default Network ACL for %s", d.Id())
err := revokeRulesForType(d.Id(), "ingress", meta)
if err != nil {
return err
}

log.Printf("[DEBUG] Revoking egress rules for Default Network ACL for %s", d.Id())
err = revokeRulesForType(d.Id(), "egress", meta)
// revoke all default and pre-existing rules on the default network acl.
// In the UPDATE method, we'll apply only the rules in the configuration.
log.Printf("[DEBUG] Revoking default ingress and egreee rules for Default Network ACL for %s", d.Id())
err := revokeAllNetworkACLEntries(d.Id(), meta)
if err != nil {
return err
}
Expand Down Expand Up @@ -195,7 +197,7 @@ func resourceAwsDefaultNetworkAclUpdate(d *schema.ResourceData, meta interface{}
//
// NO-OP
//
// Subnets *must* belong to a Network ACL. Subnets are not "remove" from
// Subnets *must* belong to a Network ACL. Subnets are not "removed" from
// Network ACLs, instead their association is replaced. In a normal
// Network ACL, any removal of a Subnet is done by replacing the
// Subnet/ACL association with an association between the Subnet and the
Expand Down Expand Up @@ -241,9 +243,9 @@ func resourceAwsDefaultNetworkAclDelete(d *schema.ResourceData, meta interface{}
return nil
}

// revokeRulesForType will query the Network ACL for it's entries, and revoke
// any rule of the matching type.
func revokeRulesForType(netaclId, rType string, meta interface{}) error {
// revokeAllNetworkACLEntries revoke all ingress and egress rules that the Default
// Network ACL currently has
func revokeAllNetworkACLEntries(netaclId string, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn

resp, err := conn.DescribeNetworkAcls(&ec2.DescribeNetworkAclsInput{
Expand All @@ -262,8 +264,8 @@ func revokeRulesForType(netaclId, rType string, meta interface{}) error {
networkAcl := resp.NetworkAcls[0]
for _, e := range networkAcl.Entries {
// Skip the default rules added by AWS. They can be neither
// configured or deleted by users. See http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_ACLs.html#default-network-acl
if *e.RuleNumber == awsDefaultAclRuleNumber {
// configured or deleted by users. See http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_ACLs.html#default-network-acl
if *e.RuleNumber == awsDefaultAclRuleNumber {
continue
}

Expand All @@ -275,11 +277,7 @@ func revokeRulesForType(netaclId, rType string, meta interface{}) error {
rt = "egress"
}

if rType != rt {
continue
}

log.Printf("[DEBUG] Destroying Network ACL Entry number (%d) for type (%s)", int(*e.RuleNumber), rt)
log.Printf("[DEBUG] Destroying Network ACL (%s) Entry number (%d)", rt, int(*e.RuleNumber))
_, err := conn.DeleteNetworkAclEntry(&ec2.DeleteNetworkAclEntryInput{
NetworkAclId: aws.String(netaclId),
RuleNumber: e.RuleNumber,
Expand Down
35 changes: 14 additions & 21 deletions builtin/providers/aws/resource_aws_default_network_acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,14 @@ func TestAccAWSDefaultNetworkAcl_basic(t *testing.T) {
testAccCheckAWSDefaultACLAttributes(&networkAcl, []*ec2.NetworkAclEntry{}, 0),
),
},
// Add default ACL rules and veryify plan is empty
resource.TestStep{
Config: testAccAWSDefaultNetworkConfig_basicDefaultRules,
Check: resource.ComposeTestCheckFunc(
testAccGetWSDefaultNetworkAcl("aws_default_network_acl.default", &networkAcl),
testAccCheckAWSDefaultACLAttributes(&networkAcl, []*ec2.NetworkAclEntry{defaultEgressAcl, defaultIngressAcl}, 0),
),
},
},
})
}

func TestAccAWSDefaultNetworkAcl_deny_ingress(t *testing.T) {
// TestAccAWSDefaultNetworkAcl_deny_ingress will deny all Ingress rules, but
// not Egress. We then expect there to be 3 rules, 2 AWS defaults and 1
// additional Egress. Without specifying the Egress rule in the configuration,
// we expect a follow up plan to prompt it's removal, thus we expect a non
// emtpy plan
// additional Egress.
var networkAcl ec2.NetworkAcl

resource.Test(t, resource.TestCase{
Expand Down Expand Up @@ -123,10 +113,18 @@ func TestAccAWSDefaultNetworkAcl_SubnetReassign(t *testing.T) {
),
},

// Here we've re-assinged the subnets to a different ACL, however, we
// still arn't updating the Default resource, so we introduce a depends_on
// reference, to ensure the right order (Network ACL, then Default Network
// ACL)
// Here we've re-assinged the subnets to a different ACL.
// Without any otherwise association between the `aws_network_acl` and
// `aws_default_network_acl` resources, we cannot guarnetee that the
// reassignment of the two subnets to the `aws_network_acl` will happen
// before the update/read on the `aws_default_network_acl` resource.
// Because of this, there could be a non-empty plan if a READ is done on
// the default before the reassignment occurs on the other resource.
//
// For the sake of testing, here we introduce a depends_on attribute from
// the default resource to the other acl resource, to ensure the latter's
// update occurs first, and the former's READ will correctly read zero
// subnets
resource.TestStep{
Config: testAccAWSDefaultNetworkConfig_Subnets_move,
Check: resource.ComposeTestCheckFunc(
Expand All @@ -139,12 +137,7 @@ func TestAccAWSDefaultNetworkAcl_SubnetReassign(t *testing.T) {
}

func testAccCheckAWSDefaultNetworkAclDestroy(s *terraform.State) error {
for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_default_network_acl" {
continue
}
}

// We can't destroy this resource; it comes and goes with the VPC itself.
return nil
}

Expand Down

0 comments on commit 7abb13a

Please sign in to comment.