From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 477C890CE for ; Thu, 17 Nov 2022 10:49:29 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 235CE29FEB for ; Thu, 17 Nov 2022 10:48:59 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 17 Nov 2022 10:48:58 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0F8AE43CF4 for ; Thu, 17 Nov 2022 10:48:58 +0100 (CET) Date: Thu, 17 Nov 2022 10:48:56 +0100 From: Wolfgang Bumiller To: Stefan Hrdlicka Cc: pve-devel@lists.proxmox.com Message-ID: <20221117094856.grwopspovz7h526w@casey.proxmox.com> References: <20221024143359.4194299-1-s.hrdlicka@proxmox.com> <20221024143359.4194299-2-s.hrdlicka@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221024143359.4194299-2-s.hrdlicka@proxmox.com> X-SPAM-LEVEL: Spam detection results: =?UTF-8?Q?0=0A=09?=AWL 0.230 Adjusted score from AWL reputation of From: =?UTF-8?Q?address=0A=09?=BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict =?UTF-8?Q?Alignment=0A=09?=SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF =?UTF-8?Q?Record=0A=09?=SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2022 09:49:29 -0000 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 > --- > 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