r/aws_ec2_client_vpn_network_association: Adding resource to manage Client VPN network associations #7030

slapula · 2019-01-04T05:44:34Z

Changes proposed in this pull request:

Adds a new resource that manages AWS Client VPN endpoints: aws_ec2_client_vpn_network_association

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsEc2ClientVpnNetworkAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsEc2ClientVpnNetworkAssociation_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsEc2ClientVpnNetworkAssociation_basic
=== PAUSE TestAccAwsEc2ClientVpnNetworkAssociation_basic
=== CONT  TestAccAwsEc2ClientVpnNetworkAssociation_basic
--- PASS: TestAccAwsEc2ClientVpnNetworkAssociation_basic (661.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	661.424s

…lient VPN network associations

slapula · 2019-01-04T05:48:50Z

This branch is based off #7009 since I need that functionality to run tests.

slapula · 2019-01-04T05:50:27Z

@gazoakley Here's where your WaitForState() logic comes into play for AWS Client VPNs.

slapula · 2019-01-04T05:53:19Z

I am aware the documentation is missing at the moment. I'll be adding that tomorrow.

estein9825 · 2019-01-25T15:57:53Z

When is this going to be merged into the master?

slapula · 2019-01-25T16:01:39Z

@estein9825 this is currently waiting on #7009 and, once that is merged, a proper review by the maintainers.

bflad

Hey again @slapula 👋 Appreciate yet another good contribution! Left some initial review items below aside from rebasing with master since it now includes the aws_ec2_client_vpn_endpoint resource. Please reach out with any questions or let us know if you do not have time to implement the feedback.

bflad · 2019-02-05T00:28:12Z

aws/resource_aws_ec2_client_vpn_network_association.go

+		Create: resourceAwsEc2ClientVpnNetworkAssociationCreate,
+		Read:   resourceAwsEc2ClientVpnNetworkAssociationRead,
+		Delete: resourceAwsEc2ClientVpnNetworkAssociationDelete,
+		Importer: &schema.ResourceImporter{


This import support is missing:

Acceptance testing in the form of a TestStep that includes ImportState: true and ImportStateVerify: true

Documentation in the form of a ## Import section in the resource documentation.

The support should be fully implemented or removed from the pull request. 👍

If you are opting to keep the support, you will notice that you will need to either implement a custom import function to that calls d.Set("client_vpn_endpoint_id", ...) and trims that part of the ID for d.SetId() (then use ImportStateIdFunc in acceptance testing) or switch the resource to implementing a "complex" ID that includes both IDs in its string, e.g. ENDPOINT/ASSOCIATION. For new implementations, we have generally preferred the latter form where we parse the complex ID and continue to use ImportStatePassthrough, but either form is acceptable.

Oh, this is cruft from my copy/paste when I started working on this. I'm going to opt to not include import functionality here since I wasn't planning on adding it initially. I can be swayed to add it however because I can see the need for it.

aws/resource_aws_ec2_client_vpn_network_association.go

aws/resource_aws_ec2_client_vpn_network_association_test.go

slapula · 2019-02-06T21:47:39Z

aws/resource_aws_ec2_client_vpn_network_association.go

+	log.Printf("[DEBUG] Waiting for Client VPN endpoint to disassociate with target network: %s", d.Id())
+	_, err = stateConf.WaitForState()
+	if err != nil {
+		if strings.Contains(err.Error(), "couldn't find resource") != true {


@bflad This seems super janky to me. Is there a better way to handle this exception? I can't seem to find the specific error code that is tripping on this condition so that I can put it in the RefreshFunc (where this logic should actually live IMO).

Great question! Yeah, don't fret too much about this as EC2 does things a bit differently than other AWS services for errors and they do not make the error codes available in the AWS Go SDK as constants. 🙁

For reference, they mostly (I've seen cases where they are missing for a long period of time) can be found here: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html

Rather than guessing from the API reference though, I personally tend to look for the error codes by enabling debug logging in Terraform during an acceptance test run. For example, in this case, I ran the below:

$ rm -f aws/log.txt; TF_LOG_PATH=log.txt TF_LOG=debug TF_ACC=1 go test ./aws -v -timeout 120m -parallel 20 -run=TestAccAwsEc2ClientVpnNetworkAssociation_basic === RUN TestAccAwsEc2ClientVpnNetworkAssociation_basic === PAUSE TestAccAwsEc2ClientVpnNetworkAssociation_basic === CONT TestAccAwsEc2ClientVpnNetworkAssociation_basic --- PASS: TestAccAwsEc2ClientVpnNetworkAssociation_basic (690.14s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 691.015s

Which tells Terraform to create a log file with the debug output (you can stick it anywhere, I'm just lazy with log.txt 😄) and keep the console output "normal". In the debug log it output this:

2019/02/07 21:22:56 [DEBUG] [aws-sdk-go] DEBUG: Response ec2/DescribeClientVpnTargetNetworks Details: ---[ RESPONSE ]-------------------------------------- HTTP/1.1 400 Bad Request Connection: close Transfer-Encoding: chunked Date: Fri, 08 Feb 2019 02:22:55 GMT Server: AmazonEC2 ----------------------------------------------------- 2019/02/07 21:22:56 [DEBUG] [aws-sdk-go] <?xml version="1.0" encoding="UTF-8"?> <Response><Errors><Error><Code>InvalidClientVpnEndpointId.NotFound</Code><Message>Endpoint cvpn-endpoint-0f2e85e16167a3465 does not exist</Message></Error></Errors><RequestID>31e4906a-8249-4e2d-9284-a03a2672ff86</RequestID></Response> 2019/02/07 21:22:56 [DEBUG] [aws-sdk-go] DEBUG: Validate Response ec2/DescribeClientVpnTargetNetworks failed, not retrying, error InvalidClientVpnEndpointId.NotFound: Endpoint cvpn-endpoint-0f2e85e16167a3465 does not exist

The InvalidClientVpnEndpointId.NotFound is the error code we can use for easily checking, e.g.

if isAWSErr(err, "InvalidClientVpnEndpointId.NotFound", "") {

🎉

Haha and after writing this I notice we should check for the above and InvalidClientVpnAssociationId.NotFound

slapula · 2019-02-06T21:48:04Z

@bflad This is ready for another look.

bflad

This LGTM with one little error handling bit to fix which I'll adjust on merge so we can release today. Great job, @slapula!! 🚀

--- PASS: TestAccAwsEc2ClientVpnNetworkAssociation_basic (690.14s)

bflad · 2019-02-08T04:00:38Z

aws/resource_aws_ec2_client_vpn_network_association.go

+	log.Printf("[DEBUG] Waiting for Client VPN endpoint to disassociate with target network: %s", d.Id())
+	_, err = stateConf.WaitForState()
+	if err != nil {
+		if !strings.Contains(err.Error(), "couldn't find resource") {


Just so we can get this released today, I'm going to fix this last little bit up given my previous comment. I hope you don't mind. 😄

bflad · 2019-02-08T08:37:01Z

This has been released in version 1.58.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-02-19T14:50:25Z

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!

slapula added 3 commits January 3, 2019 12:53

initial commit

1a20855

fixing merge conflict

b199c4c

r/aws_ec2_client_vpn_network_association: Adding resource to manage C…

fc1ff56

…lient VPN network associations

slapula added 2 commits January 4, 2019 11:08

adding doc to website

c6c3c36

fixing typo

370d78d

slapula mentioned this pull request Feb 4, 2019

Support for AWS Client VPN #6949

Closed

bflad added the new-resource Introduces a new resource. label Feb 4, 2019

bflad reviewed Feb 5, 2019

View reviewed changes

Merge branch 'master' into resource-aws-client-vpn-network-association

6b9a661

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Feb 5, 2019

slapula added 2 commits February 6, 2019 08:39

more changes based on review feedback

cf6d047

more review changes

9e31e5e

slapula commented Feb 6, 2019

View reviewed changes

removing bool constant

5d587d8

bflad approved these changes Feb 8, 2019

View reviewed changes

bflad added this to the v1.58.0 milestone Feb 8, 2019

bflad merged commit 5d587d8 into hashicorp:master Feb 8, 2019

bflad added a commit that referenced this pull request Feb 8, 2019

Update CHANGELOG for #7030

9c7ff98

ghost locked and limited conversation to collaborators Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_ec2_client_vpn_network_association: Adding resource to manage Client VPN network associations #7030

r/aws_ec2_client_vpn_network_association: Adding resource to manage Client VPN network associations #7030

slapula commented Jan 4, 2019 •

edited

Loading

slapula commented Jan 4, 2019

slapula commented Jan 4, 2019

slapula commented Jan 4, 2019

estein9825 commented Jan 25, 2019

slapula commented Jan 25, 2019

bflad left a comment

bflad Feb 5, 2019

slapula Feb 6, 2019

slapula Feb 6, 2019

bflad Feb 8, 2019

bflad Feb 8, 2019

slapula commented Feb 6, 2019

bflad left a comment

bflad Feb 8, 2019

bflad commented Feb 8, 2019

ghost commented Feb 19, 2020

r/aws_ec2_client_vpn_network_association: Adding resource to manage Client VPN network associations #7030

r/aws_ec2_client_vpn_network_association: Adding resource to manage Client VPN network associations #7030

Conversation

slapula commented Jan 4, 2019 • edited Loading

slapula commented Jan 4, 2019

slapula commented Jan 4, 2019

slapula commented Jan 4, 2019

estein9825 commented Jan 25, 2019

slapula commented Jan 25, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 5, 2019

Choose a reason for hiding this comment

slapula Feb 6, 2019

Choose a reason for hiding this comment

slapula Feb 6, 2019

Choose a reason for hiding this comment

bflad Feb 8, 2019

Choose a reason for hiding this comment

bflad Feb 8, 2019

Choose a reason for hiding this comment

slapula commented Feb 6, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 8, 2019

Choose a reason for hiding this comment

bflad commented Feb 8, 2019

ghost commented Feb 19, 2020

slapula commented Jan 4, 2019 •

edited

Loading