aws-ia / terraform-aws-vpc

AWS VPC Module
https://registry.terraform.io/modules/aws-ia/vpc/aws/latest
Apache License 2.0
82 stars 89 forks source link

Configure additional routes for the subnets #128

Open findmyname666 opened 11 months ago

findmyname666 commented 11 months ago

Adding new optional attribute routes into variable subnets. This is useful in environments where core network, transit gateways, etc. are created out of operator control.

Notes to the implementation: We are limited here by Terraform because for_each works only with set or map. As we have multiple subnets for a subnet / route table we cannot use it. For that reason the code creates list of maps where each map represents a route for the specific route table (attribute route_table_name). In such case we can use count to iterate over it.

We are limited here by Terraform because for_each works only with a set or map. As we have multiple routes for a subnet / route table, we cannot use it. For that reason, the code creates a list of maps where each map represents a route for the specific route table (attribute route_table_name). In such case, we can use count to iterate over it.

drewmullen commented 11 months ago

Overall this looks great to me. Question, what is the potential overlap / desired interaction if a user uses the predefined route configurations such as connect_to_public_natgw = true? Are they mutually exclusive or can they both be used at the same time?

If they are mutually exclusive I might suggest writing a validation rule to prevent 😬 I'm aware that might be complicated but its worth considering

Bravo btw. I had talked with the network SAs about this kind of feature for a while and knew it would be a bear to implement

findmyname666 commented 11 months ago

Overall this looks great to me. Question, what is the potential overlap / desired interaction if a user uses the predefined route configurations such as connect_to_public_natgw = true? Are they mutually exclusive or can they both be used at the same time?

I was thinking about it, and in general, they are not mutually exclusive. However, there can be a race condition if, for example, the operator configures a custom route for 0.0.0.0/0 and sets connect_to_public_natgw to true at the same time.

drewmullen commented 11 months ago

Few ideas:

There is no formal method for depreciating vars afaik so best you can do is update the description and leave that for 3m then rip off the band-aid and release a new major version with a doc

findmyname666 commented 11 months ago
  • mutually exclusive

I think this would be "bad" from an user perspective in a case when NAT GW is created by this module, route 0.0.0.0/0 should be added into a subnet. It is much better to use connect_to_public_natgw because NAT GW ID isn't available before the creation.

  • remove the bools in favor of routes (breaking)

The same as above.

  • deprecate the bool arguments and Inject logic to generate those routes if the bool is set There is no formal method for depreciating vars afaik so best you can do is update the description and leave that for 3m then rip off the band-aid and release a new major version with a doc

This is possible and shouldn't be difficult to implement. However the code would trigger the route recreation which can be unpleasant in the production environment.

I see 1 more option. To add validation rule to ensure that there isn't route 0.0.0.0/0 in routes and connect_to_public_natgw set to true at the same time.

drewmullen commented 11 months ago

I guess i considered the validation rule to mean the inputs are mutually exclusive. so i think we agree on implementation strategies! :D

nice validation too! LGTM