r/aws_ec2_client_vpn_endpoint_network_association: Adding option to apply additional security groups to target network #7500

slapula · 2019-02-11T01:39:56Z

Changes proposed in this pull request:

Making vpc_id and security_groups actual parameters so we can use those values to add additional security groups to a target network.

Output from acceptance testing:

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

…pply additional security groups to target network

bflad

Hey @slapula 👋Thanks for submitting this. Left some initial review items below. Please reach out if you have any questions or do not have time to implement the feedback.

aws/resource_aws_ec2_client_vpn_network_association.go

website/docs/r/ec2_client_vpn_network_association.html.markdown

slapula · 2019-02-12T15:08:52Z

@bflad Thanks again for your feedback. I believe I've addressed all of your comments.

aws/resource_aws_ec2_client_vpn_network_association.go

bflad · 2019-02-12T15:53:20Z

aws/resource_aws_ec2_client_vpn_network_association_test.go

+					testAccCheckAwsEc2ClientVpnNetworkAssociationExists("aws_ec2_client_vpn_network_association.test", &assoc1),
+					resource.TestCheckResourceAttr("aws_ec2_client_vpn_network_association.test", "security_groups.#", "2"),
+				),
+			},


We should include another TestStep here that performs some form of update 👍

One thing I'm noticing with the Update function is that ApplySecurityGroupsToClientVpnTargetNetwork call doesn't zero out security_groups it just replaces ones that are already there. Going from 2 security groups to zero means that the call ignores the empty request and leaves the security groups untouched. Weird, but not unmanageable.

Sorry that was what I was trying to say in #7500 (comment)

Ok, I think I see what you are saying. See my latest commit. I was able to get the behavior I'm looking for by making security_groups a required parameter (to account for the API adding the default VPC security group in the absence of custom groups). Not ideal but it works.

Scratch that, Travis doesn't like the Required + Computed part.

bflad · 2019-02-13T13:59:04Z

website/docs/r/ec2_client_vpn_network_association.html.markdown

-  subnet_id = "${aws_subnet.example.id}"
+  client_vpn_endpoint_id  = "${aws_ec2_client_vpn_endpoint.example.id}"
+  subnet_id               = "${aws_subnet.example.id}"
+  security_groups         = [""]


This should be omitted in this case.

bflad · 2019-02-13T14:04:06Z

website/docs/r/ec2_client_vpn_network_association.html.markdown

@@ -26,12 +37,13 @@ The following arguments are supported:

 * `client_vpn_endpoint_id` - (Required) The ID of the Client VPN endpoint.
 * `subnet_id` - (Required) The ID of the subnet to associate with the Client VPN endpoint.
+* `security_groups` - (Required) A list of up to five custom security groups to apply to the target network. Not populating this parameter will result in the subnet inheriting the default VPC security group.  Regardless of what you choose,  you must define this parameter due to the AWS API automatically assigning the default VPC security group if no other groups are included in the call.


This should be updated back to Optional.

Suggested change

* `security_groups` - (Required) A list of up to five custom security groups to apply to the target network. Not populating this parameter will result in the subnet inheriting the default VPC security group. Regardless of what you choose, you must define this parameter due to the AWS API automatically assigning the default VPC security group if no other groups are included in the call.

* `security_groups` - (Optional) A list of up to five custom VPC security groups to apply to the target network. Not populating this parameter will result in the subnet inheriting the default VPC security group. To remove all custom VPC security groups, this list must be configured to contain only the default VPC security group.

slapula · 2019-02-14T19:35:33Z

@bflad I did a lot of thinking about how all these parts work together around the VPN endpoint and I think it makes sense to consolidate network associations into the endpoint resource (See #7564). I have pulled in the changes made here into that PR. Take a look and let me know what you think. If that's the preferred route, then we can probably close this out.

bryanlikes · 2019-05-29T21:34:16Z

Is this PR being abandoned in favor of #7564? It would be great to get this functionality in since client vpn still requires manual configuration of ingress authorization, securitygroups, routes.

slapula · 2019-05-30T15:45:18Z

@bryanlikes That is my intention, but I've been waiting on more movement with my other PR before closing out this one.

connor-tyndall · 2020-05-19T14:28:24Z

+1

bflad · 2020-08-20T16:55:35Z

These commits were merged as part of #14146 and will release with version 3.3.0, later today. 👍

ghost · 2020-08-20T23:42:36Z

This has been released in version 3.3.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-09-19T17:10:32Z

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 added 2 commits February 10, 2019 19:26

r/aws_ec2_client_vpn_endpoint_network_association: Adding option to a…

8d6ef30

…pply additional security groups to target network

spaces/tabs

a0580d2

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Feb 12, 2019

bflad reviewed Feb 12, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2019

changes based on review feedback

a0bb484

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2019

bflad reviewed Feb 12, 2019

View reviewed changes

aws/resource_aws_ec2_client_vpn_network_association.go Outdated Show resolved Hide resolved

bflad reviewed Feb 12, 2019

View reviewed changes

more changes based on review feedback

40ff75a

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 13, 2019

clearing travis failure

4fe42d6

bflad reviewed Feb 13, 2019

View reviewed changes

gdavison self-assigned this Jun 25, 2020

gdavison mentioned this pull request Jul 10, 2020

Resource aws client vpn net addl sgs #14146

Merged

gdavison added this to the v3.0.0 milestone Jul 10, 2020

slapula requested a review from a team July 21, 2020 18:26

bflad modified the milestones: v3.0.0, v3.1.0 Jul 30, 2020

bflad removed this from the v3.1.0 milestone Aug 7, 2020

bflad added this to the v3.2.0 milestone Aug 7, 2020

gdavison modified the milestones: v3.2.0, v3.3.0 Aug 14, 2020

bflad closed this in #14146 Aug 20, 2020

ghost locked and limited conversation to collaborators Sep 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_ec2_client_vpn_endpoint_network_association: Adding option to apply additional security groups to target network #7500

r/aws_ec2_client_vpn_endpoint_network_association: Adding option to apply additional security groups to target network #7500

slapula commented Feb 11, 2019

bflad left a comment

slapula commented Feb 12, 2019

bflad Feb 12, 2019

slapula Feb 12, 2019

bflad Feb 12, 2019

slapula Feb 13, 2019

slapula Feb 13, 2019

bflad Feb 13, 2019

bflad Feb 13, 2019

slapula commented Feb 14, 2019

bryanlikes commented May 29, 2019

slapula commented May 30, 2019

connor-tyndall commented May 19, 2020

bflad commented Aug 20, 2020

ghost commented Aug 20, 2020

ghost commented Sep 19, 2020

	* `security_groups` - (Required) A list of up to five custom security groups to apply to the target network. Not populating this parameter will result in the subnet inheriting the default VPC security group. Regardless of what you choose, you must define this parameter due to the AWS API automatically assigning the default VPC security group if no other groups are included in the call.
	* `security_groups` - (Optional) A list of up to five custom VPC security groups to apply to the target network. Not populating this parameter will result in the subnet inheriting the default VPC security group. To remove all custom VPC security groups, this list must be configured to contain only the default VPC security group.

r/aws_ec2_client_vpn_endpoint_network_association: Adding option to apply additional security groups to target network #7500

r/aws_ec2_client_vpn_endpoint_network_association: Adding option to apply additional security groups to target network #7500

Conversation

slapula commented Feb 11, 2019

bflad left a comment

Choose a reason for hiding this comment

slapula commented Feb 12, 2019

bflad Feb 12, 2019

Choose a reason for hiding this comment

slapula Feb 12, 2019

Choose a reason for hiding this comment

bflad Feb 12, 2019

Choose a reason for hiding this comment

slapula Feb 13, 2019

Choose a reason for hiding this comment

slapula Feb 13, 2019

Choose a reason for hiding this comment

bflad Feb 13, 2019

Choose a reason for hiding this comment

bflad Feb 13, 2019

Choose a reason for hiding this comment

slapula commented Feb 14, 2019

bryanlikes commented May 29, 2019

slapula commented May 30, 2019

connor-tyndall commented May 19, 2020

bflad commented Aug 20, 2020

ghost commented Aug 20, 2020

ghost commented Sep 19, 2020