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 83CD691177 for ; Mon, 26 Sep 2022 11:47:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 64CF52534 for ; Mon, 26 Sep 2022 11:46:49 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 26 Sep 2022 11:46:47 +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 784CE44564 for ; Mon, 26 Sep 2022 11:46:47 +0200 (CEST) From: Leo Nunner To: pve-devel@lists.proxmox.com Date: Mon, 26 Sep 2022 11:45:07 +0200 Message-Id: <20220926094507.46263-1-l.nunner@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.109 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 Subject: [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: Mon, 26 Sep 2022 09:47:19 -0000 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}; + 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}; - 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 + $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}); + + # 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 { - 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