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

redevelop the VPC module upgrade to Terraform 0.12 #28

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions locals.tf
Original file line number Diff line number Diff line change
@@ -1,18 +0,0 @@
locals {
route_table_id = var.vpc_id == "" ? concat(alicloud_vpc.vpc.*.route_table_id, [""])[0] : data.alicloud_route_tables.this.ids.0

# Get ID of created Security Group
this_vpc_id = var.vpc_id != "" ? var.vpc_id : concat(alicloud_vpc.vpc.*.id, [""])[0]
# Whether to create other resources in which the vpc
create_sub_resources = var.vpc_id != "" || var.create ? true : false
this_vpc_cidr_block = local.this_vpc_id == "" ? "" : concat(data.alicloud_vpcs.this.vpcs.*.cidr_block, [""])[0]
this_vpc_name = local.this_vpc_id == "" ? "" : concat(data.alicloud_vpcs.this.vpcs.*.vpc_name, [""])[0]
}

data "alicloud_route_tables" "this" {
vpc_id = local.this_vpc_id
}

data "alicloud_vpcs" "this" {
ids = var.vpc_id != "" ? [var.vpc_id] : null
}
121 changes: 98 additions & 23 deletions main.tf
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
# ----------------------------------------------------------------------------------------------------------------------
# REQUIRE A SPECIFIC TERRAFORM VERSION OR HIGHER
# This module has been updated with 0.12.6 syntax, which means it is no longer compatible with any versions below 0.12.6
# ----------------------------------------------------------------------------------------------------------------------
terraform {
required_version = ">= 0.12.6"
}

locals {
vpc_id = var.create_vpc ? alicloud_vpc.this[0].id : var.vpc_id
brianmori marked this conversation as resolved.
Show resolved Hide resolved
nat_gateway_name = var.nat_gateway_name != "" ? var.nat_gateway_name : var.vpc_name
nat_gateway_eip_name = var.nat_gateway_eip_name != "" ? var.nat_gateway_eip_name : "${var.vpc_name}-natgw"
}

provider "alicloud" {
profile = var.profile != "" ? var.profile : null
shared_credentials_file = var.shared_credentials_file != "" ? var.shared_credentials_file : null
Expand All @@ -6,9 +20,8 @@ provider "alicloud" {
configuration_source = "terraform-alicloud-modules/vpc"
}

// If there is not specifying vpc_id, the module will launch a new vpc
resource "alicloud_vpc" "vpc" {
count = var.vpc_id != "" ? 0 : var.create ? 1 : 0
resource "alicloud_vpc" "this" {
count = var.create_vpc ? 1 : 0
name = var.vpc_name
cidr_block = var.vpc_cidr
resource_group_id = var.resource_group_id
Expand All @@ -17,35 +30,97 @@ resource "alicloud_vpc" "vpc" {
{
"Name" = format("%s", var.vpc_name)
},
var.vpc_tags,
var.tags,
var.vpc_tags
)
}

// According to the vswitch cidr blocks to launch several vswitches
resource "alicloud_vswitch" "vswitches" {
count = local.create_sub_resources ? length(var.vswitch_cidrs) : 0
vpc_id = var.vpc_id != "" ? var.vpc_id : concat(alicloud_vpc.vpc.*.id, [""])[0]
cidr_block = var.vswitch_cidrs[count.index]
availability_zone = element(var.availability_zones, count.index)
name = length(var.vswitch_cidrs) > 1 || var.use_num_suffix ? format("%s%03d", var.vswitch_name, count.index + 1) : var.vswitch_name
description = var.vswitch_description

# ---------------------------------------------------------------------------------------------------------------------
# LAUNCH THE NAT GATEWAY
# A NAT Gateway enables instances in the private subnet to connect to the Internet or other Alibaba services, but prevents
# the Internet from initiating a connection to those instances.
# See https://www.alibabacloud.com/product/nat
# ---------------------------------------------------------------------------------------------------------------------

resource "alicloud_nat_gateway" "this" {
count = var.create_nat_gateway ? 1 : 0
vpc_id = local.vpc_id
name = local.nat_gateway_name
specification = var.nat_gateway_specification
}

# create one or more eips for NAT GW
resource "alicloud_eip" "nat" {
name = local.nat_gateway_eip_name
count = var.create_nat_gateway && var.nat_gateway_eip_num > 0 ? var.nat_gateway_eip_num : 0
bandwidth = var.nat_eip_bandwidth
tags = var.tags
}

# attach eips to nat gateway
resource "alicloud_eip_association" "nat" {
count = var.create_nat_gateway && var.nat_gateway_eip_num > 0 ? var.nat_gateway_eip_num : 0
allocation_id = alicloud_eip.nat[count.index].id
instance_id = alicloud_nat_gateway.this[0].id
}

# ---------------------------------------------------------------------------------------------------------------------
# VSwitch
# ---------------------------------------------------------------------------------------------------------------------
resource "alicloud_vswitch" "this" {
for_each = var.vswitches
vpc_id = local.vpc_id
name = each.value.name
availability_zone = each.value.availability_zone
cidr_block = each.value.cidr_block
description = each.value.description

tags = merge(
{
Name = format(
"%s%03d",
var.vswitch_name,
count.index + 1
"%s",
"${each.value.name}"
)
},
var.vswitch_tags,
var.tags,
var.vswitches_tags,
)

}

// According to the destination cidr block to launch a new route entry
resource "alicloud_route_entry" "route_entry" {
count = local.create_sub_resources ? length(var.destination_cidrs) : 0
route_table_id = local.route_table_id
destination_cidrblock = var.destination_cidrs[count.index]
nexthop_type = "Instance"
nexthop_id = var.nexthop_ids[count.index]
# Create a dedicated Route Table for vswitches
# Alibaba has no notion of Internet Gateway, it may be necessary to add custom routes for certain VSwitches
resource "alicloud_route_table" "this" {
count = var.route_table_id == "" && length(var.vswitches) > 0 ? 1 : 0
vpc_id = local.vpc_id
name = var.route_table_name
description = var.route_table_description
tags = var.tags
}

# Associate each vswitch with the route table
resource "alicloud_route_table_attachment" "this" {
for_each = alicloud_vswitch.this
vswitch_id = each.value.id
route_table_id = var.route_table_id != "" ? var.route_table_id : alicloud_route_table.this[0].id
}

resource "alicloud_route_entry" "this" {
for_each = var.custom_routes
route_table_id = var.route_table_id != "" ? var.route_table_id : alicloud_route_table.this[0].id
destination_cidrblock = each.value.destination_cidrblock
nexthop_type = each.value.nexthop_type
nexthop_id = each.value.nexthop_id
}

# ---------------------------------------------------------------------------------------------------------------------
# CREATE CEN Grant
# Grant access to CEN
# ---------------------------------------------------------------------------------------------------------------------
resource "alicloud_cen_instance_grant" "this" {
count = var.cen_enabled ? 1 : 0
cen_id = var.cen_id
cen_owner_id = var.cen_owner_id
child_instance_id = local.vpc_id
}
96 changes: 31 additions & 65 deletions outputs.tf
Original file line number Diff line number Diff line change
@@ -1,89 +1,55 @@
// Output the IDs of the ECS instances created
output "vpc_id" {
description = "Deprecated and use this_vpc_id instead"
value = local.this_vpc_id
}

output "cidr_block" {
description = "Deprecated and use this_vpc_cidr_block instead"
value = concat(alicloud_vpc.vpc.*.cidr_block, [""])[0]
}

output "vswitch_ids" {
description = "Deprecated and use this_vswitch_ids instead"
value = alicloud_vswitch.vswitches.*.id
}

output "availability_zones" {
description = "Deprecated and use this_availability_zones instead"
value = alicloud_vswitch.vswitches.*.availability_zone
}

output "router_id" {
description = "Deprecated and use this_router_id instead"
value = alicloud_route_entry.route_entry.*.router_id
}

output "route_table_id" {
description = "Deprecated and use this_route_table_id instead"
value = alicloud_route_entry.route_entry.*.route_table_id
output "vpc_id" {
description = "The VPC ID"
value = local.vpc_id
}

output "this_vpc_id" {
description = "The VPC id"
value = local.this_vpc_id
output "vpc_cidr_block" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

it is to be consistent with the other outputs of vpc

Copy link
Contributor

Choose a reason for hiding this comment

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

The other outputs of vpc also have a prefix this_

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, I think them should be added.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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:

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" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ,)

description = "The VPC CIDR Block"
value = concat(alicloud_vpc.this.*.cidr_block, [""])[0]
}

output "this_vpc_name" {
output "vpc_name" {
description = "The VPC name"
value = local.this_vpc_name
value = concat(alicloud_vpc.this.*.name, [""])[0]
}

output "this_vpc_cidr_block" {
description = "The VPC cidr block"
value = local.this_vpc_cidr_block
}

output "this_vpc_tags" {
output "vpc_tags" {
description = "The VPC tags"
value = concat(alicloud_vpc.vpc.*.tags, [{}])[0]
value = concat(alicloud_vpc.this.*.tags, [{}])[0]
}

output "this_resource_group_id" {
description = "The Id of resource group which the instance belongs."
value = concat(alicloud_vpc.vpc.*.resource_group_id, [])[0]
output "resource_group_id" {
description = "The Id of resource group which the instance belongs"
value = concat(alicloud_vpc.this.*.resource_group_id, [""])[0]
}

output "this_vswitch_ids" {
description = "List of vswitch ids"
value = alicloud_vswitch.vswitches.*.id
output "nat_gateway_id" {
description = "The NAT Gateway Identifier if created"
value = concat(alicloud_nat_gateway.this.*.id, [""])[0]
}

output "this_vswitch_names" {
description = "List of vswitch names"
value = alicloud_vswitch.vswitches.*.name
output "nat_gateway_eips_ids" {
description = "The EIPs associated to the NAT Gateway"
value = concat(alicloud_eip.nat.*.id, [""])[0]
}

output "this_vswitch_cidr_blocks" {
description = "The vswitch cidr block"
value = alicloud_vswitch.vswitches.*.cidr_block
output "nat_gateway_snat_table_id" {
description = "The SNAT table of the NAT Gateway"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

value = concat(alicloud_nat_gateway.this.*.snat_table_ids, [""])[0]
}

output "this_vswitch_tags" {
description = "List of vswitch tags."
value = alicloud_vswitch.vswitches.*.tags
output "nat_gateway_dnat_table_id" {
description = "The DNAT table of the NAT Gateway"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, will do. Thanks.

value = concat(alicloud_nat_gateway.this.*.forward_table_ids, [""])[0]
}

output "this_availability_zones" {
description = "List of availability zones in which vswitches launched."
value = alicloud_vswitch.vswitches.*.availability_zone
output "vswitches_ids" {
description = "List of vswitch ids"
value = [for value in alicloud_vswitch.this : value.id]
}

output "this_route_table_id" {
description = "The vpc route table id."
value = local.route_table_id
output "vswitches" {
description = "List of vswitches created"
value = [for value in alicloud_vswitch.this : value]
}
output "this_router_id" {
description = "The vpc router id."
value = concat(alicloud_route_entry.route_entry.*.router_id, [""])[0]
}
Loading