[pve-devel] [PATCH firewall manager] Make firewall config case-insensitive

[pve-devel] [PATCH firewall manager] Make firewall config case-insensitive

[Leo Nunner]

Until now, the firewall config was quite inconsistent when it came to
capitalization; groups and ipsets were only allowed in lower-case, while
aliases were allowed to retain the original capitalization.

This patch removes all of these restrictions, and instead keeps the
original user-supplied values for groups, ipsets and aliases.
Furthermore, variable naming for rename parameters was cleaned up (due
to touching these lines anyway) and the docs for them were clarified a
bit.

Optionally, this patch also merges group_comments and ipset_comments
with their groups/ipsets into one unified structure.

firewall:

Leo Nunner (4):
  api: factor out renaming parameters to more descriptive names
  docs: clarify usage of 'rename' parameters
  config: make groups, IPSets and aliases case-insensitive
  config: combine group/ipset and their comments

 src/PVE/API2/Firewall/Aliases.pm |  42 +++++++------
 src/PVE/API2/Firewall/Cluster.pm |   2 +-
 src/PVE/API2/Firewall/Groups.pm  |  63 +++++++++----------
 src/PVE/API2/Firewall/IPSet.pm   |  54 ++++++++--------
 src/PVE/API2/Firewall/Rules.pm   |   4 +-
 src/PVE/API2/Firewall/VM.pm      |   2 +-
 src/PVE/Firewall.pm              | 105 +++++++++++++++++--------------
 7 files changed, 144 insertions(+), 128 deletions(-)

manager:

Leo Nunner (1):
  ui: the API doesn't pass 'name' for aliases anymore

 www/manager6/grid/FirewallAliases.js | 1 +
 1 file changed, 1 insertion(+)

-- 
2.30.2

[pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names

[Leo Nunner]

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

[pve-devel] [PATCH firewall 2/4] docs: clarify usage of 'rename' parameters

[Leo Nunner]

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

[pve-devel] [PATCH firewall 3/4] config: make groups, IPSets and aliases case-insensitive

[Leo Nunner]

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

[pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments

[Leo Nunner]

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

[pve-devel] [PATCH manager] ui: the API doesn't pass 'name' for aliases anymore

[Leo Nunner]

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

Re: [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names

[Wolfgang Bumiller]

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);

Re: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments

[Wolfgang Bumiller]

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.

Re: [pve-devel] [PATCH firewall 2/4] docs: clarify usage of 'rename' parameters

[Thomas Lamprecht]

as you need to sent another revision anyway, for the subject please do
s/^docs/api schema/

Re: [pve-devel] [PATCH manager] ui: the API doesn't pass 'name' for aliases anymore

[Thomas Lamprecht]

can you please add some context in the commit message to record since when the
API doesn't pass this along anymore, e.g. `since commit abcdef0123 ("foo bar")
in pve-firewall [...]`

As of now it sounds a bit like your series changed that behavior, which would
be a breaking change.

Re: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments

[Leo Nunner]

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