[pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
Leo Nunner
l.nunner at proxmox.com
Tue Sep 27 11:28:26 CEST 2022
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 at 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
More information about the pve-devel
mailing list