[pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments
Leo Nunner
l.nunner at proxmox.com
Thu Jan 26 15:30:19 CET 2023
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 at 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
More information about the pve-devel
mailing list