Implement testcase rewrite: Connectivity01 and Connectivity02 + remove Basic04 · Pull Request #1143 · zonemaster/zonemaster-engine

ghost · 2022-10-17T14:54:41Z

Purpose

Implementation of updated testcase specification for Connectivity01 (zonemaster/zonemaster#1097) and Connectivity02 (zonemaster/zonemaster#1098)
Remove Basic04 (zonemaster/zonemaster#1099).

Context

Addresses #1136 and #1137 and #905

Changes

Test/Connectivity.pm
t/Test-connectivity.t
Remove Basic04

How to test this PR

Should match specification and tests should pass (btw it would be nice to have more unit test with broken name servers).

marc-vanderwal

There are some minor issues.

That being said, I’ve compared both connectivity01 and connectivity02 functions in their new states and the only difference I see is the transport protocol (TCP vs. UDP) in use. Maybe there is an opportunity to be seized for deduplicating the code?

marc-vanderwal · 2022-10-18T07:07:13Z

By the way, your comment about missing test cases reminds me of #905.

matsduf · 2022-10-18T07:49:18Z

Can implementation of zonemaster/zonemaster#1099 be included in this PR too? It is only to remove Basic04. If so, I will merge it.

matsduf · 2022-11-03T12:53:19Z

@PNAX, can implementation of zonemaster/zonemaster#1099 be included in this PR too? It is only to remove Basic04. If so, I will merge it.

ghost · 2022-11-03T13:42:54Z

can implementation of zonemaster/zonemaster#1099 be included in this PR too? It is only to remove Basic04.

Yes, see 5860ffa

matsduf · 2022-11-03T13:55:10Z

can implementation of zonemaster/zonemaster#1099 be included in this PR too? It is only to remove Basic04.

Yes, see 5860ffa

I have now merged zonemaster/zonemaster#1099. Can you update the description of this PR with a reference to zonemaster/zonemaster#1099 too?

matsduf · 2022-11-03T14:27:30Z

Inconsistent behavior between IPv4/v6.

# zonemaster-cli iis.se --test connectivity/connectivity01 --level debug --show-testcase --raw --no-ipv4 | grep IPV4
   0.18 DEBUG     CONNECTIVITY01 SKIP_IPV4_DISABLED   ns=a.root-servers.net/198.41.0.4; rrtype=NS
   0.27 DEBUG     CONNECTIVITY01 SKIP_IPV4_DISABLED   ns=a.ns.se/192.36.144.107; rrtype=NS
   0.50 NOTICE    CONNECTIVITY01 CN01_IPV4_DISABLED   ns_list=nsa.dnsnode.net/194.58.192.46;nsp.dnsnode.net/194.58.198.32;nsu.dnsnode.net/185.42.137.98

# zonemaster-cli iis.se --test connectivity/connectivity01 --level debug --show-testcase --raw --no-ipv6 | grep IPV6
   0.69 NOTICE    CONNECTIVITY01 CN01_IPV6_DISABLED   ns_list=nsa.dnsnode.net/2a01:3f1:46::53;nsp.dnsnode.net/2a01:3f1:3032::53;nsu.dnsnode.net/2a01:3f0:400::32

# zonemaster-cli iis.se --test connectivity/connectivity02 --level debug --show-testcase --raw --no-ipv4 | grep IPV4
   0.18 DEBUG     CONNECTIVITY02 SKIP_IPV4_DISABLED   ns=a.root-servers.net/198.41.0.4; rrtype=NS
   0.27 DEBUG     CONNECTIVITY02 SKIP_IPV4_DISABLED   ns=a.ns.se/192.36.144.107; rrtype=NS
   0.50 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsa.dnsnode.net/194.58.192.46; rrtype=SOA
   0.50 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsa.dnsnode.net/194.58.192.46; rrtype=NS
   0.51 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsp.dnsnode.net/194.58.198.32; rrtype=SOA
   0.51 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsp.dnsnode.net/194.58.198.32; rrtype=NS
   0.52 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsu.dnsnode.net/185.42.137.98; rrtype=SOA
   0.52 DEBUG     CONNECTIVITY02 IPV4_DISABLED   ns=nsu.dnsnode.net/185.42.137.98; rrtype=NS

# zonemaster-cli iis.se --test connectivity/connectivity02 --level debug --show-testcase --raw --no-ipv6 | grep IPV6
   0.70 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsa.dnsnode.net/2a01:3f1:46::53; rrtype=SOA
   0.70 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsa.dnsnode.net/2a01:3f1:46::53; rrtype=NS
   0.72 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsp.dnsnode.net/2a01:3f1:3032::53; rrtype=SOA
   0.72 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsp.dnsnode.net/2a01:3f1:3032::53; rrtype=NS
   0.74 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsu.dnsnode.net/2a01:3f0:400::32; rrtype=SOA
   0.74 DEBUG     CONNECTIVITY02 IPV6_DISABLED   ns=nsu.dnsnode.net/2a01:3f0:400::32; rrtype=NS

ghost · 2022-11-03T15:17:43Z

Inconsistent behavior between IPv4/v6.

This inconsistency lies within the resolution and is the result of querying A and then AAAA records (and looping in the same order). This is not related to this PR.
Consequently I don't understand the change request. Please elaborate on it.

matsduf · 2022-11-04T14:58:43Z

This inconsistency lies within the resolution and is the result of querying A and then AAAA records (and looping in the same order). This is not related to this PR. Consequently I don't understand the change request. Please elaborate on it.

The tested domain, iis.se, has both IPv4 and IPv6 name server. When running Connectivity01 from this PR, why does Zonemaster output SKIP_IPV4_DISABLED when IPv4 is disabled but not SKIP_IPV6_DISABLED when IPv6 is disabled? And the same thing for Connectivity02.

ghost · 2022-11-07T10:48:24Z

why does Zonemaster output SKIP_IPV4_DISABLED when IPv4 is disabled but not SKIP_IPV6_DISABLED when IPv6 is disabled?

This is related to how Zonemaster proceeds with the resolution: A records have precedence over AAAA records. Such messages are displayed during DNS tree traversal only and not while testing the zone configuration, (see Zone.pm (develop/4ca9539b)).

This behavior is present in develop (grepping with some context):

$ git checkout develop
$ zonemaster-cli iis.se --test connectivity/connectivity01 --level debug --show-testcase --raw --no-ipv4 | grep -C 1 IPV4
   0.22 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN"}; ip=2620:10a:80ab::150; name=se; type=SOA
   0.23 DEBUG     CONNECTIVITY01 SKIP_IPV4_DISABLED   ns=a.root-servers.net/198.41.0.4; rrtype=NS
   0.23 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN"}; ip=2001:503:ba3e::2:30; name=se; type=NS
--
   0.31 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN"}; ip=2a01:3f0:0:301::53; name=se; type=NS
   0.36 DEBUG     CONNECTIVITY01 SKIP_IPV4_DISABLED   ns=a.ns.se/192.36.144.107; rrtype=NS
   0.36 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN"}; ip=2a01:3f0:0:301::53; name=iis.se; type=NS
--
   0.69 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN"}; ip=2a01:3f1:46::53; name=iis.se; type=NS
   0.84 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsa.dnsnode.net/194.58.192.46; rrtype=SOA
   0.84 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN","usevc":0}; ip=2a01:3f1:46::53; name=iis.se; type=SOA
   0.85 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsa.dnsnode.net/2a01:3f1:46::53
   0.85 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsp.dnsnode.net/194.58.198.32; rrtype=SOA
   0.85 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN","usevc":0}; ip=2a01:3f1:3032::53; name=iis.se; type=SOA
   0.86 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsp.dnsnode.net/2a01:3f1:3032::53
   0.86 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsu.dnsnode.net/185.42.137.98; rrtype=SOA
   0.86 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsu.dnsnode.net/2a01:3f0:400::32
   0.86 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsa.dnsnode.net/194.58.192.46; rrtype=SOA
   0.86 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsp.dnsnode.net/194.58.198.32; rrtype=SOA
   0.86 DEBUG     CONNECTIVITY01 IPV4_DISABLED   ns=nsu.dnsnode.net/185.42.137.98; rrtype=SOA
   0.87 DEBUG     CONNECTIVITY01 TEST_CASE_END   testcase=connectivity01

$ git checkout develop
$ zonemaster-cli iis.se --test connectivity/connectivity01 --level debug --show-testcase --raw --no-ipv6 | grep -C 1 IPV6
   0.77 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsa.dnsnode.net/194.58.192.46
   0.77 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsa.dnsnode.net/2a01:3f1:46::53; rrtype=SOA
   0.77 DEBUG     CONNECTIVITY01 EXTERNAL_QUERY   flags={"class":"IN","usevc":0}; ip=194.58.198.32; name=iis.se; type=SOA
   0.78 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsp.dnsnode.net/194.58.198.32
   0.78 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsp.dnsnode.net/2a01:3f1:3032::53; rrtype=SOA
   0.78 DEBUG     CONNECTIVITY01 NAMESERVER_HAS_UDP_53   ns=nsu.dnsnode.net/185.42.137.98
   0.78 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsu.dnsnode.net/2a01:3f0:400::32; rrtype=SOA
   0.78 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsa.dnsnode.net/2a01:3f1:46::53; rrtype=SOA
   0.79 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsp.dnsnode.net/2a01:3f1:3032::53; rrtype=SOA
   0.79 DEBUG     CONNECTIVITY01 IPV6_DISABLED   ns=nsu.dnsnode.net/2a01:3f0:400::32; rrtype=SOA
   0.79 DEBUG     CONNECTIVITY01 TEST_CASE_END   testcase=connectivity01

Spectification has been updated, see zonemaster/zonemaster#1097

Spectification has been updated, see zonemaster/zonemaster#1098

ghost · 2022-11-08T18:41:40Z

Maybe there is an opportunity to be seized for deduplicating the code?

Very true. I've refactored the code in that way.

To all reviewers, hopefully I've addressed everything.

tgreenx

Few comments other than that LGTM

tgreenx · 2022-11-09T09:36:49Z

-    my ( $class, $zone ) = @_;
-    push my @results, info( TEST_CASE_START => { testcase => (split /::/, (caller(0))[3])[-1] } );
-    my $query_type = q{SOA};
+sub _connectivity_loop {


Add an entry for that method in the documentation part of this file

matsduf

Looks good to me.

marc-vanderwal

Looks good to me.

matsduf · 2022-11-10T08:10:14Z

@tgreenx, could you re-review this?

matsduf · 2022-11-10T11:00:07Z

@PNAX, please merge.

marc-vanderwal · 2022-12-06T14:53:02Z

Unit tests pass. Additionally, I was able to elicit some of the more elusive messages (such as CN01_SOA_RECORD_NOT_AA_UDP) using IBDNS.

ghost added the A-TestCase Area: Test case specification or implementation of test case label Oct 17, 2022

ghost added this to the v2022.2 milestone Oct 17, 2022

ghost requested review from hannaeko, marc-vanderwal, matsduf, mattias-p and tgreenx October 17, 2022 14:54

marc-vanderwal requested changes Oct 18, 2022

View reviewed changes

marc-vanderwal reviewed Oct 18, 2022

View reviewed changes

Comment thread share/profile.json Outdated

matsduf requested changes Oct 18, 2022

View reviewed changes

This was linked to issues Nov 2, 2022

Implement updated Connectivity01 #1136

Closed

Implement updated Connectivity02 #1137

Closed

tgreenx reviewed Nov 2, 2022

View reviewed changes

tgreenx requested review from marc-vanderwal and matsduf November 2, 2022 10:58

matsduf requested changes Nov 3, 2022

View reviewed changes

Comment thread lib/Zonemaster/Engine/Test/Connectivity.pm Outdated

This was referenced Nov 3, 2022

Replace basic04 with connectivity01 zonemaster/zonemaster#1099

Merged

Rewrite of Connectivity01 (UDP connectivity) zonemaster/zonemaster#1097

Merged

Rewrite of Connectivity02 (TCP connectivity) zonemaster/zonemaster#1098

Merged

ghost changed the title ~~Implement testcase rewrite: Connectivity01 and Connectivity02~~ Implement testcase rewrite: Connectivity01 and Connectivity02 + remove Basic04 Nov 3, 2022

matsduf requested changes Nov 3, 2022

View reviewed changes

Comment thread lib/Zonemaster/Engine/Test/Connectivity.pm Outdated

Comment thread lib/Zonemaster/Engine/Test/Connectivity.pm Outdated

Alexandre Pion added 5 commits November 8, 2022 19:37

Update Connectivity01

b2a0d21

Spectification has been updated, see zonemaster/zonemaster#1097

Update Connectivity02

9436cf2

Spectification has been updated, see zonemaster/zonemaster#1098

Update unit test

275ecdb

Refactor unit test

ebcfd9c

Remove Basic04

1776ad7

tgreenx reviewed Nov 9, 2022

View reviewed changes

Refactor: deduplicate code

3c66c45

matsduf approved these changes Nov 9, 2022

View reviewed changes

marc-vanderwal approved these changes Nov 10, 2022

View reviewed changes

matsduf added the V-Minor Versioning: The change gives an update of minor in version. label Nov 10, 2022

tgreenx approved these changes Nov 10, 2022

View reviewed changes

ghost merged commit 67b1d18 into zonemaster:develop Nov 10, 2022

This was referenced Nov 10, 2022

Implement updated Connectivity01 #1136

Closed

Implement updated Connectivity02 #1137

Closed

marc-vanderwal added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 6, 2022

matsduf mentioned this pull request Jan 20, 2023

Remove Basic04 completely #1179

Merged

tgreenx mentioned this pull request May 16, 2023

Unit test for BASIC04 #905

Closed

tgreenx linked an issue May 16, 2023 that may be closed by this pull request

Unit test for BASIC04 #905

Closed

tgreenx mentioned this pull request Jun 15, 2023

Fix links zonemaster/zonemaster#1164

Merged

This pull request was closed.

Uh oh!

Conversation

ghost commented Oct 17, 2022 • edited by tgreenx 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!

marc-vanderwal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-vanderwal commented Oct 18, 2022

Uh oh!

Uh oh!

matsduf commented Oct 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 3, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matsduf commented Nov 3, 2022

Uh oh!

matsduf commented Nov 3, 2022

Uh oh!

ghost commented Nov 3, 2022

Uh oh!

Uh oh!

Uh oh!

matsduf commented Nov 4, 2022

Uh oh!

ghost commented Nov 7, 2022

Uh oh!

ghost commented Nov 8, 2022

Uh oh!

tgreenx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgreenx Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

marc-vanderwal left a comment

Choose a reason for hiding this comment

Uh oh!

matsduf commented Nov 10, 2022

Uh oh!

matsduf commented Nov 10, 2022

Uh oh!

marc-vanderwal commented Dec 6, 2022

ghost commented Oct 17, 2022 •

edited by tgreenx

Loading

matsduf commented Nov 3, 2022 •

edited

Loading

ghost commented Nov 3, 2022 •

edited by ghost

Loading