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

(ec2-alpha): rangesOverlap utility function is wrong #32145

Open
1 task
nick-kang opened this issue Nov 14, 2024 · 1 comment
Open
1 task

(ec2-alpha): rangesOverlap utility function is wrong #32145

nick-kang opened this issue Nov 14, 2024 · 1 comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@nick-kang
Copy link

Describe the bug

rangesOverlap uses string comparison to determine overlap, which I believe is incorrect.

// example
rangesOverlap(
  ["10.0.0.0", "10.0.15.255"], // 10.0.0.0/20 
  ["10.0.128.0", "10.0.143.255"] // 10.0.128.0/20
)

The above example should return false, but it returns true. Double checked some online tool to confirm that the cidrs do not actually overlap (https://cidrclash.com/clash-results).

image

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

rangesOverlap implements proper cidr comparison

Current Behavior

Implementation is incorrect

Reproduction Steps

See description

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.167.0-alpha.0

Framework Version

No response

Node.js Version

22

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

@nick-kang nick-kang added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 14, 2024
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Nov 14, 2024
@ashishdhingra ashishdhingra self-assigned this Nov 15, 2024
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Nov 15, 2024
@ashishdhingra
Copy link
Contributor

rangesOverlap() utility function appears to return the result of expression start1 <= end2 && start2 <= end1;.

Using the below example shared by user:

// example
rangesOverlap(
  ["10.0.0.0", "10.0.15.255"], // 10.0.0.0/20 
  ["10.0.128.0", "10.0.143.255"] // 10.0.128.0/20
)

start2 is 10.0.128.0 and end1 is 10.0.15.255. Testing the simple string expression "10.0.128.0" < "10.0.15.255" in web browser developer tools console, returns true. This appears to be incorrect since 10.0.128.0 is after the range 10.0.15.255.

Perhaps the logic in rangesOverlap() utility function needs to be revisited.


https://github.com/ip-num/ip-num is a TypeScript library for working with IPv4, IPv6 and ASN numbers. Some articles propose using below logic using this library:

import { IPv4, IPv6 } from "ip-num";

function rangeOverlap(range1Start: string, range1End: string, range2Start: string, range2End: string): boolean {
  const isIPv6 = IPv6.isValid(range1Start);

  if (isIPv6) {
    const start1 = IPv6.fromHexadecimalString(range1Start);
    const end1 = IPv6.fromHexadecimalString(range1End);
    const start2 = IPv6.fromHexadecimalString(range2Start);
    const end2 = IPv6.fromHexadecimalString(range2End);

    return start1.isLessThanOrEqualTo(end2) && start2.isLessThanOrEqualTo(end1);
  } else {
    const start1 = IPv4.fromDecimalDottedString(range1Start);
    const end1 = IPv4.fromDecimalDottedString(range1End);
    const start2 = IPv4.fromDecimalDottedString(range2Start);
    const end2 = IPv4.fromDecimalDottedString(range2End);

    return start1.isLessThanOrEqualTo(end2) && start2.isLessThanOrEqualTo(end1);
  }
}

@ashishdhingra ashishdhingra added effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Nov 15, 2024
@ashishdhingra ashishdhingra removed their assignment Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

2 participants