Added tunnel options to vpn_connection #1862

a-teisseire · 2017-10-11T14:33:25Z

Context

AWS recently allowed users to let them choose the CIDR and the PSK to use inside IPsec tunnels.
More details here : https://aws.amazon.com/about-aws/whats-new/2017/10/aws-vpn-update-custom-psk-inside-tunnel-ip-and-sdk-update/

Changes

This Pull Request changes the way tunnel1_preshared_key and tunnel2_preshared_key behave (from read-only to writeable).
The tunnel1_inside_cidr and tunnel2_inside_cidr have been introduced also to be used as parameters for the CreateVpnConnection API.
Test coverage has been added to test these features.

Tests

⇒  TF_ACC=1 go test ./aws -v -run=TestAccAWSVpnConnection_ -timeout 120m 
=== RUN   TestAccAWSVpnConnection_importBasic 
--- PASS: TestAccAWSVpnConnection_importBasic (236.43s) 
=== RUN   TestAccAWSVpnConnection_basic 
--- PASS: TestAccAWSVpnConnection_basic (432.65s) 
=== RUN   TestAccAWSVpnConnection_tunnelOptions 
--- PASS: TestAccAWSVpnConnection_tunnelOptions (286.92s) 
=== RUN   TestAccAWSVpnConnection_withoutStaticRoutes 
--- PASS: TestAccAWSVpnConnection_withoutStaticRoutes (298.02s) 
=== RUN   TestAccAWSVpnConnection_disappears 
--- PASS: TestAccAWSVpnConnection_disappears (166.75s) 
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1420.829s

tyrken · 2017-11-07T13:03:01Z

FYI I compiled this locally and it Worked-For-Me™ when I needed it, thanks @a-teisseire .

bflad · 2017-12-09T14:42:35Z

This PR would close #2220

bflad

This is looking pretty good! I have suggested some additional validation since Amazon is very explicit about the acceptable values for these and some other minor comments.

Can you please ensure the documentation in website/docs/r/vpn_connection.html.markdown is updated as well? Thanks!

bflad · 2017-12-09T20:55:38Z

aws/resource_aws_vpn_connection.go

@@ -94,6 +94,36 @@ func resourceAwsVpnConnection() *schema.Resource {
 				ForceNew: true,
 			},

+			"tunnel1_inside_cidr": {


We can add validation for these new tunnel#_inside_cidr attributes via:

ValidateFunc: validateVpnConnectionTunnelInsideCIDR,

Which is defined something like:

// https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_VpnTunnelOptionsSpecification.html func validateVpnConnectionTunnelInsideCIDR(v interface{}, k string) (ws []string, errors []error) { value := v.(string) _, ipnet, err := net.ParseCIDR(value) if err != nil { errors = append(errors, fmt.Errorf("%q must contain a valid CIDR, got error parsing: %s", k, err)) return } if !strings.HasSuffix(ipnet.String(), "/30") { errors = append(errors, fmt.Errorf("%q must be /30 CIDR", k)) } if !strings.HasPrefix(ipnet.String(), "169.254.") { errors = append(errors, fmt.Errorf("%q must be within 169.254.0.0/16", k)) } else if ipnet.String() == "169.254.0.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.0.0/30", k)) } else if ipnet.String() == "169.254.1.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.1.0/30", k)) } else if ipnet.String() == "169.254.2.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.2.0/30", k)) } else if ipnet.String() == "169.254.3.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.3.0/30", k)) } else if ipnet.String() == "169.254.4.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.4.0/30", k)) } else if ipnet.String() == "169.254.5.0/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.5.0/30", k)) } else if ipnet.String() == "169.254.169.252/30" { errors = append(errors, fmt.Errorf("%q cannot be 169.254.169.252/30", k)) } return }

And tested by making the tunnel1_inside_cidr/tunnel1_preshared_key configurable in testAccAwsVpnConnectionConfigTunnelOptions then calling these test steps:

{ Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "not-a-cidr"), ExpectError: regexp.MustCompile(`must contain a valid CIDR`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.254.0/31"), ExpectError: regexp.MustCompile(`must be /30 CIDR`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "172.16.0.0/30"), ExpectError: regexp.MustCompile(`must be within 169.254.0.0/16`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.0.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.0.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.1.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.1.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.2.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.2.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.3.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.3.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.4.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.4.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.5.0/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.5.0/30`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "12345678", "169.254.169.252/30"), ExpectError: regexp.MustCompile(`cannot be 169.254.169.252/30`), },

bflad · 2017-12-09T20:58:17Z

aws/resource_aws_vpn_connection.go

+				ForceNew: true,
+			},
+
+			"tunnel1_preshared_key": {


We can add validation for these now configurable tunnel#_preshared_key attributes via:

ValidateFunc: validateVpnConnectionTunnelPreSharedKey,

Which is defined something like:

// https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_VpnTunnelOptionsSpecification.html func validateVpnConnectionTunnelPreSharedKey(v interface{}, k string) (ws []string, errors []error) { value := v.(string) if (len(value) < 8) || (len(value) > 64) { errors = append(errors, fmt.Errorf("%q must be between 8 and 64 characters in length", k)) } if strings.HasPrefix(value, "0") { errors = append(errors, fmt.Errorf("%q cannot start with zero character", k)) } if !regexp.MustCompile(`^[0-9a-zA-Z_]+$`).MatchString(value) { errors = append(errors, fmt.Errorf("%q can only contain alphanumeric and underscore characters", k)) } return }

And tested by making the tunnel1_inside_cidr/tunnel1_preshared_key configurable in testAccAwsVpnConnectionConfigTunnelOptions then calling these test steps:

{ Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "1234567", "169.254.254.0/30"), ExpectError: regexp.MustCompile(`must be between 8 and 64 characters in length`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, acctest.RandStringFromCharSet(65, acctest.CharSetAlpha), "169.254.254.0/30"), ExpectError: regexp.MustCompile(`must be between 8 and 64 characters in length`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "01234567", "169.254.254.0/30"), ExpectError: regexp.MustCompile(`cannot start with zero character`), }, { Config: testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn, "1234567!", "169.254.254.0/30"), ExpectError: regexp.MustCompile(`can only contain alphanumeric and underscore characters`), },

bflad · 2017-12-09T21:02:47Z

aws/resource_aws_vpn_connection.go

@@ -245,8 +261,37 @@ func resourceAwsVpnConnection() *schema.Resource {
 func resourceAwsVpnConnectionCreate(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).ec2conn

+	// Get the optional tunnel options
+	tunnel1_cidr := d.Get("tunnel1_inside_cidr").(string)


These can be simplified by using d.GetOk() and placed where you have your other if statements 😄

e.g.

if v, ok := d.GetOk("tunnel1_preshared_key"); ok { options[0].TunnelInsideCidr = aws.String(v.(string)) }

bflad · 2017-12-09T21:08:31Z

aws/resource_aws_vpn_connection_test.go

@@ -325,6 +359,38 @@ func testAccAwsVpnConnectionConfigUpdate(rInt, rBgpAsn int) string {
 	`, rBgpAsn, rInt)
 }

+func testAccAwsVpnConnectionConfigTunnelOptions(rBgpAsn int) string {
+	return fmt.Sprintf(`
+		resource "aws_vpn_gateway" "vpn_gateway" {


Nitpick: I don't think we have a style guide I can point to, but can you please ensure the Terraform configuration inside the testing here is fully to the left with 2 space indentation? We're trying to standardize going forward. Thanks!

a-teisseire · 2017-12-22T12:38:47Z

Hey,

Thanks a lot for this feedback, I'll try to commit an update with these details today or in the upcoming days.

radeksimko · 2018-01-08T10:37:56Z

Hi @a-teisseire
thanks for the PR!

Do you need any help implementing the changes @bflad mentioned?

a-teisseire · 2018-01-10T11:31:41Z

Hello @radeksimko

I have implemented the changes and made the documentation.
@bflad changes were no difficulty to implement. I re ran the tests on my side, everything should be good now !

bflad

This looks great @a-teisseire! Thanks for the work here!

make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSVpnConnection -timeout 120m
=== RUN   TestAccAWSVpnConnection_importBasic
--- PASS: TestAccAWSVpnConnection_importBasic (1259.52s)
=== RUN   TestAccAWSVpnConnectionRoute_basic
--- PASS: TestAccAWSVpnConnectionRoute_basic (565.63s)
=== RUN   TestAccAWSVpnConnection_basic
--- PASS: TestAccAWSVpnConnection_basic (964.83s)
=== RUN   TestAccAWSVpnConnection_tunnelOptions
--- PASS: TestAccAWSVpnConnection_tunnelOptions (375.86s)
=== RUN   TestAccAWSVpnConnection_withoutStaticRoutes
--- PASS: TestAccAWSVpnConnection_withoutStaticRoutes (468.70s)
=== RUN   TestAccAWSVpnConnection_disappears
--- PASS: TestAccAWSVpnConnection_disappears (435.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4069.793s

bflad · 2018-01-29T20:06:15Z

This has been released in terraform-provider-aws version 1.8.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-08T17:28:49Z

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!

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 12, 2017

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

bflad mentioned this pull request Dec 9, 2017

Feature request: aws_vpn_connection add pre shared keys to attributes #2220

Closed

bflad reviewed Dec 9, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 11, 2017

Added tunnel options to vpn_connection

8947b6b

a-teisseire force-pushed the f-tunnel-options-vpn-connection branch from 56403c9 to 88ae76f Compare January 10, 2018 10:19

Added more tests to aws_vpn_connection and documentation

da3bce1

a-teisseire force-pushed the f-tunnel-options-vpn-connection branch from 88ae76f to da3bce1 Compare January 10, 2018 10:24

radeksimko requested a review from bflad January 10, 2018 12:44

radeksimko added this to the v1.8.0 milestone Jan 16, 2018

radeksimko assigned bflad Jan 21, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 23, 2018

bflad approved these changes Jan 23, 2018

View reviewed changes

bflad merged commit 63489c0 into hashicorp:master Jan 23, 2018

bflad added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 23, 2018

bflad added a commit that referenced this pull request Jan 23, 2018

Update CHANGELOG for #1862

25c0818

bflad removed their assignment Jan 24, 2018

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tunnel options to vpn_connection #1862

Added tunnel options to vpn_connection #1862

a-teisseire commented Oct 11, 2017 •

edited by radeksimko

Loading

tyrken commented Nov 7, 2017 •

edited

Loading

bflad commented Dec 9, 2017

bflad left a comment

bflad Dec 9, 2017

bflad Dec 9, 2017

bflad Dec 9, 2017

bflad Dec 9, 2017

a-teisseire commented Dec 22, 2017

radeksimko commented Jan 8, 2018

a-teisseire commented Jan 10, 2018

bflad left a comment

bflad commented Jan 29, 2018

ghost commented Apr 8, 2020

Added tunnel options to vpn_connection #1862

Added tunnel options to vpn_connection #1862

Conversation

a-teisseire commented Oct 11, 2017 • edited by radeksimko Loading

Context

Changes

Tests

tyrken commented Nov 7, 2017 • edited Loading

bflad commented Dec 9, 2017

bflad left a comment

Choose a reason for hiding this comment

bflad Dec 9, 2017

Choose a reason for hiding this comment

bflad Dec 9, 2017

Choose a reason for hiding this comment

bflad Dec 9, 2017

Choose a reason for hiding this comment

bflad Dec 9, 2017

Choose a reason for hiding this comment

a-teisseire commented Dec 22, 2017

radeksimko commented Jan 8, 2018

a-teisseire commented Jan 10, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jan 29, 2018

ghost commented Apr 8, 2020

a-teisseire commented Oct 11, 2017 •

edited by radeksimko

Loading

tyrken commented Nov 7, 2017 •

edited

Loading