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

More flexibility for subnets #892

Closed
Roxyrob opened this issue Feb 6, 2023 · 10 comments
Closed

More flexibility for subnets #892

Roxyrob opened this issue Feb 6, 2023 · 10 comments

Comments

@Roxyrob
Copy link

Roxyrob commented Feb 6, 2023

Is your request related to a problem? Please describe.

There is a possibility this module will take into account to move subnets from list + global variables to maps ?
Using subnets as lists of cidrs and then other lists for subnets properties (like name) and other global as map_public_ip_on_launch make more hard to change infrastructure once created.

  • If you want to remove unused subnets in the middle of the list you'll need to do a refactory to manage/avoid destruction/recreation.
  • If you want to make a public subnet private or vice versa how this can be done ?
  • If you want to have a one public subnet with map_public_ip_on_launch=true and one with map_public_ip_on_launch=false ?

Describe the solution you'd like.

Using Maps instead of lists like this for subnets and route tables, e.g.:

locals = {
  subnets       = {
    "subnet1az1" = {
      "rt_associations"                = ["rtable1", "rtable3"]
      "subnet_availability_zone_id"    = "...-az1"          # allow physical AZ id  (it wins)
      "availability_zone_name"         = null               # allow logical AZ name (if physical not defined)
      "subnet_cidr"                    = var.subnet1_az1
      "public"                         = true
      "map_public_ip_on_launch"        = true
    }
    "subnet2az1" = {
      "rt_associations"                = ["rtable2", "rtable3"]
      "subnet_availability_zone_id"    = "...-az1"          # allow physical AZ id  (it wins)
      "availability_zone_name"         = null               # allow logical AZ name (if physical not defined)
      "subnet_cidr"                    = var.subnet2_az1
      "map_public_ip_on_launch"        = false

    }
    ...  
  }
}

  route_tables   = {
    "rtable1" = var.vpc_create ? { tags = {"..." = "...", ...} : { tags = {} }
    "rtable2" = var.vpc_create ? { tags = {"..." = "...", ...} : { tags = {} }
    "rtable3" = var.vpc_create ? { tags = {"..." = "...", ...} : { tags = {} }
    ...
  }
@github-actions
Copy link

github-actions bot commented Mar 9, 2023

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

@github-actions github-actions bot added the stale label Mar 9, 2023
@adamretter
Copy link

It would also be incredibly helpful if you could explicitly configure which Availability Zone a Subnet is placed, as opposed to the current behaviour whereby the private_subnets are just assigned round-robin across the azs configured for the VPC.

@adamretter
Copy link

I believe this could be used for this also - #900

@github-actions github-actions bot removed the stale label Mar 11, 2023
@ForbiddenEra
Copy link

I believe this could be used for this also - #900

And maybe #864 depending how it's done w/regard to IPv6

@ForbiddenEra
Copy link

^ I referenced in a few other issues where 'more flexibility' could potentially be a solution.

It seems like some people are using some of the other subnet types, but not for their actual use, just to 'group' subnets for other uses together. Myself, I was using the _database subnets for my EKS nodes at one time.

Now, those pre-defined types/use-cases can definitely be useful but perhaps in a format similar to what was expressed by the OP, one could specify subnet_type = 'database' as an example to bring in the additional definitions for that type of subnet.

In that same vein, it'd be nice to group certain types of subnets together in groups. Again, I was using _database subnets for my EKS nodes mainly to keep them separate from the _private subnets, even though they were essentially private subnets/behaving the same, however, my private subnets all have the label "Private" in them and are used as a private subnet. While I'm aware I can name the subnets individually, it was still nice to keep them in separate groups.

Also, associated resources can't seemingly be named. I tried switching my _database EKS subnets into _private, giving each one an appropriate label, however the associated security groups and network acls can't be named in the same way to match, at least when using _database for it I could set a suffix and more easily be able to tell what-is-what in the AWS Console etc.

I was considering adding at least an _eks subnet group and maybe a _gwlb one as well to a fork and maybe making a PR, however, I also started feeling like this was getting to be a bit much for pre-defined subnet types with private, database, elasticache, redshift, intra, etc already existing.

It also would definitely be great to be able to adjust the assignment of AZs for subnets; in most cases, I don't mind how it works now, however it's not super flexible.

Also, especially for IPv6, there can be issues; one example is with Wavelength zones - these can only have IPv4 subnets; you can still have a dualstack vpc no problem but the module will try to associate an IPv6 subnet to coincide with the previously definied IPv4 subnet for WLZ, thus causing the AWS API to error out and preventing VPC creation if using dualstack and WLZ together; one can use the subnet resource separately to create it after the VPC has been created but I try to always find a solution with the module before using a resource directly if I can.

If implemented, this would obviously be a semi-large and likely breaking change unless it's added as a new subnet type or the implementation is intentionally made backwards-compatible.

Thus, I think it's prudent that anyone who wants more flexibility or has felt limited by the existing subnet configuration implementation should speak up and add their thoughts.

@davepattie
Copy link

It would also be incredibly helpful if you could explicitly configure which Availability Zone a Subnet is placed, as opposed to the current behaviour whereby the private_subnets are just assigned round-robin across the azs configured for the VPC.

@adamretter & @ForbiddenEra I suspect that since AWS intentionally scrambles the order of az_id mapping to az_name that this won't make it into a release but you could do something like this outside the module if you really need to ensure that your subnets are created in a specific az due to known capacity etc but it does somewhat undermine the aim of scattering folks into different azs instead of everyone choosing 1,2,3 in that order

data "aws_availability_zones" "available" {}
locals {
azs = ["az1","az2","az3"]
azid_azname = zipmap([for az_id in data.aws_availability_zones.available.zone_ids : element(split("-", az_id), 1)], data.aws_availability_zones.available.names)
aznames = [for azid in local.azs : lookup(local.azid_azname, azid)]
}
module "vpc" {
...
azs = local.aznames
...
}

@ForbiddenEra
Copy link

It would also be incredibly helpful if you could explicitly configure which Availability Zone a Subnet is placed, as opposed to the current behaviour whereby the private_subnets are just assigned round-robin across the azs configured for the VPC.

@adamretter & @ForbiddenEra I suspect that since AWS intentionally scrambles the order of az_id mapping to az_name that this won't make it into a release but you could do something like this outside the module if you really need to ensure that your subnets are created in a specific az due to known capacity etc but it does somewhat undermine the aim of scattering folks into different azs instead of everyone choosing 1,2,3 in that order

data "aws_availability_zones" "available" {} locals { azs = ["az1","az2","az3"] azid_azname = zipmap([for az_id in data.aws_availability_zones.available.zone_ids : element(split("-", az_id), 1)], data.aws_availability_zones.available.names) aznames = [for azid in local.azs : lookup(local.azid_azname, azid)] } module "vpc" { ... azs = local.aznames ... }

The current round-robin implementation works fine for me in most cases and likely in most cases in general.

Where it falls apart is if you're also using something like a Wavelength zone or if you wanted extra subnets in a particular AZ; I don't particularly see the usefulness of this myself in most cases as generally you want everything duplicated in each AZ - other than the WLZ, the only case I personally have for this is a tiny subnet tied to az1 linked to a certain node with a meme IP but this is still do-able w/the current implementation (it's just added as the last subnet in the list).

That said, additional subnet flexibility should still be a thing!

@github-actions
Copy link

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

@github-actions github-actions bot added the stale label May 17, 2023
@github-actions
Copy link

This issue was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2023
@github-actions
Copy link

I'm going to lock this issue 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 similar to this, 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 Jun 27, 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

No branches or pull requests

4 participants