feat: sort subnets to calculate by their netmask to efficiently use ip space

[bbeesley]

The previous implementation calculates the ipv4 subnet cidr ranges in an arbitrary order (actually alphabetically based on the subnet type string). This means that if you have different netmasks for different subnets you end up trying to take your vpc cidr range, then cut out some small netmasks, then encounter a large netmask, at which point you have to skip a bunch of ips in order to get to the next start address for that larger netmask. In practice this causes really inefficient use of ip space. For example, with a vpc netmask of 22, 3 db subnets with a netmask of 27, 3 public subnets with a netmask of 28, 3 private subnets with a netmask of 24, and 3 transit gateway subnets with a netmask of 28, then the module is unable to calculate the cidr ranges, because first it creates the db subnets with their 27 netmask, then the public with their 28 netmask, then it gets to the private ones and has to skip a huge chunk to get to the start of a /24 block. At this point it has skipped so many that its unable to create the remaining subnets. The fix for this is to first calculate the subnets where netmask is 24, then the ones where it is 27, then the ones where it is 28. If you do this, then the subnet calculator is then able to create all the required subnets, as its no longer skipping large chunks due to starting with small netmasks.

Example:

locals {
  azs                            = slice(data.aws_availability_zones.current.names, 0, 3)
  vpc_netmask                    = 22
  private_subnet_netmask         = 24
  db_subnet_netmask              = 27
  public_subnet_netmask          = 28
  transit_gateway_subnet_netmask = 28
  subnets = {
    private = {
      netmask = local.private_subnet_netmask
    }
    database = {
      netmask = local.db_subnet_netmask
    }
    public = {
      netmask = local.public_subnet_netmask
    }
    transit_gateway = {
      netmask = local.transit_gateway_subnet_netmask
    }
  }
}

data "aws_availability_zones" "current" {
  filter {
    name   = "opt-in-status"
    values = ["opt-in-not-required"]
  }
}

module "calculate_subnets" {
  source  = "aws-ia/vpc/aws//modules/calculate_subnets"
  version = ">= 4.2.0"

  cidr = "10.108.4.0/${local.vpc_netmask}"
  azs  = local.azs

  subnets = local.subnets
}

On the existing version, this throws an error:

│ Error: Invalid function argument
│ 
│   on .terraform/modules/calculate_subnets.subnet_calculator/main.tf line 6, in locals:
│    6:   addrs_by_idx  = cidrsubnets(var.base_cidr_block, local.networks_netmask_to_bits[*].new_bits...)
│     ├────────────────
│     │ while calling cidrsubnets(prefix, newbits...)
│ 
│ Invalid value for "newbits" parameter: not enough remaining address space for a subnet with a prefix
│ of 28 bits after 10.108.7.0/24.

On this version with sorting, it outputs the following:

  {
    subnets_by_type = {
      database = {
        eu-central-1a = "10.108.7.0/27"
        eu-central-1b = "10.108.7.32/27"
        eu-central-1c = "10.108.7.64/27"
      }
      private = {
        eu-central-1a = "10.108.4.0/24"
        eu-central-1b = "10.108.5.0/24"
        eu-central-1c = "10.108.6.0/24"
      }
      public = {
        eu-central-1a = "10.108.7.144/28"
        eu-central-1b = "10.108.7.160/28"
        eu-central-1c = "10.108.7.176/28"
      }
      transit_gateway = {
        eu-central-1a = "10.108.7.96/28"
        eu-central-1b = "10.108.7.112/28"
        eu-central-1c = "10.108.7.128/28"
      }
    }
    subnets_with_ipv6_native = []
  }

[drewmullen]

Thank you for opening this feature request & pr!. You're bringing up a good issue. This would be considered a breaking change because users who have already deployed will see their subnets drift if we implement. I'm not opposed to that at all but will need to discuss internally and we'd need to prepare a upgrade document.

It might be something we sit on for a short period and include testing for when we switch to terraform test. Just trying to set expectations

Again thank you very much! this looks awesome

[bbeesley]

This would be considered a breaking change because users who have already deployed will see their subnets drift if we implement. I'm not opposed to that at all but will need to discuss internally and we'd need to prepare a upgrade document.

Yeah, I thought that would be the case, and tried to mark it as such on the commit message, but somehow forgot to note that on the PR. It does cause terraform to destroy and recreate the subnets in an existing VPC if they're different sizes.

We're using a forked version of this for now to unblock ourselves, but would like to be able to move back if/when this gets merged.

[ivan-aws]

can this be behind a feature flag for a while giving people the time to migrate and test out how it works for them?

[tbulding]

@bbeesley thanks for this contribution, since this is a breaking change, my only request is that you create an OptimizeCIDRRanges parameter option defaulted to NO. This will ensure existing deployments are not affected.