[pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
Wolfgang Bumiller
w.bumiller at proxmox.com
Fri Dec 23 11:36:53 CET 2022
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 <l.nunner at proxmox.com>
> ---
> 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
More information about the pve-devel
mailing list