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.hrdli...@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@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
