* [pve-devel] [PATCH v2 firewall 2/2] fix #4414: automatically rename usages of aliases and IPsets
2023-01-26 14:31 [pve-devel] [PATCH v2 firewall] fix #4414: automatically rename usages of aliases and IPsets Leo Nunner
2023-01-26 14:31 ` [pve-devel] [PATCH v2 firewall 1/2] allow updating an alias without providing a CIDR Leo Nunner
@ 2023-01-26 14:31 ` Leo Nunner
1 sibling, 0 replies; 3+ messages in thread
From: Leo Nunner @ 2023-01-26 14:31 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.
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 52 +++++++++++++++++++++++++++---
src/PVE/API2/Firewall/Groups.pm | 54 ++++++--------------------------
src/PVE/API2/Firewall/IPSet.pm | 47 +++++++++++++++++++++++++--
src/PVE/Firewall/Helpers.pm | 33 +++++++++++++++++++
4 files changed, 135 insertions(+), 51 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index c84c348..4c0163f 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -249,15 +249,59 @@ sub register_update_alias {
raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
- my $data = { name => $rename_from, cidr => $param->{cidr} // $aliases->{$rename_from}->{cidr} };
- $data->{comment} = $param->{comment} if $param->{comment};
-
- $aliases->{$rename_from} = $data;
+ my $alias = $aliases->{$rename_from};
+ $alias->{cidr} = $param->{cidr} if defined($param->{cidr});
+ $alias->{comment} = $param->{comment} if defined($param->{comment});
if ($rename_to && ($rename_from ne $rename_to)) {
raise_param_exc({ name => "alias '$param->{rename}' already exists" })
if grep { lc($_) eq lc($rename_to) } (keys $aliases->%*);
$aliases->{$rename_to} = $aliases->{$rename_from};
+ $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_to if lc($rule->{source}) eq lc($rename_from);
+ $rule->{dest} = $rename_to if lc($rule->{dest}) eq lc($rename_from);
+ }
+ };
+
+ my $rename_ipsets = sub {
+ my ($config) = @_;
+
+ for my $ipset (values $config->{ipset}->%*) {
+ for my $ip ($ipset->{entries}->@*) {
+ $ip->{cidr} = $rename_to if lc($ip->{cidr}) eq lc($rename_from);
+ }
+ }
+ };
+
+ # Figure out scope
+ if ($class->rule_env() eq "cluster") {
+ # Update rules and IPsets
+ PVE::Firewall::Helpers::global_update_configs($fw_conf, sub {
+ my ($config) = @_;
+
+ $rename_fw_rules->($config);
+ $rename_ipsets->($config);
+ });
+
+ # Update group definitions
+ for my $group (values $fw_conf->{groups}->%*) {
+ for my $rule ($group->{rules}->@*) {
+ $rule->{source} = $rename_to if lc($rule->{source}) eq lc($rename_from);
+ $rule->{dest} = $rename_to if lc($rule->{dest}) eq lc($rename_from);
+ }
+ }
+ }
+
+ $rename_fw_rules->($fw_conf);
+ $rename_ipsets->($fw_conf);
+
+ # Now we can delete the old one
delete $aliases->{$rename_from};
}
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index 3bf01ac..524fd46 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -28,15 +28,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 => '',
@@ -131,44 +122,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_from,
- $rename_to,
- $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_from,
- $rename_to,
- $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_from);
+ $rule->{action} = $rename_to;
+ }
+ };
- # And also update the cluster itself
- &$rename_fw_rules($rename_from,
- $rename_to,
- $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_from};
} else {
# In this context, $rename_to is the name for the new group
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index aae9006..6e0829a 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -642,6 +642,10 @@ sub register_create {
code => sub {
my ($param) = @_;
+ my $rename_to = $param->{name};
+ my $rename_from = $param->{rename};
+ my $comment = $param->{comment};
+
$class->lock_config($param, sub {
my ($param) = @_;
@@ -663,9 +667,46 @@ sub register_create {
if (grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*)) &&
$rename_to ne $rename_from;
- my $data = delete $fw_conf->{ipset}->{$rename_from};
- $fw_conf->{ipset}->{$rename_to} = $data;
- $fw_conf->{ipset}->{$rename_to}->{comment} = $param->{comment} if defined($param->{comment});
+ if ($rename_from eq $rename_to) {
+ $fw_conf->{ipset}->{$rename_from}->{comment} = $comment if defined($comment);
+ $class->save_config($param, $fw_conf);
+ return;
+ }
+
+ # Create a reference to the old IPset, so that both the old and the new name point to the same set
+ $fw_conf->{ipset}->{$rename_to} = $fw_conf->{ipset}->{$rename_from};
+
+ # Update comment if provided
+ $fw_conf->{ipset}->{$rename_to}->{comment} = $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} = "+$rename_to" if $rule->{source} eq "+$rename_from";
+ $rule->{dest} = "+$rename_to" if $rule->{dest} eq "+$rename_from";
+ }
+ };
+
+ # 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->{rules}->@*) {
+ $rule->{source} = "+$rename_to" if $rule->{source} eq "+$rename_from";
+ $rule->{dest} = "+$rename_to" if $rule->{dest} eq "+$rename_from";
+ }
+ }
+ }
+
+ $rename_fw_rules->($fw_conf);
+
+ delete $fw_conf->{ipset}->{$rename_from};
} else {
# In this context, $rename_to is the name for the new IPSet
raise_param_exc({ name => "IPSet '$rename_to' already exists" })
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