From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <l.nunner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id D0AA096C9F
for <pve-devel@lists.proxmox.com>; Thu, 26 Jan 2023 15:31:24 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id EEA4924A01
for <pve-devel@lists.proxmox.com>; Thu, 26 Jan 2023 15:30:53 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Thu, 26 Jan 2023 15:30:49 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A803B465B2
for <pve-devel@lists.proxmox.com>; Thu, 26 Jan 2023 15:30:48 +0100 (CET)
From: Leo Nunner <l.nunner@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Thu, 26 Jan 2023 15:30:19 +0100
Message-Id: <20230126143020.150338-5-l.nunner@proxmox.com>
X-Mailer: git-send-email 2.30.2
In-Reply-To: <20230126143020.150338-1-l.nunner@proxmox.com>
References: <20230126143020.150338-1-l.nunner@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.079 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [rules.pm, cluster.pm, vm.pm, groups.pm, ipset.pm]
Subject: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and
their comments
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 26 Jan 2023 14:31:24 -0000
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