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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E2B2A91483 for ; Tue, 27 Sep 2022 10:47:05 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C462F1C753 for ; Tue, 27 Sep 2022 10:46:35 +0200 (CEST) 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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 27 Sep 2022 10:46:34 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4B267445BF for ; Tue, 27 Sep 2022 10:46:34 +0200 (CEST) Date: Tue, 27 Sep 2022 10:46:32 +0200 From: Wolfgang Bumiller To: Leo Nunner Cc: pve-devel@lists.proxmox.com Message-ID: <20220927084632.c5lz3xgtzkou5asi@casey.proxmox.com> References: <20220926094507.46263-1-l.nunner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220926094507.46263-1-l.nunner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.259 Adjusted score from AWL reputation of From: address 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 Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [groups.pm, host.pm, firewall.pm, rules.pm] Subject: Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed 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: Tue, 27 Sep 2022 08:47:06 -0000 On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote: > When renaming a group, the usages didn't get updated automatically. To > get around problems with atomicity, the old rule is first cloned with the > new name, the usages are updated and only when updating has finished, the > old rule is deleted. > > The subroutines that lock/update host configs had to be changed so that > it's possible to lock any config, not just the one of the current host. > > Signed-off-by: Leo Nunner > --- > RFC: for locking hosts, I'm currently passing `undef` when I want to > access the current host. Getting the hostname for each call seems like a > bit of an overhead, and I'm unsure about introducing global variables in > the classes where this subroutine needs to be called. > > src/PVE/API2/Firewall/Groups.pm | 64 ++++++++++++++++++++++++++++++--- > src/PVE/API2/Firewall/Host.pm | 2 +- > src/PVE/API2/Firewall/Rules.pm | 2 +- > src/PVE/Firewall.pm | 14 +++++--- > 4 files changed, 71 insertions(+), 11 deletions(-) > > diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm > index 558ba8e..052ff41 100644 > --- a/src/PVE/API2/Firewall/Groups.pm > +++ b/src/PVE/API2/Firewall/Groups.pm > @@ -30,6 +30,15 @@ my $get_security_group_list = sub { > return wantarray ? ($list, $digest) : $list; > }; > > +my $rename_fw_rules = sub { > + my ($old, $new, $rules) = @_; > + > + my @matches = grep { $_->{type} eq "group" && $_->{action} eq $old } @{$rules}; I think it's a bit easier to just do `nest if ...` in a for-loop over @$rules. > + for my $rule (@matches) { > + $rule->{action} = $new; > + } > +}; > + > __PACKAGE__->register_method({ > name => 'list_security_groups', > path => '', > @@ -106,12 +115,59 @@ __PACKAGE__->register_method({ > if $cluster_conf->{groups}->{$param->{group}} && > $param->{group} ne $param->{rename}; If we're already touching the code here, I'd really like it if we moved the `$param->{group/rename/comment}` into their own `$groupname`, `$rename`, `$comment` vars, as we have just sooo many nested hash accesses all around here. > > - my $data = delete $cluster_conf->{groups}->{$param->{rename}}; > - $cluster_conf->{groups}->{$param->{group}} = $data; > - if (my $comment = delete $cluster_conf->{group_comments}->{$param->{rename}}) { > - $cluster_conf->{group_comments}->{$param->{group}} = $comment; > + if ($param->{rename} eq $param->{group}) { > + $cluster_conf->{group_comments}->{$param->{rename}} = $param->{comment} if defined($param->{comment}); > + PVE::Firewall::save_clusterfw_conf($cluster_conf); > + return; > } > + > + # Create an exact copy of the old security group (technically just a reference) > + $cluster_conf->{groups}->{$param->{group}} = $cluster_conf->{groups}->{$param->{rename}}; > + $cluster_conf->{group_comments}->{$param->{group}} = $cluster_conf->{group_comments}->{$param->{rename}}; > + > + # Update comment if provided > $cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment}); > + At this point you'd need to also store the cluster fw config, because *reading* the configs isn't necessarily done with a lock on the cluster config, and you don't want to race against readers seeing the new group being referred to without actually having the it in the config. You'll still be racing against clients having read the cluster config while you're *here* and then reading their host config *after* you've updated it... Plus, if anything after the first host-firewall was written `die()`s, all the already changed host firewalls will be broken. So it's better to literally have a copy of the security group in the cluster config in that case. > + # Update all the host configs to the new copy > + my $hosts = PVE::Cluster::get_nodelist(); > + foreach my $host (@$hosts) { > + PVE::Firewall::lock_hostfw_conf($host, 10, sub { > + my $host_conf_path = "/etc/pve/nodes/$host/host.fw"; > + my $host_conf = PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path); > + > + if(defined($host_conf)) { > + &$rename_fw_rules($param->{rename}, > + $param->{group}, > + $host_conf->{rules}); > + PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); > + } > + }); > + } > + > + # Update all the VM configs > + my $vms = PVE::Cluster::get_vmlist(); > + foreach my $vm (keys %{$vms->{ids}}) { > + PVE::Firewall::lock_vmfw_conf($vm, 10, sub { > + my $vm_type = $vms->{ids}->{$vm}->{type} eq "lxc" ? "ct" : "vm"; > + my $vm_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, $vm_type, $vm, "/etc/pve/firewall"); > + > + if (defined($vm_conf)) { > + &$rename_fw_rules($param->{rename}, > + $param->{group}, > + $vm_conf->{rules}); > + PVE::Firewall::save_vmfw_conf($vm, $vm_conf); > + } > + }); > + } > + > + # And also update the cluster itself > + &$rename_fw_rules($param->{rename}, > + $param->{group}, > + $cluster_conf->{rules}); > + > + # Now that everything has been updated, the old rule can be deleted > + delete $cluster_conf->{groups}->{$param->{rename}}; > + delete $cluster_conf->{group_comments}->{$param->{rename}}; > } else { > foreach my $name (keys %{$cluster_conf->{groups}}) { > raise_param_exc({ group => "Security group '$name' already exists" }) > diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm > index b66ca55..dfeccd0 100644 > --- a/src/PVE/API2/Firewall/Host.pm > +++ b/src/PVE/API2/Firewall/Host.pm > @@ -118,7 +118,7 @@ __PACKAGE__->register_method({ > code => sub { > my ($param) = @_; > > - PVE::Firewall::lock_hostfw_conf(10, sub { > + PVE::Firewall::lock_hostfw_conf(undef, 10, sub { > my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); > my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf); > > diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm > index 4475663..9fcfb20 100644 > --- a/src/PVE/API2/Firewall/Rules.pm > +++ b/src/PVE/API2/Firewall/Rules.pm > @@ -522,7 +522,7 @@ sub rule_env { > sub lock_config { > my ($class, $param, $code) = @_; > > - PVE::Firewall::lock_hostfw_conf(10, $code, $param); > + PVE::Firewall::lock_hostfw_conf(undef, 10, $code, $param); > } > > sub load_config { > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm > index c56e448..7ffd09b 100644 > --- a/src/PVE/Firewall.pm > +++ b/src/PVE/Firewall.pm > @@ -3625,9 +3625,11 @@ sub save_clusterfw_conf { > } > > sub lock_hostfw_conf { ^ maybe add a `: prototype($$$@)` while you're at it. Though it won't actually catch old callers due to the the `@param` style... > - my ($timeout, $code, @param) = @_; > + my ($node, $timeout, $code, @param) = @_; > + > + $node = $nodename if !defined($node); > > - my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout, $code, @param); > + my $res = PVE::Cluster::cfs_lock_firewall("host-$node", $timeout, $code, @param); > die $@ if $@; > > return $res; > @@ -3643,7 +3645,9 @@ sub load_hostfw_conf { > } > > sub save_hostfw_conf { > - my ($hostfw_conf) = @_; > + my ($hostfw_conf, $filename) = @_; > + > + $filename = $hostfw_conf_filename if !defined($filename); > > my $raw = ''; > > @@ -3658,9 +3662,9 @@ sub save_hostfw_conf { > } > > if ($raw) { > - PVE::Tools::file_set_contents($hostfw_conf_filename, $raw); > + PVE::Tools::file_set_contents($filename, $raw); > } else { > - unlink $hostfw_conf_filename; > + unlink $filename; > } > } > > -- > 2.30.2