New Resource: aws_network_acl_association #1034

takahiro9 · 2017-07-02T10:06:10Z

reopen pr

hashicorp/terraform#15393

takahiro9 · 2017-07-05T01:03:40Z

Oh sorry. i fix to be success for build.

fix to success go vet .

takahiro9 · 2017-07-25T00:39:33Z

I think this resource use case is...

if there is restriction to operation of aws resource
ex) if can't create network_acl and can attach network_acl

and i encount to it right now 😭

Ninir · 2017-08-17T21:29:46Z

Hey @takahiro9

Thanks for opening this.
Do you think you could reword your use-case please? not sure to understand it fully.

Also, what are you trying to achieve that do not work with the actual resources?

grubernaut

Agree fully with @Ninir's comment, went ahead and made a quick review of the PR as-is, in case there's a valid use-case here 😄

grubernaut · 2017-07-25T14:35:46Z

aws/resource_aws_network_acl_association.go

+ d.SetId(naclId)
+ log.Printf("[INFO] Association ID: %s", d.Id())
+
+ return nil


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

grubernaut · 2017-07-25T14:36:36Z

aws/resource_aws_network_acl_association.go

+ 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)


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.

grubernaut · 2017-07-25T14:37:26Z

aws/resource_aws_network_acl_association.go

+ d.SetId(*resp.NewAssociationId)
+ log.Printf("[INFO] Association ID: %s", d.Id())
+
+ return nil


Should return Read here as well

takahiro9 · 2017-08-27T02:20:58Z

thank you to feedback.

went ahead and made a quick review of the PR as-is

oh, thanks to tell me.

Do you think you could reword your use-case please

ok !
・Suppose there is a company 'A' which want to creates a system and a vendor 'B' which creates
・there are aws account for create a system
・company 'A' is admin to aws account
・Company 'A' send to 'B' what iam user credential
・'B' think that use terraform for building infrastructure
・'A' isnt admit that 'B' can create network acl. but ReplaceNetworkAclAssociation admit
・when 'B' want to create nacl. there need 'B' ask to 'A'

i think This resource is effectiveness at this situation. very rare case.

takahiro9 · 2017-08-27T03:00:45Z

i think this resource that like aws_autoscaling_attachment

takahiro9 · 2017-08-31T03:00:58Z

please tell me if dont know.
i will be helpful.

sorry for my clumsy english by google translation

Ninir

HI @takahiro9

Ok thanks for sharing your use-case. Just left a few comments to address: some cosmetic and some logic work, nothing critical!

Thanks for the work here, and nice first contribution! 👍

Ninir · 2017-08-31T05:55:43Z

aws/resource_aws_network_acl_association.go

+ Delete: resourceAwsNetworkAclAssociationDelete,
+
+ Schema: map[string]*schema.Schema{
+ "subnet_id": &schema.Schema{


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

"subnet_id": &schema.Schema{

can become:

"subnet_id": {

Ninir · 2017-08-31T05:57:56Z

aws/resource_aws_network_acl_association.go

+ return err
+ }
+
+ // Set the ID and return


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

Ninir · 2017-08-31T05:59:17Z

aws/resource_aws_network_acl_association.go

+ AssociationId: association.NetworkAclAssociationId,
+ NetworkAclId: aws.String(naclId),
+ }
+


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 :)

Ninir · 2017-08-31T06:01:52Z

aws/resource_aws_network_acl_association.go

+ subnetId,
+ naclId)
+
+ association, err_association := findNetworkAclAssociation(subnetId, conn)


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

Ninir · 2017-08-31T06:04:47Z

aws/resource_aws_network_acl_association.go

+
+ association, err_association := findNetworkAclAssociation(subnetId, conn)
+ if err_association != nil {
+ return fmt.Errorf("Failed to create acl %s with nacl %s: %s", d.Id(), naclId, err_association)


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)

Ninir · 2017-08-31T06:17:23Z

aws/resource_aws_network_acl_association.go

+
+func resourceAwsNetworkAclAssociationDelete(d *schema.ResourceData, meta interface{}) error {
+
+ log.Printf("[INFO] Do nothing on network acl associatioØ destroy phase: %s", d.Id())


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("")

Thank you for comment
I think too

Ninir · 2017-08-31T06:20:44Z

aws/resource_aws_network_acl_association_test.go

+ })
+}
+
+func testCheckAwsRMNetworkAclAssocExists(name string) resource.TestCheckFunc {


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

Ninir · 2017-08-31T06:21:22Z

website/aws.erb

@@ -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") %>>


Please keep the full association word here and line 1409

Ninir · 2017-08-31T06:21:50Z

website/docs/r/network_acl_assoc.html.markdown

+ Provides an network ACL association resource.
+---
+
+# aws\_network\_acl\_association


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

Ninir · 2017-08-31T06:23:24Z

website/docs/r/network_acl_assoc.html.markdown

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


Small nitpick but this last line is not needed here 😄

takahiro9 · 2017-09-24T09:24:16Z

ah. i was late for noticing.

Ok thanks for sharing your use-case

i am happy that can communication !

few comments to address

thank you. it is very informative to me.
please wait for a moment...
I will respond from now.

ColinHebert · 2017-12-19T05:39:54Z

Hey @radeksimko , @takahiro9, what are we missing to merge this in?

takahiro9 · 2017-12-26T07:10:35Z

thank you.
i do not think.

phillbaker · 2018-07-03T18:48:28Z

@Ninir @takahiro9 we also have a usecase for this resource: After an initial network ACL is created in a "base" plan, different applications then should be able to look it up (via tags in a data provider) and then create an association relevant to just the subnets relevant to the application.

What can I do to help push this forward? It looks like the outstanding comments have been addressed, however, there is now a merge conflict.

phillbaker · 2018-07-05T22:51:44Z

cc @radeksimko. What can I do to help push this forward?

It looks like the outstanding comments have been addressed, however, there is now a merge conflict. Would a new PR that squashes these commits and resolves the merge conflicts help?

thomaslarsen · 2020-02-18T14:07:10Z

Any thoughts on when this may be merged in? I have a usecase where the acl is created before the subnet, so I need to associate a new subnet to an already existing acl, which seems to fit with what this resource is achieving.
Any update would be appreciated.

teamterraform · 2020-08-18T19:12:43Z

Notification of Recent and Upcoming Changes to Contributions

Thank you for this contribution! There have been a few recent development changes that affect this pull request. We apologize for the inconvenience, especially if there have been long review delays up until now. Please note that this is automated message from an unmonitored account. See the FAQ for additional information on the maintainer team and review prioritization.

If you are unable to complete these updates, please leave a comment for the community and maintainers so someone can potentially continue the work. The maintainers will encourage other contributors to use the existing contribution as the base for additional changes as appropriate. Otherwise, contributions that do not receive updated code or comments from the original contributor may be closed in the future so the maintainers can focus on active items.

For the most up to date information about Terraform AWS Provider development, see the Contributing Guide. Additional technical debt changes can be tracked with the technical-debt label on issues.

As part of updating a pull request with these changes, the most current unit testing and linting will run. These may report issues that were not previously reported.

Action Required: Terraform 0.12 Syntax

Reference: #8950
Reference: #14417

Version 3 and later of the Terraform AWS Provider, which all existing contributions would potentially be added, only supports Terraform 0.12 and later. Certain syntax elements of Terraform 0.11 and earlier show deprecation warnings during runs with Terraform 0.12. Documentation and test configurations, such as those including deprecated string interpolations (some_attribute = "${aws_service_thing.example.id}") should be updated to the newer syntax (some_attribute = aws_service_thing.example.id). Contribution testing will automatically fail on older syntax in the near future. Please see the referenced issues for additional information.

Action Required: Terraform Plugin SDK Version 2

Reference: #14551

The Terraform AWS Provider has been upgraded to the latest version of the Terraform Plugin SDK. Generally, most changes to contributions should only involve updating Go import paths in source code files. Please see the referenced issue for additional information.

Action Required: Removal of website/aws.erb File

Reference: #14712

Any changes to the website/aws.erb file are no longer necessary and should be removed from this contribution to prevent merge issues in the near future when the file is removed from the repository. Please see the referenced issue for additional information.

Upcoming Change of Git Branch Naming

Reference: #14292

Development environments will need their upstream Git branch updated from master to main in the near future. Please see the referenced issue for additional information and scheduling.

Upcoming Change of GitHub Organization

Reference: #14715

This repository will be migrating from https://github.com/terraform-providers/terraform-provider-aws to https://github.com/hashicorp/terraform-provider-aws. No practitioner or developer action is anticipated and most GitHub functionality will automatically redirect to the new location. Go import paths including terraform-providers can remain for now. Please see the referenced issue for additional information and scheduling.

edmundcraske-bjss · 2021-02-24T15:46:48Z

@ewbankkit Do we know why this has sat around for over 3 years as a concept without any implementation? Seems like there have been a number of issues raised asking for this resource also! #12364 #16660 #17648

github-actions · 2022-06-09T02:35:19Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

yamamototakahiro and others added 2 commits June 29, 2017 13:45

add network acl association resource

f467708

add document

e4ee9fb

radeksimko added the new-resource Introduces a new resource. label Jul 3, 2017

add html document

657a1b0

fix to success go vet .

grubernaut added the enhancement Requests to existing resources that expand the functionality or scope. label Jul 25, 2017

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 17, 2017

grubernaut suggested changes Aug 23, 2017

View reviewed changes

Corresponded to point out

f749c98

Ninir suggested changes Aug 31, 2017

View reviewed changes

yamamototakahiro added 6 commits October 1, 2017 21:37

fix and add resource delete function

01ed6d2

Fixed based on indication

64014fb

Merge branch 'master' into add_resource_aws_network_acl_association

de507c5

update test

d2529da

update html

d8bae10

import not use fix

14cc390

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko requested a review from Ninir January 12, 2018 10:19

radeksimko added service/ec2 Issues and PRs that pertain to the ec2 service. and removed waiting-response Maintainers are waiting on response from community or contributor. enhancement Requests to existing resources that expand the functionality or scope. labels Jan 12, 2018

radeksimko changed the title ~~Add resource aws network acl association~~ New Resource: aws_network_acl_association Jan 16, 2018

ewbankkit mentioned this pull request Apr 8, 2018

r/aws_network_acl: Correctly handle missing associations for deleted subnets #4118

Closed

edmundcraske-bjss mentioned this pull request Feb 24, 2021

Add support for 'aws_subnet_nacl_association' resource #17648

Closed

geofflancaster mentioned this pull request Apr 14, 2021

Subnet nacl association #18807

Merged

breathingdust deleted the branch hashicorp:master November 12, 2021 16:59

breathingdust closed this Nov 12, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_network_acl_association #1034

New Resource: aws_network_acl_association #1034

takahiro9 commented Jul 2, 2017

takahiro9 commented Jul 5, 2017

takahiro9 commented Jul 25, 2017

Ninir commented Aug 17, 2017

grubernaut left a comment

grubernaut Jul 25, 2017

grubernaut Jul 25, 2017

grubernaut Jul 25, 2017

takahiro9 commented Aug 27, 2017

takahiro9 commented Aug 27, 2017

takahiro9 commented Aug 31, 2017

Ninir left a comment •

edited

Loading

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

takahiro9 Oct 1, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

Ninir Aug 31, 2017

takahiro9 commented Sep 24, 2017

ColinHebert commented Dec 19, 2017

takahiro9 commented Dec 26, 2017

phillbaker commented Jul 3, 2018

phillbaker commented Jul 5, 2018

thomaslarsen commented Feb 18, 2020

teamterraform commented Aug 18, 2020

edmundcraske-bjss commented Feb 24, 2021

github-actions bot commented Jun 9, 2022


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

		log.Printf("[INFO] Do nothing on network acl associatioØ destroy phase: %s", d.Id())

New Resource: aws_network_acl_association #1034

New Resource: aws_network_acl_association #1034

Conversation

takahiro9 commented Jul 2, 2017

takahiro9 commented Jul 5, 2017

takahiro9 commented Jul 25, 2017

Ninir commented Aug 17, 2017

grubernaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahiro9 commented Aug 27, 2017

takahiro9 commented Aug 27, 2017

takahiro9 commented Aug 31, 2017

Ninir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahiro9 commented Sep 24, 2017

ColinHebert commented Dec 19, 2017

takahiro9 commented Dec 26, 2017

phillbaker commented Jul 3, 2018

phillbaker commented Jul 5, 2018

thomaslarsen commented Feb 18, 2020

teamterraform commented Aug 18, 2020

Notification of Recent and Upcoming Changes to Contributions

Action Required: Terraform 0.12 Syntax

Action Required: Terraform Plugin SDK Version 2

Action Required: Removal of website/aws.erb File

Upcoming Change of Git Branch Naming

Upcoming Change of GitHub Organization

edmundcraske-bjss commented Feb 24, 2021

github-actions bot commented Jun 9, 2022

Ninir left a comment •

edited

Loading