-
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
New resources: aws_dx_gateway, aws_dx_gateway_assocation #2861
Conversation
Hi @aeriff Thanks for the work here, looks promising! Would you mind splitting this PR in two (if possible) with 1 resource per PR? Would be easier for us to review & merge :) Thanks! |
@Ninir thanks for the feedback. I have split them into separate PRs as requested. |
Create: resourceAwsDxGatewayCreate, | ||
Read: resourceAwsDxGatewayRead, | ||
Delete: resourceAwsDxGatewayDelete, | ||
Importer: &schema.ResourceImporter{ |
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.
If terraform import
is supported please note in the documentation and add an import acceptance test.
I did that for the existing Direct Connect resources in #2992.
@ewbankkit thanks for the reminder! Adding import support was an afterthought, so I missed writing the tests.
|
I decided to merge the 2 PRs back into one because both resources now have dependencies on each other. The resources are in 2 separate commits, so review shouldn't be any more difficult. All the discussion has happened so far in this PR, so I will close #2862 in favour of this. Acceptance tests:
|
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 @aeriff
I'm not sure why but the attached acceptance tests are currently failing for me:
=== RUN TestAccAwsDxGateway_importBasic
--- PASS: TestAccAwsDxGateway_importBasic (54.18s)
=== RUN TestAccAwsDxGateway_importComplex
--- FAIL: TestAccAwsDxGateway_importComplex (390.02s)
testing.go:573: Error destroying resource! WARNING: Dangling resources
may exist. The full state and error is shown below.
Error: Error applying: 2 error(s) occurred:
* aws_vpn_gateway.test2 (destroy): 1 error(s) occurred:
* aws_vpn_gateway.test2: IncorrectState: The VPN gateway is in use.
status code: 400, request id: e537c998-2b90-4224-9361-a41cee2d558c
* aws_vpn_gateway.test1 (destroy): 1 error(s) occurred:
* aws_vpn_gateway.test1: IncorrectState: The VPN gateway is in use.
status code: 400, request id: 394b4a08-ed9b-4c93-b90a-e546281198b0
State: aws_vpc.test1:
ID = vpc-628a0a1b
provider = provider.aws
assign_generated_ipv6_cidr_block = false
cidr_block = 10.255.255.16/28
default_network_acl_id = acl-3cf5f145
default_route_table_id = rtb-237ad65b
default_security_group_id = sg-7bcf7704
dhcp_options_id = dopt-11ac7e74
enable_classiclink = false
enable_classiclink_dns_support = false
enable_dns_hostnames = false
enable_dns_support = true
instance_tenancy = default
main_route_table_id = rtb-237ad65b
tags.% = 0
aws_vpc.test2:
ID = vpc-6c870715
provider = provider.aws
assign_generated_ipv6_cidr_block = false
cidr_block = 10.255.255.32/28
default_network_acl_id = acl-08e9ed71
default_route_table_id = rtb-6178d419
default_security_group_id = sg-bdcc74c2
dhcp_options_id = dopt-11ac7e74
enable_classiclink = false
enable_classiclink_dns_support = false
enable_dns_hostnames = false
enable_dns_support = true
instance_tenancy = default
main_route_table_id = rtb-6178d419
tags.% = 0
aws_vpn_gateway.test1:
ID = vgw-c98521d7
provider = provider.aws
amazon_side_asn = 7224
tags.% = 0
vpc_id = vpc-628a0a1b
Dependencies:
aws_vpc.test1
aws_vpn_gateway.test2:
ID = vgw-c88521d6
provider = provider.aws
amazon_side_asn = 7224
tags.% = 0
vpc_id = vpc-6c870715
Dependencies:
aws_vpc.test2
=== RUN TestAccAwsDxGatewayAssociation_basic
--- PASS: TestAccAwsDxGatewayAssociation_basic (94.76s)
=== RUN TestAccAwsDxGatewayAssociation_multiVgws
--- FAIL: TestAccAwsDxGatewayAssociation_multiVgws (403.21s)
testing.go:573: Error destroying resource! WARNING: Dangling resources
may exist. The full state and error is shown below.
Error: Error applying: 2 error(s) occurred:
* aws_vpn_gateway.test1 (destroy): 1 error(s) occurred:
* aws_vpn_gateway.test1: IncorrectState: The VPN gateway is in use.
status code: 400, request id: 108c06cf-29fb-4c07-b278-12cab7e12569
* aws_vpn_gateway.test2 (destroy): 1 error(s) occurred:
* aws_vpn_gateway.test2: IncorrectState: The VPN gateway is in use.
status code: 400, request id: 39fb5662-57f3-410d-bc97-92c6b055818c
State: aws_vpc.test1:
ID = vpc-c68c0cbf
provider = provider.aws
assign_generated_ipv6_cidr_block = false
cidr_block = 10.255.255.16/28
default_network_acl_id = acl-e8ebef91
default_route_table_id = rtb-ee7bd796
default_security_group_id = sg-c4b800bb
dhcp_options_id = dopt-11ac7e74
enable_classiclink = false
enable_classiclink_dns_support = false
enable_dns_hostnames = false
enable_dns_support = true
instance_tenancy = default
main_route_table_id = rtb-ee7bd796
tags.% = 0
aws_vpc.test2:
ID = vpc-c08d0db9
provider = provider.aws
assign_generated_ipv6_cidr_block = false
cidr_block = 10.255.255.32/28
default_network_acl_id = acl-98f0f4e1
default_route_table_id = rtb-5677db2e
default_security_group_id = sg-8db901f2
dhcp_options_id = dopt-11ac7e74
enable_classiclink = false
enable_classiclink_dns_support = false
enable_dns_hostnames = false
enable_dns_support = true
instance_tenancy = default
main_route_table_id = rtb-5677db2e
tags.% = 0
aws_vpn_gateway.test1:
ID = vgw-fe8521e0
provider = provider.aws
amazon_side_asn = 7224
tags.% = 0
vpc_id = vpc-c68c0cbf
Dependencies:
aws_vpc.test1
aws_vpn_gateway.test2:
ID = vgw-c18521df
provider = provider.aws
amazon_side_asn = 7224
tags.% = 0
vpc_id = vpc-c08d0db9
Dependencies:
aws_vpc.test2
=== RUN TestAccAwsDxGateway_basic
--- PASS: TestAccAwsDxGateway_basic (42.83s)
FAIL
FAIL github.com/terraform-providers/terraform-provider-aws/aws 985.024s
btw. you may want to consider generating random ASN so that multiple tests can run safely in parallel. See how we do it here:
return fmt.Errorf("Found %d Direct Connect Gateway associations for %s, expected 1", len(resp.DirectConnectGatewayAssociations), d.Id()) | ||
} | ||
if *resp.DirectConnectGatewayAssociations[0].VirtualGatewayId != d.Get("virtual_gateway_id").(string) { | ||
d.SetId("") |
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.
Do you mind adding a WARN
log message here so we know the resource is getting wiped? It can be helpful during debugging.
Example:
} | ||
|
||
func dxGatewayIdVgwIdHash(gatewayId, vgwId string) string { | ||
return fmt.Sprintf("ga-%s%d", gatewayId, hashcode.String(vgwId)) |
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.
Is there any particular reason we're hashing the VGW ID here? Wouldn't just simple concatenation be sufficient?
website/aws.erb
Outdated
@@ -624,6 +624,11 @@ | |||
<li<%= sidebar_current("docs-aws-resource-dx-connection-association") %>> | |||
<a href="/docs/providers/aws/r/dx_connection_association.html">aws_dx_connection_association</a> | |||
</li> | |||
<li<%= sidebar_current("docs-aws-resource-dx-gateway") %>> | |||
<a href="/docs/providers/aws/r/dx_gateway.html">aws_dx_gateway</a> | |||
<li<%= sidebar_current("docs-aws-resource-dx-gateway-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.
There's </li>
missing here. 👀
aws/resource_aws_dx_gateway.go
Outdated
} | ||
|
||
d.SetId(aws.StringValue(resp.DirectConnectGateway.DirectConnectGatewayId)) | ||
return resourceAwsDxGatewayRead(d, meta) |
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 that Terraform always hands over the resource when it's actually ready. Do you mind adding StateChangeConf
construct similar to the one that's in Delete
which waits until the DX GW is directconnect.GatewayStateAvailable
?
@radeksimko thanks for the review! All comments should all be addressed now.
The tests pass for me (almost) consistently. I also saw one I guess merging is blocked by hashicorp/terraform#17438, however. I tested with a custom random range function for now. |
@aeriff any idea when this will be released? Just checking. Thanks for your work! |
It would be great to see this merged. |
Has this been merged yet? being able to associate a direct connect gateway to VPC gateway would be really nice :) |
Definitely looking forward to seeing this merged. Will we be able to pass the ID of the created gateway to a route's |
While seeing this merged would be great, I wonder how viable it would be in a complex environment with multiple regions and accounts. Is there any plan to add a aws_dx_gateway data source at some point? |
@sleterrier Please open an issue(s) for any data source(s) you'd like added. |
@violetmythmaker The association between a Direct Connect Gateway and a Virtual Private Gateway is managed via the |
Also looking forward to seeing this merged. Are there any additional steps needed before the merge @ewbankkit, you are correct that a DX gateway cannot be associated with an Internet Gateway, only VGWs. |
@aeriff If you added your custom Also, for #3253 and #3255 I use the name |
@ewbankkit I agree on the attribute name change from I no longer have access to an AWS account for running the acceptance tests, nor access to the org that owns the repo I opened this PR against as I have changed jobs in the meantime. I won't have a lot of time to work on this in the immediate future but I'll see what I can do. |
@aeriff I can create another PR based on your commits and make the changes if you want - I assume your custom version of |
@ewbankkit thanks! Closing this in favour of your PR. |
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! |
I have used the same ASN validation code from #1888 to prove this works, but I suppose this blocks merging this PR until that is solved with the proposed addition of
TypeInt64
to Terraform.Resolves #2140.