Re: [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file

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

sorry for the delayed reply

some nits & please rebase ;-)

On Mon, Oct 24, 2022 at 04:33:58PM +0200, Stefan Hrdlicka wrote:
> for large IP sets (for example > 25k) it takes noticable longer to parse the
> files, this commit caches the cluster.fw file and reduces parsing time
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 110 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 77 insertions(+), 33 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index c56e448..9077995 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -24,6 +24,12 @@ use PVE::SafeSyslog;
>  use PVE::Tools qw($IPV4RE $IPV6RE);
>  use PVE::Tools qw(run_command lock_file dir_glob_foreach);
>  
> +PVE::Cluster::cfs_register_file(
> +    "firewall/cluster.fw",
> +    \&parse_clusterfw_config,
> +    \&_save_clusterfw_conf
> +);
> +
>  my $pvefw_conf_dir = "/etc/pve/firewall";
>  my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
>  
> @@ -2951,23 +2957,28 @@ sub parse_alias {
>      return undef;
>  }
>  
> -sub generic_fw_config_parser {
> -    my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
> -
> -    my $section;
> -    my $group;
> +sub parse_clusterfw_config {
> +    my ($filename, $raw) = @_;
> +    my $empty_conf = {
> +	rules => [],
> +	options => {},
> +	aliases => {},
> +	groups => {},
> +	group_comments => {},
> +	ipset => {} ,
> +	ipset_comments => {},
> +    };
>  
> -    my $res = $empty_conf;
> +    return _generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster', $raw);
> +}
>  
> -    my $raw;
> -    if ($filename =~ m!^/etc/pve/(.*)$!) {
> -	$raw = PVE::Cluster::get_config($1);
> -    } else {
> -	$raw = eval { PVE::Tools::file_get_contents($filename) }; # ignore errors
> -    }
> -    return {} if !$raw;
> +sub _generic_fw_config_parser {
> +    my ($filename, $cluster_conf, $empty_conf, $rule_env, $raw) = @_;
>  
> +    my $section;
> +    my $group;
>      my $curr_group_keys = {};
> +    my $res = $empty_conf;
>  
>      my $linenr = 0;
>      while ($raw =~ /^\h*(.*?)\h*$/gm) {
> @@ -3130,6 +3141,26 @@ sub generic_fw_config_parser {
>      return $res;
>  }
>  
> +sub generic_fw_config_parser {
> +    my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
> +
> +    my $section;
> +    my $group;
> +
> +    my $res = $empty_conf;
> +
> +    my $raw;
> +    if ($filename =~ m!^/etc/pve/(.*)$!) {
> +	$raw = PVE::Cluster::get_config($1);
> +    } else {
> +	$raw = eval { PVE::Tools::file_get_contents($filename) }; # ignore errors
> +    }
> +    return {} if !$raw;
> +
> +    my $curr_group_keys = {};
> +    return _generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env, $raw);
> +}
> +
>  # this is only used to prevent concurrent runs of rule compilation/application
>  # see lock_*_conf for cfs locks protectiong config modification
>  sub run_locked {
> @@ -3564,26 +3595,44 @@ sub lock_clusterfw_conf {
>  sub load_clusterfw_conf {
>      my ($filename) = @_;
>  
> -    $filename = $clusterfw_conf_filename if !defined($filename);
> -    my $empty_conf = {
> -	rules => [],
> -	options => {},
> -	aliases => {},
> -	groups => {},
> -	group_comments => {},
> -	ipset => {} ,
> -	ipset_comments => {},
> -    };
> +    # special case for tests
> +    if ($filename) {
> +	$filename = $clusterfw_conf_filename if !defined($filename);

^ The above line can be dropped given its condition now ;-)

> +	my $empty_conf = {
> +	    rules => [],
> +	    options => {},
> +	    aliases => {},
> +	    groups => {},
> +	    group_comments => {},
> +	    ipset => {} ,
> +	    ipset_comments => {},
> +	};
> +
> +	my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
> +	$set_global_log_ratelimit->($cluster_conf->{options});
>  
> -    my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
> -    $set_global_log_ratelimit->($cluster_conf->{options});
> +	return $cluster_conf;
> +
> +    } else {
> +	my $res = "";

^ should be {}

> +	eval {
> +	    $res = PVE::Cluster::cfs_read_file("firewall/cluster.fw");
> +	};

I think a `warn $@ if $@` might be good here now.

> +
> +	return $res;
> +    }
>  
> -    return $cluster_conf;
>  }
>  
>  sub save_clusterfw_conf {
>      my ($cluster_conf) = @_;
>  
> +    PVE::Cluster::cfs_write_file("firewall/cluster.fw", $cluster_conf)
> +}
> +
> +sub _save_clusterfw_conf {
> +    my ($filename, $cluster_conf) = @_;
> +
>      my $raw = '';
>  
>      my $options = $cluster_conf->{options};
> @@ -3615,13 +3664,7 @@ sub save_clusterfw_conf {
>  	    $raw .= "\n";
>  	}
>      }
> -
> -    if ($raw) {
> -	mkdir $pvefw_conf_dir;
> -	PVE::Tools::file_set_contents($clusterfw_conf_filename, $raw);
> -    } else {
> -	unlink $clusterfw_conf_filename;
> -    }
> +    return $raw
>  }
>  
>  sub lock_hostfw_conf {
> @@ -4617,4 +4660,5 @@ sub update {
>      run_locked($code);
>  }
>  
> +
>  1;
> -- 
> 2.30.2