Re: [pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Some issue due to weird and unmentioned dependence on $noerr and 
while at it some small comment and commit message style nits that
I might have either ignored or "fixed" up myself other way. 

On 25/10/2022 16:31, Stefan Hrdlicka wrote:
> They can already be set directly via the cluster.fw file. Net::IP is just a
> bit more picky with what it allows:

nit: Would suggest:

... what it allows, for example:

> For example:
>   error:   192.168.1.155/24

    fails ...

>   correct: 192.168.1.0/24

    succeeds: ...

(as for us both are obviously correct, so we just want to show when
Net::IP fails or succeeds)

> 
> also improves #3554
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index e6d6802..25e2fd0 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -69,6 +69,14 @@ sub pve_verify_ip_or_cidr {
>      if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
>  	return $cidr if Net::IP->new($cidr);
>  	return undef if $noerr;
> +
> +	# Error 171 in Net::IP comes up if the host part of the IP address isn't
> +	# zero.
> +	# for example:
> +	#  error:   192.168.1.155/24
> +	#  correct: 192.168.1.0/24

A comment for such a thing _is_ great, but it still should be somewhat concise
w.r.t. (line) space usage to avoid "bloat". E.g., the following would still
fit in the 100c upper limit

        # Net::IP sets Error `171` if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
        # fails but `192.168.1.0/24` succeeds. We allow non-zero though, so ignore.

> +	return $cidr if Net::IP::Errno() == 171;
> +

now for a actual non-nit: why only return the $cidr in that case if $noerr is falsy?

Seems odd to have that flag control the behavior.

Also, any details on that errno being restricted to really only that?
I only found some info in the actual code[0], and they don't seem to
have constant (or any management for assigner err#, meh), so just some
hint about that with a link to the source in the commit message.

Or did you find better sources?

It seems that we're also lucky that the check for this is basically the
last one in the `set` method the `new` constructor calls, so at least in
the current version we can assume that it'd be indeed a valid CIDR otherwise,
but still, feels a bit brittle.

Could another option be that we normalize CIDRs on entry, i.e., mask out
the end? I mean,. would not help existing setups, but at least future
proof it a bit for new systems if there's another call site that will
trip on this (maybe normalizing here in case of 171 could be an option
too). I don't want to shove you in that direction, just wondering if
that was considered.

[0]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L1811
[1]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L199

>  	die Net::IP::Error() . "\n";
>      }
>      return undef if $noerr;