Skip to content

Fix Basic01 #1270

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.

Sign up for GitHub

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

Jump to bottom
Merged
tgreenx merged 1 commit into from
Aug 7, 2023
Merged

Fix Basic01 #1270

tgreenx merged 1 commit into from
Aug 7, 2023
+37 −30

Conversation

@tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Aug 2, 2023 •
edited
Loading

Purpose

This PR updates Basic01 to fix a bug (regression) introduced in the latest fix release by PR #1249.

Context

Fixes #1269

Changes

How to test this PR

Tests should pass.

Also:

$ zonemaster-cli --show-testcase --test Basic/basic01 --level INFO norid.no
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.02 INFO     UNSPECIFIED    Using version v4.7.1 of the Zonemaster engine.
   9.65 INFO     BASIC01        The parent zone is "no" as returned from name servers "i.nic.no/194.146.106.6;i.nic.no/2001:67c:1010:1::53;njet.norid.no/156.154.101.12;not.norid.no/156.154.100.12;not.norid.no/2001:502:ad09::12;x.nic.no/128.39.8.40;x.nic.no/2001:700:0:412f::40;y.nic.no/193.75.4.22;y.nic.no/2001:8c0:8200:1::2;z.nic.no/158.38.8.133;z.nic.no/2001:700:0:52d:158:38:8:133".
   9.65 INFO     BASIC01        The zone "norid.no" is found.

@tgreenx tgreenx added A-TestCase Area: Test case specification or implementation of test case V-Patch Versioning: The change gives an update of patch in version. labels Aug 2, 2023
@tgreenx tgreenx added this to the v2023.1.3 milestone Aug 2, 2023
@tgreenx tgreenx linked an issue Aug 2, 2023 that may be closed by this pull request
False error in BASIC01 (B01_NO_CHILD) when testing norid.no #1269
Closed
@tgreenx tgreenx mentioned this pull request Aug 2, 2023
Closed
@tgreenx
Copy link
Contributor Author

tgreenx commented Aug 2, 2023

@matsduf I can see that branch releases/v2023.1.3 is based on top of master and not develop, is that intentional? (just making sure)

@matsduf
Copy link
Contributor

matsduf commented Aug 2, 2023

@matsduf I can see that branch releases/v2023.1.3 is based on top of master and not develop, is that intentional? (just making sure)

Yes, that is intentional. v2023.1.3 should not include other things that have been merged to develop branch after the latest release, only the fixes for that fix release.

@tgreenx
Copy link
Contributor Author

tgreenx commented Aug 2, 2023 •
edited
Loading

Unit tests are failing (on Ubuntu), while it works fine on my computer (Debian). I can see where the error is supposed to be from the logs but unsure as to why it happens yet.

@tgreenx
Fix Basic01 (zonemaster#1269)
fcf2d37 
Commit f1354e1 (zonemaster#1249) introduced a bug (regression) when comparing domain names. It was using the function 'index()' to make such a comparison, on names that were no longer in their FQDN form. Thus, any domain name beginning with the characters of its TLD (e.g. 'norid.no') would wrongly be discarded. But even if that bug was reverted, it could still be problematic for multi-level zones, e.g. 'no.norid.no'.
So, instead of relying on a pure Perl function that does string comparison, in this commit we make use of the Zonemaster::Engine::DNSName class, specifically its functions 'common()' and 'is_in_bailiwick', to count and compare the labels composing domain names.
@tgreenx
Copy link
Contributor Author

tgreenx commented Aug 2, 2023 •
edited
Loading

Unit tests are failing (on Ubuntu), while it works fine on my computer (Debian). I can see where the error is supposed to be from the logs but unsure as to why it happens yet.

So the error was due to the fact that chained comparisons support was only added in Perl v5.32 (see https://metacpan.org/release/XSAWYERX/perl-5.32.0/view/pod/perldelta.pod#Chained-comparisons-capability). I have provided an update to the code in that regard.

matsduf
matsduf reviewed Aug 3, 2023
View reviewed changes
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.

I have installed and tested the new code and it seems to behave correctly. The code of method basic01() is quite hard to follow. Could it be give some more documentation to make it easier to follow it?

@ghost ghost mentioned this pull request Aug 3, 2023
Closed
@tgreenx
Copy link
Contributor Author

tgreenx commented Aug 3, 2023

I have installed and tested the new code and it seems to behave correctly. The code of method basic01() is quite hard to follow. Could it be give some more documentation to make it easier to follow it?

I think we can proceed as is. The code is following the specification to the letter, so I don't know what more I can add in that regard.

@tgreenx tgreenx merged commit bc48a31 into zonemaster:releases/v2023.1.3 Aug 7, 2023
@tgreenx tgreenx deleted the fix-basic01 branch August 7, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matsduf matsduf matsduf left review comments

@mattias-p mattias-p Awaiting requested review from mattias-p

@hannaeko hannaeko Awaiting requested review from hannaeko

@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-TestCase Area: Test case specification or implementation of test case V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Milestone

v2023.1.3

Development

Successfully merging this pull request may close these issues.

False error in BASIC01 (B01_NO_CHILD) when testing norid.no

2 participants

@tgreenx @matsduf