* [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
@ 2022-12-22 15:52 Leo Nunner
2022-12-23 10:36 ` Wolfgang Bumiller
0 siblings, 1 reply; 3+ messages in thread
From: Leo Nunner @ 2022-12-22 15:52 UTC (permalink / raw)
To: pve-devel
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@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?
- 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.
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 };
+ $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}}) {
+ map { $_->{cidr} = $rename if $_->{cidr} eq $name } @{$ipset};
+ }
+ };
+
+ # 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} = $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" })
- 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
+ $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;
}
- $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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
2022-12-22 15:52 [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets Leo Nunner
@ 2022-12-23 10:36 ` Wolfgang Bumiller
2022-12-28 9:37 ` Leo Nunner
0 siblings, 1 reply; 3+ messages in thread
From: Wolfgang Bumiller @ 2022-12-23 10:36 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
2022-12-23 10:36 ` Wolfgang Bumiller
@ 2022-12-28 9:37 ` Leo Nunner
0 siblings, 0 replies; 3+ messages in thread
From: Leo Nunner @ 2022-12-28 9:37 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-28 9:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-22 15:52 [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets Leo Nunner
2022-12-23 10:36 ` Wolfgang Bumiller
2022-12-28 9:37 ` Leo Nunner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox