* [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
@ 2022-09-26 9:45 Leo Nunner
2022-09-27 8:46 ` Wolfgang Bumiller
0 siblings, 1 reply; 6+ messages in thread
From: Leo Nunner @ 2022-09-26 9:45 UTC (permalink / raw)
To: pve-devel
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};
+ 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};
- 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
+ $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});
+
+ # 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 {
- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
2022-09-26 9:45 [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed Leo Nunner
@ 2022-09-27 8:46 ` Wolfgang Bumiller
2022-09-27 9:28 ` Leo Nunner
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-09-27 8:46 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
2022-09-27 8:46 ` Wolfgang Bumiller
@ 2022-09-27 9:28 ` Leo Nunner
2022-09-27 9:59 ` Wolfgang Bumiller
0 siblings, 1 reply; 6+ messages in thread
From: Leo Nunner @ 2022-09-27 9:28 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
2022-09-27 9:28 ` Leo Nunner
@ 2022-09-27 9:59 ` Wolfgang Bumiller
2022-09-27 10:13 ` Leo Nunner
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-09-27 9:59 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
On Tue, Sep 27, 2022 at 11:28:26AM +0200, Leo Nunner wrote:
>
> On 9/27/22 10:46, Wolfgang Bumiller wrote:
> > On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
> > > + $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.
Well, not yet, and we'd need to distinguish between the race and the
group *actually* not existing.
Currently it'll produce a warning in the log which we might consider to
be "good enough".
We *could* try to remember which groups were missing in the previous run
and assume new missing groups are part of a race, but I'm not sure it's
worth it. Though syncing up would be simple enough as we only need to
lock/unlock the cluster fw config once.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
2022-09-27 9:59 ` Wolfgang Bumiller
@ 2022-09-27 10:13 ` Leo Nunner
2022-09-27 10:17 ` Wolfgang Bumiller
0 siblings, 1 reply; 6+ messages in thread
From: Leo Nunner @ 2022-09-27 10:13 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
On 9/27/22 11:59, Wolfgang Bumiller wrote:
> On Tue, Sep 27, 2022 at 11:28:26AM +0200, Leo Nunner wrote:
>> On 9/27/22 10:46, Wolfgang Bumiller wrote:
>>> On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
>>>> + $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.
> Well, not yet, and we'd need to distinguish between the race and the
> group *actually* not existing.
> Currently it'll produce a warning in the log which we might consider to
> be "good enough".
> We *could* try to remember which groups were missing in the previous run
> and assume new missing groups are part of a race, but I'm not sure it's
> worth it. Though syncing up would be simple enough as we only need to
> lock/unlock the cluster fw config once.
Would this still be in the scope of this patch or should we just
leave it like this for now?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed
2022-09-27 10:13 ` Leo Nunner
@ 2022-09-27 10:17 ` Wolfgang Bumiller
0 siblings, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-09-27 10:17 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
On Tue, Sep 27, 2022 at 12:13:30PM +0200, Leo Nunner wrote:
> On 9/27/22 11:59, Wolfgang Bumiller wrote:
>
> > On Tue, Sep 27, 2022 at 11:28:26AM +0200, Leo Nunner wrote:
> > > On 9/27/22 10:46, Wolfgang Bumiller wrote:
> > > > On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
> > > > > + $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.
> > Well, not yet, and we'd need to distinguish between the race and the
> > group *actually* not existing.
> > Currently it'll produce a warning in the log which we might consider to
> > be "good enough".
> > We *could* try to remember which groups were missing in the previous run
> > and assume new missing groups are part of a race, but I'm not sure it's
> > worth it. Though syncing up would be simple enough as we only need to
> > lock/unlock the cluster fw config once.
>
> Would this still be in the scope of this patch or should we just
> leave it like this for now?
That would be a separate patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-27 10:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-26 9:45 [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed Leo Nunner
2022-09-27 8:46 ` Wolfgang Bumiller
2022-09-27 9:28 ` Leo Nunner
2022-09-27 9:59 ` Wolfgang Bumiller
2022-09-27 10:13 ` Leo Nunner
2022-09-27 10:17 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox