New "resolver.source4" and "resolver.source6" properties #1203

ghost · 2023-02-27T18:36:29Z

Purpose

Provide new properties "resolver.source4" and "resolver.source6" to allow configuring an IPv4 and an IPv6 source address. The "resolver.source" is untouched:

"resolver.source" can hold an IPv4 or an IPv6 and is marked as deprecated
"resolver.source4" can only hold an IPv4 (or the empty string) and takes precedence
"resolver.source6" can only hold an IPv6 (or the empty string) and takes precedence

Context

zonemaster/zonemaster-cli#315

Changes

Check that if set, the "resolver.source4" property holds an IPv4 (or the empty string)
Check that if set, the "resolver.source6" property holds an IPv6 (or the empty string)
If set and querying over IPv4, use the "resolver.source4" value. Fallback to "resolver.source" otherwise.
If set and querying over IPv6, use the "resolver.source6" value. Fallback to "resolver.source" otherwise.

How to test this PR

New unit tests are provided. They should pass.

Set "resolver.source4" and/or "resolver.source6" to a value in share/profile.json and check that these interface are correctly used when running Zonemaster.

zonemaster-cli --show-testcase --show-module --level DEBUG --test connectivity/connectivity01 zonemaster.net

marc-vanderwal · 2023-02-28T07:51:35Z

The code looks fine as it is, but I don’t think we should allow resolver.source6 and resolver.source to be both IPv6 addresses. Sure, resolver.source6 would take precedence but it isn’t user-friendly.

ghost · 2023-02-28T08:33:26Z

I don’t think we should allow resolver.source6 and resolver.source to be both IPv6 addresses

I thought about it too, and choose not to alter the current behavior to avoid a breaking change at this stage. However I also think it would be better to separate IPv4 and IPv6. Having said that, I see several options:

avoid a breaking change: only add a new "resolver.source6" property and
1. keep the documentation of "resolver.source" unchanged
2. update the documentation of "resolver.source" to match the implementation
3. update the documentation of "resolver.source" to suggest using it for IPv4 (an IPv6 will still work with the implementation)
make it a breaking change by allowing only IPv4 in "resolver.source" and IPv6 in "resolver.source6"
mark the old "resolver.source" behavior as deprecated, and suggest using it to hold an IPv4 only (similar to 1.iii. but with deprecation warnings)

With all my current work on deprecating things in different components, I like the 3. better. What do you think?

hannaeko · 2023-02-28T08:37:36Z

However I also think it would be better to separate IPv4 and IPv6. Having said that, I see several options:

I see one more. Introduce resolver.source4 for IPv4 only, keep the behaviour of resolver.source untouched but mark it as deprecated.

marc-vanderwal · 2023-02-28T08:47:27Z

I agree with @blacksponge (although I wouldn’t mind going with suggestion number 3 from @PNAX if we feel that we are deprecating too many things at once).

The behavior of resolver.source was a mistake to begin with, because there is no way that IPv6 queries are going to work with an IPv4 source address and vice versa. There should have been two variables from the start, for IPv4 and IPv6 queries respectively. And because we are introducing a variable named source6 for an IPv6 source address, we should be consistent and name its IPv4 counterpart source4.

ghost · 2023-02-28T10:33:03Z

I've updated the PR with a new "resolver.source4" property as suggested. I've marked "resolver.source" as deprecated in documentation.

marc-vanderwal · 2023-02-28T10:54:53Z

It’s better, but in the current shape, we are allowing combinations of variables that yield confusing results.

For example, if resolver.source, resolver.source4 and resolver.source6 are all set, then resolver.source is never used. The most user-friendly thing to do here, I think, is to reject the configuration.

Furthermore, what should we do if one of resolver.source4 or resolver.source6 is set, and resolver.source is set as well? In any case, the result is going to probably confuse the user. I think we shouldn’t allow these combinations either.

So, I suggest that we only allow resolver.source, or a combination of resolver.source4 and resolver.source6, and reject any mixing of the former and any one of the latter.

matsduf

I think the solution looks fine, and deprecating old "source" is a good solution for the future. Can you update share/profile.json from

(...)
    "resolver" : {
        "source": "os_default"
(...)

to

(...)
    "resolver" : {
        "source4": ""
        "source6": ""
(...)

And then I do not think there is any reason to have it in share/profile_additional_properties.json since that is not in addition to what is in share/profile.json.

matsduf · 2023-03-06T12:57:23Z

lib/Zonemaster/Engine/Profile.pm

@@ -622,11 +642,26 @@ replay, set this flag to false.

 =head2 resolver.source

+Deprecated. Use L</resolver.source4> and L</resolver.source6>.


Maybe information on in which release it will be removed?

added, I chose v2024.1.

matsduf · 2023-03-06T15:04:28Z

To resolve the issue zonemaster/zonemaster-cli#315 (moved to Zonemaster-CLI) this PR is not enough. The issue asks for new options for zonemaster-cli.

Use it to pass an IPv6 source address. The "resolver.source" property can be used to pass either an IPv4 or an IPv6 source address. Hence if you want to define both, use "resolver.source" to store the IPv4 and "resolver.source6" to store the IPv6.

Use it to pass an IPv4 source address. Will replace "resolver.source".

ghost · 2023-03-06T15:34:28Z

I'd like to have the following allowed states for resolver.source4 (resp. resolver.source6):

undefined (the property is not present in the json file)
defined with the empty string
defined with a valid IPv4 (resp. IPv6) string
defined with an invalid string (Zonemaster will fail to load the profile)

By default the properties are undefined. Therefore I don't think they should be present in share/profile.json. That's why I've put them in share/profile_additional_properties.json.

With that in mind do you still see your remark as a change request?

and set its default value

ghost · 2023-03-06T16:25:43Z

It’s better, but in the current shape, we are allowing combinations of variables that yield confusing results.

@marc-vanderwal, I agree. I've tried to come up with a solution to this and die on error. However I've encountered an error based on how this module is implemented and loaded.

The change I was thinking about:

diff --git a/lib/Zonemaster/Engine/Profile.pm b/lib/Zonemaster/Engine/Profile.pm
index e9e445f..82f0918 100644
--- a/lib/Zonemaster/Engine/Profile.pm
+++ b/lib/Zonemaster/Engine/Profile.pm
@@ -54,7 +54,6 @@ my %profile_properties_details = (
     },
     q{resolver.source} => {
         type    => q{Str},
-        default => "$RESOLVER_SOURCE_OS_DEFAULT",
         test    => sub {
             if ( $_[0] ne $RESOLVER_SOURCE_OS_DEFAULT ) {
                 Net::IP::XS->new( $_[0] ) || die "Property resolver.source must be an IP address or the exact string $RESOLVER_SOURCE_OS_DEFAULT";
@@ -63,20 +62,18 @@ my %profile_properties_details = (
     },
     q{resolver.source4} => {
         type    => q{Str},
-        default => "",
         test    => sub {
-            if ( $_[0] ne '' and not Net::IP::XS::ip_is_ipv4( $_[0] ) ) {
-                die "Property resolver.source4 must be an IPv4 address or the empty string";
+            if ( $_[0] and not Net::IP::XS::ip_is_ipv4( $_[0] ) ) {
+                die "Property resolver.source4 must be an IPv4 address or undefined";
             }
             Net::IP::XS->new( $_[0] );
         }
     },
     q{resolver.source6} => {
         type    => q{Str},
-        default => "",
         test    => sub {
-            if ( $_[0] ne '' and not Net::IP::XS::ip_is_ipv6( $_[0] ) ) {
-                die "Property resolver.source6 must be an IPv6 address or the empty string";
+            if ( $_[0] and not Net::IP::XS::ip_is_ipv6( $_[0] ) ) {
+                die "Property resolver.source6 must be an IPv6 address or undefined";
             }
             Net::IP::XS->new( $_[0] );
         }
@@ -278,6 +275,7 @@ sub default {
             $new->set( $property_name, $profile_properties_details{$property_name}{default} );
         }
     }
+    $new->check_validity;
     return $new;
 }
 
@@ -299,6 +297,14 @@ sub set {
     $self->_set( q{DIRECT}, $property_name, $value );
 }
 
+sub check_validity {
+    my ( $self ) = @_;
+    my $resolver = $self->{profile}{resolver};
+    if ( exists $resolver->{source} and ( exists $resolver->{source4} or exists $resolver->{source6} ) ) {
+        die 'Error in profile: "resolver.source" (deprecated) can\'t be used in combination with "resolver.source4" or "resolver.source6".' . "\n";
+    }
+}
+
 sub _set {
     my ( $self, $from, $property_name, $value ) = @_;
     my $value_type = reftype($value);

If I apply such change update share/profile.json to have both resolver.source and resolver.source4 defined, I get some import errors:

$ perl -I lib/ -MZonemaster::Engine -E 'say join "\n", Zonemaster::Engine->test_module("CONNECTIVITY", "stockholm")'
Error in profile: "resolver.source" (deprecated) can't be used in combination with "resolver.source4" or "resolver.source6".
Compilation failed in require at lib/Zonemaster/Engine/Util.pm line 35.
BEGIN failed--compilation aborted at lib/Zonemaster/Engine/Util.pm line 35.
Compilation failed in require at lib/Zonemaster/Engine/Packet.pm line 9.
BEGIN failed--compilation aborted at lib/Zonemaster/Engine/Packet.pm line 9.
Compilation failed in require at lib/Zonemaster/Engine/Nameserver.pm line 10.
BEGIN failed--compilation aborted at lib/Zonemaster/Engine/Nameserver.pm line 10.
Compilation failed in require at lib/Zonemaster/Engine.pm line 19.
BEGIN failed--compilation aborted at lib/Zonemaster/Engine.pm line 19.
Compilation failed in require.
BEGIN failed--compilation aborted.

I'd say this comes from the fact that we load the profile at module import:
https://github.com/zonemaster/zonemaster-engine/blob/master/lib/Zonemaster/Engine/Profile.pm#L243

So far I haven't found how to properly deal with the errors (besides loading the module in a eval block, but I'm not sure I like this approach). Would you say this is a show stopper for this PR not to properly handle such configurations?

matsduf · 2023-03-06T18:36:13Z

By default the properties are undefined. Therefore I don't think they should be present in share/profile.json. That's why I've put them in share/profile_additional_properties.json.

There are pros and cons with both solutions. It can be helpful to see the default values. On the other hand, cutting down the profile to what is needed makes it easier to read.

However, we should be consistent. Since we have the other keys in share/profile.json even if they just have the default value, such as "resolver:defaults:usevc", we should also have these keys in this PR.

With that in mind do you still see your remark as a change request?

Yes.

matsduf · 2023-03-06T18:41:01Z

So far I haven't found how to properly deal with the errors (besides loading the module in a eval block, but I'm not sure I like this approach). Would you say this is a show stopper for this PR not to properly handle such configurations?

If it is documented that you must not set both, and if you do, you get an error, I think it should be OK.

matsduf · 2023-03-10T10:01:59Z

Something has happened with the implementation of setting the source address. I am not sure when the change was done, but it seems to be in the last release.

Before if you set a valid IPv4 address that was not configured on any interface by --sourceaddr to zonemaster-cli then you got an error message. Now there is nothing. No queries are sent and no error is provided.

ghost · 2023-03-13T09:35:14Z

I am not sure when the change was done, but it seems to be in the last release.

This happened with v2021.2. See zonemaster/zonemaster-cli#217.

matsduf · 2023-03-13T10:26:08Z

I am not sure when the change was done, but it seems to be in the last release.

This happened with v2021.2. See zonemaster/zonemaster-cli#217.

~~No, I do not think so. Then the helpful error message was dropped, but the harsh was still there. I think it was later, maybe as late as with v2022.2.~~

That seems to be correct. It was, however, not the intention that there should be no error message. The expectation was that error messages from the OS would get through. For zonemaster-cli it says

	--sourceaddr STR       Source IP address used to send queries.
	                       Setting an IP address not correctly configured
	                       on a local network interface causes cryptic
	                       error messages.

Update some message ids to report name server names and addresses together instead of separatly

Update some message ids to report name server names and addresses together instead of separately

ghost added T-Feature Type: New feature in software or test case description V-Minor Versioning: The change gives an update of minor in version. labels Feb 27, 2023

ghost added this to the v2023.1 milestone Feb 27, 2023

ghost requested review from mattias-p, matsduf, vlevigneron, hannaeko, tgreenx and marc-vanderwal February 27, 2023 18:36

ghost changed the title ~~New optional "resolver.source6" property~~ New "resolver.source4" and "resolver.source6" properties Feb 28, 2023

matsduf requested changes Mar 6, 2023

View reviewed changes

matsduf mentioned this pull request Mar 6, 2023

sourceaddr for IPv6 and IPv4 zonemaster/zonemaster-cli#315

Closed

Alexandre Pion added 3 commits March 6, 2023 16:17

New "resolver.source6" property

5f49429

Use it to pass an IPv6 source address. The "resolver.source" property can be used to pass either an IPv4 or an IPv6 source address. Hence if you want to define both, use "resolver.source" to store the IPv4 and "resolver.source6" to store the IPv6.

Join lines

b8af8c2

New "resolver.source4" property

ab7df5e

Use it to pass an IPv4 source address. Will replace "resolver.source".

Mark "resolver.source" as deprecated

91e6053

and set its default value

Alexandre Pion added 2 commits March 9, 2023 16:48

Log warning message instead of dying

887c635

Check profile validity

d410b37

matsduf approved these changes Mar 9, 2023

View reviewed changes

ghost merged commit 54fc5ee into zonemaster:develop Mar 13, 2023

ghost deleted the sourceaddr6 branch March 13, 2023 09:45

ghost mentioned this pull request Mar 13, 2023

New options "--sourceaddr4" and "--sourceaddr6" zonemaster/zonemaster-cli#317

Merged

tgreenx added a commit to tgreenx/zonemaster-engine that referenced this pull request Sep 12, 2023

Update Delegation01 - zonemaster#1203

7585a1d

Update some message ids to report name server names and addresses together instead of separatly

tgreenx added a commit to tgreenx/zonemaster-engine that referenced this pull request Sep 12, 2023

Update Delegation01 - zonemaster#1203

a2cffb0

Update some message ids to report name server names and addresses together instead of separately

tgreenx added a commit to tgreenx/zonemaster-engine that referenced this pull request Sep 12, 2023

Update Delegation01 - zonemaster#1203

b9f58a0

Update some message ids to report name server names and addresses together instead of separately

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New "resolver.source4" and "resolver.source6" properties #1203

New "resolver.source4" and "resolver.source6" properties #1203

ghost commented Feb 27, 2023 •

edited by ghost

Loading

marc-vanderwal commented Feb 28, 2023

ghost commented Feb 28, 2023

hannaeko commented Feb 28, 2023

marc-vanderwal commented Feb 28, 2023

ghost commented Feb 28, 2023

marc-vanderwal commented Feb 28, 2023

matsduf left a comment •

edited

Loading

matsduf Mar 6, 2023 •

edited

Loading

ghost Mar 6, 2023

matsduf commented Mar 6, 2023

ghost commented Mar 6, 2023 •

edited by ghost

Loading

ghost commented Mar 6, 2023

matsduf commented Mar 6, 2023

matsduf commented Mar 6, 2023

matsduf commented Mar 10, 2023

ghost commented Mar 13, 2023

matsduf commented Mar 13, 2023 •

edited

Loading

		@@ -622,11 +642,26 @@ replay, set this flag to false.

		=head2 resolver.source

		Deprecated. Use L</resolver.source4> and L</resolver.source6>.

New "resolver.source4" and "resolver.source6" properties #1203

New "resolver.source4" and "resolver.source6" properties #1203

Conversation

ghost commented Feb 27, 2023 • edited by ghost Loading

Purpose

Context

Changes

How to test this PR

marc-vanderwal commented Feb 28, 2023

ghost commented Feb 28, 2023

hannaeko commented Feb 28, 2023

marc-vanderwal commented Feb 28, 2023

ghost commented Feb 28, 2023

marc-vanderwal commented Feb 28, 2023

matsduf left a comment • edited Loading

Choose a reason for hiding this comment

matsduf Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

ghost Mar 6, 2023

Choose a reason for hiding this comment

matsduf commented Mar 6, 2023

ghost commented Mar 6, 2023 • edited by ghost Loading

ghost commented Mar 6, 2023

matsduf commented Mar 6, 2023

matsduf commented Mar 6, 2023

matsduf commented Mar 10, 2023

ghost commented Mar 13, 2023

matsduf commented Mar 13, 2023 • edited Loading

ghost commented Feb 27, 2023 •

edited by ghost

Loading

matsduf left a comment •

edited

Loading

matsduf Mar 6, 2023 •

edited

Loading

ghost commented Mar 6, 2023 •

edited by ghost

Loading

matsduf commented Mar 13, 2023 •

edited

Loading