Fix: Use map instead of list for all subnet type #535

achachw · 2020-10-26T10:38:15Z

Description

Defining subnet as list is destructive, when trying to remove one/multiple subnet from that list.

Motivation and Context

Let's say i want to deploy this configuration :

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  azs              = ["us-east-1a", "us-east-1b"]
  private_subnets  = ["10.40.3.0/24", "10.40.4.0/24","10.40.5.0/24", "10.40.6.0/24"]

Once applied, i remove one subnet from that list

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  azs              = ["us-east-1a", "us-east-1b"]
  private_subnets  = ["10.40.3.0/24", "10.40.5.0/24", "10.40.6.0/24"]

If i apply this last configuration all private subnets will be destroyed and 3 new ones will be created

#403
#178

Breaking Changes

The idea is to use map instead of list for subnets definitions.
From

private_subnets  = ["10.40.3.0/24", "10.40.5.0/24", "10.40.6.0/24"]

to

private_subnets = {
    "subnet-1" = {
      cidr = "10.40.3.0/24",
      az   = "eu-west-1a"
    },
    "subnet-2" = {
      cidr = "10.40.5.0/24",
      az   = "eu-west-1b"
    },
    "subnet-3" = {
      cidr = "10.40.6.0/24",
      az   = "eu-west-1c"
    }
  }

How Has This Been Tested?

module "vpc" {
  source = ".."
  name = "automate-vpc"

  cidr = "20.10.0.0/16"
  enable_nat_gateway = true
  one_nat_gateway_per_az = true

  private_subnets = {
    "subnet-1" = {
      cidr = "20.10.1.0/24",
      az   = "eu-west-1a"
    },
    "subnet-2" = {
      cidr = "20.10.2.0/24",
      az   = "eu-west-1a"
    }
  public_subnets = {
        "subnet-3" = {
          cidr = "20.10.11.0/24",
          az   = "eu-west-1a",
          public_subnet_ipv6_prefixes = 2
        }
     }
  }

aperigault

There is many unnecessary new lines in main.tf and README.md

aperigault · 2020-10-27T11:23:42Z

main.tf


  tags = merge(
    {
      "Name" = format(
        "%s-${var.private_subnet_suffix}-%s",
        var.name,
-        element(var.azs, count.index),
-      )
+      each.key, )


You change the Name format. Is it voluntary ?

aperigault · 2020-10-27T13:14:34Z

README.md

-private_subnets     = ["10.0.1.0/24", "10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24", "10.0.5.0/24"]
-redshift_subnets    = ["10.0.41.0/24", "10.0.42.0/24"]
-intra_subnets       = ["10.0.51.0/24", "10.0.52.0/24", "10.0.53.0/24"]
+database__subnets = {


barryib · 2020-11-02T12:56:49Z

README.md

+  private_subnets = {
+    "subnet-1" = {
+      cidr = "10.0.1.0/24",
+      az   = "eu-west-1a"
+    },
+    "subnet-2" = {
+      cidr = "10.0.2.0/24",
+      az   = "eu-west-1a"
+    }


@antonbabenko I didn't test it for now, but Is this structure of the subnets variable ok for you ?

The structure is fine, but I am thinking about an important note on the implementation - aim to have backward compatibility (type list(string) should be converted into map(map(any)). Have type = any on variables.

We can't just ask users to rewrite their code at once even if we release a new major version.

The structure is fine, but I am thinking about an important note on the implementation - aim to have backward compatibility (type list(string) should be converted into map(map(any)). Have type = any on variables.

Are you proposing to support for both list(string) and map(map(string)) inputs ? Or to help users to move as easy as possible to the new the input structure (map(map(string))) ?

We can't just ask users to rewrite their code at once even if we release a new major version.

I think we have to document it. We can't do more without adding an extra tool that we should maintain. We can probably provide something for states manipulation, but I'm not sure we want to go through terraform code generation.

type = any (since type = list(string)|map(map(any)) or similar is not supported). Yes, support both types of values transparently for the users.

Agree. We have to document but first, let's try to make it backward compatible. Code generation or even relying on tools like tfmigrate is not the best option for such modules to my mind.

type = any (since type = list(string)|map(map(any)) or similar is not supported). Yes, support both types of values transparently for the users.

I was thinking about using map(map(string)) and not map(map(any)). But using any is also ok for me.

Agree. We have to document but first, let's try to make it backward compatible. Code generation or even relying on tools like tfmigrate is not the best option for such modules to my mind.

Can you please elaborate your point ? How do you want to handle this backward compatibility without a lot of complexity ? If you have something in mind, please share it.

@barryib

locals { one_map = try(tomap(var.subnets), {for k in var.subnets: k => {}}) } variable "subnets" { type = any default = ["10.0.0.0/20", "10.0.1.0/20"] # default = { # "10.0.0.0/20" = { az = "eu-west-1a", tags = { Name: "first" }} # "10.0.1.0/20" = { az = "eu-west-1b", tags = { Name: "second" }} # } }

var.subnets can be list or map, and Terraform will convert that into a map. See around this time - https://youtu.be/xpfGBgLgpWo?t=1520

Humm.. sounds nice. I'll try that. But this still involve state manipulation right ? It just help users to not modify their code base.

Should we use this for transition only (said for 1 or 2 major release) and we invite them to change their code ?

IMHO, I think that users will be more concerne about state migration and to avoid rebuilding all their infrastructure. The minor code change can be acceptable.

Yes, manual state manipulation will probably still required, unless we can generate keys to match indexes in a list, like this (to use maps but keep old indexes):

one_map = try(tomap(var.subnets), {for k,v in var.subnets: k => { cidr = v }})

I think if we use the type conversion, we can keep using it for a long time (more than 1-2 major releases) to support both cases transparently. But we should also see if it is going to look ugly and hard to maintain, we can drop support for older structure (list).

Yes, manual state manipulation will probably still required, unless we can generate keys to match indexes in a list, like this (to use maps but keep old indexes):

That was the first thing I tried, but I'm afraid that it won't work as expected. As you mention, map keys are strings and you can't change their type (unless you have an idea).

# random_pet.node_groups will be destroyed - resource "random_pet" "node_groups" { - id = "wondrous-honeybee" -> null - keepers = { - "foo" = "10.0.0.0/20" } -> null - length = 2 -> null - separator = "-" -> null } # random_pet.node_groups[1] will be destroyed - resource "random_pet" "node_groups" { - id = "topical-grizzly" -> null - keepers = { - "foo" = "10.0.1.0/20" } -> null - length = 2 -> null - separator = "-" -> null } # random_pet.node_groups["0"] will be created + resource "random_pet" "node_groups" { + id = (known after apply) + keepers = { + "foo" = "10.0.0.0/20" } + length = 2 + separator = "-" } # random_pet.node_groups["1"] will be created + resource "random_pet" "node_groups" { + id = (known after apply) + keepers = { + "foo" = "10.0.1.0/20" } + length = 2 + separator = "-" }

random_pet.node_groups => random_pet.node_groups["0"]
random_pet.node_groups[1] => random_pet.node_groups["1"]

Please find bellow my random_pet snippet:

I run it first with a count

I used the magic to convert it to map and used with for_each

resource "random_pet" "node_groups" { for_each = local.one_map #count = length(var.list_subnets) separator = "-" length = 2 keepers = { foo = each.value.cidr #foo = var.list_subnets[count.index] } }

No surprise here then :) Good that you double-checked it.

barryib · 2020-11-23T14:35:22Z

@antonbabenko how can we move on this ? Is this something you want to add in this module ? Are you planning any general rewrite on the vpc module ? @achachw and I want to help, but need some guidance here. By example, variable map definition will be a good start. Should we experiment terraform-aws-modules/terraform-aws-eks#1101 here ?

antonbabenko · 2020-11-23T20:33:21Z

I don't plan to have any rewrites in this module but if you want to work on this PR and experiment with this feature and migrations it would be awesome. At some point, we will have to move forward and introduce migrations natively or using 3rd party tools.

I recommend working on smaller things so that it is easier to read the code.

hansh0801 · 2021-12-20T02:53:07Z

track

github-actions · 2022-01-26T00:30:24Z

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

github-actions · 2022-02-06T00:36:25Z

This PR was automatically closed because of stale in 10 days

mustafa89 · 2022-02-16T13:25:01Z

Is there any traction on this? we would love to have this feature.

bryantbiggs · 2022-02-16T13:26:43Z

@mustafa89 there is work going on in the background for this but it will take some time. based on prior experience in major changes with modules that are heavily utilized we will have to consider all options

mustafa89 · 2022-02-16T13:35:05Z

Alright, looking forward to it, just a small note

 "subnet-1" = {
      cidr = "10.40.3.0/24",
      az   = "eu-west-1a"
    },
    "subnet-2" = {
      cidr = "10.40.5.0/24",
      az   = "eu-west-1b"
    },
    "subnet-3" = {
      cidr = "10.40.6.0/24",
      az   = "eu-west-1c"
    }

the subnet-1,2,3 feels redundant. I guess a list of maps should also work and simplify it.

private_subnets = [
    {
      cidr = "10.40.3.0/24",
      az   = "eu-west-1a"
    },
    {
      cidr = "10.40.5.0/24",
      az   = "eu-west-1b"
    },
    {
      cidr = "10.40.6.0/24",
      az   = "eu-west-1c"
    },
]

but yeah, just my 2 cents.

github-actions · 2022-10-28T02:43:06Z

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.

Walid ACHACH added 10 commits October 15, 2020 11:03

use map instead of list for subnet

55c378a

better with for_each

467a7b4

typo

3a092a1

fix some typo in route table association

f39fe9f

merge some conflicts

ac19a51

align with my modif

96e5f7d

merge from upstream

ca78fb1

typo in resource

8af700b

change to adapt acl to subnet in map

3e371be

change ipv6 example

764ff7a

aperigault reviewed Oct 27, 2020

View reviewed changes

barryib reviewed Nov 2, 2020

View reviewed changes

antonbabenko mentioned this pull request Jun 29, 2021

Added Resources to support multiple public/private subnets and route tables #379

Closed

antonbabenko mentioned this pull request Aug 10, 2021

feat: Add support for Neptune cluster networking #660

Closed

1 task

antonbabenko mentioned this pull request Sep 16, 2021

Allow specification of names and tags for secondary CIDR blocks. #689

Closed

github-actions bot added the stale label Jan 26, 2022

github-actions bot closed this Feb 6, 2022

github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Use map instead of list for all subnet type #535

Fix: Use map instead of list for all subnet type #535

achachw commented Oct 26, 2020

aperigault left a comment

aperigault Oct 27, 2020

aperigault Oct 27, 2020

barryib Nov 2, 2020

antonbabenko Nov 2, 2020

barryib Nov 3, 2020

antonbabenko Nov 3, 2020

barryib Nov 3, 2020

antonbabenko Nov 13, 2020

barryib Nov 13, 2020

antonbabenko Nov 13, 2020

barryib Nov 13, 2020 •

edited

Loading

antonbabenko Nov 13, 2020

barryib commented Nov 23, 2020 •

edited

Loading

antonbabenko commented Nov 23, 2020

hansh0801 commented Dec 20, 2021

github-actions bot commented Jan 26, 2022

github-actions bot commented Feb 6, 2022

mustafa89 commented Feb 16, 2022

bryantbiggs commented Feb 16, 2022

mustafa89 commented Feb 16, 2022 •

edited

Loading

github-actions bot commented Oct 28, 2022

Fix: Use map instead of list for all subnet type #535

Fix: Use map instead of list for all subnet type #535

Conversation

achachw commented Oct 26, 2020

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

aperigault left a 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

barryib Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barryib commented Nov 23, 2020 • edited Loading

antonbabenko commented Nov 23, 2020

hansh0801 commented Dec 20, 2021

github-actions bot commented Jan 26, 2022

github-actions bot commented Feb 6, 2022

mustafa89 commented Feb 16, 2022

bryantbiggs commented Feb 16, 2022

mustafa89 commented Feb 16, 2022 • edited Loading

github-actions bot commented Oct 28, 2022

barryib Nov 13, 2020 •

edited

Loading

barryib commented Nov 23, 2020 •

edited

Loading

mustafa89 commented Feb 16, 2022 •

edited

Loading