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.

Sign up for GitHub

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

Jump to bottom

r/aws_client_vpn_endpoint: Adding new resource to manage AWS Client VPN endpoints #7009

Merged

bflad merged 8 commits into hashicorp:master from slapula:resource-aws-client-vpn-endpoint

Feb 4, 2019

Contributor

slapula commented Jan 2, 2019 •

edited

Loading

Fixes #6949

Changes proposed in this pull request:

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

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsEc2ClientVpnEndpoint_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsEc2ClientVpnEndpoint_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsEc2ClientVpnEndpoint_basic
--- PASS: TestAccAwsEc2ClientVpnEndpoint_basic (10.77s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_msAD
--- PASS: TestAccAwsEc2ClientVpnEndpoint_msAD (1748.41s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withLogGroup
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withLogGroup (18.49s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withDNSServers
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withDNSServers (16.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1793.872s


          r/aws_client_vpn_endpoint: Adding new resource to manage AWS Client V…

33af5ed

…PN endpoints

ghost added size/XL documentation provider tests labels

slapula mentioned this pull request

Support for AWS Client VPN #6949

Closed

gazoakley reviewed

View reviewed changes

Contributor

gazoakley left a comment

Hi @slapula 👋 - thanks for looking at this 😃

There's a few comments below, mostly about waiting for the resource to become available before continuing. I noticed you also created import_aws_client_vpn_endpoint.go, but it looks like all attributes could be read by calling DescribeClientVpnEndpoint - much of the code you've written here looks like it should be called in resourceAwsClientVpnEndpointRead to update the resource during terraform refresh. Let me know if you've got any question.

aws/resource_aws_client_vpn_endpoint.go Outdated

+              	})
+              	if err != nil {
+              		return fmt.Errorf("Error reading Client VPN endpoint: %s", err)

Contributor

gazoakley Jan 2, 2019

We should add a check to see if the resource still exists, and if not remove it from state e.g.

	if len(result.ClientVpnEndpoints) == 0 {
		log.Printf("[WARN] Client VPN endpoint (%s) not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

aws/resource_aws_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint.go Outdated

+              		ClientVpnEndpointId: aws.String(d.Id()),
+              	})
+              	if err != nil {
+              		return fmt.Errorf("Error deleting Client VPN endpoint: %s", err)

Contributor

gazoakley Jan 2, 2019

We should check if the resource is already deleted, and if so just log a warning/return (the docs aren't clear on what the error response is if the Client VPN endpoint doesn't exist - you'll have to try and see unfortunately)

aws/resource_aws_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint_test.go Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint_test.go Outdated Show resolved Hide resolved

website/docs/r/client_vpn_endpoint.html.markdown Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_client_vpn_endpoint.go Outdated Show resolved Hide resolved

Contributor Author

slapula commented Jan 2, 2019

@gazoakley Thanks for the feedback! Thanks for bringing up endpoint status as I wasn't quite sure how to handle it with this first pass. According to the documentation, "status" for an endpoint appears to behave differently from how we expect it to with other AWS resources. I'm looking at this as a reference: https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/cvpn-getting-started.html#cvpn-getting-started-target. A Client VPN endpoint sets its status to pending-associate upon creation then has to wait for the operator to initiate an action that associates the CIDR block to the endpoint before it can change its status to available. Based on how I have the resource operating here, this would cause Terraform to wait indefinitely until that CIDR association action completes successfully.

Now, here's the fork in the road. Do we roll this AssociateClientVpnTargetNetwork API action into this resource or do we create another resource that specifically manages those associations (like aws_client_vpn_endpoint_association)? From what I can tell, the pattern we already follow with this provider seems to align with the latter option.

Hope that all makes sense 😄

Contributor

gazoakley commented Jan 2, 2019

@slapula Doh! Waiting on the endpoint resource to become active doesn't really make sense (although I'd probably keep a wait in place for deletion).

As for your second point - I'd also go with having a separate resource:

Each associate/disassociate is a separate API call, so it makes senses to allow Terraform to track those all individually
Association state of each resource instance can be tracked separately: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_AssociationStatus.html

I'd take a separate branch for each new resource - I think we'll also need a resource for authorization rules and for routes.

Contributor Author

slapula commented Jan 2, 2019

@gazoakley Yup, that was my original plan from the start. Let me address your review comments and then I'll get started on the resource for CIDR associations.


          updates based on review feedback

f1bf55f

Contributor Author

slapula commented Jan 2, 2019

@gazoakley I believe I've addressed your comments! Let me know if you need me to take a look at anything else.

bflad reviewed

View reviewed changes

aws/provider.go Outdated

@@ @@ -337,6 +337,7 @@ func Provider() terraform.ResourceProvider { @@
               			"aws_autoscaling_policy":                           resourceAwsAutoscalingPolicy(),
               			"aws_autoscaling_schedule":                         resourceAwsAutoscalingSchedule(),
               			"aws_budgets_budget":                               resourceAwsBudgetsBudget(),
+              			"aws_client_vpn_endpoint":                          resourceAwsClientVpnEndpoint(),

Contributor

bflad Jan 3, 2019 •

edited

Loading

I apologize I don't have time for a full review right now, but I figure its worth mentioning that we are preferring to add EC2 naming to EC2 service resources going forward to help consolidate the resource naming and in this case prevent any future issues where another service might introduce a "Client VPN". 👍

Suggested change

      
            			"aws_client_vpn_endpoint":                          resourceAwsClientVpnEndpoint(),
          
            			"aws_ec2_client_vpn_endpoint":                      resourceAwsEc2ClientVpnEndpoint(),

(along with matching function and filename updates)

Contributor Author

slapula Jan 3, 2019 •

edited

Loading

@bflad This actually another good question. The API classifies this as an EC2 service but I see that Terraform also categorizes some VPC services separately even if they are still technically filed under EC2. If you look at the Client VPN service in the console, you'll see this reflected by the fact that it's accessible in the VPC section. Would it be preferable to name this aws_vpc_client_vpn_endpoint?

Contributor

bflad Jan 3, 2019

I would lean towards aws_ec2_client_vpn_endpoint. Between the two (_ec2_ vs _vpc_) we do have precedent of only including _vpc_ in the resource naming only with EC2 API operations with "Vpc" in the name (except for aws_vpc_dhcp_options). This also provides some potential future proofing if some of these EC2 networking related resources work outside VPCs should some other networking concepts be introduced in the API.

The important part to remember here is that the API naming is considerably more stable than the web console, which could (theoretically) change at any time with browser redirects.

Contributor Author

slapula Jan 3, 2019

Okay, makes sense to me! I'll go ahead and make the appropriate changes.

Contributor Author

slapula Jan 3, 2019

@bflad This has been done. See 9f077a7.


          renaming aws resource

9f077a7

ghost added the service/ec2 label

slapula mentioned this pull request

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

Merged

bflad added the new-resource label

bflad reviewed

View reviewed changes

Contributor

bflad left a comment

Hi @slapula 👋 Thanks for submitting this! Looking pretty good. Please let us know if you have any questions or do not have time to implement the feedback.

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated

+              		Type: aws.String(data["type"].(string)),
+              	}
+              	if data["type"].(string) == "certificate-authentication" {

Contributor

bflad Jan 15, 2019

Nit: AWS Go SDK provided constant available:

Suggested change

      
            	if data["type"].(string) == "certificate-authentication" {
          
            	if data["type"].(string) == ec2.ClientVpnAuthenticationTypeCertificateAuthentication {

Contributor Author

slapula Jan 16, 2019

I tried using the suggested change above but it never worked for me (both with and without defining the type as string). Am I just not comparing these values the right way?

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated

+              		}
+              	}
+              	if data["type"].(string) == "directory-service-authentication" {

Contributor

bflad Jan 15, 2019

Nit: AWS Go SDK provided constant available:

Suggested change

      
            	if data["type"].(string) == "directory-service-authentication" {
          
            	if data["type"].(string) == ec2.ClientVpnAuthenticationTypeDirectoryServiceAuthentication {

aws/resource_aws_ec2_client_vpn_endpoint.go Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

bflad added the waiting-response label

slapula added 2 commits

January 16, 2019 11:06


          changes based on review feedback

928d2a7


          more changes based on review feedback

5bd8088

Contributor Author

slapula commented Jan 17, 2019

@bflad Thanks for the feedback! I believe I have address all of your comments.

ghost removed the waiting-response label


          Merge branch 'master' into resource-aws-client-vpn-endpoint

ffca670

Contributor Author

slapula commented Jan 17, 2019

@bflad It appears that gosimple was enabled and is now alerting on a soon-to-be removed check: https://staticcheck.io/changes/2019.1

Contributor

hussfelt commented Jan 30, 2019

Stalled? :)
Awesome resource to have.

bflad reviewed

View reviewed changes

Contributor

bflad left a comment

Hi again @slapula 👋 A few things came up during testing that need to be addressed, but otherwise looks good. Once these are fixed up we can get this in. Please let us know if you have any questions or do not have time to implement the feedback. Thanks!

aws/resource_aws_ec2_client_vpn_endpoint.go Outdated Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint.go Show resolved Hide resolved

aws/resource_aws_ec2_client_vpn_endpoint_test.go Outdated Show resolved Hide resolved

bflad added the waiting-response label

slapula added 2 commits

February 4, 2019 08:38


          Merge branch 'master' into resource-aws-client-vpn-endpoint

bea1ff2


          changes based on review feedback

b468e63

Contributor Author

slapula commented Feb 4, 2019

@bflad Thanks again for your feedback! I believe I've addressed your comments. This ready for another round.

ghost removed the waiting-response label

bflad added this to the v1.58.0 milestone

bflad approved these changes

View reviewed changes

Contributor

bflad left a comment

Looks great, thanks @slapula 🚀

--- PASS: TestAccAwsEc2ClientVpnEndpoint_basic (9.08s)
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withLogGroup (10.64s)
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withDNSServers (13.51s)
--- PASS: TestAccAwsEc2ClientVpnEndpoint_msAD (1741.74s)

bflad merged commit 726f08a into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #7009

6380b4b

Contributor

bflad commented Feb 8, 2019

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.

Bwanabanana commented Feb 9, 2019

Thanks for this feature, it saves a lot of messing about! Some feedback - I couldn't find a way to automatically authorise ingress on the client vpn endpoint. Also, would be nice to be able to associate one or more custom security groups (perhaps using another resource) rather than being stuck with it using the VPC's default group.

Contributor Author

slapula commented Feb 9, 2019

@Bwanabanana can you open a new issue and tag me in it? Let get a formal request open and I'll take a look when I have some free cycles.

This was referenced Feb 10, 2019

Client VPN Endpoint - Add Ingress Authorisation #7494

Closed

Client VPN Endpoint - Allow Custom Security Groups #7495

Closed

Bwanabanana commented Feb 10, 2019 •

edited

Loading

@slapula - all done, many thanks!

Edit: I'd say #7494 would be higher priority since we can at least use the default security group for access whereas there's no way of connecting without some ingress authorisation configuration (that I'm aware of, anyway).

ghost commented Apr 1, 2020

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!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation new-resource provider service/ec2 size/XL tests