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

Update Basic01 #1212

Merged
merged 3 commits into from
May 23, 2023
Merged

Update Basic01 #1212

merged 3 commits into from
May 23, 2023

Conversation

tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Mar 21, 2023

Purpose

This PR proposes an update of Basic01 to the latest specification (zonemaster/zonemaster#1082).

TBD:

  • Update unitary tests

Context

Fixes #568

Changes

How to test this PR

$ zonemaster-cli --show-testcase --test basic/basic01 --level INFO zonemaster.net
Seconds Level     Testcase       Message
======= ========= ============== =======
   0.00 INFO      UNSPECIFIED    Using version v4.6.2 of the Zonemaster engine.
   4.27 INFO      BASIC01        The parent zone is "net." as returned from name servers "a.gtld-servers.net/192.5.6.30;a.gtld-servers.net/2001:503:a83e::2:30;b.gtld-servers.net/192.33.14.30;b.gtld-servers.net/2001:503:231d::2:30;c.gtld-servers.net/192.26.92.30;c.gtld-servers.net/2001:503:83eb::30;d.gtld-servers.net/192.31.80.30;d.gtld-servers.net/2001:500:856e::30;e.gtld-servers.net/192.12.94.30;e.gtld-servers.net/2001:502:1ca1::30;f.gtld-servers.net/192.35.51.30;f.gtld-servers.net/2001:503:d414::30;g.gtld-servers.net/192.42.93.30;g.gtld-servers.net/2001:503:eea3::30;h.gtld-servers.net/192.54.112.30;h.gtld-servers.net/2001:502:8cc::30;i.gtld-servers.net/192.43.172.30;i.gtld-servers.net/2001:503:39c1::30;j.gtld-servers.net/192.48.79.30;j.gtld-servers.net/2001:502:7094::30;k.gtld-servers.net/192.52.178.30;k.gtld-servers.net/2001:503:d2d::30;l.gtld-servers.net/192.41.162.30;l.gtld-servers.net/2001:500:d937::30;m.gtld-servers.net/192.55.83.30;m.gtld-servers.net/2001:501:b1f9::30".
   4.27 INFO      BASIC01        The zone "zonemaster.net" is found.

$ zonemaster-cli --show-testcase --test basic/basic01 --level INFO zonemaster.fr
Seconds Level     Testcase       Message
======= ========= ============== =======
   0.00 INFO      UNSPECIFIED    Using version v4.6.2 of the Zonemaster engine.
   2.19 INFO      BASIC01        The parent zone is "fr." as returned from name servers "d.nic.fr/194.0.9.1;d.nic.fr/2001:678:c::1;e.ext.nic.fr/193.176.144.22;e.ext.nic.fr/2a00:d78:0:102:193:176:144:22;f.ext.nic.fr/194.146.106.46;f.ext.nic.fr/2001:67c:1010:11::53;g.ext.nic.fr/194.0.36.1;g.ext.nic.fr/2001:678:4c::1".
   2.19 INFO      BASIC01        The zone "zonemaster.fr" is found.

$ zonemaster-cli --show-testcase --test basic/basic01 --level INFO google.com
Seconds Level     Testcase       Message
======= ========= ============== =======
   0.00 INFO      UNSPECIFIED    Using version v4.6.2 of the Zonemaster engine.
   3.92 INFO      BASIC01        The parent zone is "com." as returned from name servers "a.gtld-servers.net/192.5.6.30;a.gtld-servers.net/2001:503:a83e::2:30;b.gtld-servers.net/192.33.14.30;b.gtld-servers.net/2001:503:231d::2:30;c.gtld-servers.net/192.26.92.30;c.gtld-servers.net/2001:503:83eb::30;d.gtld-servers.net/192.31.80.30;d.gtld-servers.net/2001:500:856e::30;e.gtld-servers.net/192.12.94.30;e.gtld-servers.net/2001:502:1ca1::30;f.gtld-servers.net/192.35.51.30;f.gtld-servers.net/2001:503:d414::30;g.gtld-servers.net/192.42.93.30;g.gtld-servers.net/2001:503:eea3::30;h.gtld-servers.net/192.54.112.30;h.gtld-servers.net/2001:502:8cc::30;i.gtld-servers.net/192.43.172.30;i.gtld-servers.net/2001:503:39c1::30;j.gtld-servers.net/192.48.79.30;j.gtld-servers.net/2001:502:7094::30;k.gtld-servers.net/192.52.178.30;k.gtld-servers.net/2001:503:d2d::30;l.gtld-servers.net/192.41.162.30;l.gtld-servers.net/2001:500:d937::30;m.gtld-servers.net/192.55.83.30;m.gtld-servers.net/2001:501:b1f9::30".
   3.92 INFO      BASIC01        The zone "google.com" is found.

$ zonemaster-cli --show-testcase --test basic/basic01 --level INFO xn--mori-qsa.nz
Seconds Level     Testcase       Message
======= ========= ============== =======
   0.00 INFO      UNSPECIFIED    Using version v4.6.2 of the Zonemaster engine.
  11.80 INFO      BASIC01        The parent zone is "nz." as returned from name servers "ns1.dns.net.nz/2001:dce:2000:2::130;ns1.dns.net.nz/202.46.190.130;ns2.dns.net.nz/2001:dce:7000:2::130;ns2.dns.net.nz/202.46.187.130;ns3.dns.net.nz/2001:dce:d453::53;ns3.dns.net.nz/202.46.188.130;ns4.dns.net.nz/2001:dce:d454::53;ns4.dns.net.nz/202.46.189.130;ns5.dns.net.nz/185.159.197.130;ns5.dns.net.nz/2620:10a:80aa::130;ns6.dns.net.nz/185.159.198.130;ns6.dns.net.nz/2620:10a:80ab::130;ns7.dns.net.nz/194.146.106.54;ns7.dns.net.nz/2001:67c:1010:13::53".
  11.80 ERROR     BASIC01        "xn--mori-qsa.nz" does not exist as a DNS zone. Try to test "nz" instead.
  11.80 NOTICE    BASIC01        "xn--mori-qsa.nz" is not a zone. It is an alias for "maori.nz.". Run a test for "maori.nz." instead. Returned from name servers "ns1.dns.net.nz/2001:dce:2000:2::130;ns1.dns.net.nz/202.46.190.130;ns2.dns.net.nz/2001:dce:7000:2::130;ns2.dns.net.nz/202.46.187.130;ns3.dns.net.nz/2001:dce:d453::53;ns3.dns.net.nz/202.46.188.130;ns4.dns.net.nz/2001:dce:d454::53;ns4.dns.net.nz/202.46.189.130;ns5.dns.net.nz/185.159.197.130;ns5.dns.net.nz/2620:10a:80aa::130;ns6.dns.net.nz/185.159.198.130;ns6.dns.net.nz/2620:10a:80ab::130;ns7.dns.net.nz/194.146.106.54;ns7.dns.net.nz/2001:67c:1010:13::53.
   
$ zonemaster-cli --show-testcase --test basic/basic01 --level INFO testonly.sources.org
Seconds Level     Testcase       Message
======= ========= ============== =======
   0.00 INFO      UNSPECIFIED    Using version v4.6.2 of the Zonemaster engine.
   3.39 INFO      BASIC01        The parent zone is "sources.org." as returned from name servers "ns1.bortzmeyer.org/2602:fbb1:1:245b::42;ns1.bortzmeyer.org/80.77.95.49;ns4.bortzmeyer.org/2001:4b98:dc0:41:216:3eff:fe27:3d3f;ns4.bortzmeyer.org/92.243.4.211;ns6.gandi.net/2001:4b98:d:1::4b;ns6.gandi.net/217.70.177.42".
   3.39 ERROR     BASIC01        "testonly.sources.org" does not exist as a DNS zone. Try to test "sources.org" instead.
   3.40 NOTICE    BASIC01        "testonly.sources.org" is not a zone. It is an alias for "example.org.". Run a test for "example.org." instead. Returned from name servers "ns1.bortzmeyer.org/2602:fbb1:1:245b::42;ns1.bortzmeyer.org/80.77.95.49;ns4.bortzmeyer.org/2001:4b98:dc0:41:216:3eff:fe27:3d3f;ns4.bortzmeyer.org/92.243.4.211;ns6.gandi.net/2001:4b98:d:1::4b;ns6.gandi.net/217.70.177.42.

@tgreenx tgreenx added T-Feature Type: New feature in software or test case description A-TestCase Area: Test case specification or implementation of test case V-Major Versioning: The change gives an update of major in version. labels Mar 21, 2023
@tgreenx tgreenx added this to the v2023.1 milestone Mar 21, 2023
@tgreenx tgreenx force-pushed the update-basic01 branch 4 times, most recently from 5531584 to b2bda3d Compare March 22, 2023 11:27
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

This test case requires several test zones.

lib/Zonemaster/Engine/Test/Basic.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/Basic.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/Basic.pm Outdated Show resolved Hide resolved
@matsduf
Copy link
Contributor

matsduf commented Mar 23, 2023

B01_INCONSISTENT_DELEGATION is not implemented. I do not feel that all logic in the test case is implemented.

@tgreenx tgreenx added V-Minor Versioning: The change gives an update of minor in version. and removed T-Feature Type: New feature in software or test case description V-Major Versioning: The change gives an update of major in version. labels Apr 5, 2023
@tgreenx tgreenx changed the title Add DNAME support, and update Basic01 Update Basic01 Apr 5, 2023
@tgreenx
Copy link
Contributor Author

tgreenx commented Apr 5, 2023

As discussed, I have split this PR in two: DNAME implementation is now done in #1213, while this PR deals with the implementation update of Basic01.

Note that commit 3bfc375 is a necessary dependency for this PR. It relates to #1213, which will have to be merged first before this PR. Please ignore it and only review subsequent commits in this PR.

@tgreenx
Copy link
Contributor Author

tgreenx commented Apr 5, 2023

[ ... ] I do not feel that all logic in the test case is implemented.

Indeed, this first draft was a minimal implementation, because most of the logic in new Basic01 was already done in Engine::Recursor and I wanted to avoid redundancy. But it is not enough if we want to have a fully compliant implementation so I will provide an update.

@matsduf
Copy link
Contributor

matsduf commented Apr 9, 2023

For BASIC01 no advance DNAME functions are needed. The only test that is required is if a DNAME record with Child Zone as owner name is present in the answer section in the response to a DNAME query for Child Zone. The target is to be captured but not followed.

There must not be more than one DNAME record in a node (as with CNAME), and for BASIC01 I think we can ignore multiple DNAME records. I think it is unlikely that there is a case when multiple DNAME record with the same owner name are included. Should we still capture the case? Then we should update the test case and create a new message for that.

@tgreenx
Copy link
Contributor Author

tgreenx commented Apr 18, 2023

[ ... ] I do not feel that all logic in the test case is implemented.

Indeed, this first draft was a minimal implementation, because most of the logic in new Basic01 was already done in Engine::Recursor and I wanted to avoid redundancy. But it is not enough if we want to have a fully compliant implementation so I will provide an update.

@matsduf This implementation is now ready for review and should be fully compliant to the specification.
I have done basic, manual testing but more unitary tests (see TODO section in file t/Test-basic.t) are needed to have full testing coverage.

@tgreenx tgreenx requested a review from mattias-p April 18, 2023 13:24
@matsduf
Copy link
Contributor

matsduf commented Apr 18, 2023

I have started to review.

@tgreenx tgreenx linked an issue May 16, 2023 that may be closed by this pull request
matsduf
matsduf previously approved these changes May 22, 2023
@tgreenx
Copy link
Contributor Author

tgreenx commented May 22, 2023

@matsduf I have rebased on latest develop, as well as fixed minor things, could you re-approve?

@tgreenx tgreenx requested a review from matsduf May 22, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case V-Minor Versioning: The change gives an update of minor in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update implementation of Basic01
2 participants