Update Connectivity04 implementation by tgreenx · Pull Request #1393 · zonemaster/zonemaster-engine

tgreenx · 2024-10-22T16:09:40Z

Purpose

This PR proposes an update to the implementation of Connectivity04 following a specification update. As a drive by change, this also updates the t/TestUtil.pm module to add support for dynamic scenario testing for test cases.

Note that this is the first implementation to make use of MethodsV2.

Context

Test case specification update: zonemaster/zonemaster#1298

Test zones specification: zonemaster/zonemaster#1299

Changes

lib/Zonemaster/Engine/ASNLookup.pm
lib/Zonemaster/Engine/Test/Connectivity.pm
t/Test-connectivity04.t
t/Test-connectivity04.data
t/Test-connectivity04-A.t
t/Test-connectivity04-A.data
t/TestUtil.pm

How to test this PR

Unit tests are updated (based on zonemaster/zonemaster#1299) and should pass.

matsduf

When zonemaster/zonemaster#1298 has been fixed I will update zonemaster/zonemaster#1299 and test this implementation.

…() of the TestUtil module

matsduf · 2024-10-24T15:01:29Z

Note that scenario ERROR-PREFIX-DATABASE-3 is currently disabled because since its query ends up blacklisted (no response), it causes subsequent queries to be blacklisted as well. I'll look for a workaround.

If that scenario is run by a separate t file it will not affect other scenarios. Isn't that the simple workaround? I do not think that the code should handle ASN lookup queries special in that sense.

matsduf · 2024-10-24T15:29:28Z

Should there be double space between tag and argument?

$ zonemaster-cli --hint=hintfile.zone --test=connectivity04 --level=info --show-testcase --raw GOOD-1.connectivity04.xa                                                                                        
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0                                                                                                                                                 
   0.23 INFO     Connectivity04 CN04_IPV4_DIFFERENT_PREFIX  ns_list=dns0.connectivity04.xa/127.100.100.1;dns1.connectivity04.xa/127.100.101.1                                                                  
   0.23 INFO     Connectivity04 CN04_IPV6_DIFFERENT_PREFIX  ns_list=dns0.connectivity04.xa/fda1:b2:c3:0:127:100:100:1;dns1.connectivity04.xa/fda1:b2:c3:0:127:100:101:1

matsduf · 2024-10-24T15:36:48Z

I tested a domain that does not exist and got errors from Perl. It is not a big problem, but better if we can avoid it.

$ zonemaster-cli --hint=hintfile.zone --test=connectivity04 --level=info --show-testcase --raw ERROR-PREFIX-DATABASE-4.connectivity04.xa
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
   0.04 CRITICAL Unspecified    MODULE_ERROR  module=Connectivity; msg=Can't use an undefined value as an ARRAY reference at /usr/local/share/perl/5.34.0/Zonemaster/Engine/Test/Connectivity.pm line 807.

matsduf · 2024-10-24T15:50:08Z

Tag without argument, but with two trailing space characters, CN04_IPV4_SINGLE_PREFIX .

matsduf

Scenario name	Mandatory message tag	Forbidden message tags
ERROR-PREFIX-DATABASE-6	CN04_IPV4_DIFFERENT_PREFIX, CN04_IPV6_DIFFERENT_PREFIX, CN04_ERROR_PREFIX_DATABASE	2)

$ zonemaster-cli --hint=hintfile.zone --test=connectivity04 --level=info --show-testcase --raw ERROR-PREFIX-DATABASE-6.connectivity04.xa                                                                       
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0                                                                                                                                                 
   0.15 INFO     Connectivity04 CN04_IPV4_DIFFERENT_PREFIX  ns_list=dns1.connectivity04.xa/127.100.101.1;dns25.connectivity04.xa/127.100.125.1                                                                 
   0.15 INFO     Connectivity04 CN04_IPV6_DIFFERENT_PREFIX  ns_list=dns1.connectivity04.xa/fda1:b2:c3:0:127:100:101:1;dns25.connectivity04.xa/fda1:b2:c3:0:127:100:125:1

--> Unexpected

Two TXT are returned and one of them does not have a prefix matching the queried IP address.

;; ANSWER SECTION:
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.125.0/24 | NA | NA | NA"
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.255.0/24 | NA | NA | NA"

- Check for definedness before looping over name servers This can happen with the new methods (TestMethodsV2), for example when a domain does not exist - Move test scenario ERROR-PREFIX-DATABASE-3 to a separate unit test file Since queries for this scenario time out (no response), this is needed as to avoid the blacklisting of subsequent queries

tgreenx · 2024-10-29T16:17:29Z

Note that scenario ERROR-PREFIX-DATABASE-3 is currently disabled because since its query ends up blacklisted (no response), it causes subsequent queries to be blacklisted as well. I'll look for a workaround.

If that scenario is run by a separate t file it will not affect other scenarios. Isn't that the simple workaround? I do not think that the code should handle ASN lookup queries special in that sense.

Yes, I will do that.

Should there be double space between tag and argument?

$ zonemaster-cli --hint=hintfile.zone --test=connectivity04 --level=info --show-testcase --raw GOOD-1.connectivity04.xa                                                                                        
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0                                                                                                                                                 
   0.23 INFO     Connectivity04 CN04_IPV4_DIFFERENT_PREFIX  ns_list=dns0.connectivity04.xa/127.100.100.1;dns1.connectivity04.xa/127.100.101.1                                                                  
   0.23 INFO     Connectivity04 CN04_IPV6_DIFFERENT_PREFIX  ns_list=dns0.connectivity04.xa/fda1:b2:c3:0:127:100:100:1;dns1.connectivity04.xa/fda1:b2:c3:0:127:100:101:1

Unless I misunderstand, this is unrelated to this PR. This is also the case for other Test cases.

I tested a domain that does not exist and got errors from Perl. It is not a big problem, but better if we can avoid it.

$ zonemaster-cli --hint=hintfile.zone --test=connectivity04 --level=info --show-testcase --raw ERROR-PREFIX-DATABASE-4.connectivity04.xa
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
   0.04 CRITICAL Unspecified    MODULE_ERROR  module=Connectivity; msg=Can't use an undefined value as an ARRAY reference at /usr/local/share/perl/5.34.0/Zonemaster/Engine/Test/Connectivity.pm line 807.

I agree, fixed.

Tag without argument, but with two trailing space characters, CN04_IPV4_SINGLE_PREFIX .

I don't see the issue. Could you provide an example? I get:

$ zonemaster-cli --hint=../zonemaster/test-zone-data/Connectivity-TP/connectivity04/hintfile.zone --test=connectivity04 --level=info --show-testcase --raw IPV4-SINGLE-NS-1.connectivity04.xa
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v6.0.0
   0.07 INFO     Connectivity04 CN04_IPV4_DIFFERENT_PREFIX  ns_list=dns19.connectivity04.xa/127.100.119.1
   0.07 WARNING  Connectivity04 CN04_IPV4_SINGLE_PREFIX

$ zonemaster-cli --hint=../zonemaster/test-zone-data/Connectivity-TP/connectivity04/hintfile.zone --test=connectivity04 --level=info --show-testcase IPV4-SING
LE-NS-1.connectivity04.xa

Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     Unspecified    Using version v6.0.0 of the Zonemaster engine.
   0.11 INFO     Connectivity04 The following name server(s) are announced in unique IPv4 prefix(es): "dns19.connectivity04.xa/127.100.119.1"
   0.11 WARNING  Connectivity04 All name server(s) IPv4 address(es) are announced in the same IPv4 prefix.

[ ... ]

--> Unexpected

Two TXT are returned and one of them does not have a prefix matching the queried IP address.
;; ANSWER SECTION:
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.125.0/24 | NA | NA | NA"
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.255.0/24 | NA | NA | NA"

I believe this one is a bit tricky from a specification standpoint. We have both a valid and invalid record. Shouldn't we care more about returning the correct one? Moreover the network mask is the same in both cases, which makes it even more of an edge case. Finally, for this scenario this will lead to output of message tags CN04_IPV{4,6}_SINGLE_PREFIX.
See https://github.com/zonemaster/zonemaster/blob/b38a3bd9acf37c55211117ff387c2220fec58fd0/docs/public/specifications/tests/Connectivity-TP/connectivity04.md?plain=1#L173-L187.

matsduf · 2024-10-30T08:38:05Z

Unless I misunderstand, this is unrelated to this PR. This is also the case for other Test cases.

Not an issue for this PR.

I don't see the issue. Could you provide an example? I get:

Just trailing spaces I observed. Nothing important.

--> Unexpected
Two TXT are returned and one of them does not have a prefix matching the queried IP address.
;; ANSWER SECTION:
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.125.0/24 | NA | NA | NA"
1.125.100.127.origin.asnlookup.zonemaster.net. 3600 IN TXT "64512 | 127.100.255.0/24 | NA | NA | NA"
I believe this one is a bit tricky from a specification standpoint. We have both a valid and invalid record. Shouldn't we care more about returning the correct one? Moreover the network mask is the same in both cases, which makes it even more of an edge case. Finally, for this scenario this will lead to output of message tags CN04_IPV{4,6}_SINGLE_PREFIX. See https://github.com/zonemaster/zonemaster/blob/b38a3bd9acf37c55211117ff387c2220fec58fd0/docs/public/specifications/tests/Connectivity-TP/connectivity04.md?plain=1#L173-L187.

If more than one ASN TXT record is returned then I think it is reasonable to see that as a set. If one is more specific than the other, and both are compatible with the address, that is fine. If one record is not compatible with address, but the other is, then I think it is reasonable to say that the set is broken. That is what I think the specification says, and is meant to say:

If Input IP does not match the extracted subnet, output
[CN04_ERROR_PREFIX_DATABASE], break the processing of TXT records and
exit this loop without returning any prefix.

matsduf · 2024-10-30T08:44:42Z

Moreover the network mask is the same in both cases, which makes it even more of an edge case.

I do not understand why the size of the mask matters. One prefix covers the address, the other does not.

- Fix logic when outputting of CN04_IPV{4,6}_SINGLE_PREFIX - Update mandatory message tags in test scenario ERROR-PREFIX-DATABASE-6

matsduf · 2024-10-30T10:56:57Z

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

marc-vanderwal

Looks good to me.

matsduf · 2024-12-06T23:03:41Z

Release testing for 2024.2: No issues.

tgreenx added 3 commits October 22, 2024 10:38

Update Connectivity04 implementation

2a5a692

Update Cymru ASN lookup implementation

032c06f

Add unit tests and data for Connectivity04

e4184b3

tgreenx added the A-TestCase Area: Test case specification or implementation of test case label Oct 22, 2024

tgreenx added this to the v2024.2 milestone Oct 22, 2024

tgreenx requested review from MichaelTimbert, marc-vanderwal, matsduf and mattias-p October 22, 2024 16:09

tgreenx added the FA-MethodV2 label Oct 22, 2024

Update Connectivity module unit test data

c2e37a0

tgreenx mentioned this pull request Oct 23, 2024

Updates Connectivity04 zonemaster/zonemaster#1298

Merged

matsduf reviewed Oct 23, 2024

View reviewed changes

Comment thread t/Test-connectivity04.t

tgreenx added 2 commits October 24, 2024 09:56

Add support for dynamic scenarios testing in perform_testcase_testing…

b276cf8

…() of the TestUtil module

Fix Cymru ASN lookup implementation

7abfe9c

tgreenx requested a review from matsduf October 24, 2024 08:45

matsduf requested changes Oct 24, 2024

View reviewed changes

tgreenx requested a review from matsduf October 29, 2024 16:18

tgreenx linked an issue Oct 29, 2024 that may be closed by this pull request

CONNECTIVITY04 - IP prefix diversity requirement zonemaster/zonemaster#1174

Closed

matsduf reviewed Oct 30, 2024

View reviewed changes

Comment thread t/Test-connectivity04.t Outdated

Updates after review for Connectivity04

f4158c0

- Fix logic when outputting of CN04_IPV{4,6}_SINGLE_PREFIX - Update mandatory message tags in test scenario ERROR-PREFIX-DATABASE-6

tgreenx requested a review from matsduf October 30, 2024 10:17

matsduf previously approved these changes Oct 30, 2024

View reviewed changes

matsduf reviewed Nov 2, 2024

View reviewed changes

Comment thread t/Test-connectivity04.t Outdated

matsduf mentioned this pull request Nov 2, 2024

New unit tests for Delegation01, -02 and -03, and legacy tests are removed #1395

Merged

Rename unit test files to the current naming convention

a9a0b85

tgreenx dismissed matsduf’s stale review via a9a0b85 November 6, 2024 12:37

tgreenx requested a review from matsduf November 6, 2024 12:42

Update link of the specific hint file

18b7317

matsduf approved these changes Nov 6, 2024

View reviewed changes

marc-vanderwal approved these changes Nov 7, 2024

View reviewed changes

matsduf added V-Minor Versioning: The change gives an update of minor in version. V-Patch Versioning: The change gives an update of patch in version. and removed V-Minor Versioning: The change gives an update of minor in version. labels Nov 8, 2024

tgreenx merged commit 1b8482e into zonemaster:develop Nov 12, 2024

tgreenx deleted the update-connectivity04 branch November 12, 2024 11:11

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

Uh oh!

Conversation

tgreenx commented Oct 22, 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!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matsduf commented Oct 24, 2024

Uh oh!

matsduf commented Oct 24, 2024

Uh oh!

matsduf commented Oct 24, 2024

Uh oh!

matsduf commented Oct 24, 2024

Uh oh!

matsduf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgreenx commented Oct 29, 2024

Uh oh!

matsduf commented Oct 30, 2024

Uh oh!

matsduf commented Oct 30, 2024

Uh oh!

Uh oh!

matsduf commented Oct 30, 2024

Uh oh!

Uh oh!

marc-vanderwal left a comment

Choose a reason for hiding this comment

Uh oh!

matsduf commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tgreenx commented Oct 22, 2024 •

edited

Loading

matsduf left a comment •

edited

Loading