From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 04A4B1FF0C2 for ; Thu, 22 Oct 2026 14:50:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6D2F721A8D; Thu, 22 Oct 2026 14:48:36 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v4 11/27] firewall: tests: add tests for object reference updates Date: Thu, 22 Oct 2026 14:46:52 +0200 Message-ID: <20261022124708.429069-12-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261022124708.429069-1-a.bied-charreton@proxmox.com> References: <20261022124708.429069-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792673237266 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.814 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) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: TB7SM7A6MCPH7KX2XOWTABG55FAR7GRR X-Message-ID-Hash: TB7SM7A6MCPH7KX2XOWTABG55FAR7GRR X-MailFrom: a.bied-charreton@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 resolving unscoped references. Add a 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 --- src/test/Makefile | 2 +- src/test/test_update_refs.pl | 375 +++++++++++++++++++++++++++++++++++ 2 files changed, 376 insertions(+), 1 deletion(-) create mode 100755 src/test/test_update_refs.pl diff --git a/src/test/Makefile b/src/test/Makefile index 272a19a..ea3e378 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -1,4 +1,4 @@ -TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_simulator_protocols test_interface_names test_restore_errors +TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_simulator_protocols test_interface_names test_restore_errors test_update_refs .PHONY: all test $(TESTS) clean all: test diff --git a/src/test/test_update_refs.pl b/src/test/test_update_refs.pl new file mode 100755 index 0000000..f148eed --- /dev/null +++ b/src/test/test_update_refs.pl @@ -0,0 +1,375 @@ +#!/usr/bin/env perl + +# tests for the alias/ipset reference updating logic + +use lib qw(..); + +use v5.36; + +use Test::More; + +use PVE::Firewall::References; + +sub rewrite($conf, $kind, $old, $new, $env, $is_guest, $action = undef, $cluster_conf = {}) { + my $op = { + kind => $kind, + old => ref($old) eq 'ARRAY' ? $old : [$old], + new => $new, + action => $action // (defined($new) ? 'rename' : 'disable'), + cluster_conf => $cluster_conf, + }; + return PVE::Firewall::References::rewrite_refs_in_conf($conf, $op, $env, $is_guest); +} + +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 $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0); + + ok(scalar($changes->@*), 'reports changes'); + 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 $changes = rewrite($conf, 'ipset', 'foo', undef, 'cluster', 0); + + ok(scalar($changes->@*), 'reports changes'); + 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 $changes = rewrite($conf, 'ipset', 'foo', undef, 'cluster', 0, 'drop'); + + ok(scalar($changes->@*), 'reports changes'); + 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 $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 1); + + ok(scalar($changes->@*), 'reports changes'); + 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', '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', '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 $changes = rewrite($conf, 'ipset', $vnet_ipsets, undef, 'sdn', 0, 'disable', $conf); + + ok(scalar($changes->@*), 'reports changes'); + 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', $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 $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0); + + ok(scalar($changes->@*), 'reports changes'); + 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 $changes = rewrite($conf, 'aliases', 'al', 'new', 'cluster', 0); + + ok(scalar($changes->@*), 'reports changes'); + 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 $changes = rewrite($conf, 'aliases', 'al', undef, 'cluster', 0); + + ok(scalar($changes->@*), 'reports changes'); + + 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, 'aliases', '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, 'aliases', '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', '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 no changes' => sub { + my $conf = { rules => [{ source => '+other' }, { dest => 'somealias' }] }; + + my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0); + + is(scalar($changes->@*), 0, 'nothing matched -> no changes'); + is($conf->{rules}->[0]->{source}, '+other', 'unrelated refs not touched'); +}; + +subtest 'changes describe every reference that was touched' => sub { + my $conf = { + rules => [{ source => 'al', enable => 1 }, { source => 'keep', enable => 1 }], + groups => { grp => [{ dest => 'dc/al', enable => 1 }] }, + ipset => { set => [{ cidr => '10.0.0.1' }, { cidr => 'al' }] }, + }; + + my $changes = rewrite($conf, 'aliases', 'al', undef, 'cluster', 0, 'drop'); + + is(scalar($changes->@*), 3, 'one change per reference'); + + my $by_section = { map { ($_->{section} => $_) } $changes->@* }; + + is($by_section->{rules}->{pos}, 0, 'rule position recorded'); + is($by_section->{rules}->{ref}, 'al', 'matched reference recorded'); + is($by_section->{rules}->{action}, 'drop', 'action recorded'); + is($by_section->{group}->{name}, 'grp', 'group name recorded'); + is($by_section->{group}->{ref}, 'dc/al', 'scoped reference recorded'); + is($by_section->{ipset}->{name}, 'set', 'ipset name recorded'); + is($by_section->{ipset}->{pos}, 1, 'member position recorded, literal member skipped'); + + # update_refs tags each change with the config it was made in + $_->{scope} = 'cluster' for $changes->@*; + + is( + PVE::Firewall::References::format_ref_change($by_section->{ipset}), + "cluster: ipset 'set': member 1: 'al' dropped", + 'drop formatted for the task log', + ); +}; + +subtest 'rename changes record the new reference' => sub { + my $conf = { rules => [{ source => '+dc/foo' }] }; + + my $changes = rewrite($conf, 'ipset', 'foo', 'baz', 'cluster', 0); + + is($changes->[0]->{new}, '+dc/baz', 'new reference recorded'); + + $changes->[0]->{scope} = 'vm/100'; + + is( + PVE::Firewall::References::format_ref_change($changes->[0]), + "vm/100: rules: rule 0: '+dc/foo' renamed to '+dc/baz'", + 'rename formatted for the task log', + ); +}; + +subtest 'update_refs: rejects invalid argument combinations' => sub { + my $run = sub { + my (%args) = @_; + + eval { + PVE::Firewall::References::update_refs({ + conf => { rules => [] }, + kind => 'ipset', + env => 'vm', + subject => 'test', + log => sub { }, + %args, + }); + }; + + return $@; + }; + + like( + $run->(old => 'foo', new => 'baz', action => 'bogus'), + qr/invalid action/, + 'unknown action rejected', + ); + like( + $run->(kind => 'bogus', old => 'foo', new => 'baz', action => 'rename'), + qr/unknown object kind/, + 'unknown object kind rejected', + ); + like( + $run->(old => ['foo', 'bar'], new => 'baz', action => 'rename'), + qr/cannot rename more than one object/, + 'renaming several objects rejected', + ); + like( + $run->(old => 'foo', new => 'baz', env => 'cluster', action => 'rename'), + qr/\$save is required/, + 'cluster rename without a save callback rejected', + ); + # it goes on to walk the cluster, which this has no business doing, but it gets past the + # argument checks without a save callback + unlike( + $run->(old => 'foo', env => 'cluster', action => 'drop'), + qr/\$save is required/, + 'a drop needs no save callback', + ); +}; + +done_testing(); -- 2.47.3