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

awsdro · 2024-11-25T05:28:08Z

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.

godwingrs22

@awsdro Thanks for your contribution. Left some minor comments.

packages/@aws-cdk/aws-ec2-alpha/test/integ.subnet-v2.ts

godwingrs22 · 2024-12-17T00:28:08Z

packages/@aws-cdk/aws-ec2-alpha/test/util.test.ts

+  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];


Ips from 10.0.15.255 to 10.0.255.255 consists of multiple CIDR range blocks such as below and not a single CIDR range block

10.0.15.255/32 10.0.16.0/20 10.0.32.0/19 10.0.64.0/18 10.0.128.0/17

nit: It would be better to have ips that are from a single CIDR range block so it will be easy to understand. I would suggest to provide a single CIDR range block something like below.

Range 1: 10.0.0.0/24 [10.0.0.0 - 10.0.0.255]
Range 2: 10.0.0.255/32 [10.0.0.255 - 10.0.0.255]

Note: In a single CIDR range, usually the last ip can't be first ip of another valid CIDR range except for /32. So for this unit test, testing last ip of one range to another range, we could test only for /32 range ip(range 2).

The IPs used in some of the unit tests are not valid CIDR ranges, I only added them to test the actual comparison logic in the rangesOverlap method. During actual use, the method would not be called with invalid CIDR ranges, but it might worth having tests around the logic regardless, as was recommended in previous feedback.

Pull request has been modified.

aws-cdk-automation · 2024-12-17T17:48:20Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 9c148f6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-12-17T19:55:27Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-12-17T19:55:42Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #32269 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 4 of 6 required status checks are expected.).

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

awsdro · 2024-12-17T20:44:42Z

@Mergifyio refresh

mergify · 2024-12-17T20:44:46Z

refresh

✅ Pull request refreshed

mergify · 2024-12-17T20:45:39Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #32269 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 4 of 6 required status checks are expected.).

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

awsdro · 2024-12-17T20:47:16Z

@Mergifyio requeue

mergify · 2024-12-17T20:47:21Z

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

sender-permission >= write

awsdro · 2024-12-17T20:52:37Z

Hi @godwingrs22, could you please help with merging this Pull Request? I am seeing the following message:
Pull request https://github.com/aws/aws-cdk/pull/32269 has been dequeued. The pull request could not be merged. This could be related to an activated [branch protection](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches) or [ruleset rule](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets) that prevents us from merging. (detail: 4 of 6 required status checks are expected.)

I do not have permissions to requeue the PR.

godwingrs22 · 2024-12-17T22:15:42Z

@Mergifyio requeue

mergify · 2024-12-17T22:15:50Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify · 2024-12-17T22:16:31Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #32269 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 4 of 6 required status checks are expected.).

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2024-12-17T22:34:18Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

codecov · 2024-12-17T22:42:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.85%. Comparing base (d4f6946) to head (9c148f6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32269   +/-   ##
=======================================
  Coverage   78.85%   78.85%           
=======================================
  Files         108      108           
  Lines        7165     7165           
  Branches     1319     1319           
=======================================
  Hits         5650     5650           
  Misses       1330     1330           
  Partials      185      185

Flag	Coverage Δ
suite.unit	`78.85% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.85% <ø> (ø)`

godwingrs22 · 2024-12-17T23:27:19Z

@Mergifyio refresh

mergify · 2024-12-17T23:27:24Z

refresh

✅ Pull request refreshed

github-actions · 2024-12-17T23:28:21Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

Do not use string comparison in rangesOverlap

5ac647b

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 25, 2024

aws-cdk-automation requested a review from a team November 25, 2024 05:28

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Nov 25, 2024

Merge branch 'main' into ec2-alpha-rangesoverlap

4804a83

aws-cdk-automation previously requested changes Nov 25, 2024

View reviewed changes

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

638bc4b

awsdro changed the title ~~Do not use string comparison in rangesOverlap~~ fix(ec2-alpha): Do not use string comparison in rangesOverlap Nov 25, 2024

awsdro changed the title ~~fix(ec2-alpha): Do not use string comparison in rangesOverlap~~ fix(ec2-alpha): do not use string comparison in rangesOverlap Nov 25, 2024

awsdro changed the title ~~fix(ec2-alpha): do not use string comparison in rangesOverlap~~ fix(ec2-alpha): do not use string comparison in rangesOverlap Nov 25, 2024

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Nov 25, 2024

awsdro added 3 commits November 25, 2024 11:01

Merge branch 'main' into ec2-alpha-rangesoverlap

d58dc0c

Merge branch 'main' into ec2-alpha-rangesoverlap

ddee673

Merge branch 'main' into ec2-alpha-rangesoverlap

7182ddd

github-actions bot mentioned this pull request Dec 1, 2024

Monthly PR metrics report #32340

Open

awsdro added 7 commits December 2, 2024 11:36

Merge branch 'main' into ec2-alpha-rangesoverlap

4c866fa

Merge branch 'main' into ec2-alpha-rangesoverlap

172813e

Merge branch 'main' into ec2-alpha-rangesoverlap

fe81e68

Merge branch 'main' into ec2-alpha-rangesoverlap

5626f4b

Merge branch 'main' into ec2-alpha-rangesoverlap

ea0f2f2

Add attribution for ip-num to NOTICE

1e5de29

Update subnet-v2 integration test

fd28024

Merge branch 'main' into ec2-alpha-rangesoverlap

5bf4fd2

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 10, 2024

awsdro added 3 commits December 10, 2024 12:24

Merge branch 'main' into ec2-alpha-rangesoverlap

7de950d

Merge branch 'main' into ec2-alpha-rangesoverlap

1b29712

Merge branch 'main' into ec2-alpha-rangesoverlap

66303d0

awsdro added 2 commits December 13, 2024 14:16

Merge branch 'main' into ec2-alpha-rangesoverlap

d2bedbe

Merge branch 'main' into ec2-alpha-rangesoverlap

593a5bc

godwingrs22 self-requested a review December 16, 2024 22:54

godwingrs22 previously requested changes Dec 17, 2024

View reviewed changes

Merge branch 'main' into ec2-alpha-rangesoverlap

e88682f

awsdro requested a review from godwingrs22 December 17, 2024 16:47

awsdro added 2 commits December 17, 2024 11:47

Merge branch 'main' into ec2-alpha-rangesoverlap

fdf8e24

Merge branch 'main' into ec2-alpha-rangesoverlap

9c148f6

godwingrs22 approved these changes Dec 17, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 17, 2024

mergify bot merged commit 87e21d6 into aws:main Dec 17, 2024
22 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

awsdro commented Nov 25, 2024

aws-cdk-automation left a comment

godwingrs22 left a comment

godwingrs22 Dec 17, 2024

awsdro Dec 17, 2024

aws-cdk-automation commented Dec 17, 2024

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

awsdro commented Dec 17, 2024

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

awsdro commented Dec 17, 2024

mergify bot commented Dec 17, 2024

awsdro commented Dec 17, 2024

godwingrs22 commented Dec 17, 2024

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

codecov bot commented Dec 17, 2024

godwingrs22 commented Dec 17, 2024

mergify bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

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

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

Conversation

awsdro commented Nov 25, 2024

Issue #32145

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

godwingrs22 left a comment

Choose a reason for hiding this comment

godwingrs22 Dec 17, 2024

Choose a reason for hiding this comment

awsdro Dec 17, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 17, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

awsdro commented Dec 17, 2024

mergify bot commented Dec 17, 2024

✅ Pull request refreshed

mergify bot commented Dec 17, 2024

awsdro commented Dec 17, 2024

mergify bot commented Dec 17, 2024

❌ Command disallowed due to command restrictions in the Mergify configuration.

awsdro commented Dec 17, 2024

godwingrs22 commented Dec 17, 2024

mergify bot commented Dec 17, 2024

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot commented Dec 17, 2024

mergify bot commented Dec 17, 2024

codecov bot commented Dec 17, 2024

Codecov Report

godwingrs22 commented Dec 17, 2024

mergify bot commented Dec 17, 2024

✅ Pull request refreshed

github-actions bot commented Dec 17, 2024

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

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