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

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Apr 13, 2016

Provides a resource to manage the default AWS Network ACL. This resource is VPC Only.

The aws_default_network_acl behaves differently from normal resources, in that
Terraform does not create this resource, but instead attempts to "adopt" it
into management. We can do this because each VPC created has a Default Network
ACL that cannot be destroyed, and is created with a known set of default rules.

When Terraform first adopts the Default Network ACL, it immeidately removes all
rules in the ACL
. It then proceeds to create any rules specified in the
configuration. This step is required so that only the rules specificed in the
configuration are created.

Example: Default Network ACL, with default rules in place:

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

resource "aws_default_network_acl" "default" {
  default_network_acl_id = "${aws_vpc.mainvpc.default_network_acl_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
  }
}

Example: Default Network ACL, denying all traffic:

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

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

For more information about Network ACLs, including the default, see the AWS Documentation on Network ACLs.

Fixes #5971

Provides a resource to manage the default AWS Network ACL. VPC Only.
@catsby
Copy link
Contributor Author

catsby commented Apr 13, 2016

cc @clstokes

@clstokes
Copy link
Contributor

Related to #6093.

ForceNew: true,
Computed: false,
Deprecated: "Attribute subnet_id is deprecated on default_network_acl resources. Use subnet_ids instead",
},
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

for _, e := range networkAcl.Entries {
// Skip the default rules added by AWS. They can be neither
// configured or deleted by users.
if *e.RuleNumber == 32767 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you lift this number up into a constant.

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
"subnet_id": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
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

@phinze
Copy link
Contributor

phinze commented Apr 18, 2016

Ok last pass complete - just those docs Qs and the subnet_id Q

@phinze
Copy link
Contributor

phinze commented Apr 18, 2016

LGTM!

@catsby
Copy link
Contributor Author

catsby commented Apr 18, 2016

Passing acceptance tests, merging

$ make testacc TEST=./builtin/providers/aws TESTARGS="-run=TestAccAWSDefaultNetworkAcl_"
==> Checking that code complies with gofmt requirements...
/Users/clint/Projects/Go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDefaultNetworkAcl_ -timeout 120m
=== RUN   TestAccAWSDefaultNetworkAcl_basic
--- PASS: TestAccAWSDefaultNetworkAcl_basic (13.59s)
=== RUN   TestAccAWSDefaultNetworkAcl_deny_ingress
--- PASS: TestAccAWSDefaultNetworkAcl_deny_ingress (13.60s)
=== RUN   TestAccAWSDefaultNetworkAcl_SubnetRemoval
--- PASS: TestAccAWSDefaultNetworkAcl_SubnetRemoval (24.94s)
=== RUN   TestAccAWSDefaultNetworkAcl_SubnetReassign
--- PASS: TestAccAWSDefaultNetworkAcl_SubnetReassign (30.86s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    83.008s

@catsby catsby merged commit fcdcb4b into master Apr 18, 2016
@stack72 stack72 deleted the f-aws-default-network-acl branch April 21, 2016 23:22
@ghost
Copy link

ghost commented Apr 26, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to delete default ACLs on VPC creation.
4 participants