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.nun...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
