Skip to content

Commit

Permalink
fix(ec2-alpha): do not use string comparison in rangesOverlap (aws#…
Browse files Browse the repository at this point in the history
…32269)

### Issue aws#32145

Closes aws#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
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
awsdro authored Dec 17, 2024
1 parent d4f6946 commit 87e21d6
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 41 deletions.
7 changes: 3 additions & 4 deletions packages/@aws-cdk/aws-ec2-alpha/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export class CidrBlock {
}

/**
* Checks if two IP address ranges overlap.
* Checks if two IPv4 address ranges overlap.
*
* @param range1 The first IP address range represented as an array [start, end].
* @param range2 The second IP address range represented as an array [start, end].
Expand All @@ -269,9 +269,8 @@ export class CidrBlock {
* Note: This method assumes that the start and end addresses are valid IPv4 addresses.
*/
public rangesOverlap(range1: [string, string], range2: [string, string]): boolean {
const [start1, end1] = range1;
const [start2, end2] = range2;

const [start1, end1] = range1.map(ip => NetworkUtils.ipToNum(ip));
const [start2, end2] = range2.map(ip => NetworkUtils.ipToNum(ip));
// Check if ranges overlap
return start1 <= end2 && start2 <= end1;
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"SubnetTest3296A161": {
"Type": "AWS::EC2::VPC",
"Properties": {
"CidrBlock": "10.0.0.0/16",
"CidrBlock": "10.1.0.0/16",
"EnableDnsHostnames": true,
"EnableDnsSupport": true,
"InstanceTenancy": "default"
Expand All @@ -26,7 +26,7 @@
"Properties": {
"AssignIpv6AddressOnCreation": false,
"AvailabilityZone": "us-west-2a",
"CidrBlock": "10.0.0.0/24",
"CidrBlock": "10.1.0.0/20",
"VpcId": {
"Fn::GetAtt": [
"SubnetTest3296A161",
Expand Down Expand Up @@ -221,7 +221,7 @@
"Properties": {
"AssignIpv6AddressOnCreation": false,
"AvailabilityZone": "us-west-2a",
"CidrBlock": "10.0.1.0/24",
"CidrBlock": "10.1.128.0/20",
"VpcId": {
"Fn::GetAtt": [
"SubnetTest3296A161",
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-cdk-vpcv2-alpha-new');

const vpc = new vpc_v2.VpcV2(stack, 'SubnetTest', {
primaryAddressBlock: vpc_v2.IpAddresses.ipv4('10.0.0.0/16'),
primaryAddressBlock: vpc_v2.IpAddresses.ipv4('10.1.0.0/16'),
secondaryAddressBlocks: [vpc_v2.IpAddresses.amazonProvidedIpv6( {
cidrBlockName: 'SecondaryTest',
})],
Expand All @@ -36,7 +36,7 @@ const vpc = new vpc_v2.VpcV2(stack, 'SubnetTest', {
new SubnetV2(stack, 'testSubnet1', {
vpc,
availabilityZone: 'us-west-2a',
ipv4CidrBlock: new IpCidr('10.0.0.0/24'),
ipv4CidrBlock: new IpCidr('10.1.0.0/20'),
//defined on the basis of allocation done in IPAM console
//ipv6CidrBlock: new Ipv6Cidr('2a05:d02c:25:4000::/60'),
subnetType: SubnetType.PRIVATE_ISOLATED,
Expand Down Expand Up @@ -64,7 +64,7 @@ routeTable.addRoute('eigwRoute', '0.0.0.0/0', { gateway: igw });
new SubnetV2(stack, 'testSubnet2', {
vpc,
availabilityZone: 'us-west-2a',
ipv4CidrBlock: new IpCidr('10.0.1.0/24'),
ipv4CidrBlock: new IpCidr('10.1.128.0/20'),
routeTable: routeTable,
subnetType: SubnetType.PUBLIC,
});
Expand Down
38 changes: 38 additions & 0 deletions packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { CidrBlock } from '../lib/util';

describe('Tests for the CidrBlock.rangesOverlap method to check if IPv4 ranges overlap', () =>{
test('Should return false for non-overlapping IP ranges', () => {
const testCidr = new CidrBlock('10.0.0.0/16');
const range1 = ['10.0.0.0', '10.0.15.255'] as [string, string];
const range2 = ['10.0.128.0', '10.0.143.255'] as [string, string];
expect(testCidr.rangesOverlap(range1, range2)).toBe(false);
});

test('Should return true for overlapping IP ranges', () => {
const testCidr = new CidrBlock('54.0.0.0/17');
const range1 = ['54.0.0.0', '54.0.127.255'] as [string, string];
const range2 = ['54.0.100.0', '54.0.192.255'] as [string, string];
expect(testCidr.rangesOverlap(range1, range2)).toBe(true);
});

test('Should return true for overlapping IP ranges where one range is completely inside the other', () => {
const testCidr = new CidrBlock('10.0.0.0/16');
const range1 = ['10.0.0.0', '10.0.127.255'] as [string, string];
const range2 = ['10.0.64.0', '10.0.65.255'] as [string, string];
expect(testCidr.rangesOverlap(range1, range2)).toBe(true);
});

test('Should return true for overlapping IP ranges where the last IP of one range is the first IP of the other', () => {
const testCidr = new CidrBlock('10.0.0.0/16');
const range1 = ['10.0.0.0', '10.0.15.255'] as [string, string];
const range2 = ['10.0.15.255', '10.0.255.255'] as [string, string];
expect(testCidr.rangesOverlap(range1, range2)).toBe(true);
});

test('Should return false for non-overlapping IP ranges where one range starts immediately after the other ends', () => {
const testCidr = new CidrBlock('10.0.0.0/16');
const range1 = ['10.0.0.0', '10.0.15.255'] as [string, string];
const range2 = ['10.0.16.0', '10.0.19.255'] as [string, string];
expect(testCidr.rangesOverlap(range1, range2)).toBe(false);
});
});

0 comments on commit 87e21d6

Please sign in to comment.