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

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

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