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 D1CA61FF0C2 for ; Thu, 22 Oct 2026 14:47:21 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 74814216C7; Thu, 22 Oct 2026 14:47:21 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v4 03/27] firewall: references: log the references an object update touched Date: Thu, 22 Oct 2026 14:46:44 +0200 Message-ID: <20261022124708.429069-4-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: 1792673236747 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.933 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: WZXCKEDL77OJKHVFDPA4LRNWIZYJV2ZY X-Message-ID-Hash: WZXCKEDL77OJKHVFDPA4LRNWIZYJV2ZY 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: Make update_refs() log each reference as it is rewritten, so a run that dies halfway through the cluster still leaves an account of what it got to: firewall: ipset 'set' renamed to 'set2': cluster: rules: rule 6: '+set' renamed to '+set2' cluster: group 'fixgrp': rule 1: '+set' renamed to '+set2' firewall: 2 references updated The tally only counts what landed, so it is logged whether or not the walk ran to the end. Errors are raised unchanged once that is written, as what they mean for the caller's API is the caller's to phrase. The lines go to stdout in a worker, and to the syslog otherwise, since PVE::Daemon sends a daemon's stdout to /dev/null. Callers that have to rewrite before their task exists can pass a log sink and replay the lines in the worker. Signed-off-by: Arthur Bied-Charreton --- src/PVE/Firewall/References.pm | 93 +++++++++++++++++++++++++++++++--- 1 file changed, 87 insertions(+), 6 deletions(-) diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm index 8998380..fb25bc2 100644 --- a/src/PVE/Firewall/References.pm +++ b/src/PVE/Firewall/References.pm @@ -6,6 +6,8 @@ use warnings; use PVE::Cluster; use PVE::Firewall; use PVE::Network::SDN::Vnets; +use PVE::RESTEnvironment; +use PVE::SafeSyslog qw(syslog); use base 'Exporter'; our @EXPORT_OK = qw( @@ -71,7 +73,11 @@ sub rewrite_refs_in_env { my ($fw_conf, $env, $is_guest, $scope) = @_; my $conf_changes = rewrite_refs_in_conf($fw_conf, $op, $env, $is_guest); - push $changes->@*, map { { $_->%*, scope => $scope } } $conf_changes->@*; + for my $change ($conf_changes->@*) { + my $scoped = { $change->%*, scope => $scope }; + push $changes->@*, $scoped; + $op->{on_change}->($scoped) if $op->{on_change}; + } return scalar($conf_changes->@*); }; @@ -317,9 +323,16 @@ sub rewrite_refs_in_conf { return $ctx->{changes}; } +my $ref_change_verbs = { + rename => 'updated', + disable => 'disabled', + drop => 'dropped', +}; + =head3 update_refs($opts) -Rename, disable or drop all references to a firewall object across the environment. +Rename, disable or drop all references to a firewall object across the environment, logging each +one as it is rewritten. References are matched in rules, security groups and IPSet members. Matching is case-insensitive and renames are written back lowercased. @@ -333,6 +346,10 @@ C<$opts> holds: env # 'cluster', 'sdn', or 'vm'/'ct' for a guest-defined object action # 'rename', 'disable' or 'drop', or undef to leave the references alone save # persists C, required for a cluster-wide rename + subject # what the log should call the operation + log # where the logs go, task log if in worker or syslog by default + +The caller is responsible for locking and saving C itself. IPSet members have no disabled state, so C removes them as well. @@ -342,7 +359,8 @@ C itself have been rewritten and before any downstream config is: downstre rewritten to reference the object under its new name, and a concurrent compilation parsing the cluster config would fail to resolve that name until it has been written. -The caller is responsible for locking and saving C itself. +If the logs cannot be written as we go (SDN apply is an example, since the conf update is done +before forking a worker and logs would go to syslog), pass C with custom collection logic. Return the number of references changed. @@ -351,9 +369,15 @@ Return the number of references changed. sub update_refs { my ($opts) = @_; - my ($conf, $kind, $old, $new, $env, $action) = $opts->@{qw(conf kind old new env action)}; + my ($conf, $kind, $old, $new, $env, $action, $subject) = + $opts->@{qw(conf kind old new env action subject)}; - return 0 if !defined($action); + my $log = $opts->{log} // \&log_ref_line; + + if (!defined($action)) { + $log->("firewall: $subject: references left as they are"); + return 0; + } die "invalid action '$action'" if $action !~ m/^(rename|disable|drop)$/; die "\$new must be set if \$action is 'rename'" if $action eq 'rename' && !defined($new); @@ -365,6 +389,10 @@ sub update_refs { die "cannot rename more than one object at once\n" if $action eq 'rename' && scalar($lc_old->@*) != 1; + $log->("firewall: $subject:"); + + my $count = 0; + my $op = { kind => $kind, old => $lc_old, @@ -372,9 +400,62 @@ sub update_refs { action => $action, cluster_conf => $conf, save => $opts->{save}, + on_change => sub { + $count++; + $log->(' ' . format_ref_change($_[0])); + }, }; - return scalar(rewrite_refs_in_env($conf, $env, $op)->@*); + eval { rewrite_refs_in_env($conf, $env, $op) }; + my $err = $@; + + # if the walk died before incrementing $count there is nothing to log + if ($count) { + my $noun = $count == 1 ? 'reference' : 'references'; + $log->("firewall: $count $noun $ref_change_verbs->{$action}"); + } elsif (!$err) { + $log->("firewall: no references found"); + } + + die $err if $err; + + return $count; +} + +=head3 format_ref_change($change) + +Format one recorded config change as a single line, e.g. + + "cluster: rules: rule 6: '+ipset' renamed to '+ipset1'" + +=cut + +sub format_ref_change { + my ($change) = @_; + + my $section = $change->{section}; + my $where = $section eq 'rules' ? 'rules' : "$section '$change->{name}'"; + my $item = $section eq 'ipset' ? 'member' : 'rule'; + + my $what = "$change->{scope}: $where: $item $change->{pos}: '$change->{ref}'"; + + return "$what renamed to '$change->{new}'" if $change->{action} eq 'rename'; + return "$what dropped" if $change->{action} eq 'drop'; + + # an IPSet member has no disabled state, so disabling it removed it + return "$what " . ($section eq 'ipset' ? 'removed' : 'disabled'); +} + +sub log_ref_line { + my ($line) = @_; + + if (PVE::RESTEnvironment::is_worker()) { + print("$line\n"); + } else { + syslog('info', '%s', $line); + } + + return; } 1; -- 2.47.3