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 4928B9141B for ; Tue, 27 Sep 2022 11:28:30 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 287D51CCC1 for ; Tue, 27 Sep 2022 11:28:30 +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 11:28:28 +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 F0E68444B7 for ; Tue, 27 Sep 2022 11:28:27 +0200 (CEST) Message-ID: Date: Tue, 27 Sep 2022 11:28:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Content-Language: en-US To: Wolfgang Bumiller Cc: pve-devel@lists.proxmox.com References: <20220926094507.46263-1-l.nunner@proxmox.com> <20220927084632.c5lz3xgtzkou5asi@casey.proxmox.com> From: Leo Nunner In-Reply-To: <20220927084632.c5lz3xgtzkou5asi@casey.proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.246 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 NICE_REPLY_A -2.319 Looks like a legit reply (A) 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. [rules.pm, groups.pm, host.pm, firewall.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 09:28:30 -0000 On 9/27/22 10:46, Wolfgang Bumiller wrote: > 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. Agreed, that would make it much more straightforward to read. >> >> - 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... Is there actually a way around this? Unless we use something like inotify, there'll be no way for them to actually know about the new group if they've read the cluster config before I 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