[pve-devel] [RFC common 0/1] Optional parameters `since` and `until` for firewall log filtering

[pve-devel] [RFC common 0/1] Optional parameters `since` and `until` for firewall log filtering

[Christian Ebner]

This patch series introduces 2 optional api parametes `since` and `until` to firewall log
endpoints, in order to make them filterable.

In contrast to the current `dump_logfile`, the specialized `dump_fw_logfile` takes into
consideration and scans also rotated logfiles, if `since` or `until` are given.

Without optional parameters, `dump_logfile` is used.

Christian Ebner (1):
  tools: Add specialized `dump_fw_logfile` for `since` and `until`
    filtering of firewall logs

 src/PVE/Tools.pm | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

Christian Ebner (1):
  api: Add optional parameters `since` and `until` for timestamp filter

 src/PVE/API2/Firewall/Host.pm | 15 ++++++++++++++-
 src/PVE/API2/Firewall/VM.pm   | 18 +++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.30.2

[pve-devel] [RFC firewall] api: Add optional parameters `since` and `until` for timestamp filter

[Christian Ebner]

The optional unix epoch timestamps parameters `since` and `until` are introduced in order to filter
firewall logs files.

Since the `dump_logfile` function is used in multiple places, not neccessarily following the firewall
log output format, a specialized function `dump_fw_logfile` is introduced in the PVE::Tools,
with fallback to the previous `dump_logfile` function if none of the parameters is present.

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 | 15 ++++++++++++++-
 src/PVE/API2/Firewall/VM.pm   | 18 +++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
index dfeccd0..8470f7f 100644
--- a/src/PVE/API2/Firewall/Host.pm
+++ b/src/PVE/API2/Firewall/Host.pm
@@ -172,6 +172,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 => {
@@ -197,7 +209,8 @@ __PACKAGE__->register_method({
 	my $user = $rpcenv->get_user();
 	my $node = $param->{node};
 
-	my ($count, $lines) = PVE::Tools::dump_logfile("/var/log/pve-firewall.log", $param->{start}, $param->{limit});
+	my($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
+		$param->{start}, $param->{limit}, undef, $param->{since}, $param->{until});
 
 	$rpcenv->set_result_attrib('total', $count);
 
diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
index 48b8c5f..a0c0f94 100644
--- a/src/PVE/API2/Firewall/VM.pm
+++ b/src/PVE/API2/Firewall/VM.pm
@@ -176,6 +176,18 @@ sub register_handlers {
 		    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 => {
@@ -201,9 +213,9 @@ sub register_handlers {
 	    my $user = $rpcenv->get_user();
 	    my $vmid = $param->{vmid};
 
-	    my ($count, $lines) = PVE::Tools::dump_logfile("/var/log/pve-firewall.log",
-							   $param->{start}, $param->{limit},
-							   "^$vmid ");
+	    my ($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
+		$param->{start}, $param->{limit}, "^$vmid ", $param->{since},
+		$param->{until});
 
 	    $rpcenv->set_result_attrib('total', $count);
 
-- 
2.30.2

Re: [pve-devel] [RFC firewall] api: Add optional parameters `since` and `until` for timestamp filter

[Fiona Ebner]

Just two refreshers about style ;)

Am 05.01.23 um 10:18 schrieb Christian Ebner:
> The optional unix epoch timestamps parameters `since` and `until` are introduced in order to filter
> firewall logs files.
> 
> Since the `dump_logfile` function is used in multiple places, not neccessarily following the firewall
> log output format, a specialized function `dump_fw_logfile` is introduced in the PVE::Tools,
> with fallback to the previous `dump_logfile` function if none of the parameters is present.
> 
> This patch depends on the corresponding patch in the pve-common repository.
> 

Style nit: lines in the commit message should be <= 70 characters [0]

> @@ -197,7 +209,8 @@ __PACKAGE__->register_method({
>  	my $user = $rpcenv->get_user();
>  	my $node = $param->{node};
>  
> -	my ($count, $lines) = PVE::Tools::dump_logfile("/var/log/pve-firewall.log", $param->{start}, $param->{limit});
> +	my($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
> +		$param->{start}, $param->{limit}, undef, $param->{since}, $param->{until});

Style nit: please use 1 line for each parameter if it gets too long [1]

> @@ -201,9 +213,9 @@ sub register_handlers {
>  	    my $user = $rpcenv->get_user();
>  	    my $vmid = $param->{vmid};
>  
> -	    my ($count, $lines) = PVE::Tools::dump_logfile("/var/log/pve-firewall.log",
> -							   $param->{start}, $param->{limit},
> -							   "^$vmid ");
> +	    my ($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
> +		$param->{start}, $param->{limit}, "^$vmid ", $param->{since},
> +		$param->{until});

Same here

[0]:
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages
[1]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Arguments

Re: [pve-devel] [RFC firewall] api: Add optional parameters `since` and `until` for timestamp filter

[Thomas Lamprecht]

Am 05/01/2023 um 14:51 schrieb Fiona Ebner:
>> +	my($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
>> +		$param->{start}, $param->{limit}, undef, $param->{since}, $param->{until});
> Style nit: please use 1 line for each parameter if it gets too long [1]
> 

In general we try to adhere to rustfmt style, at least in spirit as not everything 
can be mapped to perl. That format has an intermediate step between one arg per line
namely something like:

    PVE::Tools::foo(
        $bar, $baz, $very_long_argument_bla_bla)

Here, that might still work out if we pull out the $param stuff first, e.g.::

    my ($start, $limit, $since, $until) = param->@{qw(start limit since until)};

    PVE::Tools::dump_fw_logfile(
        "/var/log/pve-firewall.log", $start, $limit, undef, $since, $until)

Re: [pve-devel] [RFC firewall] api: Add optional parameters `since` and `until` for timestamp filter

[Christian Ebner]

Okay, I will take your comments into consideration, thx!

> On 05.01.2023 14:59 CET Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> Am 05/01/2023 um 14:51 schrieb Fiona Ebner:
> >> +	my($count, $lines) = PVE::Tools::dump_fw_logfile("/var/log/pve-firewall.log",
> >> +		$param->{start}, $param->{limit}, undef, $param->{since}, $param->{until});
> > Style nit: please use 1 line for each parameter if it gets too long [1]
> > 
> 
> In general we try to adhere to rustfmt style, at least in spirit as not everything 
> can be mapped to perl. That format has an intermediate step between one arg per line
> namely something like:
> 
>     PVE::Tools::foo(
>         $bar, $baz, $very_long_argument_bla_bla)
> 
> Here, that might still work out if we pull out the $param stuff first, e.g.::
> 
>     my ($start, $limit, $since, $until) = param->@{qw(start limit since until)};
> 
>     PVE::Tools::dump_fw_logfile(
>         "/var/log/pve-firewall.log", $start, $limit, undef, $since, $until)

[pve-devel] [RFC common 1/1] tools: Add specialized `dump_fw_logfile` for `since` and `until` filtering of firewall logs

[Christian Ebner]

This furhter includes the contents of rotated logfiles if present. All files are scanned in
sequential order, as there is no guarantee that the rotated logs contain only entries for
a single day.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/PVE/Tools.pm | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index cdbee6d..fdbf0e1 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use Date::Format qw(time2str);
+use Date::Parse qw(str2time);
 use Digest::MD5;
 use Digest::SHA;
 use Encode;
@@ -17,6 +18,7 @@ use IO::Handle;
 use IO::Pipe;
 use IO::Select;
 use IO::Socket::IP;
+use IO::Zlib;
 use IPC::Open3;
 use JSON;
 use POSIX qw(EINTR EEXIST EOPNOTSUPP);
@@ -1320,6 +1322,84 @@ sub dump_logfile {
     return ($count, $lines);
 }
 
+sub dump_fw_logfile {
+    my ($filename, $start, $limit, $filter, $since, $until) = @_;
+
+    if (!(defined($since) || defined($until))) {
+	# Use previous version without `since` and `until` parameters
+	return dump_logfile($filename, $start, $limit, $filter);
+    }
+
+    my $lines = [];
+    my $count = 0;
+
+    # Take into consideration also rotated logs                                              
+    my ($basename, $logdir, $type) = fileparse($filename);
+    my @files = ();                                                                          
+
+    opendir(LOGDIR, $logdir) || die "Cannot open $logdir";
+    my $entry;
+    while ($entry = readdir(LOGDIR)) {
+	my $namefilter = $basename."*";
+	next if $entry !~ m/$namefilter/;
+	push @files, $entry;
+    }
+    closedir(LOGDIR);
+    @files = reverse sort @files;
+    print @files,"\n";
+
+    $start = $start // 0;
+    $limit = $limit // 50;
+
+    my $read_until_end = $limit == 0;
+    my $line;
+    my $fh;
+
+    foreach (@files) {
+	my ($base, $path, $type) = fileparse($_, ".gz");
+
+	if ($type eq '.gz') {
+	    $fh = IO::Zlib->new($logdir.$_, "r");
+	} else {
+	    $fh = IO::File->new($logdir.$_, "r");
+	}
+
+	if (!$fh) {
+	    $count++;
+	    push @$lines, { n => $count, t => "unable to open file - $!"};
+	    return ($count, $lines);
+	}
+
+	while (defined($line = <$fh>)) {
+	    next if defined($filter) && $line !~ m/$filter/;
+	    if ($since || $until) {
+		my @words = split / /, $line;
+		my $timestamp = str2time($words[3], $words[4]);
+		next if $since && $timestamp < $since;
+		next if $until && $timestamp > $until;
+	    }
+	    next if $count++ < $start;
+	    if (!$read_until_end) {
+		next if $limit <= 0;
+		$limit--;
+	    }
+	    chomp $line;
+	    push @$lines, { n => $count, t => $line};
+	}
+
+	close($fh);
+    }
+
+    # HACK: ExtJS store.guaranteeRange() does not like empty array
+    # so we add a line
+    if (!$count) {
+	$count++;
+	push @$lines, { n => $count, t => "no content"};
+    }
+
+    return ($count, $lines);
+}
+
 sub dump_journal {
     my ($start, $limit, $since, $until, $service) = @_;
 
-- 
2.30.2

Re: [pve-devel] [RFC common 1/1] tools: Add specialized `dump_fw_logfile` for `since` and `until` filtering of firewall logs

[Wolfgang Bumiller]

On Thu, Jan 05, 2023 at 10:18:04AM +0100, Christian Ebner wrote:
> This furhter includes the contents of rotated logfiles if present. All files are scanned in
> sequential order, as there is no guarantee that the rotated logs contain only entries for
> a single day.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/PVE/Tools.pm | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index cdbee6d..fdbf0e1 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -4,6 +4,7 @@ use strict;
>  use warnings;
>  
>  use Date::Format qw(time2str);
> +use Date::Parse qw(str2time);
>  use Digest::MD5;
>  use Digest::SHA;
>  use Encode;
> @@ -17,6 +18,7 @@ use IO::Handle;
>  use IO::Pipe;
>  use IO::Select;
>  use IO::Socket::IP;
> +use IO::Zlib;
>  use IPC::Open3;
>  use JSON;
>  use POSIX qw(EINTR EEXIST EOPNOTSUPP);
> @@ -1320,6 +1322,84 @@ sub dump_logfile {
>      return ($count, $lines);
>  }
>  
> +sub dump_fw_logfile {

So initially I thought, With this being firewall-specific I'd rather put
this in pve-firewall I think.
But given the 'HACK' and the copying of the $start/$limit/$filter
portion, maybe just split dump_logfile into a reusable part to which you
provide the IO handle and running $count as parameter instead of the
filename (IO handle because of the zlib part), and which recognizes when
`$filter` is a `sub` to call instead of just a regex (`ref($filter) eq
'CODE'), or simply add those 2 parameters to dump_logfile directly.
At least I'd prefer more code reuse if it's already in this module.

Re: [pve-devel] [RFC common 1/1] tools: Add specialized `dump_fw_logfile` for `since` and `until` filtering of firewall logs

[Christian Ebner]

Okay, I will adapt the code based on your comments. Thx!

> On 05.01.2023 14:25 CET Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
>  
> On Thu, Jan 05, 2023 at 10:18:04AM +0100, Christian Ebner wrote:
> > This furhter includes the contents of rotated logfiles if present. All files are scanned in
> > sequential order, as there is no guarantee that the rotated logs contain only entries for
> > a single day.
> > 
> > Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> > ---
> >  src/PVE/Tools.pm | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> > index cdbee6d..fdbf0e1 100644
> > --- a/src/PVE/Tools.pm
> > +++ b/src/PVE/Tools.pm
> > @@ -4,6 +4,7 @@ use strict;
> >  use warnings;
> >  
> >  use Date::Format qw(time2str);
> > +use Date::Parse qw(str2time);
> >  use Digest::MD5;
> >  use Digest::SHA;
> >  use Encode;
> > @@ -17,6 +18,7 @@ use IO::Handle;
> >  use IO::Pipe;
> >  use IO::Select;
> >  use IO::Socket::IP;
> > +use IO::Zlib;
> >  use IPC::Open3;
> >  use JSON;
> >  use POSIX qw(EINTR EEXIST EOPNOTSUPP);
> > @@ -1320,6 +1322,84 @@ sub dump_logfile {
> >      return ($count, $lines);
> >  }
> >  
> > +sub dump_fw_logfile {
> 
> So initially I thought, With this being firewall-specific I'd rather put
> this in pve-firewall I think.
> But given the 'HACK' and the copying of the $start/$limit/$filter
> portion, maybe just split dump_logfile into a reusable part to which you
> provide the IO handle and running $count as parameter instead of the
> filename (IO handle because of the zlib part), and which recognizes when
> `$filter` is a `sub` to call instead of just a regex (`ref($filter) eq
> 'CODE'), or simply add those 2 parameters to dump_logfile directly.
> At least I'd prefer more code reuse if it's already in this module.