r/aws_ec2_client_vpn_endpoint: Reworked endpoint resource to include network associations and authorization rules as parameters

[slapula]

Fixes #7494

Changes proposed in this pull request:

• Removed aws_ec2_client_vpn_network_association resource and worked it into the aws_ec2_client_vpn_endpoint resource
• Added security_groups sub-parameter to network_association parameter
• Added authorization_rule parameter

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 (19.03s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_msAD
--- PASS: TestAccAwsEc2ClientVpnEndpoint_msAD (1752.23s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withLogGroup
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withLogGroup (30.81s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withDNSServers
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withDNSServers (27.35s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withNetworkAssociation
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withNetworkAssociation (513.87s)
=== RUN   TestAccAwsEc2ClientVpnEndpoint_withAuthorizationRules
--- PASS: TestAccAwsEc2ClientVpnEndpoint_withAuthorizationRules (496.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2839.502s

[slapula]

Reviews are appreciated but, at the moment, this is a WIP. @bflad @Bwanabanana

After thinking about it and reviewing outstanding issues around the Client VPN resources, I've decided to make authorization rules and network associations as parameters to aws_ec2_client_vpn_endpoint. This makes more sense now that I've dug a little deeper. Authorization rules are tied to the endpoint's associated networks and it didn't make sense to manage network associations outside of the endpoint resource when the authorization rule depend on them. The authorization rules themselves are tied specifically to endpoints meaning that they are not tracked at all by a separate ID just the endpoint ID (that makes them a bad candidate for stand-alone resource imo). All of that led me to this PR.

[Bwanabanana]

Can't comment on the code as I don't have an understanding of the terraform internals but, reading the documentation changes, the approach looks sensible to me. Thanks!

[dimisjim]

@slapula Any chance that we include route creation into this pull request?

[slapula]

@dimisjim Yeah, I actually had a separate resource written for that when I realized these features should be consolidated into the endpoint resource. I'm tied up on other projects at the moment but when I have some free time I'll definitely look into it.

[slapula]

@dimisjim @bflad Added the ability to manage routes within the VPN Endpoint resource:

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

[dimisjim]

@dimisjim @bflad Added the ability to manage routes within the VPN Endpoint resource:

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

@slapula That's awesome! Let's merge!

[bflad]

If a Terraform resource is to be removed, it should follow these steps so it can properly be deprecated so operators are aware of the breaking change and later removed in a major version update: https://www.terraform.io/docs/extend/best-practices/deprecations.html#provider-data-source-or-resource-removal

Decisions like these should be performed in a proposal issue so the community can understand why a breaking change is necessary, have the opportunity to potentially weigh in between various options, and see design sketches of configuration and workflow changes that may eventually wind up documented in an upgrade guide.

[bryanlikes]

What else do we need to make progress on this?

[giuliocalzolari]

AWS recently released the full support of Client VPN with CloudFormation
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-clientvpnroute.html

@bflad @aeschright can we merge this PR in order to have feature-parity with CloudFormation?
Cheers

[giuliocalzolari]

@aeschright any luck?

[drankard]

Any progress here ?

[tommyminds]

Any update on when this will be merged?

[slapula]

@bflad @aeschright I removed the parts that delete the network_authorization resource. This should just be considered a feature addition now. Is there anything else I can do to help move this along?

[aeschright]

Hi @slapula ! Since this is a breaking change, we're unlikely to merge it until the next major release. I don't have the full background on what's in the PR, but it looks like we also need to have an internal discussion about the proposal in #7994 to make sure this is the direction we want to go. Thanks for your patience while we work through the process -- the Terraform AWS team is still growing and we have a lot to tackle!

[borisroman]

Hi @aeschright

Thanks for your comment. How's the internal discussion going? As @slapula mentioned here (#7564 (comment)) it is not a breaking change anymore as the deletion of the network_authorization resource has been excluded from this PR, the PR only contains additions now.

Would you be so kind to reconsider merging this?

Thanks in advance,
Boris

[dcw77]

Also missing from the functionality in aws_ec2_client_vpn_endpoint is the ability to select multiple authentication options to make use of MFA.

Also the route table cannot be updated along with the issue tracked here

[rabidscorpio]

@slapula Any way you can fix the failing tests and the conflicting files so we can get the terraform team to merge this? Cheers!

[Rakhmanov]

Please remove breaking-change label. This has been reworked by author to not have any.

[trebidav]

I would like to see this one merged too! Please!

[jaredtkatz]

Ugh, let's get this PR merged. Are there any alternative methods of authorizing ingresses to Client VPN Endpoints without the proposed approach of parameterizing aws_ec2_client_vpn_endpoint? Do I really have to manually create the ingress authorizations from the AWS console, CLI, etc.?

[connor-tyndall]

+1

[AdamiHamza]

+1

[gdavison]

Hi @slapula. Thanks for the PR and our apologies for the delay on addressing this.

This functionality would be better implemented with separate resources instead of inline in aws_ec2_client_vpn_endpoint. This would allow for greater flexibility for some use cases.

Networking especially can sometimes have different stakeholders managing different parts of the infrastructure, with perhaps a core network team managing the VPN itself, and other teams managing the connections into their infrastructure. These teams could have separate Terraform states, which would require separate resources.

Defining them inline could also limit flexibility in cases where, for example, the number of subnets is not known beforehand.

A case could be made for allowing a simple case where these are defined inline, but also creating the standalone resource for more complex cases. However, this multiples the efforts for maintenance. It can also cause confusion for practitioners, especially if they try to combine the two methods and get unexpected results.

[gdavison]

Hi @slapula. I will finish up this PR. If you'd like to continue, please let me know by Monday, 22 June

[ghost]

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

[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!