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: sort subnets to calculate by their netmask to efficiently use ip space #130

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbeesley
Copy link

@bbeesley bbeesley commented Aug 9, 2023

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 = []
  }

…p space

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.

BREAKING CHANGE: Since the subnets calculated after sorting wont be the same as those calculated
without sorting, this change would cause a delete and recreate of existing subnets that were created
with older versions.
@drewmullen
Copy link
Contributor

drewmullen commented Aug 11, 2023

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
Copy link
Author

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
Copy link

ivan-aws commented Apr 2, 2024

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants