[pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes

[Wolfgang Bumiller]

On Wed, Sep 27, 2017 at 11:09:29AM +0200, Tom Weber wrote:
> My goal are defined structures for rules, chains, macros (which i think
> are just arrays of "rule templates") etc and code which deals with
> these structures instead of printing out iptables commands in various
> places.

That's long overdue anyway and might be useful later on when we want to
start using nftables.

> While doing this, keeping "input" and "output" identical to whats there
> doesn't make this an easy task and therefore I sometimes use these
> 'temporary ugliness' approaches at places that I intend to replace
> anyway.

That's fine.

> > > @@ -3127,26 +3146,21 @@ sub generate_std_chains {
> > >      my $std_chains = $pve_std_chains->{$ipversion} || die
> > > "internal error";
> > >  
> > >      my $loglevel = get_option_log_level($options,
> > > 'smurf_log_level');
> > > -
> > > -    my $chain;
> > > -
> > > -    if ($ipversion == 4) {
> > > -	# same as shorewall smurflog.
> > > -	$chain = 'PVEFW-smurflog';
> > > -	$std_chains->{$chain} = [];
> > > -	
> > > -	push @{$std_chains->{$chain}}, get_log_rule_base($chain,
> > > 0, "DROP: ", $loglevel) if $loglevel;
> > > -	push @{$std_chains->{$chain}}, "-j DROP";
> > > +    my $chain = 'PVEFW-smurflog';
> > > +    if ( $loglevel && $std_chains->{$chain} ) {
> > This shouldn't check for $loglevel as it can also have changed from
> > non-zero to zero in which case the change would happen. Iow. you
> > cannot
> > turn off the smurf logs anymore.
> 
> huh? if $loglevel == 0 it will not turn on logging.

This modifies the fields in the global $pve_std_chains, so it doesn't
need to not turn it on, it needs to turn if off. Such modifications are
something I'd prefer to reduce if possible.

> 
> > > +	foreach my $r (@{$std_chains->{$chain}}) {
> > > +	  $r->{log} = $loglevel;
> > Wouldn't it be better to just give ruleset_generate_rule an optional
> > default loglevel parameter for when rules don't define their own
> > {log}?
> 
> how would this help with turning smurf / tcpflags logging separately on
> and off then? (not that I like the current way of setting these - but
> that's what's in the frontend right now)

Could be passed depending on $chain in the loop below, then we could
avoid touching the global hashes entirely.
The function would still prefer the rule's ->{log} member over the
default if it exists.