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

feat: Add custom subnet names #816

Merged
merged 4 commits into from
Oct 21, 2022
Merged

feat: Add custom subnet names #816

merged 4 commits into from
Oct 21, 2022

Conversation

andrewtcymmer
Copy link
Contributor

@andrewtcymmer andrewtcymmer commented Jul 18, 2022

Description

This PR adds the option to override the values used in resource tags with key "Name" (Name tags) with a list of strings, on public and private subnets. By default (if the optional variables are not specified, or if they are empty arrays), the module will generate the same Name tag values it did prior to this change.

Please reference issue #817 requesting this feature.

Motivation and Context

For VPCs which support multiple applications in multiple subnets, allowing the module to generate Name tags on subnets may cause multiple subnets to have the same value for their Name tag. Example VPC which generates such a condition, using two groups of private subnets:

module "vpc" {
  source = "terraform-aws-modules/vpc/aws"

  name = "problem-vpc"
  cidr = "10.0.0.0/16"

  azs = ["eu-west-1a", "eu-west-1b"]
  private_subnets = [
    # For an application named "Alpha"
    "10.0.1.0/24", "10.0.2.0/24",
    # For an application named "Beta"
    "10.0.3.0/24", "10.0.4.0/24"
  ]
  public_subnets  = ["10.0.101.0/24", "10.0.102.0/24"]
}

# Problem: Private subnets 10.0.1.0 and 10.0.3.0 both have the same value for Name, which can get confusing.
# 10.0.1.0 => "Name = problem-vpc-eu-west-1a"
# 10.0.3.0 => "Name = problem-vpc-eu-west-1a"
# 10.0.2.0 => "Name = problem-vpc-eu-west-1b"
# 10.0.4.0 => "Name = problem-vpc-eu-west-1b"
# Trying to add private_subnet_suffix does not help, because the same suffix is used for all names. Changing this would not be backward compatible.

It may be useful to specify the names of the subnets from the consumer side of the module, rather than let the module generate it. Proposed solution:

module "vpc" {
  source = "terraform-aws-modules/vpc/aws"

  name = "solution-vpc"
  cidr = "10.0.0.0/16"

  azs = ["eu-west-1a", "eu-west-1b"]
  private_subnets = [
    # For an application named "Alpha"
    "10.0.1.0/24", "10.0.2.0/24",
    # For an application named "Beta"
    "10.0.3.0/24", "10.0.4.0/24"
  ]
  private_subnet_names = [
    "Alpha Primary", "Alpha Backup",
    "Beta Primary", "Beta Backup"
  ]

  public_subnets  = ["10.0.101.0/24", "10.0.102.0/24"]
}

# Solution: Subnets 10.0.1.0 and 10.0.3.0 now have different names.
# 10.0.1.0 => "Name = Alpha Primary"
# 10.0.3.0 => "Name = Beta Primary"
# 10.0.2.0 => "Name = Alpha Backup"
# 10.0.4.0 => "Name = Beta Backup"

This solution also accommodates a case where a consumer would want to generate their own naming convention. As an example, consider the example below which adds in the availability zone to the names.

locals {
  private_app_names = ["Alpha private", "Beta private"]
  public_app_names = ["Alpha public", "Beta public"]
  azs  = ["eu-west-1a", "eu-west-1b"]

  private_subnet_names = split(";", trim(join("", flatten([for k, v in local.private_app_names : [for a, b in local.azs : "${v} ${b};"]])), ";"))
  public_subnet_names  = split(";", trim(join("", flatten([for k, v in local.public_app_names : [for a, b in local.azs : "${v} ${b};"]])), ";"))
}

module "vpc" {
  source = "terraform-aws-modules/vpc/aws"

  name = "solution-vpc"
  cidr = "10.0.0.0/16"

  azs = local.azs
  private_subnets = [
    # For an application named "Alpha"
    "10.0.1.0/24", "10.0.2.0/24",
    # For an application named "Beta"
    "10.0.3.0/24", "10.0.4.0/24"
  ]
  private_subnet_names = local.private_subnet_names

  public_subnets  = ["10.0.101.0/24", "10.0.102.0/24"]
  public_subnet_names = local.public_subnet_names
}

# Solution: Subnets 10.0.1.0 and 10.0.3.0 now have different names.
# 10.0.1.0 => "Name = Alpha private eu-west-1a"
# 10.0.3.0 => "Name = Beta private eu-west-1a"
# 10.0.2.0 => "Name = Alpha private eu-west-1b"
# 10.0.4.0 => "Name = Beta private eu-west-1b"

Breaking Changes

This change should be backwards compatible with the current major version, since the change adds an optional (opt-in) variable.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects I have run this change with a Terraform project in my organization. That project has the characteristics explained in the Motivation and Context section above (the example is the same concept, but reduced to simplest terms and scrubbed of company-specific information). PR includes updates to the examples/* which are exactly the same as I have tested on my project.
  • I have executed pre-commit run -a on my pull request

@andrewtcymmer andrewtcymmer changed the title Subnet name override feat: override generated subnet names Jul 18, 2022
@andrewtcymmer andrewtcymmer changed the title feat: override generated subnet names Improvement: override generated subnet names Jul 18, 2022
@andrewtcymmer andrewtcymmer changed the title Improvement: override generated subnet names Feat: override generated subnet names Jul 18, 2022
main.tf Outdated
@@ -389,6 +393,10 @@ resource "aws_subnet" "private" {
ipv6_cidr_block = var.enable_ipv6 && length(var.private_subnet_ipv6_prefixes) > 0 ? cidrsubnet(aws_vpc.this[0].ipv6_cidr_block, 8, var.private_subnet_ipv6_prefixes[count.index]) : null

tags = merge(
length(var.private_subnet_names) > 0 ?
{
"Name" = element(var.private_subnet_names, count.index)
Copy link

Choose a reason for hiding this comment

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

In your approach would make sense to keep the az value appended? e.g.

Suggested change
"Name" = element(var.private_subnet_names, count.index)
"Name" = format(
"${element(var.private_subnet_names, count.index)}-%s",
element(var.azs, count.index),

Copy link
Contributor Author

@andrewtcymmer andrewtcymmer Sep 13, 2022

Choose a reason for hiding this comment

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

@v-rosa Thank you for the suggestion. I tested this out and it worked well. I rebased my branch onto latest master, and while I did so I also re-wrote my original commit to include your suggested change.

Copy link

Choose a reason for hiding this comment

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

I'm sorry, I don't get it. Why would you append AZ when you want to control subnet names manually? Is this the whole point of this PR - to be able to set custom names to subnets?

Copy link

Choose a reason for hiding this comment

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

If you define your network with multiple AZs, e.g. 3, you would get per custom name, 3 subnets with the same custom name. Also the naming wouldn't be consistent with the other subnets created using default naming.

Copy link

@rooty0 rooty0 Sep 17, 2022

Choose a reason for hiding this comment

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

@v-rosa I think default naming and custom naming are two separate things, you usually want to pick just one approach. If you want to include AZ to your custom naming you just do something like this

module "vpc" {
  source  = "terraform-aws-modules/vpc/aws"
  version = "3.14.4"

  name = "development"
  azs            = ["us-west-2a", "us-west-2b"]
  public_subnets = ["10.16.0.0/21", "10.16.8.0/21"]
...
  public_subnet_names = ["mysuper-custom-name-us-west-2a", "another-name-us-west-2b"]
}

In my case I don't want to include AZ to my custom subnet schema, I should be able to do that since it's custom :)

@NIXKnight
Copy link

Anyone still working on this PR?

@andrewtcymmer andrewtcymmer changed the title Feat: override generated subnet names feat: override generated subnet names Sep 13, 2022
@andrewtcymmer andrewtcymmer changed the title feat: override generated subnet names feat: add custom subnet names Sep 13, 2022
@andrewtcymmer andrewtcymmer changed the title feat: add custom subnet names feat: Add custom subnet names Sep 13, 2022
@andrewtcymmer
Copy link
Contributor Author

@NIXKnight Yes, I am still interested in bringing this PR out of draft status and getting it accepted. I plan to finish a minor detail and publish it today.

@andrewtcymmer andrewtcymmer marked this pull request as ready for review September 13, 2022 16:27
@rooty0
Copy link

rooty0 commented Sep 17, 2022

I was about to make my PR to be able to provide custom tags per a subnet, tho just found your PR, and it actually looks fantastic! I like the tag name auto-generation compatibility part. I think forcefully inserting the availability zone at the end of a custom name is redundant. Why would you do it if you want to manually control the names of your subnets?

@andrewtcymmer
Copy link
Contributor Author

@rooty0 Thanks for the support on this PR. Your question about forcing the availability zone has me thinking again, because my first pass at this did not include the AZ in the name for simplicity. Your question also made me realize that, as implemented at the current moment in this PR, this feature should have also considered the private_subnet_suffix and public_subnet_suffix in its convention to be complete. (An example name would be mysuper-custom-name-public-us-west-2a). With that addition, this feature would be getting closer to changing the way names are generated, rather than being custom. Which would not be a bad thing IMO, but it would be redundant and confusing to have two competing naming strategies in the same module. Changing the default naming strategy would be breaking change as well, which I want to avoid. So now I have arrived back at what you are suggesting, which is that a custom name which can be anything is actually a much simpler approach which can include anything it wants, and in any order.

I think it would be good for me to change this as you suggest, but also include an example Terraform snippet that could generate a naming convention based on a map(object) type, if desired. Let me try this out later today, and I will update the PR.

Adds optional inputs that specify strings to assign to the tags.Name property
of private and public subnets. Includes example usage in complete-vpc
example.
@andrewtcymmer
Copy link
Contributor Author

andrewtcymmer commented Sep 20, 2022

I updated the PR description to reflect the change (which I rewrote over the old commit to try and keep the review neat and readable). The snippet I came up with to generate names is a bit messy. Any ways to neaten it up would be welcome. Put this into a new file in its own directory and run terraform plan to try it out.

locals {
  app_names = ["Alpha private", "Beta private"]
  azs  = ["eu-west-1a", "eu-west-1b"]
}

output "example" {
  value = split(";", trim(join("", flatten([for k, v in local.app_names : [for a, b in local.azs : "${v} ${b};"]])), ";"))
}

Screen Shot 2022-09-20 at 10 54 54 AM

@dfradejas
Copy link

Hello there!

When is going to be merged this PR? I am really interested!

Thanks.

@tcharewicz
Copy link
Contributor

I added example PR with code how external subnets can be created in VPC.

#834

@rooty0
Copy link

rooty0 commented Oct 1, 2022

@antonbabenko any chance you can review/merge this please?

@lordz-md
Copy link

This would be also usefull when you have private subnets and want to have separate EKS subnets.

@tcharewicz
Copy link
Contributor

This would be also usefull when you have private subnets and want to have separate EKS subnets.

Yes I create my code to create additional subnets, when I need to create dedicated EKS subnets.

@antonbabenko antonbabenko merged commit 4416e37 into terraform-aws-modules:master Oct 21, 2022
@antonbabenko
Copy link
Member

@lordz-md Here we go!

Thank you, @andrewtcymmer , for the PR!

antonbabenko pushed a commit that referenced this pull request Oct 21, 2022
## [3.17.0](v3.16.1...v3.17.0) (2022-10-21)

### Features

* Add custom subnet names ([#816](#816)) ([4416e37](4416e37))
@antonbabenko
Copy link
Member

This PR is included in version 3.17.0 🎉

@andrewtcymmer andrewtcymmer deleted the subnet-name-override branch October 25, 2022 18:10
spr-mweber3 pushed a commit to spring-media/terraform-aws-vpc that referenced this pull request Oct 28, 2022
Co-authored-by: Anton Babenko <anton@antonbabenko.com>
spr-mweber3 pushed a commit to spring-media/terraform-aws-vpc that referenced this pull request Oct 28, 2022
@lordz-md
Copy link

This would be also usefull when you have private subnets and want to have separate EKS subnets.

Yes I create my code to create additional subnets, when I need to create dedicated EKS subnets.

I recently opened a PR to have separated EKS subnets also.

@balq60
Copy link

balq60 commented Nov 16, 2022

This feature is not working.

Here is my network.tf file.

`provider "aws" {
region = local.region
}

locals {
name = "ex-${replace(basename(path.cwd), "_", "-")}"
region = "us-east-1"

tags = {
Name = local.name
Environment = "dev"
Customer = "Blue Origin"
}
}

################################################################################

VPC Module -

################################################################################
module "vpc" {
source = "terraform-aws-modules/vpc/aws"
version = "3.18.1"

name = "blue-vpn-vpc"
cidr = "10.0.0.0/16"

azs = ["${local.region}a", "${local.region}b", "${local.region}c"]
private_subnets = ["10.0.1.0/24", "10.0.2.0/24", "10.0.3.0/24"]
public_subnets = []
database_subnets = []
elasticache_subnets = []
redshift_subnets = []
intra_subnets = []

private_subnet_names = ["blue-subnet-us-gov-west-1a", "blue-subnet-us-gov-west-1b", "blue-subnet-us-gov-west-1c"]
public_subnet_names = []
database_subnet_names = []
elasticache_subnet_names = []
redshift_subnet_names = []
intra_subnet_names = []

VPC Flow Logs (Cloudwatch log group and IAM role will be created)

enable_flow_log = true
create_flow_log_cloudwatch_log_group = true
create_flow_log_cloudwatch_iam_role = true
flow_log_max_aggregation_interval = 60

create_vpc = true

create_database_internet_gateway_route = false
create_database_nat_gateway_route = false
create_database_subnet_group = false
create_database_subnet_route_table = false
create_egress_only_igw = true
create_elasticache_subnet_group = false
create_elasticache_subnet_route_table = false

create_igw = true

create_redshift_subnet_group = false
create_redshift_subnet_route_table = false

manage_default_network_acl = true
default_network_acl_tags = { Name = "${local.name}-default" }

manage_default_route_table = true
default_route_table_tags = { Name = "${local.name}-default" }

manage_default_security_group = true
default_security_group_tags = { Name = "${local.name}-default" }

enable_dns_hostnames = true
enable_dns_support = true

enable_nat_gateway = false
single_nat_gateway = false

customer_gateways = {

IP1 = {

bgp_asn = 65112

ip_address = "1.2.3.4"

device_name = "some_name"

},

IP2 = {

bgp_asn = 65112

ip_address = "5.6.7.8"

}

}

enable_vpn_gateway = false

enable_dhcp_options = true
dhcp_options_domain_name = "service.consul"
dhcp_options_domain_name_servers = ["127.0.0.1", "10.10.0.2"]

tags = local.tags
}
`

This results in subnets all created with the name name from locals.name

You can see the local.name repeated for the Name of the Subnets.

Name | Subnet ID | State | VPC | IPv4 CIDR
ex-IaC | subnet-097d4221401bc0671 | Available | vpc-00abe999801f08aae | ex-IaC | 10.0.1.0/24
ex-IaC | subnet-05d94c33d42d1ae4c | Available | vpc-00abe999801f08aae | ex-IaC | 10.0.2.0/24
ex-IaC | subnet-00ea9d53c896eb254 | Available | vpc-00abe999801f08aae | ex-IaC | 10.0.3.0/24

@dduzgun-security
Copy link

Hey, this didn't work on my side too.
Subnet names are not changed.

module "vpc" {
  source  = "terraform-aws-modules/vpc/aws"
  version = "3.18.1"
  name    = "${var.prefix}-vpc-default"
  cidr    = var.cidr

  azs             = var.azs
  private_subnets = local.private_subnets
  public_subnets  = local.public_subnets

  private_subnet_names = local.private_subnet_names
  public_subnet_names  = local.public_subnet_names
....

@rooty0
Copy link

rooty0 commented Nov 23, 2022

@balq60 @dduzgun-security
I just want to confirm it works like a charm for me.
I'd recommend opening an Issue with detailed information rather than posting to this PR.

@balq60
Copy link

balq60 commented Nov 23, 2022 via email

@bryantbiggs
Copy link
Member

The problem for me was no support for Terraform files.

So if your file type is not supported it won’t work. Even if they are in
the root folder

What?!

@balq60
Copy link

balq60 commented Nov 24, 2022 via email

@cooka
Copy link

cooka commented Dec 14, 2022

@balq60 @dduzgun-security @rooty0 @andrewtcymmer
it seems,
if tags_all has 'Name' tag, the custom xxx_subnet_names would not work.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.