Skip to content
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.

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

Add VPN options and enable acceleration to aws_vpn_connection resource #14740

Conversation

MRinalducci
Copy link

@MRinalducci MRinalducci commented Aug 19, 2020

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

Relates OR Closes #11584 , #14698 , #11135 and #12237

Release note for CHANGELOG:

Adding support for VPN tunnel options and enable acceleration, DPDTimeoutAction, StartupAction, local/remote ipv4/ipv6 network CIDR and tunnel inside ip version to enable tunnel inside ipv6 CIDR parameters

Usage:

resource "aws_vpc" "vpc" {
  cidr_block = "10.0.0.0/16"
}

resource "aws_vpn_gateway" "vpn_gateway" {
  vpc_id = aws_vpc.vpc.id
}

resource "aws_customer_gateway" "customer_gateway" {
  bgp_asn    = 65000
  ip_address = "172.0.0.1"
  type       = "ipsec.1"
}

resource "aws_vpn_connection" "main" {
  vpn_gateway_id      = aws_vpn_gateway.vpn_gateway.id
  customer_gateway_id = aws_customer_gateway.customer_gateway.id
  type                = "ipsec.1"
  static_routes_only  = true
  //enable_acceleration  = true # Only valid for TGW

  local_ipv4_network_cidr              = "192.168.1.1/32"
  //local_ipv6_network_cidr              = "fd00:2001:db8:2:2d1:81ff:fe41:d201/128"
  remote_ipv4_network_cidr             = "192.168.1.2/32"
  //remote_ipv6_network_cidr             = "fd00:2001:db8:2:2d1:81ff:fe41:d202/128"
  //tunnel_inside_ip_version             = "ipv6" # Only valid for TGW

  //tunnel1_inside_ipv6_cidr             = "fd00:2001:db8:2:2d1:81ff:fe41:d200/126"
  tunnel1_dpd_timeout_action           = "clear"
  tunnel1_dpd_timeout_seconds          = 30
  tunnel1_ike_versions                 = ["ikev2"]
  tunnel1_phase1_dh_group_numbers      = [2, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24]
  tunnel1_phase1_encryption_algorithms = ["AES128", "AES256", "AES128-GCM-16", "AES256-GCM-16"]
  tunnel1_phase1_integrity_algorithms  = ["SHA1", "SHA2-256", "SHA2-384", "SHA2-512"]
  tunnel1_phase1_lifetime_seconds      = 28800
  tunnel1_phase2_dh_group_numbers      = [2, 5, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24]
  tunnel1_phase2_encryption_algorithms = ["AES128", "AES256", "AES128-GCM-16", "AES256-GCM-16"]
  tunnel1_phase2_integrity_algorithms  = ["SHA1", "SHA2-256", "SHA2-384", "SHA2-512"]
  tunnel1_phase2_lifetime_seconds      = 3600
  tunnel1_rekey_fuzz_percentage        = 100
  tunnel1_rekey_margin_time_seconds    = 540
  tunnel1_replay_window_size           = 1024
  tunnel1_startup_action               = "add"

  //tunnel2_inside_ipv6_cidr             = "fd00:2001:db8:2:2d1:81ff:fe41:d204/126"
  tunnel2_dpd_timeout_action           = "clear"
  tunnel2_dpd_timeout_seconds          = 30
  tunnel2_ike_versions                 = ["ikev1", "ikev2"]
  tunnel2_phase1_dh_group_numbers      = [2, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24]
  tunnel2_phase1_encryption_algorithms = ["AES128", "AES256", "AES128-GCM-16", "AES256-GCM-16"]
  tunnel2_phase1_integrity_algorithms  = ["SHA1", "SHA2-256", "SHA2-384", "SHA2-512"]
  tunnel2_phase1_lifetime_seconds      = 28800
  tunnel2_phase2_dh_group_numbers      = [2, 5, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24]
  tunnel2_phase2_encryption_algorithms = ["AES128", "AES256", "AES128-GCM-16", "AES256-GCM-16"]
  tunnel2_phase2_integrity_algorithms  = ["SHA1", "SHA2-256", "SHA2-384", "SHA2-512"]
  tunnel2_phase2_lifetime_seconds      = 3600
  tunnel2_rekey_fuzz_percentage        = 100
  tunnel2_rekey_margin_time_seconds    = 540
  tunnel2_replay_window_size           = 1024
  tunnel2_startup_action               = "add"
}

Output from acceptance testing:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withEnableAcceleration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_withEnableAcceleration -timeout 120m
=== RUN   TestAccAWSVpnConnection_withEnableAcceleration
=== PAUSE TestAccAWSVpnConnection_withEnableAcceleration
=== CONT  TestAccAWSVpnConnection_withEnableAcceleration
--- PASS: TestAccAWSVpnConnection_withEnableAcceleration (902.49s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       903.930s
make testacc TEST=./aws   40.90s user 14.21s system 6% cpu 15:16.47 total

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_tunnelOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_tunnelOptions -timeout 120m
=== RUN   TestAccAWSVpnConnection_tunnelOptions
=== PAUSE TestAccAWSVpnConnection_tunnelOptions
=== CONT  TestAccAWSVpnConnection_tunnelOptions
--- PASS: TestAccAWSVpnConnection_tunnelOptions (251.94s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       253.383s
make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_tunnelOptions'  99.77s user 22.30s system 42% cpu 4:45.43 total

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withIpv6'     
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_withIpv6 -timeout 120m
=== RUN   TestAccAWSVpnConnection_withIpv6
=== PAUSE TestAccAWSVpnConnection_withIpv6
=== CONT  TestAccAWSVpnConnection_withIpv6
--- PASS: TestAccAWSVpnConnection_withIpv6 (565.61s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       567.045s
make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withIpv6'  39.13s user 13.12s system 9% cpu 9:39.19 total

@MRinalducci MRinalducci requested a review from a team August 19, 2020 17:22
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 19, 2020
@dthvt
Copy link
Contributor

dthvt commented Sep 15, 2020

Not an official terraform developer just a public review comment - if you could add some tests as well that would probably be appreciated by the reviewers. Thanks for developing this and submitting it!

@wimnat
Copy link

wimnat commented Nov 6, 2020

This is missing options for:

  1. LocalIpv4NetworkCidr
  2. RemoteIpv4NetworkCidr
  3. LocalIpv6NetworkCidr
  4. RemoteIpv6NetworkCidr

It would be good to add those before merge

@dthvt
Copy link
Contributor

dthvt commented Nov 9, 2020

@wimnat I don't see those options under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-tunnel-options.html. Are you referring to the InsideCIDR values? Those are already supported by terraform (https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpn_connection#tunnel1_inside_cidr). Or are you referring to something else?

@wimnat
Copy link

wimnat commented Nov 10, 2020

@wimnat I don't see those options under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-tunnel-options.html. Are you referring to the InsideCIDR values? Those are already supported by terraform (https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpn_connection#tunnel1_inside_cidr). Or are you referring to something else?

@dthvt They live under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-connection-options.html

@dthvt
Copy link
Contributor

dthvt commented Nov 12, 2020

@dthvt They live under https://docs.aws.amazon.com/cli/latest/reference/ec2/modify-vpn-connection-options.html

@wimnat Ah, I honestly wasn't even aware of those options. Thanks!

@anGie44 anGie44 added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 12, 2020
@MRinalducci MRinalducci requested a review from a team as a code owner November 21, 2020 15:54
@MRinalducci
Copy link
Author

Hi @wimnat and @dthvt,
I added support for:

  • LocalIpv4NetworkCidr
  • RemoteIpv4NetworkCidr
  • LocalIpv6NetworkCidr
  • RemoteIpv6NetworkCidr

And also to close #14698:

  • TunnelInsideIpVersion
  • TunnelInsideIpv6Cidr

@bill-rich
Copy link
Contributor

Hi @MRinalducci! The PR is looking good. I'm working on reviewing it now, but it will also need acceptance tests to be added. Please let me know if you'll have time to add the tests. If not, I can take a look at handling that.

It looks like #12218 has done some of the same work, but since those features are a subset of what you have here, I'd like to move forward here. There is an acceptance test in #12218 you can take a look at to get started.

@bill-rich bill-rich self-assigned this Dec 3, 2020
@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XL Managed by automation to categorize the size of a PR. labels Dec 5, 2020
@MRinalducci
Copy link
Author

MRinalducci commented Dec 5, 2020

Hi @bill-rich! Thanks for reviewing this PR.
I added the acceptance tests as you asked:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withEnableAcceleration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_withEnableAcceleration -timeout 120m
=== RUN   TestAccAWSVpnConnection_withEnableAcceleration
=== PAUSE TestAccAWSVpnConnection_withEnableAcceleration
=== CONT  TestAccAWSVpnConnection_withEnableAcceleration
--- PASS: TestAccAWSVpnConnection_withEnableAcceleration (902.49s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       903.930s
make testacc TEST=./aws   40.90s user 14.21s system 6% cpu 15:16.47 total

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_tunnelOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_tunnelOptions -timeout 120m
=== RUN   TestAccAWSVpnConnection_tunnelOptions
=== PAUSE TestAccAWSVpnConnection_tunnelOptions
=== CONT  TestAccAWSVpnConnection_tunnelOptions
--- PASS: TestAccAWSVpnConnection_tunnelOptions (251.94s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       253.383s
make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_tunnelOptions'  99.77s user 22.30s system 42% cpu 4:45.43 total

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withIpv6'     
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpnConnection_withIpv6 -timeout 120m
=== RUN   TestAccAWSVpnConnection_withIpv6
=== PAUSE TestAccAWSVpnConnection_withIpv6
=== CONT  TestAccAWSVpnConnection_withIpv6
--- PASS: TestAccAWSVpnConnection_withIpv6 (565.61s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       567.045s
make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpnConnection_withIpv6'  39.13s user 13.12s system 9% cpu 9:39.19 total

I think TestAccAWSVpnConnection_tunnelOptions can be improved with pre-checks and result testing.
Won't have much time to work on it before next week-end, if you have some time any help would be appreciated.

Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. I've got a few suggestions, but mostly related to style tweaks. Thank you for taking the time to add all these options!

aws/resource_aws_vpn_connection.go Show resolved Hide resolved
aws/resource_aws_vpn_connection.go Outdated Show resolved Hide resolved
aws/resource_aws_vpn_connection.go Outdated Show resolved Hide resolved
aws/resource_aws_vpn_connection.go Show resolved Hide resolved
aws/resource_aws_vpn_connection.go Outdated Show resolved Hide resolved
aws/resource_aws_vpn_connection_test.go Outdated Show resolved Hide resolved
aws/resource_aws_vpn_connection_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@MRinalducci MRinalducci force-pushed the f-aws_vpn_connection-add_vpn_tunnel_options branch from 0f2e15e to 3fada75 Compare December 12, 2020 14:49
Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few small changes, and I think this is ready to go.

@bill-rich bill-rich merged commit 2266428 into hashicorp:master Dec 16, 2020
@github-actions github-actions bot added this to the v3.22.0 milestone Dec 16, 2020
@MRinalducci
Copy link
Author

I made a few small changes, and I think this is ready to go.

Nice! Thank you 👍

@ghost
Copy link

ghost commented Dec 18, 2020

This has been released in version 3.22.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!

@wayneworkman
Copy link
Contributor

This change has caused some aws_vpn_connection resources to be recreated at my org, leaving the VPN in a down state. This is the change that forces it:
~ tunnel2_inside_cidr = "169.254.48.112/30" -> "169.254.48.116/30" # forces replacement
We've not changed these settings for half a year.

@MRinalducci
Copy link
Author

@wayneworkman it seems to come from commit 4acc6d8 which implements the refresh/read of tunnel attributes.
What is inside CIDR of tunnel 1 value?
Do you have more information/logs?
Please open an issue if you think that it is related to this PR.
Thanks in advance.

@ghost
Copy link

ghost commented Jan 16, 2021

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 ghost locked as resolved and limited conversation to collaborators Jan 16, 2021
@breathingdust breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to manage VPN tunnel options
8 participants