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 4ABBD91AE0 for ; Fri, 23 Dec 2022 11:36:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3383123FCA for ; Fri, 23 Dec 2022 11:36: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 ; Fri, 23 Dec 2022 11:36:56 +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 4BBC244A01 for ; Fri, 23 Dec 2022 11:36:56 +0100 (CET) Date: Fri, 23 Dec 2022 11:36:53 +0100 From: Wolfgang Bumiller To: Leo Nunner Cc: pve-devel@lists.proxmox.com Message-ID: <20221223103525.4kth7kuzllqzlnnv@fwblub> References: <20221222155232.149115-1-l.nunner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20221222155232.149115-1-l.nunner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.219 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, aliases.pm, helpers.pm, ipset.pm, firewall.pm] Subject: Re: [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets 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: Fri, 23 Dec 2022 10:36:59 -0000 On Thu, Dec 22, 2022 at 04:52:32PM +0100, Leo Nunner wrote: > Renaming an alias or an IPset broke existing firewall rules, both > standalone and as part of a security group. This was already fixed with > security groups (see issue #4204), so it was only a matter of factoring > out the relevant code and providing a more general implementation, so > that it could be reused for the other components. > > The mechanism works the same for all three components: first, the > existing object is duplicated with a new name. Then, all the occurences > inside rules are replaced with the new name, and, when this is done, the > old object is deleted. This ensures that a failure while updating (such > as a timeout due to the inability to lock a config file) does not break > the rules that could not be updated yet. > > In PVE::Firewall::Helpers, a function is now provided to globally update > all configs (nodes, VMs, CTs) using a custom function that is passed as > a parameter. Each config is locked, passed to the function and then > written. > > The update endpoint for aliases was also cleaned up to make the needed > parameters more logical, i.e. just updating the name/comment does not > require a CIDR to be passed along as well anymore. > > Signed-off-by: Leo Nunner > --- > RFC: > - I'm not really sure if the approach with the function in > PVE::Firewall::Helpers is the best option, but it might also be > useful in other places? Better than duplicate code, though I'm beginning to have a strong aversion to modules named 'Helpers' or 'Tools' and would rather have this in `PVE::Firewall` even if it's quite full. BUT, that's for another time, for now, keep it that way, maybe at some later point we can find a better way to organize this for the whole Firewall.pm. > - Should the cleanup in the update endpoint for aliases go in a separate > patch? I figured I might as well do that now, since I was changing > quite a few things around anyway in that function. Generally speaking, yes, separating patches are usually better since we can also apply partial series to move forward a bit more easily, making future versions of series containing fewer changes. If I'm seeing this right it'll amount to a single line change in this case, (2, but the 2nd patch would have to touch the same other yet again), so one would think it doesn't matter much, yet I did end up looking for remaining `$param->{cidr}` uses in the file, (which were unlikely given the change), which would end up being one thing less to worry about... So yeah... if it's easy, I'd prefer separate patches, but there can of course be situations where that's more work than it's worth. > > src/PVE/API2/Firewall/Aliases.pm | 59 ++++++++++++++++++++++---- > src/PVE/API2/Firewall/Groups.pm | 54 +++++------------------- > src/PVE/API2/Firewall/IPSet.pm | 72 +++++++++++++++++++++++++------- > src/PVE/Firewall/Helpers.pm | 33 +++++++++++++++ > 4 files changed, 149 insertions(+), 69 deletions(-) > > diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm > index 33ac669..b154454 100644 > --- a/src/PVE/API2/Firewall/Aliases.pm > +++ b/src/PVE/API2/Firewall/Aliases.pm > @@ -205,6 +205,7 @@ sub register_update_alias { > $properties->{name} = $api_properties->{name}; > $properties->{rename} = $api_properties->{rename}; > $properties->{cidr} = $api_properties->{cidr}; > + $properties->{cidr}->{optional} = 1; > $properties->{comment} = $api_properties->{comment}; > $properties->{digest} = get_standard_option('pve-config-digest'); > > @@ -235,22 +236,62 @@ sub register_update_alias { > PVE::Tools::assert_if_modified($digest, $param->{digest}); > > my $name = lc($param->{name}); > + my $rename = lc($param->{rename}) if defined($param->{rename}); > > raise_param_exc({ name => "no such alias" }) if !$aliases->{$name}; > > - my $data = { name => $param->{name}, cidr => $param->{cidr} }; > - $data->{comment} = $param->{comment} if $param->{comment}; > - > - $aliases->{$name} = $data; > - > - my $rename = $param->{rename}; > - $rename = lc($rename) if $rename; > + my $alias = $aliases->{$name}; > + $alias->{cidr} = $param->{cidr} if defined($param->{cidr}); > + $alias->{comment} = $param->{comment} if defined($param->{comment}); > > if ($rename && ($name ne $rename)) { > raise_param_exc({ name => "alias '$param->{rename}' already exists" }) > if defined($aliases->{$rename}); > - $aliases->{$name}->{name} = $param->{rename}; > - $aliases->{$rename} = $aliases->{$name}; > + > + # Don't delete old alias yet, only copy it, change the name, and save. > + $aliases->{$rename}->{$_} = $alias->{$_} for keys %{ $alias }; A quicker way to make a shallow clone of a hash is to just put the `%` expansion inside braces: $aliases->{$rename} = { %$alias }; or actually with arrow-dereferencing it looks a bit more explicit IMO, but optional (in this case ;-), see below): $aliases->{$rename} = { $alias->%* }; > + $aliases->{$rename}->{name} = $param->{rename}; > + > + $class->save_aliases($param, $fw_conf, $aliases); > + > + my $rename_fw_rules = sub { > + my ($config) = @_; > + > + for my $rule (@{$config->{rules}}) { > + next if $rule->{type} eq "group"; > + $rule->{source} = $rename if $rule->{source} eq $name; > + $rule->{dest} = $rename if $rule->{dest} eq $name; > + } > + }; > + > + my $rename_ipsets = sub { > + my ($config) = @_; > + > + for my $ipset (values %{$config->{ipset}}) { For anything other than a simple `%$something` code is much easier to follow when sticking to consistent arrows: $config->{ipset}->%* instead of %{$config->{ipset}} since the latter creates "spirals" akin to cdecls (just do an image-search for cdecl spiral :-P) Or compare: - You a $config, you take its `ipset` member, and of that you take the content. - You take the content of the thing you get when you take the config and then fetch its `ipset` member. > + map { $_->{cidr} = $rename if $_->{cidr} eq $name } @{$ipset}; Don't use `map` as a replacement for `for` loops just to cram it into a single line with a suffix-`if` ;-) Use `for`. > + } > + }; > + > + # Figure out scope > + if ($class->rule_env() eq "cluster") { Do we need to go through the files twice here? Can we not do all 3 replacements in 1 run? Since this can be quite expensive as it'll take cluster-wide locks of potentially lots of files... twice. > + PVE::Firewall::Helpers::global_update_configs($fw_conf, $rename_fw_rules); > + > + # Update group definitions > + for my $group (values %{$fw_conf->{groups}}) { > + for my $rule (@{$group}) { > + $rule->{source} = $rename if $rule->{source} eq $name; > + $rule->{dest} = $rename if $rule->{dest} eq $name; > + } > + } > + > + # Update IPsets, since aliases can also be used here > + PVE::Firewall::Helpers::global_update_configs($fw_conf, $rename_ipsets); > + } > + > + $rename_fw_rules->($fw_conf); > + $rename_ipsets->($fw_conf); > + > + # Now we can delete the old one > delete $aliases->{$name}; > } > > diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm > index ffdc45c..6080c6d 100644 > --- a/src/PVE/API2/Firewall/Groups.pm > +++ b/src/PVE/API2/Firewall/Groups.pm > @@ -30,15 +30,6 @@ my $get_security_group_list = sub { > return wantarray ? ($list, $digest) : $list; > }; > > -my $rename_fw_rules = sub { > - my ($old, $new, $rules) = @_; > - > - for my $rule (@{$rules}) { > - next if ($rule->{type} ne "group" || $rule->{action} ne $old); > - $rule->{action} = $new; > - } > -}; > - > __PACKAGE__->register_method({ > name => 'list_security_groups', > path => '', > @@ -136,44 +127,19 @@ __PACKAGE__->register_method({ > # rules won't be broken when the new name is referenced > PVE::Firewall::save_clusterfw_conf($cluster_conf); > > - # 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($rename, > - $group, > - $host_conf->{rules}); > - PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); > - } > - }); > - } > + my $rename_fw_rules = sub { > + my ($config) = @_; > > - # 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($rename, > - $group, > - $vm_conf->{rules}); > - PVE::Firewall::save_vmfw_conf($vm, $vm_conf); > - } > - }); > - } > + for my $rule (@{$config->{rules}}) { > + next if ($rule->{type} ne "group" || $rule->{action} ne $rename); > + $rule->{action} = $group; > + } > + }; > > - # And also update the cluster itself > - &$rename_fw_rules($rename, > - $group, > - $cluster_conf->{rules}); > + # Update VM/CT/cluster rules > + PVE::Firewall::Helpers::global_update_configs($cluster_conf, $rename_fw_rules); > + $rename_fw_rules->($cluster_conf); > > - # Now that everything has been updated, the old rule can be deleted > delete $cluster_conf->{groups}->{$rename}; > delete $cluster_conf->{group_comments}->{$rename}; > } else { > diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm > index 14bcfcb..aeb462d 100644 > --- a/src/PVE/API2/Firewall/IPSet.pm > +++ b/src/PVE/API2/Firewall/IPSet.pm > @@ -642,37 +642,77 @@ sub register_create { > code => sub { > my ($param) = @_; > > + my $name = $param->{name}; > + my $rename = $param->{rename}; > + my $comment = $param->{comment}; > + > $class->lock_config($param, sub { > my ($param) = @_; > > my ($cluster_conf, $fw_conf) = $class->load_config($param); > > - if ($param->{rename}) { > + if ($rename) { > my (undef, $digest) = &$get_ipset_list($fw_conf); > PVE::Tools::assert_if_modified($digest, $param->{digest}); > > - raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" }) Okay I'm confused. Do we really have `rename` mean "rename-TO" in one case and "rename-FROM" in the other? 😒 If so, can we please use `$rename_from` and `$rename_to` as variable names, because this is highly annoying. Also maybe add a patch to expand the `rename` parameter documentation to include what it actually does... > - if !$fw_conf->{ipset}->{$param->{rename}}; > + raise_param_exc({ name => "IPSet '$rename' does not exist" }) > + if !$fw_conf->{ipset}->{$rename}; > > # prevent overwriting existing ipset > - raise_param_exc({ name => "IPSet '$param->{name}' does already exist"}) > - if $fw_conf->{ipset}->{$param->{name}} && > - $param->{name} ne $param->{rename}; > - > - my $data = delete $fw_conf->{ipset}->{$param->{rename}}; > - $fw_conf->{ipset}->{$param->{name}} = $data; > - if (my $comment = delete $fw_conf->{ipset_comments}->{$param->{rename}}) { > - $fw_conf->{ipset_comments}->{$param->{name}} = $comment; > + raise_param_exc({ name => "IPSet '$name' does already exist"}) > + if $fw_conf->{ipset}->{$name} && > + $name ne $rename; > + > + if ($rename eq $name) { > + $fw_conf->{ipset_comments}->{$rename} = $comment if defined($comment); > + $class->save_config($param, $fw_conf); > + return; > } > - $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment}); > + > + # Create a copy of the old IPset ^ Maybe note that we're only referencing it and not actually copying since we don't need to touch the contents, unless I'm missing something and this should instead perform a copy like in the alias case? > + $fw_conf->{ipset}->{$name} = $fw_conf->{ipset}->{$rename}; > + $fw_conf->{ipset_comments}->{$name} = $fw_conf->{ipset_comments}->{$rename}; > + > + # Update comment if provided > + $fw_conf->{ipset_comments}->{$name} = $comment if defined($comment); > + > + $class->save_config($param, $fw_conf); > + > + my $rename_fw_rules = sub { > + my ($config) = @_; > + > + for my $rule (@{$config->{rules}}) { > + next if $rule->{type} eq "group"; > + $rule->{source} = "+$name" if $rule->{source} eq "+$rename"; > + $rule->{dest} = "+$name" if $rule->{dest} eq "+$rename"; > + } > + }; > + > + # Figure out scope > + if ($class->rule_env() eq "cluster") { > + PVE::Firewall::Helpers::global_update_configs($fw_conf, $rename_fw_rules); > + > + # Update group definitions > + for my $group (values %{$fw_conf->{groups}}) { > + for my $rule (@{$group}) { > + $rule->{source} = "+$name" if $rule->{source} eq "+$rename"; > + $rule->{dest} = "+$name" if $rule->{dest} eq "+$rename"; > + } > + } > + } > + > + $rename_fw_rules->($fw_conf); > + > + delete $fw_conf->{ipset}->{$rename}; > + delete $fw_conf->{ipset_comments}->{$rename}; > } else { > - foreach my $name (keys %{$fw_conf->{ipset}}) { > + foreach my $set (keys %{$fw_conf->{ipset}}) { > raise_param_exc({ name => "IPSet '$name' already exists" }) > - if $name eq $param->{name}; > + if $set eq $name; Hold on... 🧐 So either I'm missing something or this should just not be a loop? just use raise_... if $fw_conf->{ipset}->{$name}; like in the other branch? > } > > - $fw_conf->{ipset}->{$param->{name}} = []; > - $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment}); > + $fw_conf->{ipset}->{$name} = []; > + $fw_conf->{ipset_comments}->{$name} = $comment if defined($comment); > } > > $class->save_config($param, $fw_conf); > diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm > index 154fca5..3137b33 100644 > --- a/src/PVE/Firewall/Helpers.pm > +++ b/src/PVE/Firewall/Helpers.pm > @@ -11,6 +11,7 @@ our @EXPORT_OK = qw( > lock_vmfw_conf > remove_vmfw_conf > clone_vmfw_conf > +global_update_configs > ); > > my $pvefw_conf_dir = "/etc/pve/firewall"; > @@ -52,4 +53,36 @@ sub clone_vmfw_conf { > }); > } > > +# Updates all VM and CT configs. For every host/VM/CT, a custom function > +# $fun is called with the config object as a parameter. > +sub global_update_configs { > + my ($cluster_conf, $fun) = @_; > + > + 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)) { > + $fun->($host_conf); > + PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); > + } > + }); > + } > + > + 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)) { > + $fun->($vm_conf); > + PVE::Firewall::save_vmfw_conf($vm, $vm_conf); > + } > + }); > + } > +} > + > 1; > -- > 2.30.2