fix(ec2-alpha): do not use string comparison in rangesOverlap

[awsdro]

Issue #32145

Closes #32145.

Reason for this change

The rangesOverlap method was using string comparison to check if IP ranges overlapped.

Description of changes

The rangesOverlap method was updated to compare IP ranges using the ip-num library

Description of how you validated changes

Added two unit tests to verify correct behavior

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[nmussy]

Given how little ip-num is being used, I would advise against adding it as a dependency

[nmussy]

Adding a dependency to convert an IP address to a sortable numeric value seems a bit excessive, you could implement something similar to what's being done with CidrBlockIpv6.getIPv6Range:

private ipv4ToNumber(ipv4Address: string): number {
  return parseInt(ipv4Address.split('.').map(part => parseInt(part).toString(2).padStart(8, '0')).join(''), 2);
}

A BigInt shouldn't be necessary for IPv4s, Number.MAX_SAFE_INTEGER should always be greater than 255.255.255.255

[awsdro]

Thanks for the feedback. I agree, adding a dependency just to compare IPv4 addresses is a bit too much. I will add a private method to convert the IP addresses to comparable numbers, like you suggested.

[nmussy]

Could you add a few more test cases here, such as:

• Test that range1 being fully contained within range2 is evaluated to be an overlap
• Test that given end1 = start2 + 1, they are not overlapping. Same with end1 = start2 are considered to be overlapping
• Not sure if that's allowed later on by the CDK or CloudFormation, but test that having a reversed start and end (start1 > end1) does not mess up the overlapping evaluation

[awsdro]

Everywhere rangesOverlap is called, the range is calculated like cidrBlock.minIp(), cidrBlock.maxIp(), so it's always "in order". The implicit assumption with checking for overlap using start1 <= end2 && start2 <= end1 is that start1 <= end1 and start2 <= end2.

[nmussy]

This would go back to being:

const [start1, end1] = range1.map(this.ipv4ToNumber);
const [start2, end2] = range2.map(this.ipv4ToNumber);

// Check if ranges overlap
return start1 <= end2 && start2 <= end1;

[nmussy]

LGTM. And I didn't see there was already a ipToNum method. Gave it a look, kind of funny to see the "hardcore" maths vs my naive proposed implementation 😆

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 593a5bc
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository