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

provider/aws: Default Network ACL resource #6165

Merged
merged 5 commits into from
Apr 18, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions builtin/providers/aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ func Provider() terraform.ResourceProvider {
"aws_main_route_table_association": resourceAwsMainRouteTableAssociation(),
"aws_nat_gateway": resourceAwsNatGateway(),
"aws_network_acl": resourceAwsNetworkAcl(),
"aws_default_network_acl": resourceAwsDefaultNetworkAcl(),
"aws_network_acl_rule": resourceAwsNetworkAclRule(),
"aws_network_interface": resourceAwsNetworkInterface(),
"aws_opsworks_stack": resourceAwsOpsworksStack(),
Expand Down
290 changes: 290 additions & 0 deletions builtin/providers/aws/resource_aws_default_network_acl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
package aws

import (
"fmt"
"log"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/helper/schema"
)

// ACL Network ACLs all contain an explicit deny-all rule that cannot be
// destroyed or changed by users. This rule is numbered very high to be a
// catch-all.
// See http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_ACLs.html#default-network-acl
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
Read: resourceAwsNetworkAclRead,
Delete: resourceAwsDefaultNetworkAclDelete,
Update: resourceAwsDefaultNetworkAclUpdate,

Schema: map[string]*schema.Schema{
"vpc_id": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"default_network_acl_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
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,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new resource - why are we starting w/ a deprecated field? Perhaps just a leftover from basing this on the network_acl resource?

Copy link
Contributor Author

@catsby catsby Apr 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a leftover. It was Computed when I was "inheriting" the schema. I missed re-adding that change when I went to fully copying it over.

We're re-using resourceAwsNetworkAclRead though, so it needs to be present. I'll mark it as Computed

Copy link
Contributor

@phinze phinze Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a read through Read (ha!) on network ACL and didn't see the subnet_id field mentioned there. Looks like it's only referenced in Delete and Update, nether of which we borrow. So I think that means we can drop this without consequence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smote in 78cd903

// We want explicit management 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 until the
// Subnets are themselves destroyed or reassigned to a different Network
// ACL
"subnet_ids": &schema.Schema{
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
// We want explicit management of Rules here, so we do not allow them to be
// computed. Instead, an empty config will enforce just that; removal of the
// rules
"ingress": &schema.Schema{
Type: schema.TypeSet,
Required: false,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"from_port": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"to_port": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"rule_no": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"action": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
"protocol": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
"cidr_block": &schema.Schema{
Type: schema.TypeString,
Optional: true,
},
"icmp_type": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
},
"icmp_code": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
},
},
},
Set: resourceAwsNetworkAclEntryHash,
},
"egress": &schema.Schema{
Type: schema.TypeSet,
Required: false,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"from_port": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"to_port": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"rule_no": &schema.Schema{
Type: schema.TypeInt,
Required: true,
},
"action": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
"protocol": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
"cidr_block": &schema.Schema{
Type: schema.TypeString,
Optional: true,
},
"icmp_type": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
},
"icmp_code": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
},
},
},
Set: resourceAwsNetworkAclEntryHash,
},

"tags": tagsSchema(),
},
}
}

func resourceAwsDefaultNetworkAclCreate(d *schema.ResourceData, meta interface{}) error {
d.SetId(d.Get("default_network_acl_id").(string))

// 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 egress rules for Default Network ACL for %s", d.Id())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check here if the given ACL is actually already the default for some VPC? Otherwise this resource feels more like "adopt some arbitrary Network ACL".

Alternatively, could the argument actually be vpc_id rather than default_network_acl_id, and then we look up the actual ACL id from the VPC, thus ensuring that we'll always get the right one. IMO this interface seems more intuitive, as it makes the relationship to the VPC clearer:

resource "aws_vpc" "mainvpc" {
  cidr_block = "10.1.0.0/16"
}

resource "aws_default_network_acl" "default" {
  vpc_id = "${aws_vpc.mainvpc.vpc_id}"

  ingress {
    protocol   = -1
    rule_no    = 100
    action     = "allow"
    cidr_block = "0.0.0.0/0"
    from_port  = 0
    to_port    = 0
  }

  egress {
    protocol   = -1
    rule_no    = 100
    action     = "allow"
    cidr_block = "0.0.0.0/0"
    from_port  = 0
    to_port    = 0
  }
}

I suppose this model might be trickier since a user might choose a different Network ACL to be the default via the AWS console. In that case, I expect the id of this resource would be the VPC id rather than the ACL id, and Read would re-read the default ACL from the VPC each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @apparentlymart these are good points. I did consider using the vpc_id and looking up the default, but thought that in most cases I would already know this default's id so went with the approach above. I think for now we'll leave as is, and can adopt this suggestion with little trouble in the future if we think it's needed.

Do you see this as a blocker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern here was two things, really:

  • It seems confusing to have a resource described as "the default network ACL for a VPC" that doesn't take a VPC as an argument. It took me a while of staring at your example to understand what was going on.
  • If a user mis-understands this and puts in some other random ACL id, e.g. thinking this means "set the VPC's default ACL to be this one I've specified", they are rewarded by it deleting the contents of that ACL.

At the very least I think it'd be good to protect against the latter case by failing if the specified route table isn't the default for some VPC. It looks like DescribeNetworkAcls returns enough information answer this question with just one additional API request.

With that said, not a blocker if you're not worried about it. Mostly I was just expressing poorly some frustration of it taking me a while to understand what was going on here.

err := revokeAllNetworkACLEntries(d.Id(), meta)
if err != nil {
return err
}

return resourceAwsDefaultNetworkAclUpdate(d, meta)
}

func resourceAwsDefaultNetworkAclUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn
d.Partial(true)

if d.HasChange("ingress") {
err := updateNetworkAclEntries(d, "ingress", conn)
if err != nil {
return err
}
}

if d.HasChange("egress") {
err := updateNetworkAclEntries(d, "egress", conn)
if err != nil {
return err
}
}

if d.HasChange("subnet_ids") {
o, n := d.GetChange("subnet_ids")
if o == nil {
o = new(schema.Set)
}
if n == nil {
n = new(schema.Set)
}

os := o.(*schema.Set)
ns := n.(*schema.Set)

remove := os.Difference(ns).List()
add := ns.Difference(os).List()

if len(remove) > 0 {
//
// NO-OP
//
// 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
// Default Network ACL. Because we're managing the default here, we cannot
// do that, so we simply log a NO-OP. In order to remove the Subnet here,
// it must be destroyed, or assigned to different Network ACL. Those
// operations are not handled here
log.Printf("[WARN] Cannot remove subnets from the Default Network ACL. They must be re-assigned or destroyed")
}

if len(add) > 0 {
for _, a := range add {
association, err := findNetworkAclAssociation(a.(string), conn)
if err != nil {
return fmt.Errorf("Failed to find acl association: acl %s with subnet %s: %s", d.Id(), a, err)
}
log.Printf("[DEBUG] Updating Network Association for Default Network ACL (%s) and Subnet (%s)", d.Id(), a.(string))
_, err = conn.ReplaceNetworkAclAssociation(&ec2.ReplaceNetworkAclAssociationInput{
AssociationId: association.NetworkAclAssociationId,
NetworkAclId: aws.String(d.Id()),
})
if err != nil {
return err
}
}
}
}

if err := setTags(conn, d); err != nil {
return err
} else {
d.SetPartial("tags")
}

d.Partial(false)
// Re-use the exiting Network ACL Resources READ method
return resourceAwsNetworkAclRead(d, meta)
}

func resourceAwsDefaultNetworkAclDelete(d *schema.ResourceData, meta interface{}) error {
log.Printf("[WARN] Cannot destroy Default Network ACL. Terraform will remove this resource from the state file, however resources may remain.")
d.SetId("")
return nil
}

// 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{
NetworkAclIds: []*string{aws.String(netaclId)},
})

if err != nil {
log.Printf("[DEBUG] Error looking up Network ACL: %s", err)
return err
}

if resp == nil {
return fmt.Errorf("[ERR] Error looking up Default Network ACL Entries: No results")
}

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 {
continue
}

// track if this is an egress or ingress rule, for logging purposes
rt := "ingress"
if *e.Egress == true {
rt = "egress"
}

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,
Egress: e.Egress,
})
if err != nil {
return fmt.Errorf("Error deleting entry (%s): %s", e, err)
}
}

return nil
}
Loading