Extended aws_vpn_gateway use case. #67
Conversation
|
Hi @antonbabenko, Is this something you can look at / comment on? Thanks |
|
Yes, @robh007 , I looked at it already briefly. I will be able to take another look and respond properly in a few hours. Sorry for the delay! |
There was a problem hiding this comment.
Looks really good in my opinion, few comments here and there. Lets hear what @patkar thinks.
variables.tf
Outdated
| @@ -102,14 +102,19 @@ variable "enable_vpn_gateway" { | |||
| default = false | |||
| } | |||
|
|
|||
| variable "attach_vpn_gateway" { | |||
There was a problem hiding this comment.
vpn_gateway_id is a better name for such variable
variables.tf
Outdated
| description = "A list of VGWs the private route table should propagate" | ||
| default = [] | ||
| description = "Should be true if you want route table propagation" | ||
| default = false |
There was a problem hiding this comment.
The variable type is changed from list to boolean. Usually, it makes more sense to keep backward compatibility for as long as possible, but here I don't see how we can make it. If you have a suggestion, please tell.
private_propagating_vgws should be renamed to propagate_private_route_tables_vgw (or smth like that).
public_propagating_vgws should be renamed to propagate_public_route_tables_vgw.
main.tf
Outdated
| @@ -333,3 +331,24 @@ resource "aws_vpn_gateway" "this" { | |||
|
|
|||
| tags = "${merge(var.tags, map("Name", format("%s", var.name)))}" | |||
| } | |||
|
|
|||
| resource "aws_vpn_gateway_attachment" "vgw" { | |||
There was a problem hiding this comment.
Rename vgw to this. VPN gateway has at most just one attachment.
main.tf
Outdated
| @@ -333,3 +331,24 @@ resource "aws_vpn_gateway" "this" { | |||
|
|
|||
| tags = "${merge(var.tags, map("Name", format("%s", var.name)))}" | |||
| } | |||
|
|
|||
| resource "aws_vpn_gateway_attachment" "vgw" { | |||
| count = "${var.attach_vpn_gateway != "default" ? 1 : 0}" | |||
There was a problem hiding this comment.
Please update this PR to support the conditional creation of VPC, which I have introduced earlier today. (count will change in all VPC resources you are adding)
main.tf
Outdated
| } | ||
|
|
||
| resource "aws_vpn_gateway_route_propagation" "private" { | ||
| count = "${var.private_propagating_vgws && var.enable_vpn_gateway || var.private_propagating_vgws && var.attach_vpn_gateway != "default" ? length(var.private_subnets) : 0}" |
There was a problem hiding this comment.
count can be updated like this:
count = "${var.create_vpc && var.propagate_private_route_tables_vgw && (var.enable_vpn_gateway || var.vpn_gateway_id = "") ? length(var.private_subnets) : 0}"
variables.tf
Outdated
| @@ -102,14 +102,19 @@ variable "enable_vpn_gateway" { | |||
| default = false | |||
| } | |||
|
|
|||
| variable "attach_vpn_gateway" { | |||
| description = "ID of VPN Gateway to attach to the VPC" | |||
| default = "default" | |||
There was a problem hiding this comment.
default = "" is better, because it means "it was not specified".
main.tf
Outdated
| } | ||
|
|
||
| resource "aws_vpn_gateway_route_propagation" "public" { | ||
| count = "${var.public_propagating_vgws && var.enable_vpn_gateway || var.public_propagating_vgws && var.attach_vpn_gateway != "default " ? length(var.public_subnets) : 0}" |
There was a problem hiding this comment.
similar here, as for private
|
Hi @antonbabenko, I've made the changes you've suggested. I've also changed the public "aws_vpn_gateway_route_propagation" resource from a count of public subnets to 1. This created addtional resources on the same route table. |
|
v1.23.0 has been released |
|
No problem @antonbabenko happy to help. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This extends using the aws_vpn_gateway_route_propagation resource and closes #9
Previously if you enabled a VPN gateway there was no way to use that gateway within this module. You could also pass in a list of VGWs to associate with a route table type (Public / Private). However this wouldn't have worked either because you need to attach a VGW to a VPC to then allow a route table to turn on route propagation.
Also a VPC can only ever have 1 VGW attached to it. So I'm struggling to understand why a list of VGWs is expected to be passed in when you are only creating 1 VPC.