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

[Roland]

nice!

btw, what about this one ?

https://bugzilla.proxmox.com/show_bug.cgi?id=3909#c3

actually, the firewall stuff is getting blindly executed every 10
seconds, that's causing a lot of noise.

couldn't/shouldn't this be handled more intelligenty ?

roland

Am 17.11.22 um 10:48 schrieb Wolfgang Bumiller:
> 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 at 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
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>