Add Authorization Rules for Client VPN #13950

gdavison · 2020-06-25T22:01:15Z

This continues the work done in #7564 to add Authorization Rules for Client VPN. The Authorization Rules are implemented as a standalone resource.

Routes will be addressed by #10508.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #7494
Closes #13959
Closes #13910

Release note for CHANGELOG:

**New Resource:** `aws_ec2_client_vpn_authorization_rule`

Output from acceptance testing:

$ TEST=./aws make testacc TESTARGS='-run=TestAccAwsEc2ClientVpn'

--- PASS: TestAccAwsEc2ClientVpn (4.19s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_disappears (10.99s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_Subnets (47.84s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_groups (62.22s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_basic (33.63s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_disappears (32.72s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_tags (25.65s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_basic (12.06s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_splitTunnel (18.12s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_withLogGroup (17.66s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_withDNSServers (16.74s)
    --- PASS: TestAccAwsEc2ClientVpn/NetworkAssociation_disappears (589.53s)
    --- PASS: TestAccAwsEc2ClientVpn/NetworkAssociation_basic (705.36s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_msAD (1833.75s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_mutualAuthAndMsAD (1845.04s)

…rule

…network associations and authorization rules as parameters

# Conflicts: # aws/resource_aws_ec2_client_vpn_endpoint.go # aws/resource_aws_ec2_client_vpn_endpoint_test.go # website/docs/r/ec2_client_vpn_endpoint.html.markdown

…emoves `network_association` field from `aws_ec2_client_vpn_endpoint` resource

…ic test

gdavison · 2020-06-27T00:45:19Z

Acceptance tests are failing due to provider not waiting for deletion of Client VPN endpoint #13910

…t deletion

…nstants

# Conflicts: # aws/resource_aws_ec2_client_vpn_network_association_test.go

…ases

bflad

Hey @gdavison 👋 Left some initial feedback below since its introducing a few complexities that currently are not present anywhere else in the codebase and we already discussed the semaphore business, but please reach out with any questions. 👍

aws/internal/service/ec2/errors.go

aws/internal/service/ec2/id.go

bflad · 2020-07-06T15:05:22Z

aws/internal/service/ec2/waiter/status.go

+		result, err := conn.DescribeClientVpnEndpoints(&ec2.DescribeClientVpnEndpointsInput{
+			ClientVpnEndpointIds: aws.StringSlice([]string{endpointID}),
+		})
+		if tfec2.ErrCodeEquals(err, tfec2.ErrCodeClientVpnEndpointIdNotFound) {


I think we'll need to globally decide whether errors like these should be handled in these service packages or downstream in the resource implementations so they can decide what to do with them. For example, this swallows available request ID information from the SDK unless debug logging is enabled.

In this case, we need to handle at least some of the errors here, or else the calling WaitForState() will error out instead of returning cleanly

I guess the real difference is that if/when this code is generated, it then needs an additional layer of knowledge of errors rather than just how to handle the API structures. Not a big deal in the short term, but something to consider for longer term.

bflad · 2020-07-06T15:06:47Z

aws/internal/service/ec2/waiter/status.go

@@ -39,3 +40,41 @@ func LocalGatewayRouteTableVpcAssociationState(conn *ec2.EC2, localGatewayRouteT
 		return association, aws.StringValue(association.State), nil
 	}
 }
+
+const (


Should the plan be to always separate const declarations in these files based on some criteria?

I was following the model of a separate const block for each resource type as well as keeping them together with the corresponding Status function

Definitely not something for this PR, but wonder if we should change these to per "resource" files then like status_ClientVpnEndpoint.go -- something to noodle over in #12840!

aws/resource_aws_ec2_client_vpn_authorization_rule_test.go

aws/resource_aws_ec2_client_vpn_endpoint_test.go

aws/resource_aws_ec2_client_vpn_network_association_test.go

website/docs/r/ec2_client_vpn_endpoint.html.markdown

aws/internal/tfawsresource/sync.go

bflad

Let's go! 🚀

Output from acceptance testing:

--- PASS: TestAccAwsEc2ClientVpn (4.23s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_withDNSServers (17.91s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_withLogGroup (19.48s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_tags (26.70s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_basic (33.05s)
    --- PASS: TestAccAwsEc2ClientVpn/NetworkAssociation_disappears (613.82s)
    --- PASS: TestAccAwsEc2ClientVpn/NetworkAssociation_basic (623.53s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_Subnets (48.77s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_disappears (33.33s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_basic (11.02s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_disappears (9.88s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_splitTunnel (17.77s)
    --- PASS: TestAccAwsEc2ClientVpn/AuthorizationRule_groups (62.18s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_mutualAuthAndMsAD (1813.60s)
    --- PASS: TestAccAwsEc2ClientVpn/Endpoint_msAD (1883.59s)

aws/internal/service/ec2/filter.go

aws/internal/service/ec2/waiter/status.go

bflad · 2020-07-07T19:14:24Z

aws/internal/service/ec2/waiter/status.go

@@ -39,3 +40,41 @@ func LocalGatewayRouteTableVpcAssociationState(conn *ec2.EC2, localGatewayRouteT
 		return association, aws.StringValue(association.State), nil
 	}
 }
+
+const (


Definitely not something for this PR, but wonder if we should change these to per "resource" files then like status_ClientVpnEndpoint.go -- something to noodle over in #12840!

aws/internal/service/ec2/finder/finder.go

aws/resource_aws_ec2_client_vpn_authorization_rule.go

bflad · 2020-07-07T19:26:38Z

aws/internal/service/ec2/waiter/status.go

+		result, err := conn.DescribeClientVpnEndpoints(&ec2.DescribeClientVpnEndpointsInput{
+			ClientVpnEndpointIds: aws.StringSlice([]string{endpointID}),
+		})
+		if tfec2.ErrCodeEquals(err, tfec2.ErrCodeClientVpnEndpointIdNotFound) {


I guess the real difference is that if/when this code is generated, it then needs an additional layer of knowledge of errors rather than just how to handle the API structures. Not a big deal in the short term, but something to consider for longer term.

Removes outdated comment Co-authored-by: Brian Flad <[email protected]>

Removes unneeded checks Co-authored-by: Brian Flad <[email protected]>

ghost · 2020-07-10T19:06:40Z

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 · 2020-08-08T17:10: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 and others added 23 commits February 12, 2019 07:39

initial commit

d8912bc

Merge branch 'master' into resource-aws-ec2-client-vpn-authorization-…

bbd7315

…rule

another progess commit

a97816b

r/aws_ec2_client_vpn_endpoint: Reworked endpoint resource to include …

ef5cba6

…network associations and authorization rules as parameters

updating doc with network_association

598d1cb

more acc test tweaks

6638498

more acctest changes

7995ce7

Merge branch 'master' into resource-aws-ec2-client-vpn-consolidation

91e12b9

adding route functionality

d0c9bbf

Merge branch 'master' into resource-aws-ec2-client-vpn-consolidation

20f36e2

readding network assoc resource w/ deprecation message

f7fa8f4

re-adding network association docs too

6cc4c31

renaming net assoc refresh func

5132423

Merge branch 'master' into resource-aws-ec2-client-vpn-consolidation

88362fb

Merge branch 'master' into resource-aws-ec2-client-vpn-consolidation

299594d

# Conflicts: # aws/resource_aws_ec2_client_vpn_endpoint.go # aws/resource_aws_ec2_client_vpn_endpoint_test.go # website/docs/r/ec2_client_vpn_endpoint.html.markdown

Authorization Rules do not require a Network Association

69207fe

Formatting cleanup

baaedf8

Authorization Rules are not required for Routes

d73e537

Adds API resource parameter to Exists function

cac47ca

Removes deprecation of aws_ec2_client_vpn_network_association and r…

63f6b94

…emoves `network_association` field from `aws_ec2_client_vpn_endpoint` resource

Moves Client VPN authorization rule to separate resource and adds bas…

85d5e03

…ic test

Adds waiters and disappears test

0fede62

Handles multiple authorization groups

8300857

gdavison added 2 commits June 25, 2020 15:44

Removes route from aws_ec2_client_vpn_endpoint.test

540f123

Cleanup

9f7c429

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Jun 26, 2020

gdavison added 8 commits June 29, 2020 14:46

Removes unneeded Client VPN network association deletion from endpoin…

706c2d2

…t deletion

Adds waiter for Client VPN to be deleted and removed

ebc3e37

Moves Endpoint delete waiter to waiter package. Renames some error co…

923ed2f

…nstants

Combines Client VPN tests for synchronization

aaf82b7

Fixes test name

9c04ab5

Merge branch 'master' into resource-aws-ec2-client-vpn-consolidation

1b589c4

# Conflicts: # aws/resource_aws_ec2_client_vpn_network_association_test.go

Removes unused function

269186a

Fixes synchronization for network association and consolidates test c…

d85342a

…ases

gdavison force-pushed the resource-aws-ec2-client-vpn-consolidation branch from f0e7904 to d85342a Compare July 1, 2020 00:16

gdavison marked this pull request as ready for review July 1, 2020 06:15

bflad suggested changes Jul 6, 2020

View reviewed changes

gdavison added 5 commits July 6, 2020 17:51

Moves waiter and status functions to waiter package

a76229c

Adds validity check for ID parsing

20569d6

Moves synchronization functions to experimental package

bf963ee

Fixes documentation re-ordering

bb1f70a

Makes acceptance tests more stand-alone

3858262

bflad approved these changes Jul 8, 2020

View reviewed changes

gdavison and others added 2 commits July 8, 2020 09:29

Update aws/internal/service/ec2/waiter/status.go

0a56fc8

Removes outdated comment Co-authored-by: Brian Flad <[email protected]>

Update aws/resource_aws_ec2_client_vpn_authorization_rule.go

4403fca

Removes unneeded checks Co-authored-by: Brian Flad <[email protected]>

gdavison merged commit f2a92cf into master Jul 8, 2020

gdavison deleted the resource-aws-ec2-client-vpn-consolidation branch July 8, 2020 21:44

gdavison added this to the v2.70.0 milestone Jul 8, 2020

gdavison added a commit that referenced this pull request Jul 8, 2020

Update CHANGELOG for #13950

cc375da

connor-tyndall mentioned this pull request Jul 13, 2020

aws_ec2_client_vpn_authorization_rule timeout after 1m0s #14156

Closed

ghost locked and limited conversation to collaborators Aug 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Authorization Rules for Client VPN #13950

Add Authorization Rules for Client VPN #13950

gdavison commented Jun 25, 2020 •

edited

Loading

gdavison commented Jun 27, 2020

bflad left a comment

bflad Jul 6, 2020

gdavison Jul 6, 2020

bflad Jul 7, 2020

bflad Jul 6, 2020

gdavison Jul 6, 2020

bflad Jul 7, 2020

bflad left a comment

bflad Jul 7, 2020

bflad Jul 7, 2020

ghost commented Jul 10, 2020

ghost commented Aug 8, 2020

Add Authorization Rules for Client VPN #13950

Add Authorization Rules for Client VPN #13950

Conversation

gdavison commented Jun 25, 2020 • edited Loading

Community Note

gdavison commented Jun 27, 2020

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 10, 2020

ghost commented Aug 8, 2020

gdavison commented Jun 25, 2020 •

edited

Loading