Check for object type in returned values from some TestMethodsV2 methods used in Test Cases

[tgreenx]

Purpose

Methods Zonemaster::Engine::TestMethodsV2::get_{zone,del}_ns_names_and_ips() can return a mix of Zonemaster::Engine::Nameserver or Zonemaster::Engine::DNSName objects, depending on whether the name could be resolved into an IP address. Due to a previous oversight, this can cause issues in Test Cases that use them. We can filter Zonemaster::Engine::DNSName objects out since they can't be queried.

Moreover it also fixes a buggy ternary expression in the same place (see #1427 (comment) below).

Context

Fixes #1426

Changes

• Filter non-Zonemaster::Engine::Nameserver objects out of the list of to-be-queried name servers in Test Cases
• Rework buggy ternary expression to a working alternative

How to test this PR

Unit tests could be created but they are kind of out of scope for either the TestMethodsV2 or the Test Cases themselves.
Manual testing (from #1426):

$ zonemaster-cli --level=info --no-ipv6 --show-testcase --test dnssec10 --test connectivity04 robindesboispaysage.fr

Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     Unspecified    Using version v7.0.0 of the Zonemaster engine.
  18.30 NOTICE   DNSSEC10       The zone is not DNSSEC signed or not properly DNSSEC signed. Testing for NSEC and NSEC3 has been skipped. Fetched from name servers "ns0.wixdns.net/216.239.32.101;ns1.wixdns.net/216.239.34.101".
   0.00 INFO     Unspecified    Using version v7.0.0 of the Zonemaster engine.
   2.01 INFO     Connectivity04 The following name server(s) are announced in unique IPv4 prefix(es): "ns0.wixdns.net/216.239.32.101;ns1.wixdns.net/216.239.34.101"

[marc-vanderwal]

Actually, the output of zonemaster-cli in the “How to test this PR” section is wrong: there are two name servers and both are announced in different prefixes, so the WARNING from Connectivity04 is a false warning. And the NOTICE in DNSSEC10 only lists ns1.wixdns.net at the end of the message; ns0.wixdns.net is missing.

Applying my review suggestions gives a more correct output from zonemaster-cli:

Seconds Level    Message
======= ======== =======
   0.00 INFO     Using version v7.0.0 of the Zonemaster engine.
   2.00 INFO     The following name server(s) are announced in unique IPv4 prefix(es): "ns0.wixdns.net/216.239.32.101;ns1.wixdns.net/216.239.34.101"
   0.00 INFO     Using version v7.0.0 of the Zonemaster engine.
   0.16 NOTICE   The zone is not DNSSEC signed or not properly DNSSEC signed. Testing for NSEC and NSEC3 has been skipped. Fetched from name servers "ns0.wixdns.net/216.239.32.101;ns1.wixdns.net/216.239.34.101".

[tgreenx]

@mattias-p @marc-vanderwal @matsduf I've rebased on latest release-v2024.2.1 and fixed a conflict, please re-approve.

[matsduf]

Most test cases only use name servers by IP address. If a name server only has a name and no address it will be disregarded. The name is only needed to make the messages more readable, a nice feature for the users.

The PR adds the same code to two test cases to filter out name servers without address. I feel it is wrong approach. We would then need a scenario for each test case to make sure this filtering is done correctly. And the test case code get more complex.

It would be a better solution if MethodsV2 methods provide the name servers twice, one set of IP addresses, and one set of name/IP pairs. In that way we lower the risk of errors and make the test case code simpler. Maybe even a method that provide the combination of delegation and zone, which is what we use in most test cases (no separation is needed).

If an update of the MethodsV2 specification is needed, we can take that time. I am willing to work on that.

[mattias-p]

I agree that the MethodV2 return types aren't the most ergonomic. AFAICT this is partly a consequence of how MethodsV2 are specified. I thought about how the situation could be improved and updated zonemaster/zonemaster#1345 with a design proposal that would allow nicer programmatic interfaces. One of the ideas is that methods should return lists of homogenous types that can be deduplicated using List::Util::uniq instead of arrayrefs of sometimes heterogeneous types.

[tgreenx]

As discussed in the work group and previous comments, this PR will be merged as-is until a more long-term solution is provided, probably based on zonemaster/zonemaster#1345.