-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
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
Conversation
1e1d660
to
5aa5ec6
Compare
cd5c36d
to
15e5f6d
Compare
More success with acceptance tests having cherry picked the commit from #8576: $ 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
--- PASS: TestAccAwsDxGatewayAssociation_deprecated (1381.75s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 1381.806s |
This PR will now incorporate the cross-account Gateway Proposal acceptance work started in #8455. These changes will be incorporated into the |
Cross-account Direct Connect gateway association changes from #8455 have been incorporated and all acceptance tests are now passing. Note that I am including the commit from #8576 to ensure that VGW associations are correctly cleaned up. 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 2 -run=TestAccAwsDxGatewayAssociationProposal_ -timeout 120m
=== RUN TestAccAwsDxGatewayAssociationProposal_deprecated
=== PAUSE TestAccAwsDxGatewayAssociationProposal_deprecated
=== RUN TestAccAwsDxGatewayAssociationProposal_basicVpnGateway
=== PAUSE TestAccAwsDxGatewayAssociationProposal_basicVpnGateway
=== RUN TestAccAwsDxGatewayAssociationProposal_basicTransitGateway
=== PAUSE TestAccAwsDxGatewayAssociationProposal_basicTransitGateway
=== RUN TestAccAwsDxGatewayAssociationProposal_disappears
=== PAUSE TestAccAwsDxGatewayAssociationProposal_disappears
=== RUN TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
=== PAUSE TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
=== CONT TestAccAwsDxGatewayAssociationProposal_deprecated
=== CONT TestAccAwsDxGatewayAssociationProposal_allowedPrefixes
--- PASS: TestAccAwsDxGatewayAssociationProposal_deprecated (71.43s)
=== CONT TestAccAwsDxGatewayAssociationProposal_disappears
--- PASS: TestAccAwsDxGatewayAssociationProposal_allowedPrefixes (94.61s)
=== CONT TestAccAwsDxGatewayAssociationProposal_basicTransitGateway
--- PASS: TestAccAwsDxGatewayAssociationProposal_disappears (58.27s)
=== CONT TestAccAwsDxGatewayAssociationProposal_basicVpnGateway
--- PASS: TestAccAwsDxGatewayAssociationProposal_basicVpnGateway (60.52s)
--- PASS: TestAccAwsDxGatewayAssociationProposal_basicTransitGateway (146.13s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 240.815s aws_dx_gateway_association$ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 2 -run=TestAccAwsDxGatewayAssociation_ -timeout 120m
=== RUN TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
=== RUN TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
=== RUN TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
=== RUN TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
=== CONT TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== CONT TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
--- PASS: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (1348.63s)
=== CONT TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount (1885.66s)
=== CONT TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount (2193.34s)
=== CONT TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
--- PASS: TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount (1657.42s)
=== CONT TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount (1232.65s)
=== CONT TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount (1235.44s)
=== CONT TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount (1844.14s)
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount (1847.29s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 6623.071s |
|
d.Set("dx_gateway_owner_account_id", assoc.DirectConnectGatewayOwnerAccount) | ||
|
||
tgwAttachmentId := "" | ||
if aws.StringValue(assoc.AssociatedGateway.Type) == directconnect.GatewayTypeTransitGateway { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get the transit gateway attachment here to ensure that we wait for it's deletion when this resource is deleted.
See discussion: #8490 (comment).
Maybe move this code plus ec2TransitGatewayAttachmentStateRefresh()
and waitForEc2TransitGatewayAttachmentDeletion()
below to ec2_transit_gateway.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic for storing the AttachmentID should be removed from this resource. The logic for properly handling the deletion error has non-deleted DirectConnect Gateway Attachment
needs to be addressed in the existing aws_ec2_transit_gateway
resource as a separate PR. Please see my comment below.
Removing WIP as now ready for review, |
04354ca
to
5dafe20
Compare
Rebased to remove the fixes incorporated in #8576. |
Hi @ewbankkit ! So looks like you have code ready, it just needs to be merged? |
@aleksap Functionality is complete, a review is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ewbankkit thanks for your patience on this PR. I think the the single resource with the ability to handle single, and cross, account proposals is easier to follow and use. I am in the middle of the review, but I do have some quick suggestions for you. Please let me know if you have an questions so far.
gwIdRaw, gwIdOk := d.GetOk("associated_gateway_id") | ||
vgwIdRaw, vgwIdOk := d.GetOk("vpn_gateway_id") | ||
if !gwIdOk && !vgwIdOk { | ||
return errors.New("one of associated_gateway_id or vpn_gateway_id must be configured") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use fmt.Errorf
to keep error creation consistent across this code base.
input := &directconnect.CreateDirectConnectGatewayAssociationProposalInput{ | ||
AddAllowedPrefixesToDirectConnectGateway: expandDirectConnectGatewayAssociationProposalAllowedPrefixes(d.Get("allowed_prefixes").(*schema.Set).List()), | ||
DirectConnectGatewayId: aws.String(d.Get("dx_gateway_id").(string)), | ||
DirectConnectGatewayOwnerAccount: aws.String(d.Get("dx_gateway_owner_account_id").(string)), | ||
GatewayId: aws.String(d.Get("vpn_gateway_id").(string)), | ||
} | ||
if gwIdOk { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this conditional and the conditional on line 89 separated by a directconnect.CreateDirectConnectGatewayAssociationProposalInput
creation makes this a bit difficult to follow. I suggest replacing lines 89 - 104 with the following
allowedPrefixes := expandDirectConnectGatewayAssociationProposalAllowedPrefixes(d.Get("allowed_prefixes").(*schema.Set).List())
input := &directconnect.CreateDirectConnectGatewayAssociationProposalInput{
AddAllowedPrefixesToDirectConnectGateway: allowedPrefixes,
DirectConnectGatewayId: aws.String(d.Get("dx_gateway_id").(string)),
DirectConnectGatewayOwnerAccount: aws.String(d.Get("dx_gateway_owner_account_id").(string)),
}
var gwID string
if v, ok := d.GetOk("vpn_gateway_id"); ok {
gwID = v.(string)
} else if v, ok := d.GetOk("associated_gateway_id"); ok {
gwID = v.(string)
}
if gwID == "" {
return fmt.Errorf("gateway id not provided, one of associated_gateway_id or vpn_gateway_id must be configured")
}
input.GatewayId = aws.String(gwID)
@@ -71,7 +152,7 @@ func TestAccAwsDxGatewayAssociationProposal_disappears(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccAwsDxGatewayAssociationProposal_AllowedPrefixes(t *testing.T) { | |||
func TestAccAwsDxGatewayAssociationProposal_allowedPrefixes(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to camelcase the argument names so that they match their respective AWS SDK names.
@@ -101,7 +102,7 @@ func testSweepDirectConnectGatewayAssociations(region string) error { | |||
return nil | |||
} | |||
|
|||
func TestAccAwsDxGatewayAssociation_basic(t *testing.T) { | |||
func TestAccAwsDxGatewayAssociation_deprecatedSingleAccount(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't tend to rename or change existing tests on attribute deprecation as they should continue to work with the deprecated fields. Instead you should add a test case that goes from the deprecated attribute to the new attribute TestAccAwsDxGatewayAssociation_VpnGatewayId_Migration_AssociatedGatewayId
Review comment. Co-Authored-By: Wilken Rivera <dev@wilkenrivera.com>
Review comment. Co-Authored-By: Wilken Rivera <dev@wilkenrivera.com>
Review comment. Co-Authored-By: Wilken Rivera <dev@wilkenrivera.com>
Review comment. Co-Authored-By: Wilken Rivera <dev@wilkenrivera.com>
associated_gateway_owner_account_id = "${data.aws_caller_identity.creator.account_id}" | ||
} | ||
``` | ||
A full example of how to to create a VPN Gateway in one AWS account, create a Direct Connect Gateway in a second AWS account, and associate the VPN Gateway with the Direct Connect Gateway via the `aws_dx_gateway_association_proposal` and `aws_dx_gateway_association` resources can be found in [the `./examples/dx-gateway-cross-account-vgw-association` directory within the Github Repository](https://github.com/terraform-providers/terraform-provider-aws/tree/master/examples/dx-gateway-cross-account-vgw-association). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to there's an extra "to"
``` | ||
|
||
A full example of how to to create a VPN Gateway in one AWS account, create a Direct Connect Gateway in a second AWS account, and associate the VPN Gateway with the Direct Connect Gateway via the `aws_dx_gateway_association_proposal` and `aws_dx_gateway_association` resources can be found in [the `./examples/dx-gateway-cross-account-vgw-association` directory within the Github Repository](https://github.com/terraform-providers/terraform-provider-aws/tree/master/examples/dx-gateway-cross-account-vgw-association). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -0,0 +1,18 @@ | |||
# Direct Connect Gateway Cross-Account VGW Association |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Good call on moving this out to the examples directory.
f775493
to
93e2df2
Compare
Rebased from master with fixes from #8752. |
Acceptance tests: $ AWS_ALTERNATE_ACCESS_KEY_ID=AAAAAAAAAAAAAAAAAAAA AWS_ALTERNATE_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsDxGatewayAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 2 -run=TestAccAwsDxGatewayAssociation_ -timeout 120m
=== RUN TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
=== RUN TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
=== RUN TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
=== RUN TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
=== PAUSE TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
=== RUN TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
=== PAUSE TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
=== CONT TestAccAwsDxGatewayAssociation_deprecatedSingleAccount
=== CONT TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_deprecatedSingleAccount (1356.21s)
=== CONT TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount
--- PASS: TestAccAwsDxGatewayAssociation_multiVpnGatewaysSingleAccount (1356.50s)
=== CONT TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicTransitGatewaySingleAccount (1760.16s)
=== CONT TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount
--- FAIL: TestAccAwsDxGatewayAssociation_basicTransitGatewayCrossAccount (1762.87s)
testing.go:568: Step 0 error: errors during apply:
Error: error waiting for Direct Connect gateway association (ga-xxxxxxxx) to become available: timeout while waiting for state to become 'associated' (last state: 'associating', timeout: 15m0s)
on /var/folders/4j/lf5jdq5j4y91g80ytf6qzjwmk6drp5/T/tf-test232917661/main.tf line 39:
(source code not available)
=== CONT TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewaySingleAccount (1966.67s)
=== CONT TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewayCrossAccount
--- PASS: TestAccAwsDxGatewayAssociation_basicVpnGatewayCrossAccount (1971.17s)
=== CONT TestAccAwsDxGatewayAssociation_allowedPrefixesVpnGatewaySingleAccount
panic: test timed out after 2h0m0s I don't think these failures are anything to do with the changes but would like independent verification. |
Running the test by itself appears to work properly. Have you tried updating the timeout interval to see if all tests pass without timeout? I'm currently running that now with a 30 minute timeout.
In the meantime, I am looking to see if there are any missing pieces as @bflad pointed me to this note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ewbankkit each of the tests past individually. Adding additional timing (e.g 30 minutes) does not change much. I've seen the VpnGateway and TransitGateway test time out when trying to run all the tests in parallel at various times.
I am going to approve this PR as I would like to get this out with v2.12.0 release. Lets monitor its usage to see if the timeout is an issue for users. In the meantime, I will continue to dive into the tests.
Thanks for the time and patience. Nicely done 👍
This has been released in version 2.12.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
@ewbankkit @nywilken Thank you for your work on this feature. Error: module.dx_vgw_assoc.aws_dx_gateway_association.dx_gw_assoc: "vpn_gateway_id": required field is not set It almost looks like that "destroy" part is not aware that vpn_gateway_id is deprecated and is not recognizing associated_gateway_id argument. Please let me know if this makes sense and if it does should I open bug ticket. Thank you, |
@aleksap Strange. It looks like somehow an earlier version of the provider (pre-v2.12.0) is being run here, hence the errors about missing |
@ewbankkit I am not sure what happened but after deleting my .terraform folder and re-downloading provider it just started working! Thanks for the quick reply! |
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! |
Community Note
Transit gateway support
Part of the work for #8490.
Cross-account proposal acceptance
Fixes #8100.
Follows on from:
Release note for CHANGELOG: