Remove planned deprecated 'resolver.source' profile property by tgreenx · Pull Request #1343 · zonemaster/zonemaster-engine

tgreenx · 2024-05-07T18:17:53Z

Purpose

This PR removes the planned deprecated resolver.source profile property. It also adds a proper IP validation module (and functions) in Zonemaster::Engine::Validation.

Other related changes to the resolver.source{4,6} profile properties and Zonemaster::Engine::Nameserver source_address{4,6} attributes are made. See the Changes section.

Context

Fixes #1360
Also fixes Perl warnings from #1342

Changes

This PR removes the source_address{4,6} and source_address attributes of Zonemaster::Engine::Nameserver objects.

Other related changes:

Changes resolver.source_address{4,6} profile properties to not accept illegal values.
Removes now unused constant $RESOLVER_SOURCE_OS_DEFAULT
Removes now unused function Zonemaster::Engine::Profile::check_validity()
~~Rename Zonemaster::Engine::Nameserver::source_address() to Zonemaster::Engine::Nameserver::_build_source_address()~~
Add IP validation module and functions Zonemaster::Engine::Validation::validate_ipv{4,6}()
Refactoring
Add and updates documentation
Add and updates unit tests

How to test this PR

Unit tests are updated and should pass.

mattias-p

Looks great!

Once this is merged I'll rebase #1356 on top of it.

lib/Zonemaster/Engine/Profile.pm

mattias-p

It seems I went too fast reviewing this the first time. When I had a look at it again I realized there is a problem with how the resolver.source4 and resolver.source6 properties are defined.

lib/Zonemaster/Engine/Profile.pm

tgreenx · 2024-06-10T16:03:10Z

@matsduf @mattias-p @marc-vanderwal As discussed in the meeting, I added a proper IP validation function to Zonemaster::Engine::Util in this PR. Please re-review.

mattias-p · 2024-06-11T08:54:12Z

lib/Zonemaster/Engine/Util.pm

           );
 }

+sub validate_ip {


IMHO, having separate function for IPv4 and IPv6 would make for a better API.

I didn't do that, because I couldn't see a major benefit. However, to go in that direction I renamed this function from validate_ip() to validate_ip_for_version().

As discussed in group meeting, it is now done (in commit f29f884).

lib/Zonemaster/Engine/Constants.pm

lib/Zonemaster/Engine/Profile.pm

tgreenx · 2024-06-11T10:15:43Z

@mattias-p I'm integrating your comments, but I have one problem.

I see now that I get a Undefined subroutine &Zonemaster::Engine::Util::validate_ip called at zonemaster-engine/lib/Zonemaster/Engine/Profile.pm line 87. error when adding the source{4,6} property in the profile. I suspect it has to do with how Perl modules and imports work (compile and run times): currently Zonemaster::Engine::Util already has a use Zonemaster::Engine::Profile import, and now in this PR I added a use Zonemaster::Engine::Util qw( validate_ip ) import in the Zonemaster::Engine::Profile module.

Do you have any clue on how to solve this issue (mutual/circular module imports)?

mattias-p

I had another look at this PR and started questioning the use of an accessor for the source address. The following review comments are this topic.

lib/Zonemaster/Engine/Nameserver.pm

mattias-p · 2024-06-11T14:48:17Z

Do you have any clue on how to solve this issue (mutual/circular module imports)?

Oh, this again. In general I think we should try to avoid circular module dependencies as much as possible, but I we do have a lot of them already. Zonemaster::Engine and Zonemaster::Engine::Util seem to be the worst offenders in this respect. As I refactor code to clean up or make room for other features I try to reduce the amount of circular dependencies. Maybe we should come up with some plan to reduce circular dependencies to sane levels. I'm sure that would involve deprecating a fair number of public methods.

For this particular case maybe we could introduce a new Zonemaster::Engine::Validation module that doesn't import any other modules. Zonemaster::Engine::Profile would be able to import that module without problem.

tgreenx · 2024-06-11T17:24:58Z

Do you have any clue on how to solve this issue (mutual/circular module imports)?
[ ... ]
For this particular case maybe we could introduce a new Zonemaster::Engine::Validation module that doesn't import any other modules. Zonemaster::Engine::Profile would be able to import that module without problem.

Now done. And your other comments have been addressed as well. Thanks for the good feedback !

@mattias-p @matsduf @marc-vanderwal please re-review.

This commit also removes the 'source_address{4,6}' attributes of Zonemaster::Engine::Nameserver objects by using an unified one, 'source_address'. I believe the change that introduced them was unecessary, as Zonemaster::Engine::Nameserver objects are tied to only one IP address (protocol). Other tied changes: - Changes 'resolver.source_address{4,6}' profile properties to not accept illegal values such as undefined or empty values. - Removes now unused constant '$RESOLVER_SOURCE_OS_DEFAULT' - Removes now unused function Zonemaster::Engine::Profile::check_validity() - Rename Zonemaster::Engine::Nameserver::source_address() to Zonemaster::Engine::Nameserver::_build_source_address() - Refactoring - Updates documentation - Updates unit tests

Co-authored-by: Mattias Päivärinta <mattias@paivarinta.se>

- Add constants IPV{4,6}_RE - Add function Zonemaster::Engine::Util->validate_ip() - Update documentation - Update unit tests

…w IP address validation module - Remove 'source_address' attribute in Zonemaster::Engine::Nameserver - Rename function '_build_source_address()' to 'source_address()' and make it a public instance method - Move IP address validation from Zonemaster::Engine::Util to a new module, Zonemaster::Engine::Validation - Fixes for 'resolver.source{4,6}' properties in Zonemaster::Engine::Profile - Update and add documentation - Update unit tests for Zonemaster::Engine::Profile - Add unit tests for Zonemaster::Engine::Validation

mattias-p

I reviewing the test code I found one place where relevant tests are removed and a couple of places of missing tests that we should add. I feel bad about asking you to fix stuff in this PR that you didn't introduce. If you don't feel like fixing those, just say so and I'll make my own PR.

mattias-p · 2024-06-12T13:27:02Z

t/profiles.t

-# JSON representation of an example profile where all set values are sentinels
-Readonly my $EXAMPLE_PROFILE_3 => qq(
-{
-  "resolver": {
-    "source": "$RESOLVER_SOURCE_OS_DEFAULT"
-  }
-}
-);
-


The empty strings for resolver.source4 and resolver.source6 are also sentinel values, so we need an example profile where those values are present.

# JSON representation of an example profile where all set values are sentinels Readonly my $EXAMPLE_PROFILE_3 => qq( { "resolver": { "source4": "", "source6": "" } } );

mattias-p · 2024-06-12T13:35:49Z

t/profiles.t

-subtest 'from_json() parses sentinel values from a string' => sub {
-    my $profile = Zonemaster::Engine::Profile->from_json( $EXAMPLE_PROFILE_3 );
-
-    is $profile->get( 'resolver.source' ), $RESOLVER_SOURCE_OS_DEFAULT, 'resolver.source was parsed from JSON';
-};


We should test parsing of the sentinel values for resolver.source4 and resolver.source6 too.

subtest 'from_json() parses sentinel values from a string' => sub { my $profile = Zonemaster::Engine::Profile->from_json( $EXAMPLE_PROFILE_3 ); is $profile->get( 'resolver.source4' ), '', 'resolver.source4 was parsed from JSON'; is $profile->get( 'resolver.source6' ), '', 'resolver.source6 was parsed from JSON'; };

mattias-p · 2024-06-12T13:39:04Z

t/profiles.t

-subtest 'set() accepts sentinel values' => sub {
-    my $profile = Zonemaster::Engine::Profile->new;
-
-    $profile->set( 'resolver.source', $RESOLVER_SOURCE_OS_DEFAULT );
-    is $profile->get( 'resolver.source' ), $RESOLVER_SOURCE_OS_DEFAULT, 'resolver.source was updated';
-
-    $profile->set( 'resolver.source4', '' );
-    is $profile->get( 'resolver.source4' ), '', 'resolver.source4 was updated';
-
-    $profile->set( 'resolver.source6', '' );
-    is $profile->get( 'resolver.source6' ), '', 'resolver.source6 was updated';
-};
-


We should keep the checks for resolver.source4 and resolver.source6.

subtest 'set() accepts sentinel values' => sub { my $profile = Zonemaster::Engine::Profile->new; $profile->set( 'resolver.source4', '' ); is $profile->get( 'resolver.source4' ), '', 'resolver.source4 was updated'; $profile->set( 'resolver.source6', '' ); is $profile->get( 'resolver.source6' ), '', 'resolver.source6 was updated'; };

… two functions

tgreenx · 2024-06-12T14:41:12Z

@mattias-p @matsduf @marc-vanderwal Comments have been addressed, please re-review.

matsduf · 2024-06-27T08:00:31Z

Release testing for v2024.1

I have verified that there is no more resolver.source support.

tgreenx added the V-Major Versioning: The change gives an update of major in version. label May 7, 2024

tgreenx added this to the v2024.1 milestone May 7, 2024

tgreenx requested review from marc-vanderwal, matsduf and mattias-p May 7, 2024 18:17

This was referenced May 7, 2024

Update minimum Perl version to 5.16.0 and add missing 'warnings' #1342

Merged

Remove planned deprecated '--sourceaddr' option zonemaster/zonemaster-cli#370

Merged

marc-vanderwal previously approved these changes May 16, 2024

View reviewed changes

mattias-p previously approved these changes Jun 4, 2024

View reviewed changes

mattias-p mentioned this pull request Jun 4, 2024

Profile fixes #1356

Merged

matsduf requested changes Jun 4, 2024

View reviewed changes

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

tgreenx requested a review from matsduf June 4, 2024 16:49

mattias-p self-requested a review June 5, 2024 16:30

mattias-p reviewed Jun 5, 2024

View reviewed changes

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

mattias-p self-requested a review June 5, 2024 16:55

tgreenx dismissed stale reviews from mattias-p and marc-vanderwal via f42aabd June 10, 2024 14:23

tgreenx linked an issue Jun 10, 2024 that may be closed by this pull request

Move IP address validation to Zonemaster::Engine::Util #1360

Closed

tgreenx mentioned this pull request Jun 10, 2024

Move IP address validation to Zonemaster::Engine::Util #1360

Closed

matsduf previously approved these changes Jun 10, 2024

View reviewed changes

mattias-p reviewed Jun 11, 2024

View reviewed changes

tgreenx dismissed matsduf’s stale review via f677fbb June 11, 2024 17:22

tgreenx requested review from marc-vanderwal, matsduf and mattias-p June 11, 2024 17:22

tgreenx and others added 4 commits June 12, 2024 11:07

Apply suggestions from code review

1fd8a22

Co-authored-by: Mattias Päivärinta <mattias@paivarinta.se>

Add proper IP address validation in Engine

a7d365e

- Add constants IPV{4,6}_RE - Add function Zonemaster::Engine::Util->validate_ip() - Update documentation - Update unit tests

Add missing documentation for Zonemaster::Engine::Util->validate_ip()

5422e35

tgreenx dismissed marc-vanderwal’s stale review via 330914a June 12, 2024 09:07

tgreenx force-pushed the remove-deprecated branch from f677fbb to 330914a Compare June 12, 2024 09:07

tgreenx requested a review from marc-vanderwal June 12, 2024 09:07

marc-vanderwal previously approved these changes Jun 12, 2024

View reviewed changes

tgreenx dismissed marc-vanderwal’s stale review via fab619f June 12, 2024 09:25

tgreenx force-pushed the remove-deprecated branch 2 times, most recently from fab619f to cb03801 Compare June 12, 2024 09:35

tgreenx force-pushed the remove-deprecated branch from cb03801 to 2dd7139 Compare June 12, 2024 09:39

marc-vanderwal previously approved these changes Jun 12, 2024

View reviewed changes

mattias-p reviewed Jun 12, 2024

View reviewed changes

Split up Zonemaster::Engine::Validation::validate_ip_for_version into…

f29f884

… two functions

tgreenx dismissed marc-vanderwal’s stale review via f29f884 June 12, 2024 14:20

Update profiles.t unit tests

4db8bee

tgreenx requested review from marc-vanderwal and mattias-p June 12, 2024 14:40

Cleanup

429f7c0

matsduf approved these changes Jun 13, 2024

View reviewed changes

matsduf added the P-High Priority: Issue to be solved before other label Jun 13, 2024

marc-vanderwal approved these changes Jun 17, 2024

View reviewed changes

tgreenx merged commit 3501cc1 into zonemaster:develop Jun 17, 2024

tgreenx deleted the remove-deprecated branch June 17, 2024 09:31

tgreenx mentioned this pull request Jun 17, 2024

Fix undef eq comparison #1363

Merged

matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 27, 2024

Conversation

tgreenx commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Context

Changes

How to test this PR

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgreenx commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgreenx commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattias-p commented Jun 11, 2024

Uh oh!

tgreenx commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgreenx commented Jun 12, 2024

Uh oh!

matsduf commented Jun 27, 2024

Release testing for v2024.1

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgreenx commented May 7, 2024 •

edited

Loading

tgreenx commented Jun 11, 2024 •

edited

Loading

tgreenx commented Jun 11, 2024 •

edited

Loading