Updates the ipv4 and ipv6 command line options #185

matsduf · 2021-01-19T15:16:38Z

Makes the ipv4 and ipv6 options undefined unless explicitly set on the command line. Changes so that the ip setting is only changed if the option is explicitly set on the command line.

This resolves a bug that appeared after #182 was fixed by #183.

The bug is described in #182 (comment).

Makes the ipv4 and ipv6 options undefined unless explicitly set on the command line. Changes so that the ip setting is only changed if the option is explicitly set on the command line.

mattias-p · 2021-01-19T15:25:41Z

lib/Zonemaster/CLI.pm

+    Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 ) if defined ($self->ipv4);
+    Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 ) if defined ($self->ipv6);


Could you update this to use if (...) { ... } syntax? This is in line with the rest of the code. Also, the structure of the code is more apparent to the reader that way.

I do realize there are a couple of instances of trailing if statements in this file, but adding more of them is a mistake from a readability point of view.

Is that only your personal opinion that trailing "if" is always less readable, or do you know of any studies of that supports that? In that case we get three lines, instead of one, and more separators. And the condition is short.

mattias-p · 2021-01-19T15:31:02Z

lib/Zonemaster/CLI.pm

-    # to override the profile setting
-    Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 );
-    Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 );
+    # to override the profile setting, but only if selected.


If the control flow is apparent from the syntax (like I suggest below), this added clarification is unnecessary and distracting. And it's better to remove it.

ghost · 2021-01-20T08:14:15Z

lib/Zonemaster/CLI.pm

@@ -147,15 +147,13 @@ has 'restore' => (
 has 'ipv4' => (
    is      => 'ro',
    isa     => 'Bool',
-    default => 1,


This default behavior is not moved into the code. I mean, before the change if you do not have the net stanza in the profile file and you just run the CLI, IPv4 and IPv6 will be enabled by default.

Here with the current changes, removing the stanza from the profile file gives a CRITICAL message :

0.01 CRITICAL Both IPv4 and IPv6 are disabled.

I feel an else statement is needed in the code to enable IPv4 and IPv6 by default.

Good catch. I had to remove that "default" to make it possible to detect if the option had been actively set or not. It is more complex than I thought.

Now I see what you mean. If the default profile.json does not have any IPv4 or IPv6 enabled, then there will no network enabled. -- In Backend both are enabled if both are disabled.

You will get the same effect if you run

# zonemaster-cli --level INFO --test basic iis.se --no-ipv6 --no-ipv4

Should CLI be helpful in that case?

ghost · 2021-01-20T08:15:41Z

lib/Zonemaster/CLI.pm

-    Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 );
-    Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 );
+    # to override the profile setting, but only if selected.
+    Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 ) if defined ($self->ipv4);


The else statement I see :

else { Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 1 ); }

That "else" should first check if any of ipv4/ipv6 is enabled, if not set both. Otherwise we will, again, override the setting in any selected config file.

I will check and come with an updated version.

Oh yes I didn't see that it was as twisted as that.

I think this cumbersome code covers every cases (maybe some perl magic might make it lighter)

if ( defined ($self->ipv4) ) { Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, $self->ipv4 ); } else { if ( ! defined ($self->ipv6) ) { Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 1 ); } } if ( defined ($self->ipv6) ) { Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, $self->ipv6 ); } else { if ( ! defined ($self->ipv4) ) { Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 1 ); } }

This will also be wrong, because if you have selected a profile where IPv6 has been disabled, then your suggestion will enable IPv6 unless you explicitly have selected --no-ipv6.

I will proved an update.

This is really twisted ^^. I'll let you find a solution ;)

matsduf · 2021-01-20T10:15:53Z

@mattias-p and @PNAX, should CLI be "helpful" and enable both IPv4 and IPv6 if both are disabled? I do not think so. I think it is better that the user gets the message rather than adding that magic.

When could that happen?

The user has disabled both in the default profile.
The user has loaded a profile which in combination with the default profile disables both.
The user has given --no-ipvX option or options that in combination with the profiles disables both.

mattias-p · 2021-01-20T10:38:16Z

I don't think we should support modifying the default profile.

If a user specifies a profile with both IPv4 and IPv6 disabled but forgets to override either one on the command line, it seems reasonable to just let them know that nothing can be done because both IPv4 and IPv6 are disabled. Maybe it would be better to give the user feedback in the form of an input validation error than in the form of a message tag.

matsduf · 2021-01-20T10:44:19Z

I don't think we should support modifying the default profile.

I agree.

Maybe it would be better to give the user feedback in the form of an input validation error than in the form of a message tag.

That would be beyond the scope of this PR for fixing the bug late in the release cycle. I suggest a issue to do that update in next release.

ghost

From my point of view, if IPv4 and/or IPv6 are explicitly disabled, then the CLI should comply with this choice.

However, I think this does not solve a specific case. From my understanding the net stanza in the profile is not mandatory. Hence if the profile.json file do not have the net stanza (because the user forgot to add it, because it was shipped without it...) then there is no default fallback. I think the CLI should use IPv4 and IPv6 by default in this specific case.
Or maybe this stanza is already mandatory and I missed this.

Is there any documentation about the precedence between the CLI --(no)ipvx options and the profile.json entries ?

ghost · 2021-01-20T10:38:45Z

lib/Zonemaster/CLI.pm

+    }
+    if defined ($self->ipv6) {
+        Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 );
+    };


unnecessary trailing semicolon

And I think that the 0+ can be removed as well.

And I think that the 0+ can be removed as well.

That's correct. It's in the contract of the Profile properties to always return the correct data type.

Concerning "0+" I also looked at that, but I wanted to change as little as possible since this is a bug fix late in the cycle.

The trailing semicolon it not needed. When I started with Perl 1996 I think it was recommended, or maybe even mandatory in Perl 4, so I have a habit of writing them. It does not harm anything.

I appreciate the historical note :)

matsduf · 2021-01-20T11:04:49Z

From my point of view, if IPv4 and/or IPv6 are explicitly disabled, then the CLI should comply with this choice.

If you select a profile with "--profile" and that has disabled both or you have disable by "--no-ipv4 --no-ipv6" then I think we can say that the user has explicitly disabled both.

However, I think this does not solve a specific case. From my understanding the net stanza in the profile is not mandatory. Hence if the profile.json file do not have the net stanza (because the user forgot to add it, because it was shipped without it...) then there is no default fallback. I think the CLI should use IPv4 and IPv6 by default in this specific case.

The profile.json is part of the installation, and that is always loaded. We should not support editing of that. That profile has both ipv4 and ipv6 enabled.

If you choose another profile by "--profile" then first the default profile.json will be loaded and then your profile, which will override all parameters that are defined in your profile.

Or maybe this stanza is already mandatory and I missed this.

It is not mandatory, but is is included in the default profile.json that is provided by the installation and always loaded.

Is there any documentation about the precedence between the CLI --(no)ipvx options and the profile.json entries ?

No, not explicitly.

mattias-p · 2021-01-20T11:12:00Z

@PNAX I feel there's a misunderstanding here.

The Profile module was designed to never have any undefined properties in the default profile, nor in the effective profile. They always start out with a valid default value - in particular the default value that is specified in the documentation (i.e. true for both net.ipv4 and net.ipv6). If users modify the default profile, they're breaking the Profile contract and all bets are off.

Users should be able to override the default profile with --profile and other arguments. So the only way to get to no-IPv4 and no-IPv6 should be to specify that on the command line.

Lastly, no, I don't think we have any documentation on precedence. We should have that.

ghost · 2021-01-20T11:23:38Z

Indeed this is not the logic I had in mind. Thank you for your insights.

matsduf · 2021-01-20T11:27:31Z

@mattias-p and @PNAX, is this good enough? Can you approve?

ghost · 2021-01-20T12:04:44Z

lib/Zonemaster/CLI.pm

+    if defined ($self->ipv4) {
+        Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 );
+    }
+    if defined ($self->ipv6) {
+        Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 );
+    }


I tested it, and it looks like the parentheses usage in the if statement makes perl complain.

the following solves the complaint

if ( defined (...) ) { ... }

I should have kept the first version... Fix in a minute.

mattias-p · 2021-01-20T12:20:46Z

@matsduf Yeah, just fix the syntax errors and I'll approve.

matsduf · 2021-01-20T12:34:41Z

I have verified that the code works after my last commit. @mattias-p and @PNAX, please review.

Updates the ipv4 and ipv6 command line options

2a81218

Makes the ipv4 and ipv6 options undefined unless explicitly set on the command line. Changes so that the ip setting is only changed if the option is explicitly set on the command line.

matsduf added P-High Priority: Issue to be solved before other A-Code labels Jan 19, 2021

matsduf added this to the v2020.2 milestone Jan 19, 2021

matsduf requested review from mattias-p and a user January 19, 2021 15:16

mattias-p reviewed Jan 19, 2021

View reviewed changes

ghost reviewed Jan 20, 2021

View reviewed changes

Editorial updates.

8bc72a8

matsduf requested review from mattias-p and a user January 20, 2021 10:28

ghost reviewed Jan 20, 2021

View reviewed changes

Very minor editorial update.

0801d9a

matsduf requested a review from a user January 20, 2021 11:27

ghost reviewed Jan 20, 2021

View reviewed changes

Adds missing parentheses that created failing code.

11e753e

matsduf requested a review from a user January 20, 2021 12:33

ghost approved these changes Jan 20, 2021

View reviewed changes

matsduf merged commit 47314d8 into zonemaster:develop Jan 20, 2021

matsduf deleted the update-cli.pm-issue-182bis branch January 20, 2021 13:44

matsduf mentioned this pull request Feb 9, 2021

Selected profile overrides IPv6 setting #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates the ipv4 and ipv6 command line options #185

Updates the ipv4 and ipv6 command line options #185

matsduf commented Jan 19, 2021 •

edited

Loading

mattias-p Jan 19, 2021

matsduf Jan 19, 2021

mattias-p Jan 19, 2021

ghost Jan 20, 2021

matsduf Jan 20, 2021 •

edited

Loading

matsduf Jan 20, 2021 •

edited

Loading

ghost Jan 20, 2021

matsduf Jan 20, 2021

ghost Jan 20, 2021

matsduf Jan 20, 2021

ghost Jan 20, 2021

matsduf commented Jan 20, 2021 •

edited

Loading

mattias-p commented Jan 20, 2021

matsduf commented Jan 20, 2021

ghost left a comment •

edited by ghost

Loading

ghost Jan 20, 2021

mattias-p Jan 20, 2021

matsduf Jan 20, 2021 •

edited

Loading

mattias-p Jan 20, 2021

matsduf commented Jan 20, 2021 •

edited

Loading

mattias-p commented Jan 20, 2021

ghost commented Jan 20, 2021

matsduf commented Jan 20, 2021

ghost Jan 20, 2021 •

edited by ghost

Loading

matsduf Jan 20, 2021

mattias-p commented Jan 20, 2021

matsduf commented Jan 20, 2021

		Zonemaster::Engine::Profile->effective->set( q{net.ipv4}, 0+$self->ipv4 ) if defined ($self->ipv4);
		Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 ) if defined ($self->ipv6);

Updates the ipv4 and ipv6 command line options #185

Updates the ipv4 and ipv6 command line options #185

Conversation

matsduf commented Jan 19, 2021 • edited Loading

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

matsduf Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

matsduf Jan 20, 2021 • edited Loading

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

Choose a reason for hiding this comment

matsduf commented Jan 20, 2021 • edited Loading

mattias-p commented Jan 20, 2021

matsduf commented Jan 20, 2021

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Jan 20, 2021 • edited Loading

mattias-p commented Jan 20, 2021

ghost commented Jan 20, 2021

matsduf commented Jan 20, 2021

ghost Jan 20, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p commented Jan 20, 2021

matsduf commented Jan 20, 2021

matsduf commented Jan 19, 2021 •

edited

Loading

matsduf Jan 20, 2021 •

edited

Loading

matsduf Jan 20, 2021 •

edited

Loading

matsduf commented Jan 20, 2021 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

matsduf Jan 20, 2021 •

edited

Loading

matsduf commented Jan 20, 2021 •

edited

Loading

ghost Jan 20, 2021 •

edited by ghost

Loading