[WIP] Second half of support for Gateway Association Proposals #8455

ewbankkit · 2019-04-26T18:35:58Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8100.
Follows on from:

ewbankkit · 2019-04-26T18:55:45Z

Changing tack and modifying aws_dx_gateway_association instead - #8100 (comment).

ewbankkit · 2019-04-26T23:04:27Z

Third time lucky.
Adding resource aws_dx_cross_account_gateway_association - #8100 (comment).

ewbankkit · 2019-04-26T23:06:11Z

Current state of acceptance tests:

$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxCrossAccountGatewayAssociation_basic'==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxCrossAccountGatewayAssociation_basic -timeout 120m
=== RUN   TestAccAwsDxCrossAccountGatewayAssociation_basic
=== PAUSE TestAccAwsDxCrossAccountGatewayAssociation_basic
=== CONT  TestAccAwsDxCrossAccountGatewayAssociation_basic
--- FAIL: TestAccAwsDxCrossAccountGatewayAssociation_basic (36.60s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: error accepting Direct Connect gateway association proposal: DirectConnectClientException: Could not find a Direct Connect Gateway Association Proposal for the provided combination of Direct Connect Gateway ID, Virtual Gateway Owner Account, and Proposal ID
        	status code: 400, request id: ffb47e9e-6872-11e9-911f-4d247d966a1c
        
          on /tmp/tf-test395757204/main.tf line 47:
          (source code not available)
        
        
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	36.614s
GNUmakefile:20: recipe for target 'testacc' failed
make: *** [testacc] Error 1

ewbankkit · 2019-04-29T00:30:48Z

Acceptance tests:

aws_dx_cross_account_gateway_association

$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxCrossAccountGatewayAssociation_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxCrossAccountGatewayAssociation_basic -timeout 120m
=== RUN   TestAccAwsDxCrossAccountGatewayAssociation_basic
=== PAUSE TestAccAwsDxCrossAccountGatewayAssociation_basic
=== CONT  TestAccAwsDxCrossAccountGatewayAssociation_basic
--- PASS: TestAccAwsDxCrossAccountGatewayAssociation_basic (1269.86s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1269.894s
$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes -timeout 120m
=== RUN   TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes
=== PAUSE TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes
=== CONT  TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes
--- PASS: TestAccAwsDxCrossAccountGatewayAssociation_allowedPrefixes (1647.82s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1647.874s

aws_dx_gateway_association_proposal

$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociationProposal_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxGatewayAssociationProposal_ -timeout 120m
=== RUN   TestAccAwsDxGatewayAssociationProposal_basic
=== PAUSE TestAccAwsDxGatewayAssociationProposal_basic
=== RUN   TestAccAwsDxGatewayAssociationProposal_disappears
=== PAUSE TestAccAwsDxGatewayAssociationProposal_disappears
=== RUN   TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
=== PAUSE TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
=== CONT  TestAccAwsDxGatewayAssociationProposal_basic
=== CONT  TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
=== CONT  TestAccAwsDxGatewayAssociationProposal_disappears
--- PASS: TestAccAwsDxGatewayAssociationProposal_disappears (70.28s)
--- PASS: TestAccAwsDxGatewayAssociationProposal_basic (75.87s)
--- PASS: TestAccAwsDxGatewayAssociationProposal_allowedPrefixes (94.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	94.510s

Removing WIP.
Ready for review

ewbankkit · 2019-04-29T00:33:47Z

aws/resource_aws_dx_gateway_association_proposal.go

@@ -18,12 +19,29 @@ func resourceAwsDxGatewayAssociationProposal() *schema.Resource {
 			State: schema.ImportStatePassthrough,
 		},

+		CustomizeDiff: customdiff.Sequence(


Accepting the proposal with overridden prefixes changes the returned RequestedAllowedPrefixesToDirectConnectGateway value.
We only want to force a new resource if this value changes and the current proposal state is requested.

This would be helpful to add as a comment within the code base.

Will do (once the changes are made in #8528).

ewbankkit · 2019-04-29T00:34:35Z

aws/resource_aws_dx_cross_account_gateway_association_test.go

+					resource.TestCheckResourceAttr(resourceName, "allowed_prefixes.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "allowed_prefixes.1642241106", "10.255.255.8/29"),
+				),
+				ExpectNonEmptyPlan: true,


Non-empty plan because of overridden prefixes - see https://github.com/terraform-providers/terraform-provider-aws/pull/8455/files#r279220520.

ewbankkit · 2019-04-29T00:35:01Z

aws/resource_aws_dx_gateway_association_proposal.go

@@ -153,45 +171,3 @@ func describeDirectConnectGatewayAssociationProposal(conn *directconnect.DirectC

 	return nil, nil
 }
-


Use the equivalent functionality in structure.go.

…gateway_association.

ewbankkit · 2019-04-29T16:39:13Z

Rebased to fix conflicts.

ewbankkit · 2019-05-05T21:37:51Z

Changing to WIP to incorporate the changes for #8490.
Related:

r/aws_dx_gateway_association: Update for transit gateway support plus the second half (cross-account proposal acceptance) of support for Gateway Association Proposals #8528

nywilken · 2019-05-06T21:55:52Z

Hi @ewbankkit sorry for the delay on this review. I see that you’ve flipped this back into a WIP PR but I want to leave a quick suggestion and ask a question about the approach.

nywilken · 2019-05-06T17:43:02Z

aws/resource_aws_dx_gateway_association_proposal.go

@@ -153,45 +171,3 @@ func describeDirectConnectGatewayAssociationProposal(conn *directconnect.DirectC

 	return nil, nil
 }
-
-func expandDirectConnectGatewayAssociationProposalAllowedPrefixes(allowedPrefixes []interface{}) []*directconnect.RouteFilterPrefix {


It's actually preferred to keep these functions defined within the resource and not in structure.go. While I understand the rationale, shared functions within structure.go has actually become an antipattern for the provider and it is a convention that we are moving away from. The CONTRIBUTING guide will be updated to reflect this as well.

Moving forward the preference is to have code duplicated across similar resources with a smaller defined scope, then having one or two functions (in a high-touch file) shared across multiple resources.

Sure, I'll keep these routines once the changes are made in #8528.

nywilken · 2019-05-06T20:52:44Z

aws/resource_aws_dx_cross_account_gateway_association.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceAwsDxCrossAccountGatewayAssociation() *schema.Resource {


@ewbankkit in the original issue #8100 you mentioned updating the existing dx_gateway_association resource with the new attributesvpn_gateway_owner_account_id and proposal_id

But then you mention implementing a new resource as the path forward. I don't see a clear explanation why you found this approach to be the better option. Did you run into issues adding the additional schema attributes?

I'm concerned that having two resources that do the same thing with a slight twist may be confusing to the operator so I am wondering if your first approach is a viable option.

@nywilken Two main reasons I am thinking of a separate resource (and I'm totally open to changing):

When I thought about the changes required to aws_dx_gateway_association, the creation functionality would basically have to be split in two, one for the current case with dx_gateway_id and vpn_gateway_id and one for proposal_id and vpn_gateway_owner_account_id and most of the current Required attributes would have to be made Optional and runtime checking done to make sure that the right combinations of attributes were set

In the AWS API the association is actually visible from "both ends", proposer and accepter although this resource applies only to the accepter end

But given the work in #8528 to support transit gateways I will take another look (for example vpn_gateway_id has had to go from Required to Optional as we now have another attribute associated_gateway_id that can accept both VGW and Transit Gateway IDs).

…ays.

ewbankkit · 2019-05-08T19:01:41Z

@nywilken / @bflad Given the changes I am making to aws_dx_gateway_association to add the transit gateway support, the additional changes to support the cross-account/proposal acceptance scenario are now relatively small and I am now convinced that the least confusing solution is to go back to #8100 (comment) and make the changes to the aws_dx_gateway_association resource.
Given that are you OK for me to incorporate the relevant changes from this PR into #8528 and close this one?

BTW, the current status on #8528 is that I just have to run the acceptance tests but kept getting failures on the terraform destroy part with error like

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_deprecated'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsDxGatewayAssociation_deprecated -timeout 120m
=== RUN   TestAccAwsDxGatewayAssociation_deprecated
=== PAUSE TestAccAwsDxGatewayAssociation_deprecated
=== CONT  TestAccAwsDxGatewayAssociation_deprecated
--- FAIL: TestAccAwsDxGatewayAssociation_deprecated (1212.63s)
    testing.go:629: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: IncorrectState: The VPN gateway is in use.
        	status code: 400, request id: 1419626e-9204-4349-b2eb-0a913508f4a5
        
        State: aws_vpn_gateway.test:
          ID = vgw-0000000000000000
          provider = provider.aws
          amazon_side_asn = 64512
          tags.% = 1
          tags.Name = terraform-testacc-dxgwassoc-3462434342993075499
          vpc_id = vpc-0000000000000000
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	1212.695s
make: *** [testacc] Error 1

but I have just found the problem that causes this (and probably also the issues mentioned in #8222 (review)), this code https://github.com/terraform-providers/terraform-provider-aws/blob/3471582595cb9cd20d20abe2ca7f7463499edf6a/aws/resource_aws_vpn_gateway.go#L333-L340
which doesn't take into account the detaching state 😄.

ewbankkit · 2019-05-09T13:49:15Z

Closing this PR in favor of consolidating the changes into #8528.

ghost · 2020-03-30T17:20:56Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ewbankkit changed the title ~~[WIP] Second half of support for Gateway Association Proposals~~ Second half of support for Gateway Association Proposals Apr 29, 2019

ewbankkit commented Apr 29, 2019

View reviewed changes

ewbankkit added 2 commits April 29, 2019 12:32

Add aws_dx_gateway_association_proposal_accepter resource.

68e0285

aws_dx_gateway_association_proposal_accepter -> aws_dx_cross_account_…

90364ec

…gateway_association.

ewbankkit force-pushed the issue-8100.accepter branch from 835f931 to 43ee094 Compare April 29, 2019 16:38

ewbankkit mentioned this pull request May 1, 2019

Allow aws_dx_gateway_association and aws_dx_private_virtual_interface to reference Transit Gateway #8490

Closed

ewbankkit changed the title ~~Second half of support for Gateway Association Proposals~~ [WIP] Second half of support for Gateway Association Proposals May 5, 2019

Fixes and additional acceptance tests.

2ea6419

ewbankkit force-pushed the issue-8100.accepter branch from 43ee094 to 2ea6419 Compare May 6, 2019 19:01

nywilken added this to the v2.10.0 milestone May 6, 2019

nywilken reviewed May 6, 2019

View reviewed changes

Update aws_dx_gateway_association_proposal resource for transit gatew…

9d7f6bc

…ays.

ewbankkit mentioned this pull request May 8, 2019

aws_vpn_gateway and aws_vpn_gateway_attachment incorrectly refreshing attachment state #8575

Closed

ewbankkit closed this May 9, 2019

ewbankkit mentioned this pull request May 9, 2019

r/aws_dx_gateway_association: Update for transit gateway support plus the second half (cross-account proposal acceptance) of support for Gateway Association Proposals #8528

Merged

ewbankkit deleted the issue-8100.accepter branch May 10, 2019 12:44

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Second half of support for Gateway Association Proposals #8455

[WIP] Second half of support for Gateway Association Proposals #8455

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 29, 2019

ewbankkit Apr 29, 2019 •

edited

Loading

nywilken May 7, 2019

ewbankkit May 9, 2019

ewbankkit Apr 29, 2019

ewbankkit Apr 29, 2019

ewbankkit commented Apr 29, 2019

ewbankkit commented May 5, 2019

nywilken commented May 6, 2019

nywilken May 6, 2019

ewbankkit May 9, 2019

nywilken May 6, 2019

ewbankkit May 7, 2019

ewbankkit May 7, 2019

ewbankkit commented May 8, 2019

ewbankkit commented May 9, 2019

ghost commented Mar 30, 2020

		@@ -153,45 +171,3 @@ func describeDirectConnectGatewayAssociationProposal(conn *directconnect.DirectC

		return nil, nil
		}

[WIP] Second half of support for Gateway Association Proposals #8455

[WIP] Second half of support for Gateway Association Proposals #8455

Conversation

ewbankkit commented Apr 26, 2019

Community Note

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 26, 2019

ewbankkit commented Apr 29, 2019

aws_dx_cross_account_gateway_association

aws_dx_gateway_association_proposal

ewbankkit Apr 29, 2019 • 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

ewbankkit commented Apr 29, 2019

ewbankkit commented May 5, 2019

nywilken commented May 6, 2019

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

ewbankkit commented May 8, 2019

ewbankkit commented May 9, 2019

ghost commented Mar 30, 2020

ewbankkit Apr 29, 2019 •

edited

Loading