From 87e21d625af86873716734dd5568940d41096c45 Mon Sep 17 00:00:00 2001 From: awsdro Date: Tue, 17 Dec 2024 18:28:05 -0500 Subject: [PATCH] fix(ec2-alpha): do not use string comparison in `rangesOverlap` (#32269) ### 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 - [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* --- packages/@aws-cdk/aws-ec2-alpha/lib/util.ts | 7 +- .../aws-cdk-vpcv2-alpha-new.assets.json | 4 +- .../aws-cdk-vpcv2-alpha-new.template.json | 6 +- .../integ.subnet-v2.js.snapshot/manifest.json | 73 +++++++++++++------ .../integ.subnet-v2.js.snapshot/tree.json | 10 +-- .../aws-ec2-alpha/test/integ.subnet-v2.ts | 6 +- .../@aws-cdk/aws-ec2-alpha/test/util.test.ts | 38 ++++++++++ 7 files changed, 103 insertions(+), 41 deletions(-) create mode 100644 packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts diff --git a/packages/@aws-cdk/aws-ec2-alpha/lib/util.ts b/packages/@aws-cdk/aws-ec2-alpha/lib/util.ts index a363c26581f68..7566ce711e0c4 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/lib/util.ts +++ b/packages/@aws-cdk/aws-ec2-alpha/lib/util.ts @@ -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]. @@ -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; } diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.assets.json b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.assets.json index dabbaf87c2e2c..f6dfeb1f96dd0 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.assets.json +++ b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.assets.json @@ -1,7 +1,7 @@ { "version": "38.0.1", "files": { - "a88dc0ea3e4c5ea7d17016d0f0fb2cdac863bc0d2a1ed9aecfb42840baf64e13": { + "5d255179238bb6636b1f13a6468e815c07fcdb1bfdedac13ca9e51fdda76978f": { "source": { "path": "aws-cdk-vpcv2-alpha-new.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "a88dc0ea3e4c5ea7d17016d0f0fb2cdac863bc0d2a1ed9aecfb42840baf64e13.json", + "objectKey": "5d255179238bb6636b1f13a6468e815c07fcdb1bfdedac13ca9e51fdda76978f.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.template.json b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.template.json index 7c46d95906fb4..3c40c91db03a2 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.template.json +++ b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/aws-cdk-vpcv2-alpha-new.template.json @@ -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" @@ -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", @@ -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", diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/manifest.json b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/manifest.json index 14f3c281405a7..8e2c1fc628670 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/manifest.json +++ b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/manifest.json @@ -16,10 +16,9 @@ "templateFile": "aws-cdk-vpcv2-alpha-new.template.json", "terminationProtection": false, "validateOnSynth": false, - "notificationArns": [], "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/a88dc0ea3e4c5ea7d17016d0f0fb2cdac863bc0d2a1ed9aecfb42840baf64e13.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/5d255179238bb6636b1f13a6468e815c07fcdb1bfdedac13ca9e51fdda76978f.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -38,37 +37,55 @@ "/aws-cdk-vpcv2-alpha-new/SubnetTest/Resource": [ { "type": "aws:cdk:logicalId", - "data": "SubnetTest3296A161" + "data": "SubnetTest3296A161", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/SubnetTest/SecondaryTest/SecondaryTest": [ { "type": "aws:cdk:logicalId", - "data": "SubnetTestSecondaryTest2AB12223" + "data": "SubnetTestSecondaryTest2AB12223", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/testSubnet1/Subnet": [ { "type": "aws:cdk:logicalId", - "data": "testSubnet1Subnet72087287" + "data": "testSubnet1Subnet72087287", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/testSubnet1/RouteTable/RouteTable": [ { "type": "aws:cdk:logicalId", - "data": "testSubnet1RouteTableB5FDDF81" + "data": "testSubnet1RouteTableB5FDDF81", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/testSubnet1/RouteTableAssociation": [ { "type": "aws:cdk:logicalId", - "data": "testSubnet1RouteTableAssociation1DA9E185" + "data": "testSubnet1RouteTableAssociation1DA9E185", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/Instance/InstanceSecurityGroup/Resource": [ { "type": "aws:cdk:logicalId", - "data": "InstanceInstanceSecurityGroupF0E2D5BE" + "data": "InstanceInstanceSecurityGroupF0E2D5BE", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/Instance/InstanceRole/Resource": [ @@ -86,7 +103,10 @@ "/aws-cdk-vpcv2-alpha-new/Instance/Resource": [ { "type": "aws:cdk:logicalId", - "data": "InstanceC1063A87" + "data": "InstanceC1063A87", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/SsmParameterValue:--aws--service--ami-amazon-linux-latest--amzn-ami-hvm-x86_64-gp2:C96584B6-F00A-464E-AD19-53AFF4B05118.Parameter": [ @@ -104,31 +124,46 @@ "/aws-cdk-vpcv2-alpha-new/testIGW/GWAttachment": [ { "type": "aws:cdk:logicalId", - "data": "testIGWGWAttachment682A6782" + "data": "testIGWGWAttachment682A6782", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/TestRoutetable/RouteTable": [ { "type": "aws:cdk:logicalId", - "data": "TestRoutetableRouteTable7B7B907D" + "data": "TestRoutetableRouteTable7B7B907D", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/TestRoutetable/eigwRoute/Route": [ { "type": "aws:cdk:logicalId", - "data": "TestRoutetableeigwRouteCDE8BBAF" + "data": "TestRoutetableeigwRouteCDE8BBAF", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/testSubnet2/Subnet": [ { "type": "aws:cdk:logicalId", - "data": "testSubnet2Subnet4681911A" + "data": "testSubnet2Subnet4681911A", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/testSubnet2/RouteTableAssociation": [ { "type": "aws:cdk:logicalId", - "data": "testSubnet2RouteTableAssociation40DCE4CD" + "data": "testSubnet2RouteTableAssociation40DCE4CD", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_REPLACE" + ] } ], "/aws-cdk-vpcv2-alpha-new/BootstrapVersion": [ @@ -142,15 +177,6 @@ "type": "aws:cdk:logicalId", "data": "CheckBootstrapVersion" } - ], - "SubnetTestSecondaryTestBDE45F82": [ - { - "type": "aws:cdk:logicalId", - "data": "SubnetTestSecondaryTestBDE45F82", - "trace": [ - "!!DESTRUCTIVE_CHANGES: WILL_DESTROY" - ] - } ] }, "displayName": "aws-cdk-vpcv2-alpha-new" @@ -170,7 +196,6 @@ "templateFile": "integtestmodelDefaultTestDeployAssertCF40BD53.template.json", "terminationProtection": false, "validateOnSynth": false, - "notificationArns": [], "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json", diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/tree.json b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/tree.json index 4ca5db479ab4e..08a918bd193b3 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/tree.json +++ b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.js.snapshot/tree.json @@ -18,7 +18,7 @@ "attributes": { "aws:cdk:cloudformation:type": "AWS::EC2::VPC", "aws:cdk:cloudformation:props": { - "cidrBlock": "10.0.0.0/16", + "cidrBlock": "10.1.0.0/16", "enableDnsHostnames": true, "enableDnsSupport": true, "instanceTenancy": "default" @@ -77,7 +77,7 @@ "aws:cdk:cloudformation:props": { "assignIpv6AddressOnCreation": false, "availabilityZone": "us-west-2a", - "cidrBlock": "10.0.0.0/24", + "cidrBlock": "10.1.0.0/20", "vpcId": { "Fn::GetAtt": [ "SubnetTest3296A161", @@ -457,7 +457,7 @@ "aws:cdk:cloudformation:props": { "assignIpv6AddressOnCreation": false, "availabilityZone": "us-west-2a", - "cidrBlock": "10.0.1.0/24", + "cidrBlock": "10.1.128.0/20", "vpcId": { "Fn::GetAtt": [ "SubnetTest3296A161", @@ -542,7 +542,7 @@ "path": "integtest-model/DefaultTest/Default", "constructInfo": { "fqn": "constructs.Construct", - "version": "10.3.0" + "version": "10.4.2" } }, "DeployAssert": { @@ -588,7 +588,7 @@ "path": "Tree", "constructInfo": { "fqn": "constructs.Construct", - "version": "10.3.0" + "version": "10.4.2" } } }, diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts index a584b26038aeb..eaa7df1baa667 100644 --- a/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts +++ b/packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts @@ -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', })], @@ -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, @@ -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, }); diff --git a/packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts b/packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts new file mode 100644 index 0000000000000..2d5f1890674f4 --- /dev/null +++ b/packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts @@ -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); + }); +});