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

New Resource: aws_network_acl_association #1034

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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 aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ func Provider() terraform.ResourceProvider {
"aws_main_route_table_association": resourceAwsMainRouteTableAssociation(),
"aws_nat_gateway": resourceAwsNatGateway(),
"aws_network_acl": resourceAwsNetworkAcl(),
"aws_network_acl_association": resourceAwsNetworkAclAssociation(),
"aws_default_network_acl": resourceAwsDefaultNetworkAcl(),
"aws_network_acl_rule": resourceAwsNetworkAclRule(),
"aws_network_interface": resourceAwsNetworkInterface(),
Expand Down
138 changes: 138 additions & 0 deletions aws/resource_aws_network_acl_association.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package aws

import (
"fmt"
"log"
"time"

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

func resourceAwsNetworkAclAssociation() *schema.Resource {
return &schema.Resource{
Create: resourceAwsNetworkAclAssociationCreate,
Read: resourceAwsNetworkAclAssociationRead,
Update: resourceAwsNetworkAclAssociationUpdate,
Delete: resourceAwsNetworkAclAssociationDelete,

Schema: map[string]*schema.Schema{
"subnet_id": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Go 1.7+, redundant type declaration in composite literal can be safely removed, so:

"subnet_id": &schema.Schema{

can become:

"subnet_id": {

Type: schema.TypeString,
Required: true,
},

"network_acl_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
},
},
}
}

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

naclId := d.Get("network_acl_id").(string)
subnetId := d.Get("subnet_id").(string)

log.Printf(
"[INFO] Creating network acl association: %s => %s",
subnetId,
naclId)

association, err_association := findNetworkAclAssociation(subnetId, conn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please prefer camelCase-d variables instead of snake-case-d ones, like errAssociation

if err_association != nil {
return fmt.Errorf("Failed to create acl %s with nacl %s: %s", d.Id(), naclId, err_association)
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is not really accurate. What do you think of rewording it to:

return fmt.Errorf("Failed to find association for subnet %s: %s", subnetId, errAssociation)

}

associationOpts := ec2.ReplaceNetworkAclAssociationInput{
AssociationId: association.NetworkAclAssociationId,
NetworkAclId: aws.String(naclId),
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the log instruction from line 42 here, put it all on one line, and perhaps change it to the below?:

log.Printf("[DEBUG] Creating Network ACL association: %#v", associationOpts)

This would provide all of the parameters for the function, so that we don't need to Printf them :)

var err error
err = resource.Retry(5*time.Minute, func() *resource.RetryError {
_, err = conn.ReplaceNetworkAclAssociation(&associationOpts)
if err != nil {
if awsErr, ok := err.(awserr.Error); ok {
if awsErr != nil {
return resource.RetryableError(awsErr)
}
}
return resource.NonRetryableError(err)
}
return nil
})
if err != nil {
return err
}

// Set the ID and return
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems a bit obvious, so would prefer to remove it if you don't mind

d.SetId(naclId)
log.Printf("[INFO] Association ID: %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.

Since you can retrieve this value, this does not seem to be needed. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest...
I dont know how to use id for schema.ResourceData

Looking at comments and thinking...

naclid can get by d.Get("network_acl_id").(string)
So, i think dont need that set it by b.SetId

I wrote things like 75 lines,
wrote it that look other resources.(ex resource_aws_elb_attachment.go


return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should return resourceAwsNetworkAclAssociationRead(d, meta) here to capture any changes.

}

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

// Inspect that the association exists
subnetId := d.Get("subnet_id").(string)
_, err_association := findNetworkAclAssociation(subnetId, conn)
if err_association != nil {
return fmt.Errorf("Failed to read acl %s with subnet %s: %s", d.Id(), subnetId, err_association)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we fail to read the network acl, set the ID to "", and return nil. This will still register a diff in the Terraform state, as Read happens prior to a plan.

}

return nil
}

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

naclId := d.Get("network_acl_id").(string)
subnetId := d.Get("subnet_id").(string)

log.Printf(
"[INFO] Creating network acl association: %s => %s",
subnetId,
naclId)

association, err_association := findNetworkAclAssociation(subnetId, conn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename the var err_association for errAssociation?
We do use camelCase naming instead of the snake case one! :)

if err_association != nil {
return fmt.Errorf("Failed to update acl %s with subnet %s: %s", d.Id(), naclId, err_association)
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is not really accurate. What do you think of rewording it to:

return fmt.Errorf("Failed to find association for subnet %s: %s", subnetId, errAssociation)

At this point, we are only trying to find the association, rather than updating it. What do you think?

}

req := &ec2.ReplaceNetworkAclAssociationInput{
AssociationId: association.NetworkAclAssociationId,
NetworkAclId: aws.String(naclId),
}
resp, err := conn.ReplaceNetworkAclAssociation(req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the log instruction from line 42 here, put it all on one line, and perhaps change it to the below?:

log.Printf("[DEBUG] Updating Network ACL association: %#v", associationOpts)

This would provide all of the parameters for the function, so that we don't need to Printf them all manually :)


if err != nil {
ec2err, ok := err.(awserr.Error)
if ok && ec2err.Code() == "InvalidAssociationID.NotFound" {
// Not found, so just create a new one
return resourceAwsNetworkAclAssociationCreate(d, meta)
}

return err
}

// Update the ID
d.SetId(*resp.NewAssociationId)
log.Printf("[INFO] Association ID: %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.

This does not seem useful, could you remove it along with the comment line 128?


return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should return Read here as well

}

func resourceAwsNetworkAclAssociationDelete(d *schema.ResourceData, meta interface{}) error {

log.Printf("[INFO] Do nothing on network acl associatioØ destroy phase: %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.

I think this method should really do something here, as the association would still exist in the AWS-land.
What do you think of doing something in the idea of: replacing the Network ACL Association created with the default Network ACL, as it is done in the resource_aws_network_acl.go resource?

Also, when you need to destroy something in the TF-land, set the ID to "" and it will be remove from the state, like d.SetId("")

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for comment
I think too


return nil
}
69 changes: 69 additions & 0 deletions aws/resource_aws_network_acl_association_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package aws

import (
"fmt"
"testing"

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSNetworkAclAssociation(t *testing.T) {

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
IDRefreshName: "aws_network_acl.bar",
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSNetworkAclDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSNetworkAclAssoc,
Check: resource.ComposeAggregateTestCheckFunc(
testCheckAwsRMNetworkAclAssocExists("aws_network_acl_association.test"),
),
},
},
})
}

func testCheckAwsRMNetworkAclAssocExists(name string) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should check that the association exists, by requesting AWS.
Do you think you could get some inspiration from testAccCheckAWSNetworkAclExists?

return func(s *terraform.State) error {

_, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("Not found: %s", name)
}

return nil
}
}

const testAccAWSNetworkAclAssoc = `
resource "aws_vpc" "testespvpc" {
cidr_block = "10.1.0.0/16"
tags {
Name = "testAccAWSNetworkAclEsp"
}
}

resource "aws_network_acl" "acl_a" {
vpc_id = "${aws_vpc.testespvpc.id}"

tags {
Name = "terraform test"
}
}

resource "aws_subnet" "sunet_a" {
vpc_id = "${aws_vpc.testespvpc.id}"
cidr_block = "10.0.33.0/24"
tags {
Name = "terraform test"
}
}

resource "aws_network_acl_association" "test" {
network_acl_id = "${aws_network_acl.acl_a.id}"
subnet_id = "${aws_subnet.subnet_a.id}"
}
}`
4 changes: 4 additions & 0 deletions website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,10 @@
<a href="/docs/providers/aws/r/network_acl.html">aws_network_acl</a>
</li>

<li<%= sidebar_current("docs-aws-resource-network-acl-assoc") %>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the full association word here and line 1409

<a href="/docs/providers/aws/r/network_acl_assoc.html">aws_network_acl_association</a>
</li>

<li<%= sidebar_current("docs-aws-resource-network-acl-rule") %>>
<a href="/docs/providers/aws/r/network_acl_rule.html">aws_network_acl_rule</a>
</li>
Expand Down
36 changes: 36 additions & 0 deletions website/docs/r/network_acl_assoc.html.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
---
layout: "aws"
page_title: "AWS: aws_network_acl_association"
sidebar_current: "docs-aws-resource-network-acl-association"
description: |-
Provides an network ACL association resource.
---

# aws\_network\_acl\_association
Copy link
Contributor

Choose a reason for hiding this comment

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

antislashes are not needed anymore, it is safe to remove them here


Provides an network ACL association resource. You might set up network ACLs associate to your subnet.

## Example Usage

```hcl
resource "aws_network_acl_association" "main" {
network_acl_id = "${aws_network_acl.main.id}"
subnet_id = "${aws_subnet.main.id}"
}
```

## Argument Reference

The following arguments are supported:

* `network_acl_id` - (Required) The ID of the network acl .
* `subnet_id` - (Required) The ID of the associated Subnet.

## Attributes Reference

The following attributes are exported:

* `id` - The ID of the network ACL
* `network_acl_id` - The ID of the network ACL
* `subnet_id` - The ID of the subnet id

Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick but this last line is not needed here 😄