Re: [pve-devel] [PATCH v2 firewall 1/1] api: Add optional parameters `since` and `until` for timestamp filter

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Jan 18, 2023 at 04:10:28PM +0100, Christian Ebner wrote:
> > On 18.01.2023 11:33 CET Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> > 
> >  
> > On Wed, Jan 11, 2023 at 02:32:19PM +0100, Christian Ebner wrote:
> > > The optional unix epoch timestamps parameters `since` and `until` are introduced
> > > in order to filter firewall logs files. If one of these flags is set, also
> > > rotated logfiles are included. This is handled in the `dump_fw_logfile` helper
> > > function. Filtering is now performed based on a callback function passed to
> > > `dump_fw_logfile`.
> > > 
> > > This patch depends on the corresponding patch in the pve-common repository.
> > > 
> > > Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> > > ---
> > >  src/PVE/API2/Firewall/Host.pm | 34 +++++++++++++++++++-
> > >  src/PVE/API2/Firewall/VM.pm   | 40 +++++++++++++++++++++---
> > >  src/PVE/Firewall/Helpers.pm   | 59 +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 128 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
> > > index dfeccd0..02f090e 100644
> > > --- a/src/PVE/API2/Firewall/Host.pm
> > > +++ b/src/PVE/API2/Firewall/Host.pm
> > > @@ -11,6 +11,7 @@ use PVE::Firewall;
> > >  use PVE::API2::Firewall::Rules;
> > >  
> > >  
> > > +use Date::Parse qw(str2time);
> > >  use base qw(PVE::RESTHandler);
> > >  
> > >  __PACKAGE__->register_method ({
> > > @@ -172,6 +173,18 @@ __PACKAGE__->register_method({
> > >  		minimum => 0,
> > >  		optional => 1,
> > >  	    },
> > > +	    since => {
> > > +		type => 'integer',
> > > +		minimum => 0,
> > > +		description => "Display log since this UNIX epoch.",
> > > +		optional => 1,
> > > +	    },
> > > +	    until => {
> > > +		type => 'integer',
> > > +		minimum => 0,
> > > +		description => "Display log until this UNIX epoch.",
> > > +		optional => 1,
> > > +	    },
> > >  	},
> > >      },
> > >      returns => {
> > > @@ -196,8 +209,27 @@ __PACKAGE__->register_method({
> > >  	my $rpcenv = PVE::RPCEnvironment::get();
> > >  	my $user = $rpcenv->get_user();
> > >  	my $node = $param->{node};
> > > +	my $filename = "/var/log/pve-firewall.log";
> > > +	my ($start, $limit, $since, $until) =
> > > +	    $param->@{qw(start limit since until)};
> > > +
> > > +	my $filter = sub {
> > 
> > I think this filter could be implied by the `dump_fw_logfile` sub.
> 
> In which case I would need to re-introduce the `since` and `until` parameters to `dump_fw_logfile`, which was the idea to get rid of by passing the whole callback function. Or am I missing the point here? Maybe put it all together with `start` and `limit` in a `param` hash?

Well I mostly just wanted it out of `PVE::Tools` since it also parses fw
log specific lines, but now that part is inside the firewall package,
and it's just duplicate code.