zonemaster-cli outputs no message at all if name server given by --ns lacks required glue

[marc-vanderwal]

On current develop, if zonemaster-cli is given fake delegation by means of --ns, the name server is in-bailiwick but is missing glue, then I get no error message at all:

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net'
  \
Seconds Level    Testcase       Message
======= ======== ============== =======

Using git bisect, it seems that commit 69b71a6 is the one introducing the problem. If I check out its ancestor commit (2476edb), then I get the following output, which seems more correct:

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net'
Seconds Level    Testcase       Message
======= ======== ============== =======
   1.15 ERROR    Unspecified    The fake delegation of domain zonemaster.net includes an in-zone name server ns1.zonemaster.net without mandatory glue (without IP address).
   0.00 INFO     Unspecified    Using version v6.0.0 of the Zonemaster engine.
   0.00 INFO     Basic01        The zone "zonemaster.net" is found.
   0.01 INFO     Basic01        This is a test of an undelegated domain so finding the parent zone is disregarded.
   0.21 CRITICAL Basic02        There is no working name server for "zonemaster.net" so it is unreachable.
   0.21 ERROR    Basic02        Name server "ns1.zonemaster.net" cannot be resolved into an IP address.

Update: with --raw added to the first command line, messages do appear, so it really is related to the translator.

[matsduf]

When I try the same thing in GUI I get a strange result, https://zonemaster.net/en/result/16bfef5c7cf1fc33

[matsduf]

I think there is a change in Engine that does not stop testing. If I test xa (delegated) that does not exist, then I get a reasonable result (https://zonemaster.net/en/result/a363099fb9477c9c), but if I test it undelegated with one in-bailiwick NS without IP address it does not look right (https://zonemaster.net/en/result/b6ef3f83fc6ad5db).

[marc-vanderwal]

I think there is a change in Engine that does not stop testing.

I’m not sure about that. I’ve tried installing Zonemaster-Engine version 5.0.0 (from 2023.2) and 6.0.0 (from 2024.1) and I still get the same lack of output on the command line I tested.

If I test xa (delegated) that does not exist, then I get a reasonable result (https://zonemaster.net/en/result/a363099fb9477c9c), but if I test it undelegated with one in-bailiwick NS without IP address it does not look right (https://zonemaster.net/en/result/b6ef3f83fc6ad5db).

Yes, in the second case, the test should have been halted after Basic02 reported its errors, right?

There is something in the design of add_fake_delegation() in Zonemaster::Engine that seems strange to me. This function reports errors in two ways: either by calling croak, or by logging errors.

At this stage, however, zonemaster-cli can exit early on invalid command-line arguments, but hasn’t initialized the translation mechanism, nor the dynamic display, of proper Zonemaster::Engine log messages. As part of figuring out the root cause of this problem, I’m trying out a solution where Zonemaster::CLI buffers those early log messages until it is ready to display them. This fix looks promising.

[matsduf]

Yes, in the second case, the test should have been halted after Basic02 reported its errors, right?

I think basic should break if there is a message with level CRITICAL, which there is in both cases. But they are different messages from different sources.

There is something in the design of add_fake_delegation() in Zonemaster::Engine that seems strange to me. This function reports errors in two ways: either by calling croak, or by logging errors.

Aren't the croak for error that should be handled by the calling layer?

[tgreenx]

I think you are discussing two separate issues.

When running the full test suite, the stop condition is ruled by this line of code. It resides in the can_continue() method of each Test Module, but right now, it only exists in the Basic module and reads: https://github.com/zonemaster/zonemaster-engine/blob/v6.0.0/lib/Zonemaster/Engine/Test/Basic.pm#L101-L115. So it means that currently any undelegated test will never abort, which is probably a mistake. A simple fix would be to remove lines 104 to 107.

The change was introduced by zonemaster/zonemaster-engine@b874598 (PR zonemaster/zonemaster-engine#1312).

[matsduf]

I do not see why undelegated tests should be handled differently in this respect. I support the removal of those lines.

[tgreenx]

I do not see why undelegated tests should be handled differently in this respect. I support the removal of those lines.

I created zonemaster/zonemaster-engine#1401 to fix that.