diff --git a/builtin/providers/aws/resource_aws_default_network_acl.go b/builtin/providers/aws/resource_aws_default_network_acl.go index 2dbd04183496..637ddaed381d 100644 --- a/builtin/providers/aws/resource_aws_default_network_acl.go +++ b/builtin/providers/aws/resource_aws_default_network_acl.go @@ -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, @@ -34,6 +34,9 @@ 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, @@ -41,7 +44,9 @@ func resourceAwsDefaultNetworkAcl() *schema.Resource { // 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, @@ -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 } @@ -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 @@ -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{ @@ -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 } @@ -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, diff --git a/builtin/providers/aws/resource_aws_default_network_acl_test.go b/builtin/providers/aws/resource_aws_default_network_acl_test.go index beab5f6ab878..d97b3ba632b5 100644 --- a/builtin/providers/aws/resource_aws_default_network_acl_test.go +++ b/builtin/providers/aws/resource_aws_default_network_acl_test.go @@ -40,14 +40,6 @@ 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), - ), - }, }, }) } @@ -55,9 +47,7 @@ func TestAccAWSDefaultNetworkAcl_basic(t *testing.T) { 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{ @@ -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( @@ -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 }