redevelop the VPC module upgrade to Terraform 0.12 #28

brianmori · 2020-08-13T15:07:18Z

I redeveloped the VPC module to include different resources (NAT GW, CEN, ...) and the use of Terraform 0.12 constructs.

Local tests are fine and we will deploy the module on the final infrastructure in the coming days.
Please let me know your thoughts

P.S.: I need to readapt some part of the documentation

…orm 0.12 constructs

jeanpierremilan · 2020-08-14T09:19:28Z

+1

supertom25 · 2020-08-14T09:20:34Z

+1

supertom25 · 2020-08-14T09:21:24Z

👍

xiaozhu36 · 2020-08-19T12:59:16Z

@brianmori I am sorry for late reply. Thanks for your contribution. I think this PR should be submitted into https://github.com/terraform-alicloud-modules/terraform-alicloud-network-with-nat. Please check it.

brianmori · 2020-08-19T13:50:11Z

@brianmori I am sorry for late reply. Thanks for your contribution. I think this PR should be submitted into https://github.com/terraform-alicloud-modules/terraform-alicloud-network-with-nat. Please check it.

Hi @xiaozhu36

the repository https://github.com/terraform-alicloud-modules/terraform-alicloud-network-with-nat actually calls different Alicloud modules. It is not an implementation like the PR I did.

The goal of my PR is to have this repository/module to provision the Alicloud Network with different resources and different topologies.

VPC (optional in the context of Shared VPC, this module can be used to create only the VSwitches)
VSwitch (optional in the context of Shared VPC, this module can be used to create only the VPC)
Route Table
CEN Grant (optional)
NAT Gateway (optional)

Alicloud supports "Standalone/Standard" VPC and "Shared VPC".

The Alicloud NAT Gateway is optional as companies may prefer a different solution to allow egress traffic to the internet.
I thought about the Alicloud NAT Gateway SNATs and I think I should remove it from this module because it should be managed externally with this module: https://github.com/terraform-alicloud-modules/terraform-alicloud-snat
It has dependencies with the VPC and VSwitches.

brianmori · 2020-08-20T08:15:12Z

I removed the SNAT from this module as it would require to cover multiple use cases and not only the NAT Gateway. What do you think of my latest commit?

xiaozhu36 · 2020-08-21T11:59:19Z

main.tf

+
+resource "alicloud_nat_gateway" "this" {
+  count         = var.create_nat_gateway ? 1 : 0
+  vpc_id        = alicloud_vpc.this[0].id


Does this PR have a test?
I think there is a potential issue: if create_vpc = false, the alicloud_vpc.this will be null and there will throw an error.
In addition, this resource does not support the var.vpc_id.

The NAT Gateway would be provisioned for this VPC as it is a 'VPC resource'. It is not going to be created for a different VPC, henceforth the only VPC ID will be the one of the VPC created in this module.
I have not found a test pipeline for this module, I only did local tests

I did the modification to support the var.vpc_id although the aim of this module is to provision a complete alibaba network

xiaozhu36 · 2020-08-21T12:00:32Z

outputs.tf

-output "this_vpc_id" {
-  description = "The VPC id"
-  value       = local.this_vpc_id
+output "vpc_cidr_block" {


I think this_vpc_id is ok and why change it to vpc_id?

it is to be consistent with the other outputs of vpc

The other outputs of vpc also have a prefix this_

If not, I think them should be added.

Please improve the outputs and set a prefix this_ for them.

I am a bit lost on the advantage to use "this". When I saw other Terraform modules in GitHub, the maintainer omits the name of the resource, in this case "this" because it is unique.

You can take as example these other alibaba modules:

https://github.com/terraform-alicloud-modules/terraform-alicloud-kubernetes/blob/master/outputs.tf

I propose I add the "this" and I keep also the other ones.

As for the description, the "The VPC id" is the actual description of today's module. How do you suggest to improve ?
"The VPC id created by the Terraform module" ?

I think "The VPC id created by the Terraform module." is ok. But, I think there is no need to keep both. Other modules' outputs also need to be improved. The module https://github.com/terraform-alicloud-modules/terraform-alicloud-kubernetes/blob/master/outputs.tf has not improved yet. But most modules have had this prefix for each output.

I have not worked on the alicloud kubernetes, no PRs from my side at the moment. I looked at other official terraform modules and "this" or other identifiers are not used when there is only one type of resource.
I think we should follow this current best practice as it is also easier to read

In any case, I removed the duplicates ,)

variables.tf

main.tf

xiaozhu36 · 2020-08-22T02:27:35Z

main.tf

+resource "alicloud_nat_gateway" "this" {
+  count         = var.create_nat_gateway ? 1 : 0
+  vpc_id        = local.vpc_id
+  name          = var.vpc_name


why use vpc_name to name nat gateway?

As there is one NAT Gateway per VPC, it could make sense to name it the same.
I am going to add a custom value decided by the user and as default value I propose to put the vpc_name. Does it work for you?

xiaozhu36 · 2020-08-22T02:27:43Z

main.tf

+
+# create one or more eips for NAT GW
+resource "alicloud_eip" "nat" {
+  name      = var.vpc_name


still applies

The goal of this module is to bootstrap the network, it is not meant to provision single resources. In any case, I add the option to customize the name

main.tf

xiaozhu36 · 2020-08-22T02:31:30Z

outputs.tf

-output "this_vpc_id" {
-  description = "The VPC id"
-  value       = local.this_vpc_id
+output "vpc_cidr_block" {


The other outputs of vpc also have a prefix this_

xiaozhu36 · 2020-08-22T02:32:19Z

outputs.tf

-output "this_vpc_id" {
-  description = "The VPC id"
-  value       = local.this_vpc_id
+output "vpc_cidr_block" {


If not, I think them should be added.

outputs.tf

xiaozhu36 · 2020-08-22T02:33:34Z

outputs.tf

-  description = "List of vswitch tags."
-  value       = alicloud_vswitch.vswitches.*.tags
+output "nat_gateway_dnat_table_ids" {
+  description = "The DNAT table of the NAT Gateway"


One nat gateway only has one dnat table. Please use nat_gateway_dnat_table_id instead.

I think it should be corrected also in the attributes of the resource because it returns forward_table_ids

https://registry.terraform.io/providers/aliyun/alicloud/latest/docs/resources/nat_gateway#attributes-reference

Ok, will do. Thanks.

xiaozhu36 · 2020-08-22T02:33:51Z

outputs.tf

-  description = "The vswitch cidr block"
-  value       = alicloud_vswitch.vswitches.*.cidr_block
+output "nat_gateway_snat_table_ids" {
+  description = "The SNAT table of the NAT Gateway"


One nat gateway only has one snat table. Please usenat_gateway_snat_table_idinstead.

I think it should be corrected also in the attributes of the resource because it returns snat_table_ids

https://registry.terraform.io/providers/aliyun/alicloud/latest/docs/resources/nat_gateway#attributes-reference

outputs.tf

xiaozhu36 · 2020-08-24T09:55:03Z

outputs.tf

-output "this_vpc_id" {
-  description = "The VPC id"
-  value       = local.this_vpc_id
+output "vpc_cidr_block" {


Please improve the outputs and set a prefix this_ for them.

xiaozhu36 · 2020-08-24T09:55:32Z

outputs.tf

-  description = "List of vswitch tags."
-  value       = alicloud_vswitch.vswitches.*.tags
+output "nat_gateway_dnat_table_ids" {
+  description = "The DNAT table of the NAT Gateway"


Ok, will do. Thanks.

xiaozhu36 · 2020-08-24T11:10:18Z

outputs.tf

-output "this_vpc_id" {
-  description = "The VPC id"
-  value       = local.this_vpc_id
+output "vpc_cidr_block" {


I think "The VPC id created by the Terraform module." is ok. But, I think there is no need to keep both. Other modules' outputs also need to be improved. The module https://github.com/terraform-alicloud-modules/terraform-alicloud-kubernetes/blob/master/outputs.tf has not improved yet. But most modules have had this prefix for each output.

redevelop the VPC module to include NAT GW, CEN and upgrade to Terraf…

3098fd5

…orm 0.12 constructs

brianmori added 3 commits August 20, 2020 09:53

Removed the SNAT, to be moved to a separate module. Added more outputs

d1b034d

Fixed outputs

a792e1e

Terraform format

48bfdd6

brianmori added 2 commits August 20, 2020 10:37

Added NAT Gateway EIPs identifiers

72d1865

renamed the resources with this according to naming convention

d699b48

xiaozhu36 requested changes Aug 21, 2020

View reviewed changes

brianmori added 3 commits August 21, 2020 14:20

Added route table description and name

3e9b3b3

add missing variables description and types

038e33f

support for vpc_id for nat gateway

82b7cdf

brianmori requested a review from xiaozhu36 August 21, 2020 13:00

support for vpc_id for cen instance grant

ac16eb3

xiaozhu36 requested changes Aug 22, 2020

View reviewed changes

added custom route table and minor improvements

68c62cb

brianmori requested a review from xiaozhu36 August 24, 2020 08:55

xiaozhu36 requested changes Aug 24, 2020

View reviewed changes

brianmori added 2 commits August 24, 2020 12:33

added this to outputs

d031e88

added output from vpc resource

482681b

brianmori requested a review from xiaozhu36 August 24, 2020 10:38

brianmori added 2 commits August 28, 2020 17:58

added SNAT for NAT GW and remove duplicated outputs

677e174

added examples for standalone and shared

336c598

xiaozhu36 requested changes Sep 28, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redevelop the VPC module upgrade to Terraform 0.12 #28

redevelop the VPC module upgrade to Terraform 0.12 #28

brianmori commented Aug 13, 2020

jeanpierremilan commented Aug 14, 2020

supertom25 commented Aug 14, 2020

supertom25 commented Aug 14, 2020

xiaozhu36 commented Aug 19, 2020 •

edited

Loading

brianmori commented Aug 19, 2020

brianmori commented Aug 20, 2020

xiaozhu36 Aug 21, 2020

brianmori Aug 21, 2020

xiaozhu36 Aug 21, 2020

brianmori Aug 21, 2020

xiaozhu36 Aug 22, 2020

xiaozhu36 Aug 22, 2020

xiaozhu36 Aug 24, 2020

brianmori Aug 24, 2020

xiaozhu36 Aug 24, 2020

brianmori Sep 29, 2020

xiaozhu36 Aug 22, 2020

brianmori Aug 22, 2020

xiaozhu36 Aug 22, 2020

brianmori Aug 24, 2020

xiaozhu36 Aug 22, 2020

xiaozhu36 Aug 22, 2020

xiaozhu36 Aug 22, 2020

brianmori Aug 22, 2020

xiaozhu36 Aug 24, 2020

xiaozhu36 Aug 22, 2020

brianmori Aug 22, 2020

xiaozhu36 Aug 24, 2020

xiaozhu36 Aug 24, 2020

xiaozhu36 Aug 24, 2020

redevelop the VPC module upgrade to Terraform 0.12 #28

Are you sure you want to change the base?

redevelop the VPC module upgrade to Terraform 0.12 #28

Conversation

brianmori commented Aug 13, 2020

jeanpierremilan commented Aug 14, 2020

supertom25 commented Aug 14, 2020

supertom25 commented Aug 14, 2020

xiaozhu36 commented Aug 19, 2020 • edited Loading

brianmori commented Aug 19, 2020

brianmori commented Aug 20, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaozhu36 commented Aug 19, 2020 •

edited

Loading