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

[slapula]

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

[gazoakley]

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.

[gazoakley]

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
	}

[gazoakley]

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)

[slapula]

@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 😄

[gazoakley]

@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.

[slapula]

@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.

[slapula]

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

[bflad]

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)

[slapula]

@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?

[bflad]

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.

[slapula]

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

[slapula]

@bflad This has been done. See 9f077a7.

[bflad]

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.

[bflad]

Nit: AWS Go SDK provided constant available:

Suggested change

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

[slapula]

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?

[bflad]

Nit: AWS Go SDK provided constant available:

Suggested change

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

[slapula]

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

[slapula]

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

[hussfelt]

Stalled? :)
Awesome resource to have.

[bflad]

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!

[slapula]

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

[bflad]

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]

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]

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.

[slapula]

@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.

[Bwanabanana]

@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]

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!