[pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
Leo Nunner
l.nunner at proxmox.com
Wed Dec 28 10:37:16 CET 2022
On 2022-12-23 11:36, Wolfgang Bumiller wrote:
> 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.
Alright, I think for a future patch it shouldn't be too hard to merge
this back into another file, since it's basically just a collection of
functions to manipulate config files…
>> - 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.
I'll try to see how I can best separate this into two patches.
>> 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->%* };
I considered going with the first solution actually, but thought it
might be more readable the other way ^^ I'll change it to the
dereferencing style.
>> + $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.
That does seem like a cleaner approach, thanks!
>> + 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`.
Will do.
>> + }
>> + };
>> +
>> + # 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.
True, calling rename_fw_rules and rename_ipsets in a single run does
make way more sense. The group replacement needs to be separate since
groups can only be declared on a cluster-wide level, but it doesn't do
any locking on its own anyway.
>> + 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...
Yep, with groups and IPsets, "rename" specifies the object to be
renamed, while with aliases, it's the new name… I'll send a patch to
update the documentation, and will rename the variables accordingly.
>> - 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?
Will do, a copying operation isn't necessary here.
>> + $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?
The same loop seems to exist in the groups endpoint too [1] 🤔. But I
agree, it doesn't make much sense to have a loop here…
[1]
https://git.proxmox.com/?p=pve-firewall.git;a=blob;f=src/PVE/API2/Firewall/Groups.pm;h=ffdc45c1889e3d2af1906e412bca2601601c595f;hb=HEAD#l180
>> }
>>
>> - $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