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

upkeep/216: Add support for IPv6 #217

Merged
merged 7 commits into from
Oct 13, 2022
Merged

upkeep/216: Add support for IPv6 #217

merged 7 commits into from
Oct 13, 2022

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Sep 26, 2022

Closes #216

Description of the Change

This PR adds support for IPv6 addresses.

In addition to that, both IPv4 and IPv6 addresses can be added in the following formats:

Single address

  • 172.10.23.4, 2001:0db8:85a3:0000:0000:8a2e:0370:7334, 2001:0db8:3333:4444::0000, etc

Subnet range

  • 172.10.23.4/33, 2001:0db8:3333:4444:0000:0000:0000:0000/64, etc

Pattern

  • 172.10.*.5, 172.10.*.*, 2001:db8:3333:4444:5555:6666:*:*, etc

Verification Process

  • Verified with positive and negative test case assertions.
  • Ensured existing test cases passed.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog

Add support to IPv6
Add support for subnet range and pattern formats for both IPv4 and IPv6

@Sidsector9 Sidsector9 marked this pull request as ready for review October 5, 2022 13:42
@Sidsector9 Sidsector9 requested review from a team and cadic and removed request for a team October 5, 2022 13:42
@jeffpaul jeffpaul added this to the 7.4.0 milestone Oct 5, 2022
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Sidsector9, the code looks good. I left a couple suggestions.

It also seems like we need to update push-deploy.yml script: include composer install --no-dev to add vendor with the new IPLib dependency. And Looks like PHPUnit needs to be moved to require-dev then.

composer.json Outdated
@@ -29,6 +29,7 @@
}
},
"require": {
"phpunit/phpunit": "9.4.4"
"phpunit/phpunit": "9.4.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it was here before, but do we really need PHPUnit in the require and include it to the release together with IPLib?

$this->assertTrue( $rsa::ip_in_range( '172.10.23.4', '172.10.23.*' ) );
$this->assertTrue( $rsa::ip_in_range( '172.10.23.4', '172.10.*.*' ) );

// IPv6 not in pattern range.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// IPv6 not in pattern range.
// IPv4 not in pattern range.


if ( $parsed_range instanceof \IPLib\Range\Single ) {
$in_range = $user_address->matches( $parsed_range );
} else if ( $parsed_range instanceof \IPLib\Range\Subnet || $parsed_range instanceof \IPLib\Range\Pattern ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if ( $parsed_range instanceof \IPLib\Range\Subnet || $parsed_range instanceof \IPLib\Range\Pattern ) {
} elseif ( $parsed_range instanceof \IPLib\Range\Subnet || $parsed_range instanceof \IPLib\Range\Pattern ) {

Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Sidsector9 Sidsector9 merged commit 4d2187d into develop Oct 13, 2022
@Sidsector9 Sidsector9 deleted the upkeep/216 branch October 13, 2022 11:40
@Sidsector9 Sidsector9 modified the milestones: 7.4.0, 7.3.3 Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IPv6 support
3 participants