Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
> 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>
> ---
> RFC: for locking hosts, I'm currently passing `undef` when I want to
> access the current host. Getting the hostname for each call seems like a
> bit of an overhead, and I'm unsure about introducing global variables in
> the classes where this subroutine needs to be called.
> 
>  src/PVE/API2/Firewall/Groups.pm | 64 ++++++++++++++++++++++++++++++---
>  src/PVE/API2/Firewall/Host.pm   |  2 +-
>  src/PVE/API2/Firewall/Rules.pm  |  2 +-
>  src/PVE/Firewall.pm             | 14 +++++---
>  4 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
> index 558ba8e..052ff41 100644
> --- 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) = @_;
> +
> +    my @matches = grep { $_->{type} eq "group" && $_->{action} eq $old } @{$rules};

I think it's a bit easier to just do `nest if ...` in a for-loop over
@$rules.

> +    for my $rule (@matches) {
> +	$rule->{action} = $new;
> +    }
> +};
> +
>  __PACKAGE__->register_method({
>      name => 'list_security_groups',
>      path => '',
> @@ -106,12 +115,59 @@ __PACKAGE__->register_method({
>  		    if $cluster_conf->{groups}->{$param->{group}} &&
>  		    $param->{group} ne $param->{rename};

If we're already touching the code here, I'd really like it if we moved
the `$param->{group/rename/comment}` into their own `$groupname`, `$rename`,
`$comment` vars, as we have just sooo many nested hash accesses all
around here.

>  
> -		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;
> +		if ($param->{rename} eq $param->{group}) {
> +		   $cluster_conf->{group_comments}->{$param->{rename}} = $param->{comment} if defined($param->{comment});
> +		    PVE::Firewall::save_clusterfw_conf($cluster_conf);
> +		    return;
>  		}
> +
> +		# Create an exact copy of the old security group

(technically just a reference)

> +		$cluster_conf->{groups}->{$param->{group}} = $cluster_conf->{groups}->{$param->{rename}};
> +		$cluster_conf->{group_comments}->{$param->{group}} = $cluster_conf->{group_comments}->{$param->{rename}};
> +
> +		# Update comment if provided
>  		$cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment});
> +

At this point you'd need to also store the cluster fw config, because
*reading* the configs isn't necessarily done with a lock on the cluster
config, and you don't want to race against readers seeing the new group
being referred to without actually having the it in the config.

You'll still be racing against clients having read the cluster config
while you're *here* and then reading their host config *after* you've
updated it...

Plus, if anything after the first host-firewall was written `die()`s,
all the already changed host firewalls will be broken.
So it's better to literally have a copy of the security group in the
cluster config in that case.

> +		# 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($param->{rename},
> +			        $param->{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($param->{rename},
> +			            $param->{group},
> +			            $vm_conf->{rules});
> +			        PVE::Firewall::save_vmfw_conf($vm, $vm_conf);
> +			    }
> +		    });
> +		}
> +
> +		# And also update the cluster itself
> +		&$rename_fw_rules($param->{rename},
> +		    $param->{group},
> +		    $cluster_conf->{rules});
> +
> +		# Now that everything has been updated, the old rule can be deleted
> +		delete $cluster_conf->{groups}->{$param->{rename}};
> +		delete $cluster_conf->{group_comments}->{$param->{rename}};
>  	    } else {
>  		foreach my $name (keys %{$cluster_conf->{groups}}) {
>  		    raise_param_exc({ group => "Security group '$name' already exists" })
> diff --git a/src/PVE/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
> index b66ca55..dfeccd0 100644
> --- 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 4475663..9fcfb20 100644
> --- 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 c56e448..7ffd09b 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -3625,9 +3625,11 @@ sub save_clusterfw_conf {
>  }
>  
>  sub lock_hostfw_conf {

^ maybe add a `: prototype($$$@)` while you're at it.
Though it won't actually catch old callers due to the the `@param` style...

> -    my ($timeout, $code, @param) = @_;
> +    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;
>      }
>  }
>  
> -- 
> 2.30.2