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

Add support for Client VPN Route resource #10508

Closed

RickyRajinder wants to merge 1 commit into hashicorp:master from RickyRajinder:resource-ec2-client-vpn-route

Contributor

RickyRajinder commented Oct 15, 2019 •

edited by gdavison

Loading

Adds support for the following resource:
https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/cvpn-working-routes.html

Curently a workaround is implemented to access this API:
#7831 (comment)

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #10437
Closes #7831

Release note for CHANGELOG:

New Resource: aws_ec2_client_vpn_route

Output from acceptance testing:

=== RUN   TestAccAwsEc2ClientVpnRoute_basic
=== PAUSE TestAccAwsEc2ClientVpnRoute_basic
=== CONT  TestAccAwsEc2ClientVpnRoute_basic
--- PASS: TestAccAwsEc2ClientVpnRoute_basic (587.52s)
PASS

=== RUN   TestAccAwsEc2ClientVpnRoute_disappears
=== PAUSE TestAccAwsEc2ClientVpnRoute_disappears
=== CONT  TestAccAwsEc2ClientVpnRoute_disappears
--- PASS: TestAccAwsEc2ClientVpnRoute_disappears (635.60s)
PASS


...

RickyRajinder requested a review from a team

October 15, 2019 07:48

ghost added size/XL provider service/ec2 tests labels

RickyRajinder force-pushed the resource-ec2-client-vpn-route branch 2 times, most recently from 2101378 to f459858 Compare

October 15, 2019 07:58

ghost added the service/apigateway label

RickyRajinder force-pushed the resource-ec2-client-vpn-route branch 2 times, most recently from ef30e87 to cbad222 Compare

October 15, 2019 08:04

RickyRajinder force-pushed the resource-ec2-client-vpn-route branch 2 times, most recently from 2632d4f to b8f0856 Compare

October 25, 2019 23:56

RickyRajinder changed the title ~~[WIP] Add support for Client VPN Route resource~~ Add support for Client VPN Route resource


          Add resource schema and test file

7c964f3

Minor fixes to schema and test config methods

Adds tests and fixes bug in schema

Change destination cidr block value in test config

Change destination cidr block to internet access

fix tab issue

fix tab issue

fix typo in Read

Revert "tests/service/apigateway: Use internal implementation for TLS key/certificate"

This reverts commit da4e688157425156683562803484924addf5e002.

Fix test setup and add missing params

Fix delete issue

Fix gomft issues

RickyRajinder force-pushed the resource-ec2-client-vpn-route branch from b8f0856 to 7c964f3 Compare

October 26, 2019 00:18

Contributor Author

RickyRajinder commented Oct 29, 2019

@bflad Can you please review?

Contributor

gazoakley commented Apr 7, 2020

Looks like this does the same as #10560

gdavison requested changes

View reviewed changes

Contributor

gdavison left a comment

Hi @RickyRajinder, thanks for the PR. It's a great start for this feature. I've indicated a number of changes that can be made.

We'd also like to see some documentation added for the resource.

aws/resource_aws_ec2_client_vpn_route.go

+              			"origin": {
+              				Type:     schema.TypeString,
+              				Computed: true,
+              				ForceNew: true,

Contributor

gdavison Jun 12, 2020

ForceNew does not work with Computed fields

aws/resource_aws_ec2_client_vpn_route.go

+              		return fmt.Errorf("error creating client VPN route: %s", err)
+              	}
+              	d.SetId(resource.UniqueId())

Contributor

gdavison Jun 12, 2020

We should create a "synthetic id" which can be built out of the arguments that will uniquely identify this route. Something like < client_vpn_endpoint_id>_< target_vpc_subnet_id>_< destination_cidr_block>. This is especially important since the resource is importable. The separator can be anything, but it should exclude any symbols that could be in the values, so in this case, avoid -, ., and /.

The ID can then be unpacked to use in the Read function to specify all of the parameters.

We use these in a number of resources, such as aws_route53_record and aws_security_group_rule.

aws/resource_aws_ec2_client_vpn_route.go

+              func resourceAwsEc2ClientVpnRouteRead(d *schema.ResourceData, meta interface{}) error {
+              	conn := meta.(*AWSClient).ec2conn
+              	resp, err := conn.DescribeClientVpnRoutes(&ec2.DescribeClientVpnRoutesInput{

Contributor

gdavison Jun 12, 2020

You can add values for the Filters field, to completely specify the route.

aws/resource_aws_ec2_client_vpn_route.go

Comment on lines +113 to +140

+              	_, exists := d.GetOk("client_vpn_endpoint_id")
+              	if !exists {
+              		d.Set("client_vpn_endpoint_id", resp.Routes[0].ClientVpnEndpointId)
+              	}
+              	_, exists = d.GetOk("destination_cidr_block")
+              	if !exists {
+              		d.Set("destination_cidr_block", resp.Routes[0].DestinationCidr)
+              	}
+              	_, exists = d.GetOk("description")
+              	if !exists {
+              		d.Set("description", resp.Routes[0].Description)
+              	}
+              	_, exists = d.GetOk("target_vpc_subnet_id")
+              	if !exists {
+              		d.Set("target_vpc_subnet_id", resp.Routes[0].TargetSubnet)
+              	}
+              	_, exists = d.GetOk("origin")
+              	if !exists {
+              		d.Set("origin", resp.Routes[0].Origin)
+              	}
+              	_, exists = d.GetOk("status")
+              	if !exists {
+              		d.Set("status", resp.Routes[0].Status)
+              	}
+              	_, exists = d.GetOk("type")
+              	if !exists {
+              		d.Set("type", resp.Routes[0].Type)
+              	}

Contributor

gdavison Jun 12, 2020

This should set all of the values, rather than checking for it being set. This way we ensure that the Terraform state contains the values as seen by AWS, and we can check for drift and update if needed.

aws/resource_aws_ec2_client_vpn_route.go

+              	}
+              	if err != nil {
+              		return fmt.Errorf("error reading client VPN route: %s", err)

Contributor

gdavison Jun 12, 2020

We're updating our error messages to use the Go 1.13 error wrapping verb %w

aws/resource_aws_ec2_client_vpn_route_test.go

+              		},
+              	})
+              }

Contributor

gdavison Jun 12, 2020

Some other tests that would be useful for this resource:

Update the values on a route and ensure that the old one is deleted and the new one is created
Have more than one route associated with the Client VPN to ensure that the Read function is getting the correct route
- Also update one of the routes to make sure that only the correct route is updated
Have a test for adding and then removing a route
- 0 -> 1 -> 0
- 1 -> 2 -> 1

aws/resource_aws_ec2_client_vpn_route_test.go

+              		})
+              		for _, r := range resp.Routes {
+              			if *r.ClientVpnEndpointId == rs.Primary.Attributes["client_vpn_endpoint_id"] && !(*r.Status.Code == ec2.ClientVpnRouteStatusCodeDeleting) {

Contributor

gdavison Jun 12, 2020

This should ensure that the route has been fully deleted, so ec2.ClientVpnRouteStatusCodeDeleting should not be accepted for this check

aws/resource_aws_ec2_client_vpn_route_test.go


		conn := testAccProvider.Meta().(*AWSClient).ec2conn

		resp, err := conn.DescribeClientVpnRoutes(&ec2.DescribeClientVpnRoutesInput{

Contributor

gdavison Jun 12, 2020

This will need to use the three required arguments to fully specify the correct route

aws/resource_aws_ec2_client_vpn_route_test.go

+                target_vpc_subnet_id   = "${aws_ec2_client_vpn_network_association.test.subnet_id}"
+                description            = "test client VPN route"
+              }
+              `, rName, rName, rName)

Contributor

gdavison Jun 12, 2020

Since rName is repeated here, you can use argument indexes, like %[1]s

aws/resource_aws_ec2_client_vpn_route_test.go

+              }
+              func testAccEc2ClientVpnRouteConfig(rName string) string {
+              	return testAccEc2ClientVpnRouteConfigAcmCertificateBase() + fmt.Sprintf(`

Contributor

gdavison Jun 12, 2020

We've added a function composeConfig() that can be used to merge together configurations. E.g.

return composeConfig(
    testAccEc2ClientVpnRouteConfigAcmCertificateBase(),
    fmt.Sprint(`...`, rName))

As you add more tests, you can make a base configuration snippet that contains the common pieces, like the VPC and subnets

gdavison self-assigned this

gdavison added new-resource waiting-response labels

gdavison mentioned this pull request

New resource - aws_ec2_client_vpn_route #12774

Closed

This was referenced Jun 25, 2020

Add Authorization Rules for Client VPN #13950

Merged

Add synchronization for Client VPN tests #13959

Closed

anGie44 force-pushed the master branch from 6c6566f to 081bf57 Compare

June 29, 2020 16:14

gdavison mentioned this pull request

Resource ec2 client vpn route #14103

Merged

gdavison closed this in #14103

ghost commented Aug 9, 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

breathingdust removed the waiting-response label

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

Labels

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