Add option to provide a VPN gateway to share gateways with multiple VPN connections #25

paulvandenburg · 2022-10-07T13:06:20Z

what

Add an optional option to provide the Virtual Private Gateway/VPN gateway to be used for the VPN connection
If a VPN gateway is provided, there won't be one created in the model
If a VPN gateway is NOT provided, the old behavior remains of creating one here.

why

In our case we needed to create multiple VPN connections, however we don't need multiple VPN gateways.
Since there is a relatively low default limit on the amount of VPN gateways you can create limitting the amount of VPN gateways we need is useful.

If you want to use this new feature, you need to create the VPN gateway elsewhere in terraform and then pass along the ID.

sample

Sample usage could look like this:

resource "aws_vpn_gateway" "default" {
  vpc_id          = var.vpc_id
  amazon_side_asn = 64512
}

module "my_vpn" {
  count          = 3
  source         = "cloudposse/vpn-connection/aws"
  version        = "0.7.2"
  vpn_gateway_id = aws_vpn_gateway.default.id
  # Other needed VPN settings...
}

Which will create the various VPN connections you need while only using 1 VPN gateway.

…VPN connections

paulvandenburg · 2023-02-08T15:02:01Z

Since it is not looking like this is going to be merged, but we need some additional features. The source branch will contain more than just the initially mentioned change.

If people still want to get this moving, you can mention me and/or look at the 0.7.2 tag on my fork which was from before the additional stuff.

Gowiem

@paulvandenburg these look like legit changes and I'd be happy to help you get them upstreamed -- I've made a bunch of requests, some small, some large. Check them out, let me know if you have any questions and we can work to move this forward. Thanks for the contribution and I hope we can get this merged!

Gowiem · 2024-02-19T21:46:22Z

main.tf

@@ -4,26 +4,30 @@ locals {

 # https://www.terraform.io/docs/providers/aws/r/vpn_gateway.html
 resource "aws_vpn_gateway" "default" {
-  count           = local.enabled ? 1 : 0
+  count           = var.vpn_gateway_id != null ? 0 : (local.enabled ? 1 : 0)


Can we pull this out to a local like so?

locals { vpn_gateway_enabled = local.enabled && var.vpn_gateway_id == null } # usage count = local.vpn_gateway_enabled ? 1 : 0

Gowiem · 2024-02-19T21:47:10Z

main.tf

  vpc_id          = var.vpc_id
  amazon_side_asn = var.vpn_gateway_amazon_side_asn
  tags            = module.this.tags
 }

 # https://www.terraform.io/docs/providers/aws/r/customer_gateway.html
 resource "aws_customer_gateway" "default" {
-  count      = local.enabled ? 1 : 0
+  count      = local.enabled && var.customer_gateway_id == null ? 1 : 0


Same here, let's pull this up into a local.

Gowiem · 2024-02-19T21:50:25Z

main.tf

+  tags = merge(
+    module.this.tags,
+    var.customer_gateway_name_override != null ? { Name = var.customer_gateway_name_override } : {},
+    { ip = var.customer_gateway_ip_address }
+  )


We typically like to avoid supporting name overrides (it happens for sure, but I like to avoid it) because even if you're not following the module.this conventions you can create the name by just passing name. That said, I'd consider this without the ip tag. You can pass that in via the tags variable if you're interested in including that as a specific tag.

Mind cleaning this up a bit and just consuming the module so that your use-case fits or is there a specific need here that I'm missing?

Gowiem · 2024-02-19T21:51:56Z

main.tf

 }

 # https://www.terraform.io/docs/providers/aws/r/vpn_connection.html
 resource "aws_vpn_connection" "default" {
  count                    = local.enabled ? 1 : 0
-  vpn_gateway_id           = join("", aws_vpn_gateway.default.*.id)
-  customer_gateway_id      = join("", aws_customer_gateway.default.*.id)
+  vpn_gateway_id           = var.vpn_gateway_id != null ? var.vpn_gateway_id : join("", aws_vpn_gateway.default.*.id)


I like to see this logic done elsewhere, particularly when it's reused like you do below. Let's pull this to a local like so:

locals { vpn_gateway_id = coalesce(var.vpn_gateway_id, join("", aws_vpn_gateway.default.*.id)) }

Then you can just reference the local and avoid the duplication of logic.

Gowiem · 2024-02-19T21:52:07Z

main.tf

-  vpn_gateway_id           = join("", aws_vpn_gateway.default.*.id)
-  customer_gateway_id      = join("", aws_customer_gateway.default.*.id)
+  vpn_gateway_id           = var.vpn_gateway_id != null ? var.vpn_gateway_id : join("", aws_vpn_gateway.default.*.id)
+  customer_gateway_id      = var.customer_gateway_id == null ? join("", aws_customer_gateway.default.*.id) : var.customer_gateway_id


Same as above 👍

Gowiem · 2024-02-19T21:55:44Z

main.tf

 # https://www.terraform.io/docs/providers/aws/r/vpn_gateway_route_propagation.html
 resource "aws_vpn_gateway_route_propagation" "default" {
  count          = local.enabled ? length(var.route_table_ids) : 0
-  vpn_gateway_id = join("", aws_vpn_gateway.default.*.id)
+  vpn_gateway_id = var.vpn_gateway_id != null ? var.vpn_gateway_id : join("", aws_vpn_gateway.default.*.id)


Use local as mentioned above.

Gowiem · 2024-02-19T21:56:22Z

variables.tf

@@ -3,11 +3,28 @@ variable "vpc_id" {
  description = "The ID of the VPC to which the Virtual Private Gateway will be attached"
 }

+variable "vpn_gateway_id" {
+  description = "VPN gateway to use if you want to share a virtual private gateway with multiple VPN connections"


Suggested change

description = "VPN gateway to use if you want to share a virtual private gateway with multiple VPN connections"

description = "VPN gateway to use if you want to share a virtual private gateway with multiple VPN connections"

type = string

Gowiem · 2024-02-19T21:56:38Z

variables.tf

+  type = bool
+  description = "Enable logging for the VPN tunnel"
+  default = true


Suggested change

default = true

default = false

Gowiem · 2024-02-19T21:56:55Z

variables.tf

+variable "vpn_tunnel_logging_retention" {
+  type = number
+  description = "Logging retention for the VPN tunnel"


Suggested change

description = "Logging retention for the VPN tunnel"

description = "Logging retention for the VPN tunnel in days."

Gowiem · 2024-02-19T21:57:25Z

variables.tf

+}
+
+variable "vpn_tunnel_logging_retention" {


Please be sure to run a terraform fmt locally before committing your changes. Thanks!

Gowiem · 2024-03-04T18:34:02Z

@paulvandenburg see my comments from 2 weeks ago to get this moving along -- Thanks!

mergify · 2024-03-09T04:41:46Z

This pull request now has conflicts. Could you fix it @paulvandenburg? 🙏

mergify · 2024-03-09T04:41:48Z

This PR has been closed due to inactivity and merge conflicts.
Please resolve the conflicts and reopen if necessary.

mergify · 2024-03-09T04:41:49Z

Thanks @paulvandenburg for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

mergify · 2024-03-09T04:41:50Z

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

Add option to provide an VPN gateway to share gateways with multiple …

410c8bb

…VPN connections

paulvandenburg requested review from a team as code owners October 7, 2022 13:06

paulvandenburg requested review from joe-niland and florian0410 October 7, 2022 13:06

Auto Format

9cf282c

cloudpossebot requested a review from a team as a code owner October 7, 2022 13:07

Add option to override customer gateway device name

3709509

cloudpossebot and others added 6 commits February 8, 2023 15:03

Auto Format

9e69648

Add IP tag to customer gateway

df88143

Allow overriding customer gateway by providing an ID

511b036

Auto Format

c2c1e95

Add tunnel logging

cc29073

Fix tunnel log json case

6d3c91a

Gowiem requested changes Feb 19, 2024

View reviewed changes

Gowiem self-requested a review February 19, 2024 21:58

Gowiem self-assigned this Feb 19, 2024

hans-d added the stale This PR has gone stale label Mar 4, 2024

Gowiem added enhancement New feature or request minor New features that do not break anything labels Mar 4, 2024

mergify bot added the conflict This PR has conflicts label Mar 9, 2024

mergify bot closed this Mar 9, 2024

mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Mar 9, 2024

mergify bot removed conflict This PR has conflicts needs-cloudposse Needs Cloud Posse assistance labels Mar 9, 2024

mergify bot removed the stale This PR has gone stale label Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to provide a VPN gateway to share gateways with multiple VPN connections #25

Add option to provide a VPN gateway to share gateways with multiple VPN connections #25

paulvandenburg commented Oct 7, 2022

paulvandenburg commented Feb 8, 2023

Gowiem left a comment

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem Feb 19, 2024

Gowiem commented Mar 4, 2024

mergify bot commented Mar 9, 2024

mergify bot commented Mar 9, 2024

mergify bot commented Mar 9, 2024

Need help or want to ask for a PR review to be expedited?

mergify bot commented Mar 9, 2024

Cloud Posse Engineering Team Review Required

	description = "VPN gateway to use if you want to share a virtual private gateway with multiple VPN connections"
	description = "VPN gateway to use if you want to share a virtual private gateway with multiple VPN connections"
	type = string

	description = "Logging retention for the VPN tunnel"
	description = "Logging retention for the VPN tunnel in days."

Add option to provide a VPN gateway to share gateways with multiple VPN connections #25

Add option to provide a VPN gateway to share gateways with multiple VPN connections #25

Conversation

paulvandenburg commented Oct 7, 2022

what

why

sample

paulvandenburg commented Feb 8, 2023

Gowiem 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

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

Gowiem commented Mar 4, 2024

mergify bot commented Mar 9, 2024

mergify bot commented Mar 9, 2024

mergify bot commented Mar 9, 2024

Need help or want to ask for a PR review to be expedited?

mergify bot commented Mar 9, 2024

Cloud Posse Engineering Team Review Required