First of all, this all is work in progress. I thought I commit for easier understanding of the way i'm heading - instead of one huge commit which turns everything inside out. and for feedback of course.
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. 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. Am Mittwoch, den 27.09.2017, 09:53 +0200 schrieb Wolfgang Bumiller: > On Wed, Sep 27, 2017 at 12:02:33AM +0200, Tom Weber wrote: > > > > --- > > src/PVE/Firewall.pm | 220 ++++++++++++++++++++++++++++---------- > > -------------- > > 1 file changed, 117 insertions(+), 103 deletions(-) > > > > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm > > index f8a9300..179617a 100644 > > --- a/src/PVE/Firewall.pm > > +++ b/src/PVE/Firewall.pm > > @@ -142,6 +142,20 @@ my $log_level_hash = { > > emerg => 0, > > }; > > > > +# %rule > > +# > +1 for the documentation, but what happened to the right sides of > most > of these arrows? ;-) (yeah some of these options are being abused a > bit...) still in the progress of defining this :-).. Right now it's more like a scratchpad where I collect what I encounter. > > +# name => optional > > +# action => > > +# proto => > > +# sport => > > +# dport => > > +# log => optional, loglevel > Why not call it loglevel then? Also, with this being new it should be > mentioned in the commit message. this is for now and for backward compatibility. I'd prefer this as a boolean, but I'm still carrying the loglevel around since it's used in the frontend and the logformat for the pvefw-logger. Do we really need the loglevel as it is or would a boolean be sufficient? And how would we get rid of it? > > +# logmsg => optional, logmsg - overwrites default > And this. > > > > +# iface_in > > +# iface_out > > +# match => optional, overwrites generation of match > > +# target => optional, overwrites action > > + > > # we need to overwrite some macros for ipv6 > > my $pve_ipv6fw_macros = { > > 'Ping' => [ > > @@ -536,7 +550,7 @@ my $FWACCEPTMARK_OFF = "0x00000000/0x80000000"; > > > > > > @@ -1948,19 +1970,24 @@ sub ruleset_generate_rule { > > > > # update all or nothing > > > > + # fixme: lots of temporary ugliness > +1 for the 'temporary' in there ;-) > > > > > my @mstrs = (); > > my @astrs = (); > > + my @logging = (); > > + my @logmsg = (); > > foreach my $tmp (@$rules) { > > my $m = ruleset_generate_match($ruleset, $chain, > > $ipversion, $tmp, $actions, $goto, $cluster_conf, $fw_conf); > > my $a = ruleset_generate_action($ruleset, $chain, > > $ipversion, $tmp, $actions, $goto, $cluster_conf, $fw_conf); > > if (defined $m or defined $a) { > > push @mstrs, defined($m) ? $m : ""; > > push @astrs, defined($a) ? $a : ""; > > + push @logging, $tmp->{log}; > > + push @logmsg, $tmp->{logmsg}; > > } > > } > > > > for my $i (0 .. $#mstrs) { > > - ruleset_addrule($ruleset, $chain, $mstrs[$i], $astrs[$i]); > > + ruleset_addrule($ruleset, $chain, $mstrs[$i], $astrs[$i], > > $logging[$i], $logmsg[$i]); > > } > > } > > > > @@ -1993,14 +2020,6 @@ sub ruleset_chain_exist { > > return $ruleset->{$chain} ? 1 : undef; > > } > > > > -sub ruleset_addrule_old { > > - my ($ruleset, $chain, $rule) = @_; > > - > > - die "no such chain '$chain'\n" if !$ruleset->{$chain}; > > - > > - push @{$ruleset->{$chain}}, "-A $chain $rule"; > > -} > > - > > sub ruleset_addrule { > > my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = > > @_; > > > > @@ -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. > > + 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) Tom _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel