* [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names
2023-01-26 14:30 [pve-devel] [PATCH firewall manager] Make firewall config case-insensitive Leo Nunner
@ 2023-01-26 14:30 ` Leo Nunner
2023-01-27 10:43 ` Wolfgang Bumiller
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 2/4] docs: clarify usage of 'rename' parameters Leo Nunner
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Leo Nunner @ 2023-01-26 14:30 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 20 ++++++------
src/PVE/API2/Firewall/Groups.pm | 53 ++++++++++++++++----------------
src/PVE/API2/Firewall/IPSet.pm | 39 ++++++++++++-----------
3 files changed, 56 insertions(+), 56 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 33ac669..88f20a0 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -234,24 +234,22 @@ sub register_update_alias {
PVE::Tools::assert_if_modified($digest, $param->{digest});
- my $name = lc($param->{name});
+ my $rename_to = lc($param->{rename}) if defined($param->{rename});
+ my $rename_from = lc($param->{name});
- raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
+ raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
my $data = { name => $param->{name}, cidr => $param->{cidr} };
$data->{comment} = $param->{comment} if $param->{comment};
- $aliases->{$name} = $data;
-
- my $rename = $param->{rename};
- $rename = lc($rename) if $rename;
+ $aliases->{$rename_from} = $data;
- if ($rename && ($name ne $rename)) {
+ if ($rename_to && ($rename_from ne $rename_to)) {
raise_param_exc({ name => "alias '$param->{rename}' already exists" })
- if defined($aliases->{$rename});
- $aliases->{$name}->{name} = $param->{rename};
- $aliases->{$rename} = $aliases->{$name};
- delete $aliases->{$name};
+ if defined($aliases->{$rename_to});
+ $aliases->{$rename_from}->{name} = $param->{rename};
+ $aliases->{$rename_to} = $aliases->{$rename_from};
+ delete $aliases->{$rename_from};
}
$class->save_aliases($param, $fw_conf, $aliases);
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index ffdc45c..a0695ce 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -100,37 +100,37 @@ __PACKAGE__->register_method({
code => sub {
my ($param) = @_;
- my $group = $param->{group};
- my $rename = $param->{rename};
+ my $rename_to = $param->{group};
+ my $rename_from = $param->{rename};
my $comment = $param->{comment};
PVE::Firewall::lock_clusterfw_conf(10, sub {
my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
- if ($rename) {
+ if ($rename_from) {
my (undef, $digest) = &$get_security_group_list($cluster_conf);
PVE::Tools::assert_if_modified($digest, $param->{digest});
- raise_param_exc({ group => "Security group '$rename' does not exist" })
- if !$cluster_conf->{groups}->{$rename};
+ raise_param_exc({ group => "Security group '$rename_from' does not exist" })
+ if !$cluster_conf->{groups}->{$rename_from};
# prevent overwriting an existing group
- raise_param_exc({ group => "Security group '$group' does already exist" })
- if $cluster_conf->{groups}->{$group} &&
- $group ne $rename;
+ raise_param_exc({ group => "Security group '$rename_to' does already exist" })
+ if $cluster_conf->{groups}->{$rename_to} &&
+ $rename_to ne $rename_from;
- if ($rename eq $group) {
- $cluster_conf->{group_comments}->{$rename} = $comment if defined($comment);
+ if ($rename_from eq $rename_to) {
+ $cluster_conf->{group_comments}->{$rename_from} = $comment if defined($comment);
PVE::Firewall::save_clusterfw_conf($cluster_conf);
return;
}
# 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};
+ $cluster_conf->{groups}->{$rename_to} = $cluster_conf->{groups}->{$rename_from};
+ $cluster_conf->{group_comments}->{$rename_to} = $cluster_conf->{group_comments}->{$rename_from};
# Update comment if provided
- $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
+ $cluster_conf->{group_comments}->{$rename_to} = $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
@@ -144,8 +144,8 @@ __PACKAGE__->register_method({
my $host_conf = PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path);
if (defined($host_conf)) {
- &$rename_fw_rules($rename,
- $group,
+ &$rename_fw_rules($rename_from,
+ $rename_to,
$host_conf->{rules});
PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
}
@@ -160,8 +160,8 @@ __PACKAGE__->register_method({
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,
+ &$rename_fw_rules($rename_from,
+ $rename_to,
$vm_conf->{rules});
PVE::Firewall::save_vmfw_conf($vm, $vm_conf);
}
@@ -169,21 +169,20 @@ __PACKAGE__->register_method({
}
# And also update the cluster itself
- &$rename_fw_rules($rename,
- $group,
+ &$rename_fw_rules($rename_from,
+ $rename_to,
$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};
+ delete $cluster_conf->{groups}->{$rename_from};
+ delete $cluster_conf->{group_comments}->{$rename_from};
} else {
- foreach my $name (keys %{$cluster_conf->{groups}}) {
- raise_param_exc({ group => "Security group '$name' already exists" })
- if $name eq $group;
- }
+ # In this context, $rename_to is the name for the new group
+ raise_param_exc({ group => "Security group '$rename_to' already exists" })
+ if $cluster_conf->{groups}->{$rename_to};
- $cluster_conf->{groups}->{$group} = [];
- $cluster_conf->{group_comments}->{$group} = $comment if defined($comment);
+ $cluster_conf->{groups}->{$rename_to} = [];
+ $cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment);
}
PVE::Firewall::save_clusterfw_conf($cluster_conf);
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 14bcfcb..0e6b1d6 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -647,32 +647,35 @@ sub register_create {
my ($cluster_conf, $fw_conf) = $class->load_config($param);
- if ($param->{rename}) {
+ my $rename_to = $param->{name};
+ my $rename_from = $param->{rename};
+ my $comment = $param->{comment};
+
+ if ($rename_from) {
my (undef, $digest) = &$get_ipset_list($fw_conf);
PVE::Tools::assert_if_modified($digest, $param->{digest});
- raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" })
- if !$fw_conf->{ipset}->{$param->{rename}};
+ raise_param_exc({ name => "IPSet '$rename_from' does not exist" })
+ if !$fw_conf->{ipset}->{$rename_from};
# prevent overwriting existing ipset
- raise_param_exc({ name => "IPSet '$param->{name}' does already exist"})
- if $fw_conf->{ipset}->{$param->{name}} &&
- $param->{name} ne $param->{rename};
-
- my $data = delete $fw_conf->{ipset}->{$param->{rename}};
- $fw_conf->{ipset}->{$param->{name}} = $data;
- if (my $comment = delete $fw_conf->{ipset_comments}->{$param->{rename}}) {
- $fw_conf->{ipset_comments}->{$param->{name}} = $comment;
+ raise_param_exc({ name => "IPSet '$rename_to' does already exist"})
+ if $fw_conf->{ipset}->{$rename_to} &&
+ $rename_to ne $rename_from;
+
+ my $data = delete $fw_conf->{ipset}->{$rename_from};
+ $fw_conf->{ipset}->{$rename_to} = $data;
+ if (my $comment = delete $fw_conf->{ipset_comments}->{$rename_from}) {
+ $fw_conf->{ipset_comments}->{$rename_to} = $comment;
}
- $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
+ $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
} else {
- foreach my $name (keys %{$fw_conf->{ipset}}) {
- raise_param_exc({ name => "IPSet '$name' already exists" })
- if $name eq $param->{name};
- }
+ # In this context, $rename_to is the name for the new IPSet
+ raise_param_exc({ name => "IPSet '$rename_to' already exists" })
+ if $fw_conf->{ipset}->{$rename_to};
- $fw_conf->{ipset}->{$param->{name}} = [];
- $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment});
+ $fw_conf->{ipset}->{$rename_to} = [];
+ $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
}
$class->save_config($param, $fw_conf);
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names Leo Nunner
@ 2023-01-27 10:43 ` Wolfgang Bumiller
0 siblings, 0 replies; 11+ messages in thread
From: Wolfgang Bumiller @ 2023-01-27 10:43 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
On Thu, Jan 26, 2023 at 03:30:16PM +0100, Leo Nunner wrote:
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> src/PVE/API2/Firewall/Aliases.pm | 20 ++++++------
> src/PVE/API2/Firewall/Groups.pm | 53 ++++++++++++++++----------------
> src/PVE/API2/Firewall/IPSet.pm | 39 ++++++++++++-----------
> 3 files changed, 56 insertions(+), 56 deletions(-)
>
> diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
> index 33ac669..88f20a0 100644
> --- a/src/PVE/API2/Firewall/Aliases.pm
> +++ b/src/PVE/API2/Firewall/Aliases.pm
> @@ -234,24 +234,22 @@ sub register_update_alias {
>
> PVE::Tools::assert_if_modified($digest, $param->{digest});
>
> - my $name = lc($param->{name});
> + my $rename_to = lc($param->{rename}) if defined($param->{rename});
Conditional variable declaration leads to really bad undefined behavior
in perl, please keep this split in 2 as it was before, otherwise this
might retain values from previous times the api call was used...
> + my $rename_from = lc($param->{name});
>
> - raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
> + raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
>
> my $data = { name => $param->{name}, cidr => $param->{cidr} };
> $data->{comment} = $param->{comment} if $param->{comment};
>
> - $aliases->{$name} = $data;
> -
> - my $rename = $param->{rename};
> - $rename = lc($rename) if $rename;
> + $aliases->{$rename_from} = $data;
>
> - if ($rename && ($name ne $rename)) {
> + if ($rename_to && ($rename_from ne $rename_to)) {
> raise_param_exc({ name => "alias '$param->{rename}' already exists" })
> - if defined($aliases->{$rename});
> - $aliases->{$name}->{name} = $param->{rename};
> - $aliases->{$rename} = $aliases->{$name};
> - delete $aliases->{$name};
> + if defined($aliases->{$rename_to});
> + $aliases->{$rename_from}->{name} = $param->{rename};
> + $aliases->{$rename_to} = $aliases->{$rename_from};
> + delete $aliases->{$rename_from};
> }
>
> $class->save_aliases($param, $fw_conf, $aliases);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH firewall 2/4] docs: clarify usage of 'rename' parameters
2023-01-26 14:30 [pve-devel] [PATCH firewall manager] Make firewall config case-insensitive Leo Nunner
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names Leo Nunner
@ 2023-01-26 14:30 ` Leo Nunner
2023-01-28 10:39 ` Thomas Lamprecht
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 3/4] config: make groups, IPSets and aliases case-insensitive Leo Nunner
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Leo Nunner @ 2023-01-26 14:30 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 2 +-
src/PVE/API2/Firewall/Groups.pm | 2 +-
src/PVE/API2/Firewall/IPSet.pm | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 88f20a0..c1af408 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -16,7 +16,7 @@ my $api_properties = {
},
name => get_standard_option('pve-fw-alias'),
rename => get_standard_option('pve-fw-alias', {
- description => "Rename an existing alias.",
+ description => "Rename an existing alias to the value provided.",
optional => 1,
}),
comment => {
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index a0695ce..cf9dc06 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -90,7 +90,7 @@ __PACKAGE__->register_method({
optional => 1,
},
rename => get_standard_option('pve-security-group-name', {
- description => "Rename/update an existing security group. You can set 'rename' to the same value as 'name' to update the 'comment' of an existing group.",
+ description => "Specify a group to be renamed/updated. You can set 'rename' to the same value as 'name' to update the 'comment' of an existing group.",
optional => 1,
}),
digest => get_standard_option('pve-config-digest'),
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 0e6b1d6..fee9046 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -624,7 +624,7 @@ sub register_create {
$properties->{digest} = get_standard_option('pve-config-digest');
$properties->{rename} = get_standard_option('ipset-name', {
- description => "Rename an existing IPSet. You can set 'rename' to the same value as 'name' to update the 'comment' of an existing IPSet.",
+ description => "Specify an IPset to be renamed/updated. You can set 'rename' to the same value as 'name' to update the 'comment' of an existing IPSet.",
optional => 1 });
$class->register_method({
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [pve-devel] [PATCH firewall 3/4] config: make groups, IPSets and aliases case-insensitive
2023-01-26 14:30 [pve-devel] [PATCH firewall manager] Make firewall config case-insensitive Leo Nunner
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names Leo Nunner
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 2/4] docs: clarify usage of 'rename' parameters Leo Nunner
@ 2023-01-26 14:30 ` Leo Nunner
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments Leo Nunner
2023-01-26 14:30 ` [pve-devel] [PATCH manager] ui: the API doesn't pass 'name' for aliases anymore Leo Nunner
4 siblings, 0 replies; 11+ messages in thread
From: Leo Nunner @ 2023-01-26 14:30 UTC (permalink / raw)
To: pve-devel
So far, the names of groups and IPSets were automatically converted into
lower-case. Aliases were parsed in a *kind of* case-insensitive way, where
both the original name AND the lower-case one was stored.
This patch removes all instances where names are converted to lower-case.
We need some legacy handling for the aliases here, since rules/… will
contain the lower-case name for them, while the definition itself has the
original name. As such, internally, the names still get lower-cased while
resolving aliases; this has no effect on the user.
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
src/PVE/API2/Firewall/Aliases.pm | 28 +++++++++++------
src/PVE/API2/Firewall/Groups.pm | 4 +--
src/PVE/API2/Firewall/IPSet.pm | 4 +--
src/PVE/Firewall.pm | 54 ++++++++++++++++++--------------
4 files changed, 52 insertions(+), 38 deletions(-)
diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index c1af408..9f8e71e 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -72,7 +72,16 @@ my $aliases_to_list = sub {
my $list = [];
foreach my $k (sort keys %$aliases) {
- push @$list, $aliases->{$k};
+ my $alias = $aliases->{$k};
+ my $data = {
+ name => $k,
+ cidr => $alias->{cidr},
+ ipversion => $alias->{ipversion},
+ };
+ if(my $comment = $alias->{comment}) {
+ $data->{comment} = $comment;
+ }
+ push @$list, $data;
}
return $list;
};
@@ -148,10 +157,10 @@ sub register_create_alias {
my ($fw_conf, $aliases) = $class->load_config($param);
- my $name = lc($param->{name});
+ my $name = $param->{name};
raise_param_exc({ name => "alias '$param->{name}' already exists" })
- if defined($aliases->{$name});
+ if grep { lc($_) eq lc($name) } (keys $aliases->%*);
my $data = { name => $param->{name}, cidr => $param->{cidr} };
$data->{comment} = $param->{comment} if $param->{comment};
@@ -188,7 +197,7 @@ sub register_read_alias {
my ($fw_conf, $aliases) = $class->load_config($param);
- my $name = lc($param->{name});
+ my $name = $param->{name};
raise_param_exc({ name => "no such alias" })
if !defined($aliases->{$name});
@@ -234,20 +243,19 @@ sub register_update_alias {
PVE::Tools::assert_if_modified($digest, $param->{digest});
- my $rename_to = lc($param->{rename}) if defined($param->{rename});
- my $rename_from = lc($param->{name});
+ my $rename_to = $param->{rename} if defined($param->{rename});
+ my $rename_from = $param->{name};
raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
- my $data = { name => $param->{name}, cidr => $param->{cidr} };
+ my $data = { name => $rename_from, cidr => $param->{cidr} };
$data->{comment} = $param->{comment} if $param->{comment};
$aliases->{$rename_from} = $data;
if ($rename_to && ($rename_from ne $rename_to)) {
raise_param_exc({ name => "alias '$param->{rename}' already exists" })
- if defined($aliases->{$rename_to});
- $aliases->{$rename_from}->{name} = $param->{rename};
+ if grep { lc($_) eq lc($rename_to) } (keys $aliases->%*);
$aliases->{$rename_to} = $aliases->{$rename_from};
delete $aliases->{$rename_from};
}
@@ -291,7 +299,7 @@ sub register_delete_alias {
my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list);
PVE::Tools::assert_if_modified($digest, $param->{digest});
- my $name = lc($param->{name});
+ my $name = $param->{name};
delete $aliases->{$name};
$class->save_aliases($param, $fw_conf, $aliases);
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index cf9dc06..6d30c53 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -116,7 +116,7 @@ __PACKAGE__->register_method({
# prevent overwriting an existing group
raise_param_exc({ group => "Security group '$rename_to' does already exist" })
- if $cluster_conf->{groups}->{$rename_to} &&
+ if (grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*)) &&
$rename_to ne $rename_from;
if ($rename_from eq $rename_to) {
@@ -179,7 +179,7 @@ __PACKAGE__->register_method({
} else {
# In this context, $rename_to is the name for the new group
raise_param_exc({ group => "Security group '$rename_to' already exists" })
- if $cluster_conf->{groups}->{$rename_to};
+ if grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*);
$cluster_conf->{groups}->{$rename_to} = [];
$cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment);
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index fee9046..bb7f098 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -660,7 +660,7 @@ sub register_create {
# prevent overwriting existing ipset
raise_param_exc({ name => "IPSet '$rename_to' does already exist"})
- if $fw_conf->{ipset}->{$rename_to} &&
+ if (grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*)) &&
$rename_to ne $rename_from;
my $data = delete $fw_conf->{ipset}->{$rename_from};
@@ -672,7 +672,7 @@ sub register_create {
} else {
# In this context, $rename_to is the name for the new IPSet
raise_param_exc({ name => "IPSet '$rename_to' already exists" })
- if $fw_conf->{ipset}->{$rename_to};
+ if grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*);
$fw_conf->{ipset}->{$rename_to} = [];
$fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 4924d51..cbb72f5 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1646,18 +1646,17 @@ sub verify_rule {
if ($value =~ m/^\+(${ipset_name_pattern})$/) {
&$add_error($name, "no such ipset '$1'")
if !($cluster_conf->{ipset}->{$1} || ($fw_conf && $fw_conf->{ipset}->{$1}));
-
} else {
&$add_error($name, "invalid ipset name '$value'");
}
} elsif ($value =~ m/^${ip_alias_pattern}$/){
- my $alias = lc($value);
- &$add_error($name, "no such alias '$value'")
- if !($cluster_conf->{aliases}->{$alias} || ($fw_conf && $fw_conf->{aliases}->{$alias}));
- my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
- $e = $cluster_conf->{aliases}->{$alias} if !$e && $cluster_conf;
+ my $ipversion;
+
+ eval { (undef, $ipversion) = resolve_alias($cluster_conf, $fw_conf, $value) };
+ &$add_error($name, "no such alias '$value' $@")
+ if $@;
- &$set_ip_version($e->{ipversion});
+ &$set_ip_version($ipversion);
}
}
};
@@ -2069,11 +2068,8 @@ sub ipt_gen_src_or_dst_match {
die "invalid security group name '$adr'\n";
}
} elsif ($adr =~ m/^${ip_alias_pattern}$/){
- my $alias = lc($adr);
- my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
- $e = $cluster_conf->{aliases}->{$alias} if !$e && $cluster_conf;
- die "no such alias '$adr'\n" if !$e;
- $match = "-${dir} $e->{cidr}";
+ my ($cidr) = resolve_alias($cluster_conf, $fw_conf, $adr);
+ $match = "-${dir} $cidr";
} elsif ($adr =~ m/\-/){
$match = "-m iprange --${srcdst}-range $adr";
} else {
@@ -2919,11 +2915,22 @@ sub parse_clusterfw_option {
sub resolve_alias {
my ($clusterfw_conf, $fw_conf, $cidr) = @_;
- my $alias = lc($cidr);
- my $e = $fw_conf ? $fw_conf->{aliases}->{$alias} : undef;
- $e = $clusterfw_conf->{aliases}->{$alias} if !$e && $clusterfw_conf;
+ my $alias = $cidr;
+ my $e;
+
+ # This is a special legacy case: aliases were the only thing that used to be case-insensitive. Due to this, old
+ # configs can contain upper-case aliases in the definition, but rules contain the lower-case name.
+ if ($fw_conf) {
+ my ($name) = grep { lc($_) eq lc($alias) } keys $fw_conf->{aliases}->%*;
+ $e = $fw_conf->{aliases}->{$name} if $name;
+ }
+
+ if (!$e && $clusterfw_conf) {
+ my ($name) = grep { lc($_) eq lc($alias) } keys $clusterfw_conf->{aliases}->%*;
+ $e = $clusterfw_conf->{aliases}->{$name} if $name;
+ }
- die "no such alias '$cidr'\n" if !$e;;
+ die "no such alias '$cidr'\n" if !$e;
return wantarray ? ($e->{cidr}, $e->{ipversion}) : $e->{cidr};
}
@@ -2959,12 +2966,11 @@ sub parse_alias {
($cidr, $ipversion) = parse_ip_or_cidr($cidr);
my $data = {
- name => $name,
cidr => $cidr,
ipversion => $ipversion,
};
- $data->{comment} = $comment if $comment;
- return $data;
+ $data->{comment} = $comment if $comment;
+ return ($name, $data);
}
return undef;
@@ -3010,7 +3016,7 @@ sub generic_fw_config_parser {
if ($empty_conf->{groups} && ($line =~ m/^\[group\s+(\S+)\]\s*(?:#\s*(.*?)\s*)?$/i)) {
$section = 'groups';
- $group = lc($1);
+ $group = $1;
my $comment = $2;
eval {
die "security group name too long\n" if length($group) > $max_group_name_length;
@@ -3035,7 +3041,7 @@ sub generic_fw_config_parser {
if ($empty_conf->{ipset} && ($line =~ m/^\[ipset\s+(\S+)\]\s*(?:#\s*(.*?)\s*)?$/i)) {
$section = 'ipset';
- $group = lc($1);
+ $group = $1;
my $comment = $2;
eval {
die "ipset name too long\n" if length($group) > $max_ipset_name_length;
@@ -3075,8 +3081,8 @@ sub generic_fw_config_parser {
warn "$prefix: $@" if $@;
} elsif ($section eq 'aliases') {
eval {
- my $data = parse_alias($line);
- $res->{aliases}->{lc($data->{name})} = $data;
+ my ($name, $data) = parse_alias($line);
+ $res->{aliases}->{$name} = $data;
};
warn "$prefix: $@" if $@;
} elsif ($section eq 'rules') {
@@ -3285,7 +3291,7 @@ my $format_aliases = sub {
$raw .= "[ALIASES]\n\n";
foreach my $k (keys %$aliases) {
my $e = $aliases->{$k};
- $raw .= "$e->{name} $e->{cidr}";
+ $raw .= "$k $e->{cidr}";
$raw .= " # " . encode('utf8', $e->{comment})
if $e->{comment} && $e->{comment} !~ m/^\s*$/;
$raw .= "\n";
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments
2023-01-26 14:30 [pve-devel] [PATCH firewall manager] Make firewall config case-insensitive Leo Nunner
` (2 preceding siblings ...)
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 3/4] config: make groups, IPSets and aliases case-insensitive Leo Nunner
@ 2023-01-26 14:30 ` Leo Nunner
2023-01-27 11:41 ` Wolfgang Bumiller
2023-01-26 14:30 ` [pve-devel] [PATCH manager] ui: the API doesn't pass 'name' for aliases anymore Leo Nunner
4 siblings, 1 reply; 11+ messages in thread
From: Leo Nunner @ 2023-01-26 14:30 UTC (permalink / raw)
To: pve-devel
This patch restructures the parsed config structure a bit to be more
consistent across objects.
group_comments and ipset_comments were removed from the config structure
and are now stored directly within the group/ipset objects themselves.
They now follow the same structure as aliases, with
<name> => {
comment => <...>,
[entries|rules] => { <...> },
}
We don't need to store separate instances of the original + the
lower-case name for aliases anymore, so the structure was changed to
<name> => {
comment => <...>,
cidr => <...>,
ipversion => <...>,
}
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
RFC: This one is optional, it's just that while experimenting with
the capitalization issue I also looked into using a "name" property
for everything (like for aliases), and while I was at it, I also transfered
the comments into the main object… I feel like this structure is nicer, but
we don't _need_ it. My main worry is that there might still be some calls to
$conf->{ipset}->{foo} instead of $conf->{ipset}->{foo}->{entries}, but I
couldn't find any aside from the ones modified in this patch ^^
src/PVE/API2/Firewall/Cluster.pm | 2 +-
src/PVE/API2/Firewall/Groups.pm | 22 +++++++-------
src/PVE/API2/Firewall/IPSet.pm | 23 +++++++-------
src/PVE/API2/Firewall/Rules.pm | 4 +--
src/PVE/API2/Firewall/VM.pm | 2 +-
src/PVE/Firewall.pm | 51 +++++++++++++++++---------------
6 files changed, 53 insertions(+), 51 deletions(-)
diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm
index c9c3e67..63cfb98 100644
--- a/src/PVE/API2/Firewall/Cluster.pm
+++ b/src/PVE/API2/Firewall/Cluster.pm
@@ -261,7 +261,7 @@ __PACKAGE__->register_method({
name => $name,
ref => "+$name",
};
- if (my $comment = $conf->{ipset_comments}->{$name}) {
+ if (my $comment = $conf->{ipset}->{$name}->{comment}) {
$data->{comment} = $comment;
}
push @$res, $data;
diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm
index 6d30c53..3bf01ac 100644
--- a/src/PVE/API2/Firewall/Groups.pm
+++ b/src/PVE/API2/Firewall/Groups.pm
@@ -18,10 +18,8 @@ my $get_security_group_list = sub {
foreach my $group (sort keys %{$cluster_conf->{groups}}) {
my $data = {
group => $group,
+ comment => $cluster_conf->{groups}->{$group}->{comment},
};
- if (my $comment = $cluster_conf->{group_comments}->{$group}) {
- $data->{comment} = $comment;
- }
push @$res, $data;
}
@@ -120,17 +118,14 @@ __PACKAGE__->register_method({
$rename_to ne $rename_from;
if ($rename_from eq $rename_to) {
- $cluster_conf->{group_comments}->{$rename_from} = $comment if defined($comment);
+ $cluster_conf->{groups}->{$rename_from}->{comment} = $comment if defined($comment);
PVE::Firewall::save_clusterfw_conf($cluster_conf);
return;
}
# Create an exact copy of the old security group
- $cluster_conf->{groups}->{$rename_to} = $cluster_conf->{groups}->{$rename_from};
- $cluster_conf->{group_comments}->{$rename_to} = $cluster_conf->{group_comments}->{$rename_from};
-
- # Update comment if provided
- $cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment);
+ $cluster_conf->{groups}->{$rename_to} = { $cluster_conf->{groups}->{$rename_from}->%* };
+ $cluster_conf->{groups}->{$rename_to}->{comment} = $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
@@ -175,14 +170,17 @@ __PACKAGE__->register_method({
# Now that everything has been updated, the old rule can be deleted
delete $cluster_conf->{groups}->{$rename_from};
- delete $cluster_conf->{group_comments}->{$rename_from};
} else {
# In this context, $rename_to is the name for the new group
raise_param_exc({ group => "Security group '$rename_to' already exists" })
if grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*);
- $cluster_conf->{groups}->{$rename_to} = [];
- $cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment);
+ my $data = {
+ comment => $comment,
+ rules => [],
+ };
+
+ $cluster_conf->{groups}->{$rename_to} = $data;
}
PVE::Firewall::save_clusterfw_conf($cluster_conf);
diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index bb7f098..aae9006 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -57,7 +57,7 @@ sub save_ipset {
if (!defined($ipset)) {
delete $fw_conf->{ipset}->{$param->{name}};
} else {
- $fw_conf->{ipset}->{$param->{name}} = $ipset;
+ $fw_conf->{ipset}->{$param->{name}}->{entries} = $ipset;
}
$class->save_config($param, $fw_conf);
@@ -400,7 +400,7 @@ sub load_config {
my ($class, $param) = @_;
my $fw_conf = PVE::Firewall::load_clusterfw_conf();
- my $ipset = $fw_conf->{ipset}->{$param->{name}};
+ my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries};
die "no such IPSet '$param->{name}'\n" if !defined($ipset);
return (undef, $fw_conf, $ipset);
@@ -444,7 +444,7 @@ sub load_config {
my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'vm', $param->{vmid});
- my $ipset = $fw_conf->{ipset}->{$param->{name}};
+ my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries};
die "no such IPSet '$param->{name}'\n" if !defined($ipset);
return ($cluster_conf, $fw_conf, $ipset);
@@ -488,7 +488,7 @@ sub load_config {
my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'ct', $param->{vmid});
- my $ipset = $fw_conf->{ipset}->{$param->{name}};
+ my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries};
die "no such IPSet '$param->{name}'\n" if !defined($ipset);
return ($cluster_conf, $fw_conf, $ipset);
@@ -562,7 +562,7 @@ my $get_ipset_list = sub {
my $data = {
name => $name,
};
- if (my $comment = $fw_conf->{ipset_comments}->{$name}) {
+ if (my $comment = $fw_conf->{ipset}->{$name}->{comment}) {
$data->{comment} = $comment;
}
push @$res, $data;
@@ -665,17 +665,18 @@ sub register_create {
my $data = delete $fw_conf->{ipset}->{$rename_from};
$fw_conf->{ipset}->{$rename_to} = $data;
- if (my $comment = delete $fw_conf->{ipset_comments}->{$rename_from}) {
- $fw_conf->{ipset_comments}->{$rename_to} = $comment;
- }
- $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
+ $fw_conf->{ipset}->{$rename_to}->{comment} = $param->{comment} if defined($param->{comment});
} else {
# In this context, $rename_to is the name for the new IPSet
raise_param_exc({ name => "IPSet '$rename_to' already exists" })
if grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*);
- $fw_conf->{ipset}->{$rename_to} = [];
- $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment});
+ my $data = {
+ comment => $comment,
+ entries => [],
+ };
+
+ $fw_conf->{ipset}->{$rename_to} = $data;
}
$class->save_config($param, $fw_conf);
diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 9fcfb20..30e1050 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -413,7 +413,7 @@ sub load_config {
my ($class, $param) = @_;
my $fw_conf = PVE::Firewall::load_clusterfw_conf();
- my $rules = $fw_conf->{groups}->{$param->{group}};
+ my $rules = $fw_conf->{groups}->{$param->{group}}->{rules};
die "no such security group '$param->{group}'\n" if !defined($rules);
return (undef, $fw_conf, $rules);
@@ -425,7 +425,7 @@ sub save_rules {
if (!defined($rules)) {
delete $fw_conf->{groups}->{$param->{group}};
} else {
- $fw_conf->{groups}->{$param->{group}} = $rules;
+ $fw_conf->{groups}->{$param->{group}}->{rules} = $rules;
}
PVE::Firewall::save_clusterfw_conf($fw_conf);
diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm
index 48b8c5f..5bd1944 100644
--- a/src/PVE/API2/Firewall/VM.pm
+++ b/src/PVE/API2/Firewall/VM.pm
@@ -269,7 +269,7 @@ sub register_handlers {
name => $name,
ref => "+$name",
};
- if (my $comment = $conf->{ipset_comments}->{$name}) {
+ if (my $comment = $conf->{ipset}->{$name}->{comment}) {
$data->{comment} = $comment;
}
$ipsets->{$name} = $data;
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index cbb72f5..e04fc15 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -2686,7 +2686,7 @@ sub enable_host_firewall {
sub generate_group_rules {
my ($ruleset, $cluster_conf, $group, $ipversion) = @_;
- my $rules = $cluster_conf->{groups}->{$group};
+ my $rules = $cluster_conf->{groups}->{$group}->{rules};
if (!$rules) {
warn "no such security group '$group'\n";
@@ -3028,9 +3028,12 @@ sub generic_fw_config_parser {
next;
}
- $res->{$section}->{$group} = [];
- $res->{group_comments}->{$group} = decode('utf8', $comment)
- if $comment;
+ my $data = {
+ rules => [],
+ };
+ $data->{comment} = decode('utf8', $comment) if $comment;
+
+ $res->{$section}->{$group} = $data;
next;
}
@@ -3053,11 +3056,14 @@ sub generic_fw_config_parser {
next;
}
- $res->{$section}->{$group} = [];
+ my $data = {
+ entries => [],
+ };
+ $data->{comment} = decode('utf8', $comment) if $comment;
+
+ $res->{$section}->{$group} = $data;
$curr_group_keys = {};
- $res->{ipset_comments}->{$group} = decode('utf8', $comment)
- if $comment;
next;
}
@@ -3100,7 +3106,7 @@ sub generic_fw_config_parser {
warn "$prefix: $err";
next;
}
- push @{$res->{$section}->{$group}}, $rule;
+ push @{$res->{$section}->{$group}->{rules}}, $rule;
} elsif ($section eq 'ipset') {
# we can add single line comments to the end of the rule
my $comment = decode('utf8', $1) if $line =~ s/#\s*(.*?)\s*$//;
@@ -3144,7 +3150,7 @@ sub generic_fw_config_parser {
}
}
- push @{$res->{$section}->{$group}}, $entry;
+ push @{$res->{$section}->{$group}->{entries}}, $entry;
$curr_group_keys->{$cidr} = 1;
} else {
warn "$prefix: skip line - unknown section\n";
@@ -3221,7 +3227,6 @@ sub load_vmfw_conf {
options => {},
aliases => {},
ipset => {} ,
- ipset_comments => {},
};
my $vmfw_conf = generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env);
@@ -3307,13 +3312,14 @@ my $format_ipsets = sub {
my $raw = '';
foreach my $ipset (sort keys %{$fw_conf->{ipset}}) {
- if (my $comment = $fw_conf->{ipset_comments}->{$ipset}) {
+ my $data = $fw_conf->{ipset}->{$ipset};
+ if (my $comment = $data->{comment}) {
my $utf8comment = encode('utf8', $comment);
$raw .= "[IPSET $ipset] # $utf8comment\n\n";
} else {
$raw .= "[IPSET $ipset]\n\n";
}
- my $options = $fw_conf->{ipset}->{$ipset};
+ my $options = $data->{entries};
my $nethash = {};
foreach my $entry (@$options) {
@@ -3453,7 +3459,7 @@ sub generate_ipset_chains {
foreach my $ipset (keys %{$ipsets}) {
- my $options = $ipsets->{$ipset};
+ my $options = $ipsets->{$ipset}->{entries};
if ($device_ips && $ipset =~ /^ipfilter-(net\d+)$/) {
if (my $ips = $device_ips->{$1}) {
@@ -3573,9 +3579,7 @@ sub load_clusterfw_conf {
options => {},
aliases => {},
groups => {},
- group_comments => {},
ipset => {} ,
- ipset_comments => {},
};
my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
@@ -3606,8 +3610,8 @@ sub save_clusterfw_conf {
if ($cluster_conf->{groups}) {
foreach my $group (sort keys %{$cluster_conf->{groups}}) {
- my $rules = $cluster_conf->{groups}->{$group};
- if (my $comment = $cluster_conf->{group_comments}->{$group}) {
+ my $rules = $cluster_conf->{groups}->{$group}->{rules};
+ if (my $comment = $cluster_conf->{groups}->{$group}->{comment}) {
my $utf8comment = encode('utf8', $comment);
$raw .= "[group $group] # $utf8comment\n\n";
} else {
@@ -3714,7 +3718,7 @@ sub compile {
name => 'local_network', cidr => $localnet, ipversion => $localnet_ver };
}
- push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet };
+ push @{$cluster_conf->{ipset}->{management}->{entries}}, { cidr => $localnet };
my $ruleset = {};
my $rulesetv6 = {};
@@ -3866,8 +3870,7 @@ sub compile_ipsets {
name => 'local_network', cidr => $localnet, ipversion => $localnet_ver };
}
- push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet };
-
+ push @{$cluster_conf->{ipset}->{management}->{entries}}, { cidr => $localnet };
my $ipset_ruleset = {};
@@ -3893,7 +3896,7 @@ sub compile_ipsets {
next if !$net->{firewall};
if ($vmfw_conf->{options}->{ipfilter} && !$ipsets->{"ipfilter-$netid"}) {
- $implicit_sets->{"ipfilter-$netid"} = [];
+ $implicit_sets->{"ipfilter-$netid"}->{entries} = [];
}
my $macaddr = $net->{macaddr};
@@ -3933,7 +3936,7 @@ sub compile_ipsets {
next if !$net->{firewall};
if ($vmfw_conf->{options}->{ipfilter} && !$ipsets->{"ipfilter-$netid"}) {
- $implicit_sets->{"ipfilter-$netid"} = [];
+ $implicit_sets->{"ipfilter-$netid"}->{entries} = [];
}
my $macaddr = $net->{hwaddr};
@@ -3994,7 +3997,7 @@ sub compile_ebtables_filter {
my $macaddr = $net->{macaddr};
my $arpfilter = [];
if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
- foreach my $ipaddr (@$ipset) {
+ foreach my $ipaddr ($ipset->{entries}->@*) {
my($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
next if !$ip || ($version && $version != 4);
push(@$arpfilter, $ip);
@@ -4023,7 +4026,7 @@ sub compile_ebtables_filter {
my $macaddr = $net->{hwaddr};
my $arpfilter = [];
if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
- foreach my $ipaddr (@$ipset) {
+ foreach my $ipaddr ($ipset->{entries}->@*) {
my($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
next if !$ip || ($version && $version != 4);
push(@$arpfilter, $ip);
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments Leo Nunner
@ 2023-01-27 11:41 ` Wolfgang Bumiller
2023-01-30 9:01 ` Leo Nunner
0 siblings, 1 reply; 11+ messages in thread
From: Wolfgang Bumiller @ 2023-01-27 11:41 UTC (permalink / raw)
To: Leo Nunner; +Cc: pve-devel
On Thu, Jan 26, 2023 at 03:30:19PM +0100, Leo Nunner wrote:
> This patch restructures the parsed config structure a bit to be more
> consistent across objects.
>
> group_comments and ipset_comments were removed from the config structure
> and are now stored directly within the group/ipset objects themselves.
> They now follow the same structure as aliases, with
>
> <name> => {
> comment => <...>,
> [entries|rules] => { <...> },
> }
>
> We don't need to store separate instances of the original + the
> lower-case name for aliases anymore, so the structure was changed to
>
> <name> => {
> comment => <...>,
> cidr => <...>,
> ipversion => <...>,
> }
>
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> RFC: This one is optional, it's just that while experimenting with
> the capitalization issue I also looked into using a "name" property
> for everything (like for aliases), and while I was at it, I also transfered
> the comments into the main object… I feel like this structure is nicer, but
> we don't _need_ it. My main worry is that there might still be some calls to
> $conf->{ipset}->{foo} instead of $conf->{ipset}->{foo}->{entries}, but I
> couldn't find any aside from the ones modified in this patch ^^
But in the end you dropped the `name` property of aliases instead.
Could you clarify your conclusion a bit?
Because now we have hashes with original names and need to `grep` their
keys instead of doing lookups because we don't know their
capitalization, and need to remember doing so everywhere.
To me this seems like a step backwards, given that the firewall is
already quite CPU-hungry at times?
It seems to me that all-lowercase hashes with original names inside
would be much eaiser? Sure, we'd have to "undo" this when saving or
returning stuff via the API for backward compatibility.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments
2023-01-27 11:41 ` Wolfgang Bumiller
@ 2023-01-30 9:01 ` Leo Nunner
0 siblings, 0 replies; 11+ messages in thread
From: Leo Nunner @ 2023-01-30 9:01 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
On 2023-01-27 12:41, Wolfgang Bumiller wrote:
> On Thu, Jan 26, 2023 at 03:30:19PM +0100, Leo Nunner wrote:
>> This patch restructures the parsed config structure a bit to be more
>> consistent across objects.
>>
>> group_comments and ipset_comments were removed from the config structure
>> and are now stored directly within the group/ipset objects themselves.
>> They now follow the same structure as aliases, with
>>
>> <name> => {
>> comment => <...>,
>> [entries|rules] => { <...> },
>> }
>>
>> We don't need to store separate instances of the original + the
>> lower-case name for aliases anymore, so the structure was changed to
>>
>> <name> => {
>> comment => <...>,
>> cidr => <...>,
>> ipversion => <...>,
>> }
>>
>> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
>> ---
>> RFC: This one is optional, it's just that while experimenting with
>> the capitalization issue I also looked into using a "name" property
>> for everything (like for aliases), and while I was at it, I also transfered
>> the comments into the main object… I feel like this structure is nicer, but
>> we don't _need_ it. My main worry is that there might still be some calls to
>> $conf->{ipset}->{foo} instead of $conf->{ipset}->{foo}->{entries}, but I
>> couldn't find any aside from the ones modified in this patch ^^
> But in the end you dropped the `name` property of aliases instead.
> Could you clarify your conclusion a bit?
When I added a name property for everything, it seemed to me that the
change was more invasive; the API endpoints needed to be expanded to
also return the actual name (like it was already the case with aliases),
and a bunch of changes were necessary to use that value instead of just
using the key iirc…
What also threw me off a bit was the need to add lc() calls all over the
place: for API calls, are we only going to take the lower-case value? Or
also the upper-case one? With the second one, we're going to need to
convert it in all the endpoints, since until now, they were always
expected to already be lower-cased. And not accepting the original name
in the API seems like it kind of defeats the purpose for me.
> Because now we have hashes with original names and need to `grep` their
> keys instead of doing lookups because we don't know their
> capitalization, and need to remember doing so everywhere.
Not *everywhere*, though? In the cases where I did it, it was as to not
have two groups/… with the same name (regardless of capitalization), and
that is only called when using the create/rename endpoint. I see how
that would be a non-issue when using a 'name' property, but this
shouldn't be *too* hard on the performance, since it's not called
regularly, right?
The call for aliases is a different story, since we'll have old config
files where the definition keeps the original name, while all
occurrences afterwards use the lower-case one (in rules/sets). If we
used a name property, are we going to do this everywhere? In the report
that partially motivated this patch [1], it was mentioned that
everything gets lower-cased in edit dialogues, and I feel like that
defeats the whole purpose again…
> To me this seems like a step backwards, given that the firewall is
> already quite CPU-hungry at times?
> It seems to me that all-lowercase hashes with original names inside
> would be much eaiser? Sure, we'd have to "undo" this when saving or
> returning stuff via the API for backward compatibility.
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4414
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pve-devel] [PATCH manager] ui: the API doesn't pass 'name' for aliases anymore
2023-01-26 14:30 [pve-devel] [PATCH firewall manager] Make firewall config case-insensitive Leo Nunner
` (3 preceding siblings ...)
2023-01-26 14:30 ` [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments Leo Nunner
@ 2023-01-26 14:30 ` Leo Nunner
2023-01-28 10:43 ` Thomas Lamprecht
4 siblings, 1 reply; 11+ messages in thread
From: Leo Nunner @ 2023-01-26 14:30 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
www/manager6/grid/FirewallAliases.js | 1 +
1 file changed, 1 insertion(+)
diff --git a/www/manager6/grid/FirewallAliases.js b/www/manager6/grid/FirewallAliases.js
index 00d0d74b..a1409d93 100644
--- a/www/manager6/grid/FirewallAliases.js
+++ b/www/manager6/grid/FirewallAliases.js
@@ -55,6 +55,7 @@ Ext.define('PVE.FirewallAliasEdit', {
me.load({
success: function(response, options) {
let values = response.result.data;
+ values.name = me.alias_name;
values.rename = values.name;
ipanel.setValues(values);
},
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread