fix #4204: automatically update usages of group when it is renamed

When renaming a group, the usages didn't get updated automatically. To
get around problems with atomicity, the old rule is first cloned with the
new name, the usages are updated and only when updating has finished, the
old rule is deleted.

The subroutines that lock/update host configs had to be changed so that
it's possible to lock any config, not just the one of the current host.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>

diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index 558ba8ee893c5fd29e8595bea5a857478616a02e..22da9a8519232b50db37aea24e2d900136ab4347 100644 (file)
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -30,6 +30,15 @@ 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 => '',
@@ -91,35 +100,90 @@ __PACKAGE__->register_method({
     code => sub {
        my ($param) = @_;
 
+       my $group = $param->{group};
+       my $rename = $param->{rename};
+       my $comment = $param->{comment};
+
        PVE::Firewall::lock_clusterfw_conf(10, sub {
            my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
 
-           if ($param->{rename}) {
+           if ($rename) {
                my (undef, $digest) = &$get_security_group_list($cluster_conf);
                PVE::Tools::assert_if_modified($digest, $param->{digest});
 
-               raise_param_exc({ group => "Security group '$param->{rename}' does not exist" })
-                   if !$cluster_conf->{groups}->{$param->{rename}};
+               raise_param_exc({ group => "Security group '$rename' does not exist" })
+                   if !$cluster_conf->{groups}->{$rename};
 
                # prevent overwriting an existing group
-               raise_param_exc({ group => "Security group '$param->{group}' does already exist" })
-                   if $cluster_conf->{groups}->{$param->{group}} &&
-                   $param->{group} ne $param->{rename};
-
-               my $data = delete $cluster_conf->{groups}->{$param->{rename}};
-               $cluster_conf->{groups}->{$param->{group}} = $data;
-               if (my $comment = delete $cluster_conf->{group_comments}->{$param->{rename}}) {
-                   $cluster_conf->{group_comments}->{$param->{group}} = $comment;
+               raise_param_exc({ group => "Security group '$group' does already exist" })
+                   if $cluster_conf->{groups}->{$group} &&
+                   $group ne $rename;
+
+               if ($rename eq $group) {
+                  $cluster_conf->{group_comments}->{$rename} = $comment if defined($comment);
+                   PVE::Firewall::save_clusterfw_conf($cluster_conf);
+                   return;
                }
-               $cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment});
+
+               # Create an exact copy of the old security group
+               $cluster_conf->{groups}->{$group} = $cluster_conf->{groups}->{$rename};
+               $cluster_conf->{group_comments}->{$group} = $cluster_conf->{group_comments}->{$rename};
+
+               # Update comment if provided
+               $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
+
+               # Write the copy to the cluster config, so that if something fails inbetween, the new firewall
+               # 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);
+                       }
+                   });
+               }
+
+               # 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);
+                       }
+                   });
+               }
+
+               # And also update the cluster itself
+               &$rename_fw_rules($rename,
+                   $group,
+                   $cluster_conf->{rules});
+
+               # Now that everything has been updated, the old rule can be deleted
+               delete $cluster_conf->{groups}->{$rename};
+               delete $cluster_conf->{group_comments}->{$rename};
            } else {
                foreach my $name (keys %{$cluster_conf->{groups}}) {
                    raise_param_exc({ group => "Security group '$name' already exists" })
-                       if $name eq $param->{group};
+                       if $name eq $group;
                }
 
-               $cluster_conf->{groups}->{$param->{group}} = [];
-               $cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment});
+               $cluster_conf->{groups}->{$group} = [];
+               $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
            }
 
            PVE::Firewall::save_clusterfw_conf($cluster_conf);

diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
index b66ca55e553e981af0161802a578ed6c6434a116..dfeccd08ef664e15264f87274fa3e0dc2c445633 100644 (file)
--- a/src/PVE/API2/Firewall/Host.pm
+++ b/src/PVE/API2/Firewall/Host.pm
@@ -118,7 +118,7 @@ __PACKAGE__->register_method({
     code => sub {
        my ($param) = @_;
 
-       PVE::Firewall::lock_hostfw_conf(10, sub {
+       PVE::Firewall::lock_hostfw_conf(undef, 10, sub {
            my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
            my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
 

diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 44756637117b1e7b2ad762d92c57a0dd8833843b..9fcfb20ba9a88f963fe9b3bb1dd4929d3935aaf7 100644 (file)
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -522,7 +522,7 @@ sub rule_env {
 sub lock_config {
     my ($class, $param, $code) = @_;
 
-    PVE::Firewall::lock_hostfw_conf(10, $code, $param);
+    PVE::Firewall::lock_hostfw_conf(undef, 10, $code, $param);
 }
 
 sub load_config {

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index c56e44804742b70c4e304c499421993b113d4f10..e6d68020a1e94528a841fc043a855122c818bc30 100644 (file)
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3624,10 +3624,12 @@ sub save_clusterfw_conf {
     }
 }
 
-sub lock_hostfw_conf {
-    my ($timeout, $code, @param) = @_;
+sub lock_hostfw_conf : prototype($$$@) {
+    my ($node, $timeout, $code, @param) = @_;
+
+    $node = $nodename if !defined($node);
 
-    my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout, $code, @param);
+    my $res = PVE::Cluster::cfs_lock_firewall("host-$node", $timeout, $code, @param);
     die $@ if $@;
 
     return $res;
@@ -3643,7 +3645,9 @@ sub load_hostfw_conf {
 }
 
 sub save_hostfw_conf {
-    my ($hostfw_conf) = @_;
+    my ($hostfw_conf, $filename) = @_;
+
+    $filename = $hostfw_conf_filename if !defined($filename);
 
     my $raw = '';
 
@@ -3658,9 +3662,9 @@ sub save_hostfw_conf {
     }
 
     if ($raw) {
-       PVE::Tools::file_set_contents($hostfw_conf_filename, $raw);
+       PVE::Tools::file_set_contents($filename, $raw);
     } else {
-       unlink $hostfw_conf_filename;
+       unlink $filename;
     }
 }