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

[Leo Nunner <l.nunner@proxmox.com>]

On 9/27/22 10:46, Wolfgang Bumiller wrote:
> 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.
Agreed, that would make it much more straightforward to read.
>>   
>> -		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...

Is there actually a way around this? Unless we use something like inotify,
there'll be no way for them to actually know about the new group if they've
read the cluster config before I 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