From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 281FF1FF0B3 for ; Fri, 25 Sep 2026 11:44:47 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A9BC021932; Fri, 25 Sep 2026 11:42:41 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com subject: SPAM: [PATCH pve-firewall v3 07/16] firewall: tests: add tests for object reference update logic Date: Fri, 25 Sep 2026 11:42:21 +0200 Message-ID: <20260925094230.844917-8-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260925094230.844917-1-a.bied-charreton@proxmox.com> References: <20260925094230.844917-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 3 AWL 0.669 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Message-ID-Hash: K7HARJKAFRXK6LQLYESIKFSDZ243FLCO X-Message-ID-Hash: K7HARJKAFRXK6LQLYESIKFSDZ243FLCO X-MailFrom: abied-charreton@jett.proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: The reference updating logic has quite a few edge cases, especially regarding the fact that cluster objects may be shadowed by guest objects. Add a few tests to consolidate the intended functionality. Note that these tests do not cover the actual iteration over the different configs in the cluster, rather they are focused on the core reference rewriting logic. Signed-off-by: Arthur Bied-Charreton --- test/Makefile | 1 + test/referenceupdatetests.pl | 296 +++++++++++++++++++++++++++++++++++ 2 files changed, 297 insertions(+) create mode 100755 test/referenceupdatetests.pl diff --git a/test/Makefile b/test/Makefile index fea9c21..15d2ae3 100644 --- a/test/Makefile +++ b/test/Makefile @@ -4,6 +4,7 @@ all: .PHONY: check check: ./fwtester.pl + ./referenceupdatetests.pl .PHONY: install install: check diff --git a/test/referenceupdatetests.pl b/test/referenceupdatetests.pl new file mode 100755 index 0000000..3912aab --- /dev/null +++ b/test/referenceupdatetests.pl @@ -0,0 +1,296 @@ +#!/usr/bin/perl + +# tests for the alias/ipset reference updating logic + +use lib '../src'; + +use v5.36; + +use Test::More; + +use PVE::Firewall::Helpers; + +my $ipset_spec = PVE::Firewall::Helpers::get_object_spec('ipset'); +my $alias_spec = PVE::Firewall::Helpers::get_object_spec('aliases'); + +sub rewrite($conf, $spec, $old, $new, $env, $is_guest, $action = undef, $cluster_conf = {}) { + $action //= defined($new) ? 'rename' : 'disable'; + $old = [$old] if ref($old) ne 'ARRAY'; + return PVE::Firewall::Helpers::rewrite_refs_in_conf( + $conf, $spec, $old, $new, $env, $is_guest, $cluster_conf, $action, + ); +} + +subtest 'ipset: cluster rename rewrites dc/ and bare refs' => sub { + my $conf = { + rules => [ + { source => '+dc/foo' }, + { dest => '+foo' }, + { source => '+bar' }, + { source => '+dc/bar' }, + ], + groups => { + grp => [{ source => '+dc/foo' }, { dest => '+foo' }], + }, + ipset => { + set => [{ cidr => '10.0.0.0/8' }], + }, + }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0); + + ok($modified, 'reports modified'); + is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref rewritten'); + is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref rewritten'); + is($conf->{rules}->[2]->{source}, '+bar', 'unrelated bare ref kept'); + is($conf->{rules}->[3]->{source}, '+dc/bar', 'unrelated dc/ ref kept'); + is($conf->{groups}->{grp}->[0]->{source}, '+dc/baz', 'group dc/ ref rewritten'); + is($conf->{groups}->{grp}->[1]->{dest}, '+baz', 'group bare ref rewritten'); + is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.0/8', 'ipset member not touched'); +}; + +subtest 'ipset: cluster delete disables matching rules' => sub { + my $conf = { + rules => [ + { source => '+dc/foo', enable => 1 }, + { source => '+foo', enable => 1 }, + { source => '+other', enable => 1 }, + ], + groups => + { grp => [{ dest => '+dc/foo', enable => 1 }, { dest => '+keep', enable => 1 }] }, + }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', undef, 'cluster', 0); + + ok($modified, 'reports modified'); + is(scalar($conf->{rules}->@*), 3, 'no rule dropped'); + is($conf->{rules}->[0]->{enable}, 0, 'dc/ ref rule disabled'); + is($conf->{rules}->[1]->{enable}, 0, 'bare ref rule disabled'); + is($conf->{rules}->[2]->{enable}, 1, 'unrelated rule untouched'); + is($conf->{groups}->{grp}->[0]->{enable}, 0, 'group rule disabled'); + is($conf->{groups}->{grp}->[1]->{enable}, 1, 'unrelated group rule untouched'); +}; + +subtest 'ipset: cluster drop removes matching rules' => sub { + my $conf = { + rules => [ + { source => '+dc/foo', enable => 1 }, + { source => '+foo', enable => 1 }, + { source => '+other', enable => 1 }, + ], + groups => + { grp => [{ dest => '+dc/foo', enable => 1 }, { dest => '+keep', enable => 1 }] }, + }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', undef, 'cluster', 0, 'drop'); + + ok($modified, 'reports modified'); + is(scalar($conf->{rules}->@*), 1, 'matching rules dropped'); + is($conf->{rules}->[0]->{source}, '+other', 'unrelated rule kept'); + is($conf->{rules}->[0]->{enable}, 1, 'kept rule not disabled'); + is(scalar($conf->{groups}->{grp}->@*), 1, 'matching group rule dropped'); + is($conf->{groups}->{grp}->[0]->{dest}, '+keep', 'unrelated group rule kept'); +}; + +subtest 'ipset: guest shadows cluster object' => sub { + my $conf = { + rules => [ + { source => '+foo' }, { dest => '+dc/foo' }, + ], + ipset => { foo => [] }, + }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 1); + + ok($modified, 'reports modified'); + is($conf->{rules}->[0]->{source}, '+foo', 'shadowed bare ref not touched'); + is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten'); +}; + +subtest 'ipset: guest without own object' => sub { + my $conf = { + rules => [{ source => '+foo' }, { dest => '+dc/foo' }], + ipset => {}, + }; + + rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 1); + + is($conf->{rules}->[0]->{source}, '+baz', 'implicit cluster ref rewritten'); + is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten'); +}; + +subtest 'ipset: guest-level rename' => sub { + my $conf = { + rules => [ + { source => '+foo' }, { dest => '+guest/foo' }, { source => '+dc/foo' }, + ], + }; + + rewrite($conf, $ipset_spec, 'foo', 'baz', 'vm', 0); + + is($conf->{rules}->[0]->{source}, '+baz', 'implicit guest ref rewritten'); + is($conf->{rules}->[1]->{dest}, '+guest/baz', 'guest/ ref rewritten'); + is($conf->{rules}->[2]->{source}, '+dc/foo', 'dc/ ref not touched in guest env'); +}; + +subtest 'ipset: sdn scope, shadowing and multiple objects' => sub { + my $conf = { + rules => [ + { source => '+sdn/vnet0-all', enable => 1 }, + { dest => '+vnet0-all', enable => 1 }, + { source => '+sdn/vnet0-dhcp', enable => 1 }, + { dest => '+vnet0-dhcp', enable => 1 }, + { source => '+dc/vnet0-all', enable => 1 }, + ], + # shadows the SDN-generated IPSet of the same name + ipset => { 'vnet0-dhcp' => [] }, + }; + + my $vnet_ipsets = ['vnet0-all', 'vnet0-dhcp']; + my $modified = rewrite($conf, $ipset_spec, $vnet_ipsets, undef, 'sdn', 0, 'disable', $conf); + + ok($modified, 'reports modified'); + is($conf->{rules}->[0]->{enable}, 0, 'sdn/ ref disabled'); + is($conf->{rules}->[1]->{enable}, 0, 'bare ref disabled'); + is($conf->{rules}->[2]->{enable}, 0, 'sdn/ ref disabled despite cluster IPSet'); + is($conf->{rules}->[3]->{enable}, 1, 'bare ref kept, resolves to the cluster IPSet'); + is($conf->{rules}->[4]->{enable}, 1, 'dc/ ref not touched in sdn env'); + + my $guest_conf = { + rules => [ + { source => '+sdn/vnet0-all', enable => 1 }, + { dest => '+vnet0-all', enable => 1 }, + ], + ipset => { 'vnet0-all' => [] }, + }; + + rewrite($guest_conf, $ipset_spec, $vnet_ipsets, undef, 'sdn', 1, 'disable', {}); + + is($guest_conf->{rules}->[0]->{enable}, 0, 'sdn/ ref disabled in guest conf'); + is($guest_conf->{rules}->[1]->{enable}, 1, 'bare ref kept, resolves to the guest IPSet'); +}; + +subtest 'ipset: not confused with alias' => sub { + my $conf = { rules => [{ source => 'foo' }, { dest => '+foo' }] }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0); + + ok($modified, 'reports modified'); + is($conf->{rules}->[0]->{source}, 'foo', 'bare alias ref not touched'); + is($conf->{rules}->[1]->{dest}, '+baz', 'ipset ref rewritten'); +}; + +subtest 'alias: cluster rename rewrites rules and ipset members' => sub { + my $conf = { + rules => [{ source => 'al' }, { dest => 'dc/al' }, { source => 'other' }], + groups => { grp => [{ source => 'dc/al' }] }, + ipset => { + set => [{ cidr => 'al' }, { cidr => 'dc/al' }, { cidr => '10.0.0.1' }], + }, + }; + + my $modified = rewrite($conf, $alias_spec, 'al', 'new', 'cluster', 0); + + ok($modified, 'reports modified'); + is($conf->{rules}->[0]->{source}, 'new', 'bare alias in rule rewritten'); + is($conf->{rules}->[1]->{dest}, 'dc/new', 'dc/ alias in rule rewritten'); + is($conf->{rules}->[2]->{source}, 'other', 'unrelated rule kept'); + is($conf->{groups}->{grp}->[0]->{source}, 'dc/new', 'alias in group rewritten'); + is($conf->{ipset}->{set}->[0]->{cidr}, 'new', 'bare alias in ipset member rewritten'); + is($conf->{ipset}->{set}->[1]->{cidr}, 'dc/new', 'dc/ alias in ipset member rewritten'); + is($conf->{ipset}->{set}->[2]->{cidr}, '10.0.0.1', 'literal cidr member kept'); +}; + +subtest 'alias: cluster delete disables rules, drops ipset members' => sub { + my $conf = { + rules => [ + { source => 'al', dest => '10.0.0.1', action => 'ACCEPT', enable => 1 }, + { source => 'keep', enable => 1 }, + ], + groups => + { grp => [{ source => 'dc/al', enable => 1 }, { source => 'keep', enable => 1 }] }, + ipset => { + set => + [{ cidr => 'al' }, { cidr => 'dc/al', nomatch => 1 }, { cidr => '10.0.0.1' }], + }, + }; + + my $modified = rewrite($conf, $alias_spec, 'al', undef, 'cluster', 0); + + ok($modified, 'reports modified'); + + is(scalar($conf->{rules}->@*), 2, 'no rule dropped'); + is($conf->{rules}->[0]->{enable}, 0, 'matching rule disabled'); + is($conf->{rules}->[0]->{source}, 'al', 'disabled rule keeps its reference'); + is($conf->{rules}->[0]->{dest}, '10.0.0.1', 'disabled rule keeps its other properties'); + is($conf->{rules}->[1]->{enable}, 1, 'unrelated rule untouched'); + + is(scalar($conf->{groups}->{grp}->@*), 2, 'no group rule dropped'); + is($conf->{groups}->{grp}->[0]->{enable}, 0, 'matching group rule disabled'); + is($conf->{groups}->{grp}->[1]->{enable}, 1, 'unrelated group rule untouched'); + + is(scalar($conf->{ipset}->{set}->@*), 1, 'matching members dropped, not disabled'); + is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.1', 'literal member kept'); + ok( + !(grep { exists($_->{enable}) } $conf->{ipset}->{set}->@*), + 'no ipset member carries an enable flag', + ); +}; + +subtest 'alias: cluster drop removes rules and ipset members' => sub { + my $conf = { + rules => [{ source => 'al', enable => 1 }, { source => 'keep', enable => 1 }], + ipset => { set => [{ cidr => 'al' }, { cidr => 'dc/al' }, { cidr => '10.0.0.1' }] }, + }; + + rewrite($conf, $alias_spec, 'al', undef, 'cluster', 0, 'drop'); + + is(scalar($conf->{rules}->@*), 1, 'matching rule dropped'); + is($conf->{rules}->[0]->{source}, 'keep', 'unrelated rule kept'); + is($conf->{rules}->[0]->{enable}, 1, 'kept rule not disabled'); + is(scalar($conf->{ipset}->{set}->@*), 1, 'matching members dropped'); + is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.1', 'literal member kept'); +}; + +subtest 'alias: not confused with ipset' => sub { + my $conf = { rules => [{ source => '+al' }, { dest => 'al' }] }; + + rewrite($conf, $alias_spec, 'al', 'new', 'cluster', 0); + + is($conf->{rules}->[0]->{source}, '+al', 'ipset ref (+) not touched'); + is($conf->{rules}->[1]->{dest}, 'new', 'alias ref rewritten'); +}; + +subtest 'case-insensitive match, written back lowercase' => sub { + my $conf = { rules => [{ source => '+DC/FOO' }, { dest => '+Foo' }] }; + + rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0); + + is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref matched and lowercased'); + is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref matched and lowercased'); +}; + +subtest 'no match reports not modified' => sub { + my $conf = { rules => [{ source => '+other' }, { dest => 'somealias' }] }; + + my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0); + + ok(!$modified, 'nothing matched -> not modified'); + is($conf->{rules}->[0]->{source}, '+other', 'unrelated refs not touched'); +}; + +subtest 'update_refs: rejects invalid argument combinations' => sub { + my $conf = { rules => [] }; + + eval { + PVE::Firewall::Helpers::update_refs($conf, $ipset_spec, 'foo', 'baz', 'vm', 'bogus'); + }; + like($@, qr/invalid action/, 'unknown action rejected'); + + eval { + PVE::Firewall::Helpers::update_refs($conf, $ipset_spec, ['foo', 'bar'], 'baz', 'vm'); + }; + like($@, qr/cannot rename more than one object/, 'renaming several objects rejected'); +}; + +done_testing(); -- 2.47.3