Update MethodsV2 get_parent_ns_ips() to match new Basic01 algorithm by tgreenx · Pull Request #1373 · zonemaster/zonemaster-engine

tgreenx · 2024-07-10T15:23:37Z

Purpose

This PR updates MethodsV2 get_parent_ns_ips() to match new Basic01 algorithm. Other related/convenient changes are made in other modules (e.g. Zonemaster::Engine::Recursor).

It also adds a loop protection mechanism (and message tag at the DEBUG2 level) for while() loops in case of any unforeseen issue with the methods using this new algorithm.

For easier reviewing you should discard commit b83a7a7 (which only converts indentation from tabulation to space).

Context

Specification: zonemaster/zonemaster#1287

This implementation contains slight deviations from the specification, see zonemaster/zonemaster#1287 (review).

Changes

Update Zonemaster::Engine::TestMethodsV2->get_parent_ns_ips() code
Fixes and refactoring in many Zonemaster::Engine::TestMethodsV2 methods
Add new message tag LOOP_PROTECTION (used in three places) at the DEBUG2 level
Add loop protection mechanism in Basic01 and MethodsV2 (currently the loop will break at 1000 iterations)
Add optional argument $ns in Zonemaster::Engine::Recursor->recurse()
Add optional argument @section in Zonemaster::Engine::Packet->has_rrs_of_type_for_name()
Change IP address list to a set in Zonemaster::Engine::Recursor->add_fake_addresses() (to avoid duplicates)
Update documentation
Update unit tests data

How to test this PR

Unit tests should pass.

Also, for manual testing you can run the method of your choice with the following, e.g. for get_parent_ns_ips():

$ perl -MZonemaster::Engine -MZonemaster::Engine::TestMethodsV2 -E 'Zonemaster::Engine::Recursor->remove_fake_addresses("."); Zonemaster::Engine::Recursor->add_fake_addresses("." => { ns1 => ["127.1.0.1", "fda1:b2:c3::127:1:0:1"], ns2 => ["127.1.0.2", "fda1:b2:c3::127:1:0:2"]  }); say "\n", join "\n", @{Zonemaster::Engine::TestMethodsV2->get_parent_ns_ips(Zonemaster::Engine::Zone->new({ name => Zonemaster::Engine::DNSName->from_string("child.parent.good-1.methodsv2.xa") }))};'

ns1.parent.good-1.methodsv2.xa/127.40.1.41
ns1.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:1:41
ns2.parent.good-1.methodsv2.xa/127.40.1.42
ns2.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:1:42

tgreenx · 2024-07-10T15:33:47Z

This implementation contains slight deviations from the specification, see zonemaster/zonemaster#1287 (review).

matsduf · 2024-07-10T17:40:53Z

How to test this PR

Unit tests should pass.

Also, for manual testing you can run:

I think we should test the implementations that currently fail, see #1351.

matsduf · 2024-07-11T15:41:18Z

Updated my environment to run with this update. There are four failing scenarios. One is CHLD-FOUND-INCONSIST-6

The parent is expected to be 127.40.1.41 + IPv6, and that is found by the unit test. Querying the parent for the delegation gives:

$ dig @127.40.1.41 child.parent.chld-found-inconsist-6.methodsv2.xa ns
(...)
;; ANSWER SECTION:
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns1.child.parent.chld-found-inconsist-6.methodsv2.xa.
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns2.child.parent.chld-found-inconsist-6.methodsv2.xa.
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns1.parent.chld-found-inconsist-6.methodsv2.xa.

;; ADDITIONAL SECTION:
ns1.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN A	127.40.1.51
ns1.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN AAAA fda1:b2:c3:0:127:40:1:51
ns2.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN A	127.40.1.52
ns2.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN AAAA fda1:b2:c3:0:127:40:1:52

Six ns/ip are expected from the unit test, but it only provides two:

$ ZONEMASTER_METHODSV2_SCENARIO='CHLD-FOUND-INCONSIST-6' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Nameserver 'ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.51' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.51

        #   Failed test 'Nameserver 'ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:51' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:51

        #   Failed test 'Nameserver 'ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.52' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.52

        #   Failed test 'Nameserver 'ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:52' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:52

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 327.
        # Number of nameservers in both arrays does not match (found 2, expected 6)
        # Looks like you failed 5 tests of 10.

    #   Failed test 'get_del_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 332.
(...)

matsduf · 2024-07-11T15:53:01Z

Another example, IB-NOT-IN-ZONE-1.

The address records are not defined in the zone, but the NS records are there and the delegation works. Lookup with the resolver:

$ dig @127.3.0.53 child.parent.ib-not-in-zone-1.methodsv2.xa ns
(...)
;; ANSWER SECTION:
child.parent.ib-not-in-zone-1.methodsv2.xa. 3600 IN NS ns2.child.parent.ib-not-in-zone-1.methodsv2.xa.
child.parent.ib-not-in-zone-1.methodsv2.xa. 3600 IN NS ns1.child.parent.ib-not-in-zone-1.methodsv2.xa.

The unit test does not report any name servers for the zone:

$ ZONEMASTER_METHODSV2_SCENARIO='ib-not-in-zone-1' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Nameserver 'ns1.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.ib-not-in-zone-1.methodsv2.xa

        #   Failed test 'Nameserver 'ns2.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.ib-not-in-zone-1.methodsv2.xa

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 327.
        # Number of nameservers in both arrays does not match (found 0, expected 2)
        # Looks like you failed 3 tests of 4.

    #   Failed test 'get_zone_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 332.
    # Looks like you failed 1 test of 7.

matsduf · 2024-07-11T16:03:57Z

The t/TestUtil cannot handle when the method returns an undefined value. Is it ever meaningful that is does? Or should it return an empty array instead?

$ ZONEMASTER_METHODSV2_SCENARIO='NO-CHILD-1' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Result is defined'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 297.
        # Unexpected undefined result
        # Looks like you failed 1 test of 1.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 307.
    # Looks like you failed 1 test of 1.

#   Failed test 'NO-CHILD-1'
#   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 391.
Can't use an undefined value as an ARRAY reference at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 302.

NO-CHILD-2 has the same problem as NO-CHILD-1.

tgreenx · 2024-07-11T16:21:09Z

The t/TestUtil cannot handle when the method returns an undefined value. Is it ever meaningful that is does? Or should it return an empty array instead?

It can, but you need to pass an undefined value and not an arrayref of an undefined value, i.e.: undef instead of [ undef ] in t/methodsv2.t:

    'NO-CHILD-2' => [
        1,
        q(child.parent.no-child-2.methodsv2.xa),
        undef,
        [ ], # Empty
        [ ], # Empty
        [ ],
    ],

tgreenx · 2024-07-11T16:34:57Z

@matsduf Could you push all your changes to the t/methodsv2.t file in #1351 so that I can start debugging too?

tgreenx · 2024-07-11T16:37:19Z

@matsduf Could you push all your changes to the t/methodsv2.t file in #1351 so that I can start debugging too?

Ah you just did. From what I can see in your latest commit you mistakenly defined empty sets everywhere instead of undefined values, as the specification states. See my review in zonemaster/zonemaster#1254.

These are the currently specified return values of MethodV2 "Get parent NS IP addresses":

* A set of name server IP address for the parent zone:
  * Non-empty set: The name servers have been identified.
  * Empty set: Root zone or undelegated test.
  * Undefined set: The name servers cannot be determined due to errors in the
    delegation.

matsduf · 2024-07-11T17:05:52Z

It does not seem to work to specify undef in t/methodsv2.t.

tgreenx · 2024-07-11T17:45:52Z

It does not seem to work to specify undef in t/methodsv2.t.

Indeed, I suggested a fix. See: #1351 (review)

With that fix it seems to work properly now, and (enabled) unit tests pass:

$ prove t/methodsv2.t
t/methodsv2.t .. 23/? Untested scenarios:
        Scenario CHLD-FOUND-INCONSIST-6 cannot be tested.
        Scenario CHLD-FOUND-PAR-UNDET-1 cannot be tested.
        Scenario GOOD-6 cannot be tested.
        Scenario IB-NOT-IN-ZONE-1 cannot be tested.
        Scenario NO-CHLD-PAR-UNDETER-1 cannot be tested.
t/methodsv2.t .. ok
All tests successful.
Files=1, Tests=25,  9 wallclock secs ( 0.08 usr  0.00 sys +  8.25 cusr  0.16 csys =  8.49 CPU)
Result: PASS

matsduf · 2024-07-12T20:10:53Z

Scenarios DELEG-OOB-W-ERROR-1 -- 4 fail and output

Can't locate object method "query" via package "Zonemaster::Engine::DNSName" at /usr/local/share/perl/5.34.0/Zonemaster/Engine/TestMethodsV2.pm line 581.

E.g.

$ ZONEMASTER_SINGLE_SCENARIO="DELEG-OOB-W-ERROR-4" ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/?         # No tests run!

    #   Failed test 'No tests run for subtest "get_zone_ns_names_and_ips"'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 336.
    # Looks like you failed 1 test of 3.
t/methodsv2.t .. 4/? 
#   Failed test 'DELEG-OOB-W-ERROR-4'
#   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 399.
Can't locate object method "query" via package "Zonemaster::Engine::DNSName" at /usr/local/share/perl/5.34.0/Zonemaster/Engine/TestMethodsV2.pm line 581.

Also see #1351 which is complete now.

Based on zonemaster/zonemaster#1287 - Update Zonemaster::Engine::TestMethodsV2->get_parent_ns_ips() code - Add new message tag 'LOOP_PROTECTION' (used in three places) - Add loop protection mechanism - Update unit tests data

- Add missing CNAME support in several TestMethodsV2 methods - Add support for resolution from given name server list to Recursor->recurse() - Add support of section to Packet->has_rrs_of_type_for_name() - Change usage of Recursor->_recurse() to Recursor->recurse() in TestMethodsV2.pm - Refactoring - Update documentation

…odules - Fix conditionals in TestMethodsV2->_get_delegation() - Change IP address list to a set in Recursor->add_fake_addresses(), in order to avoid duplicates - Refactoring

tgreenx · 2024-07-17T13:11:17Z

@matsduf I added a few commits in #1351. Now, together with the latest changes in this PR, all unit tests pass.

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

tgreenx · 2024-07-17T15:08:24Z

Add loop protection mechanism in Basic01 and MethodsV2 (currently the loop will break at 1000 iterations)

(Initial comment: zonemaster/zonemaster#1287 (comment))

This relates to this part of the specification: https://github.com/zonemaster/zonemaster/blob/7b85ac86264a51bb8754ee86b7dcad33215cd670/docs/public/specifications/tests/MethodsV2.md?plain=1#L181-L237

Currently one of such loop protection mechanism already existed in Engine, located in the recursive function and with a max value set at 20:

zonemaster-engine/lib/Zonemaster/Engine/Recursor.pm

Line 218 in b889bcb

return ( undef, $state ) if $state->{count} > 20; # Loop protection

I re-ran the numbers using all test zones defined in t/methodsv2.t from #1351, as well as all test zones defined in t/Test-basic01.t.
In both cases, the maximum $loop_counter value is 1 (i.e. 2 iterations).

So it seems reasonable to lower the number from 1000 to 20, to match the existing value found in Zonemaster::Engine::Recursor->_recurse(). What do you all think?

marc-vanderwal · 2024-07-18T08:39:54Z

So it seems reasonable to lower the number from 1000 to 20, to match the existing value found in Zonemaster::Engine::Recursor->_recurse(). What do you all think?

There is a risk of aborting too early in some corner cases that probably sound convoluted, but are legal.

IPv6 reverse zones have domain names with many labels: domain names in ip6.arpa mapping to IPv6 addresses have 34 labels. Any of those labels could be delegation points. Resolving such a name to PTR could thus potentially mean to iterate over 34 delegation points.

Unless there is a best practice somewhere stating that the number of delegation points be limited to some reasonable number? Or is that number similar to what other “big” resolvers (e.g. BIND, Unbound, etc.) do?

matsduf · 2024-07-18T13:29:26Z

There is a risk both in too low and too high value. I suggest that we set the value to less than 100, maybe as low as 20, and then adjust the other value to be the same. If I read the code a message will be outputted if the level is reached. It is not expected to happen, so I suggest the message to be on DEBUG level. It could even be motivated to have it on WARNING level since it might severely affect the test result.

matsduf · 2024-07-18T13:32:37Z

All tests in #1351 pass. I suggest that this PR is merged and then #1351 is rebased and merged. After that some adjustments on loop control can be done for MethodsV2 implementation (this PR).

We should figure out a settings when the loop control is fired and add that as a unit test.

tgreenx · 2024-07-23T13:17:53Z

~~Changed versioning to V-Major since this PR now introduces optional arguments to existing public methods.~~

matsduf · 2024-07-23T13:20:59Z

Changed versioning to V-Major since this PR now introduces optional arguments to existing public methods.

@tgreenx, adding optional arguments is not breaking, is it? It should be V-Minor then.

matsduf · 2024-12-03T12:58:33Z

Review for release testing for v2024.2: no issues.

tgreenx added FA-MethodV2 V-Minor Versioning: The change gives an update of minor in version. labels Jul 10, 2024

tgreenx added this to the v2024.2 milestone Jul 10, 2024

tgreenx requested review from MichaelTimbert, marc-vanderwal, matsduf and mattias-p July 10, 2024 15:23

tgreenx mentioned this pull request Jul 10, 2024

Update MethodsV2 to match Basic01 zonemaster/zonemaster#1287

Merged

tgreenx force-pushed the update-methodsv2 branch 2 times, most recently from 7350c4b to 6d6a3b4 Compare July 10, 2024 15:52

tgreenx force-pushed the update-methodsv2 branch 2 times, most recently from 5ca62cc to f53a7fa Compare July 11, 2024 13:39

matsduf mentioned this pull request Jul 11, 2024

Updates methodsv2 unit tests #1351

Merged

tgreenx added 2 commits July 16, 2024 11:44

Convert indentation to spaces

6c5dc4e

tgreenx force-pushed the update-methodsv2 branch from f53a7fa to f5c66b4 Compare July 16, 2024 09:45

tgreenx added 3 commits July 16, 2024 15:36

Various fixes in TestMethodsV2, and refactoring

8e9d55a

Fix handling of CNAME followage outside of zone

79c8b01

tgreenx added 2 commits July 17, 2024 13:37

Fix return value of TestMethodsV2->get_del_ns_names()

706024b

Fix in TestMethodsV2->_get_delegation(), and minor updates to other m…

8e09e95

…odules - Fix conditionals in TestMethodsV2->_get_delegation() - Change IP address list to a set in Recursor->add_fake_addresses(), in order to avoid duplicates - Refactoring

tgreenx added 2 commits July 17, 2024 16:14

Fix unit test

44f44b4

Fix conditional in TestMethodsV2->_get_oob_ips(), refactoring

f392e86

Refactoring

dcbe30c

matsduf approved these changes Jul 18, 2024

View reviewed changes

tgreenx added V-Major Versioning: The change gives an update of major in version. and removed V-Minor Versioning: The change gives an update of minor in version. labels Jul 23, 2024

tgreenx merged commit a9654ee into zonemaster:develop Jul 23, 2024

tgreenx deleted the update-methodsv2 branch July 23, 2024 13:18

tgreenx added V-Minor Versioning: The change gives an update of minor in version. and removed V-Major Versioning: The change gives an update of major in version. labels Jul 23, 2024

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

Uh oh!

Conversation

tgreenx commented Jul 10, 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!

tgreenx commented Jul 10, 2024

Uh oh!

matsduf commented Jul 10, 2024

How to test this PR

Uh oh!

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

Uh oh!

Uh oh!

matsduf commented Jul 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

tgreenx commented Jul 11, 2024

Uh oh!

tgreenx commented Jul 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

matsduf commented Jul 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

matsduf commented Jul 12, 2024

Uh oh!

tgreenx commented Jul 17, 2024

Uh oh!

tgreenx commented Jul 17, 2024

Uh oh!

marc-vanderwal commented Jul 18, 2024

Uh oh!

matsduf commented Jul 18, 2024

Uh oh!

matsduf commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

matsduf commented Jul 23, 2024

Uh oh!

matsduf commented Dec 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tgreenx commented Jul 10, 2024 •

edited

Loading

matsduf commented Jul 11, 2024 •

edited

Loading

matsduf commented Jul 11, 2024 •

edited

Loading

tgreenx commented Jul 11, 2024 •

edited

Loading

tgreenx commented Jul 11, 2024 •

edited

Loading

matsduf commented Jul 18, 2024 •

edited

Loading

tgreenx commented Jul 23, 2024 •

edited

Loading