Remove some unintended network dependencies from the unit tests #1045

mattias-p · 2022-03-23T09:29:50Z

Purpose

This PR makes the unit tests don't autocomplete fake glue by sending out DNS requests.

Context

Fixes #888.

Changes

A new method Zonemaster::Engine->add_fake_delegation_raw() is factored out from Zonemaster::Engine->add_fake_delegation(). The new method performs the meat of things but without the bells and whistles like autocompleting fake glue and returning a boolean. (I don't think the return value is ever used and it's kind of nonsense anyway. Maybe I should have removed it altogether.)

How to test this PR

In one terminal run sudo tcpdump -nn port 53 and keep it open.
In another terminal run make test and wait for it to complete.
Make sure tcpdump didn't list any DNS requests from the tests.

* Make unit tests not rely on fake glue for out-of-bailiwick nameservers being autocompleted. * Make unit tests add fake delegation information using a method that never attempts to autocomplete glue.

matsduf · 2022-03-25T08:41:54Z

If a "fake delegation" is something like

        'lysator.liu.se' => {
            'ns1.nic.fr' => [],
            'ns.nic.se'  => []
        }

then the IP address or addresses of ns1.nic.fr and ns.nic.se should be looked up on a resolver (built-in or external). As I read it add_fake_delegation_raw() will instead output 'FAKE_DELEGATION_NO_IP'. Do I read it wrong?

matsduf · 2022-03-25T09:26:05Z

In a second inspection, I think the idea is to use add_fake_delegation_raw() for unit tests and add_fake_delegation() else. Is that correct?

matsduf

There is also a Zonemaster::Engine::Nameserver::add_fake_delegation() but I cannot see that it is used.

matsduf · 2022-03-25T09:32:19Z

lib/Zonemaster/Engine.pm

+    if ( $incomplete_delegation ) {
+        return;
+    }


What is the purpose of this test? Does not add_fake_delegation_raw doing that job?

This is one of the differences between add_fake_delegation() and add_fake_delegation_raw(). The former has a boolean return type and the latter has a void return type. I kept the return value semantics of add_fake_delegation() unchanged for compatibility reasons. However I can't see any use cases for the return value. Maybe it would be better to just remove it.

matsduf

The two methods, add_fake_delegation_raw() and add_fake_delegation(), have very much overlap in logic. Why not have just one method and a input variable, say $raw, and if that is set it does not do any lookups for IP addresses? I think that would simplify the code and the understanding of the code.

mattias-p · 2022-03-31T06:44:06Z

The two methods, add_fake_delegation_raw() and add_fake_delegation(), have very much overlap in logic. Why not have just one method and a input variable, say $raw, and if that is set it does not do any lookups for IP addresses?

Good point. I'll change it to use some kind of flag instead.

matsduf · 2022-05-16T10:07:57Z

lib/Zonemaster/Engine.pm

+The only recognized flag is C<fill_in_empty_ib>.
+This flag is boolean and defaults to true.
+If this flag is true, this method updates the given C<$data> by looking up and
+filling in some glue addresses.
+Specifically the glue addresses for any nameserver name that are out of
+bailiwick of the given C<$domain> and that comes with an empty list of
+addresses.


If the name sever name is an strict in-bailiwick then a missing address should never be looked up. What is "ib" in the flag name? Wouldn't "fill_in_empty_ob_glue" be better?

I suggest that the terms in-bailiwick and out-of-bailiwick (hyphened) are used (https://datatracker.ietf.org/doc/html/rfc8499)

I'll make updates.

matsduf · 2022-05-16T10:15:15Z

lib/Zonemaster/Engine.pm

+Returns `1` if all name servers in C<$data> have non-empty lists of
+glue (after they've been filled in) or if `fill_in_empty_ib` is false.
+Otherwise it returns `undef`.
+
+Examples:

    Zonemaster::Engine->add_fake_delegation(
        'lysator.liu.se' => {
            'ns1.nic.fr' => [ ],
            'ns.nic.se'  => [ '212.247.7.228',  '2a00:801:f0:53::53' ],
            'i.ns.se'    => [ '194.146.106.22', '2001:67c:1010:5::53' ],
            'ns3.nic.se' => [ '212.247.8.152',  '2a00:801:f0:211::152' ]
-        }
+        },
    );

-will return 1.
+returns 1.


I think the specification says that undef should be returned, depending on what you mean by glue. The se zone should never include glue for ns1.nic.fr in this case, but I am not sure that add_fake_delegation judge that.

Yeah, I don't think the return value is meaningful at all. I only kept it out of the principle to minimize API changes. And I only documented it out of a sense of duty. I was thinking I'd remove the return value entirely in a follow-up PR, but I might as well do it in this one. WDYT?

Here I guess fail and success are the possible outcome, if anything is missing should be determined by the testing.

Fail: The data does not match the API for this method and cannot be loaded.

Success: The data matches the API for this method and can be loaded.

Data can be with or without IP address. I guess data can be with our without name server too.

Do you know if we have other breaking changes in Engine? I looked but I couldn't see anything obvious. I'd rather not force a major version number bump just for this.

matsduf · 2022-05-16T10:17:24Z

lib/Zonemaster/Engine.pm

+    if ( $fill_in_empty_ib ) {
+        foreach my $name ( keys %{$href} ) {
+            if ( !@{ $href->{$name} } ) {
+                if ( !$class->zone( $domain )->is_in_zone( $name ) ) {
+                    my @ips = Zonemaster::LDNS->new->name2addr($name);
+                    push @{ $href->{$name} }, @ips;
+                }
+                if ( !@{ $href->{$name} } ) {
+                    $incomplete_delegation = 1;
+                }
+            }
+        }
+    }


Will this fill in a strict in-bailiwick name server's IP address? It should not.

It does not. However misnamed the flag. I'll fix that.

matsduf · 2022-05-16T10:18:22Z

The milestone is till set to v2022.2.

tgreenx

Two comments else lgtm, ready to approve afterwards

lib/Zonemaster/Engine.pm

mattias-p added 5 commits March 23, 2022 10:04

Fail early

f24c0e7

Update documentation for add_fake_delegation()

9185c00

Factor out add_fake_delegation_raw()

bd20310

Refactor

abf0165

Don't send DNS requests when no_network is true

1c057dc

* Make unit tests not rely on fake glue for out-of-bailiwick nameservers being autocompleted. * Make unit tests add fake delegation information using a method that never attempts to autocomplete glue.

mattias-p added this to the v2022.1 milestone Mar 23, 2022

matsduf self-requested a review March 25, 2022 08:42

matsduf reviewed Mar 25, 2022

View reviewed changes

matsduf requested changes Mar 25, 2022

View reviewed changes

matsduf modified the milestones: v2022.1, v2022.2 May 11, 2022

Integrate add_fake_delegation_raw() into add_fake_delegation()

78662eb

mattias-p requested review from matsduf, hannaeko, marc-vanderwal, a user and tgreenx May 16, 2022 10:13

matsduf requested changes May 16, 2022

View reviewed changes

mattias-p modified the milestones: v2022.2, v2022.1 May 16, 2022

mattias-p added 2 commits May 16, 2022 16:46

Fix flag name

8127154

Fix hyphenation

b5b3828

mattias-p requested a review from matsduf May 16, 2022 14:57

matsduf previously approved these changes May 16, 2022

View reviewed changes

tgreenx reviewed May 17, 2022

View reviewed changes

lib/Zonemaster/Engine.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine.pm Show resolved Hide resolved

Refactor

e938978

mattias-p dismissed matsduf’s stale review via e938978 May 17, 2022 12:41

tgreenx approved these changes May 17, 2022

View reviewed changes

mattias-p merged commit 368e4d3 into zonemaster:develop May 17, 2022

This was referenced May 18, 2022

Fix breakage in Backend tests #1068

Merged

Provide a module for common tasks when testing Zonemaster::Backend zonemaster/zonemaster-backend#1008

Merged

matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 7, 2022

tgreenx mentioned this pull request Jun 14, 2023

Unit tests send DNS queries when "no_network" is true #888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some unintended network dependencies from the unit tests #1045

Remove some unintended network dependencies from the unit tests #1045

mattias-p commented Mar 23, 2022 •

edited

matsduf commented Mar 25, 2022

matsduf commented Mar 25, 2022

matsduf left a comment

matsduf Mar 25, 2022

mattias-p Mar 25, 2022

matsduf left a comment

mattias-p commented Mar 31, 2022

matsduf May 16, 2022

mattias-p May 16, 2022

matsduf May 16, 2022

mattias-p May 16, 2022

matsduf May 16, 2022

mattias-p May 16, 2022 •

edited

matsduf May 16, 2022

mattias-p May 16, 2022

matsduf commented May 16, 2022

tgreenx left a comment

Remove some unintended network dependencies from the unit tests #1045

Remove some unintended network dependencies from the unit tests #1045

Conversation

mattias-p commented Mar 23, 2022 • edited

Purpose

Context

Changes

How to test this PR

matsduf commented Mar 25, 2022

matsduf commented Mar 25, 2022

matsduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf left a comment

Choose a reason for hiding this comment

mattias-p commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p May 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented May 16, 2022

tgreenx left a comment

Choose a reason for hiding this comment

mattias-p commented Mar 23, 2022 •

edited

mattias-p May 16, 2022 •

edited