From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 2DF411FF183 for ; Wed, 8 Oct 2025 09:35:06 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 17B2E275F0; Wed, 8 Oct 2025 09:35:09 +0200 (CEST) Message-ID: <89765d75-d788-4bd3-b6d2-764808d0dd77@proxmox.com> Date: Wed, 8 Oct 2025 09:35:03 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , pve-devel@lists.proxmox.com References: <20251007131113.2785229-1-t.lamprecht@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: <20251007131113.2785229-1-t.lamprecht@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759908873465 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.045 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy 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 Subject: Re: [pve-devel] [PATCH firewall] cluster config: soft-deprecate auto-disable timestamp support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Generated types look good. Thanks! What I did notice though is that there were a few changes to the API spec recently which resulted in pve-api-types/pve-api.json | 426 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------- 1 file changed, 271 insertions(+), 155 deletions(-) (this is with the versions we currently have in the staging repo + proxmox-firewall master including this patch) I sent a small series[1] updating the api spec dump and fixing a small problem that was caused in pdm by the new types in the pve api spec. [1] https://lore.proxmox.com/pdm-devel/20251008073107.102213-1-h.laimer@proxmox.com/T/#t This is just a side-note, but I mentioned here it cause it is somewhat related. Consider this: Tested-by: Hannes Laimer On 10/7/25 15:10, Thomas Lamprecht wrote: > The firewall got some crude auto-disable support added in commit > 72d055f ("firewall autodisable"), but integration into our UI never > made it from the mailing list into the repo, as the last reviewed > version [0] was never followed up on. > > As it's unused by us, undocumented in general and not supported in the > newer rust based NFT alternative implementation of our firewall it's > probably best to soft-deprecate it now and remove it completely with > the next major release. > > The reason for doing this now is that Hannes is working on firewall > integration in PDM, and that uses pve-api-types which is mostly > generated from the API schema, and the integer for 'enabled' made him > question this. > > If we ever want an auto-disable we probably should encode it as > dedicated variable and also rethink the checks/process needed for > triggering it, because the original idea might have been susceptible > to temporary false positives due to connection tracking allowing the > connection that would not be allowed once the conntrack info expired. > And it might be better to spawn a worker task handling the enable + > wait + disable-if-no-ack cycle such that it's actually logged. > > Anyway, as of now switch the 'enabled' property to a boolean type, but > override the input schema to still accept a integer for backward > compat but warn if we parse anything other than a integer boolean; if > this causes issue I think it would be even fine to change the schema > for both, input and ouput schemas. > > [0]: https://lists.proxmox.com/pipermail/pve-devel/2015-July/015863.html > > Reported-by: Hannes Laimer > Signed-off-by: Thomas Lamprecht > --- > > @Hannes: please test if the rust serialization code works with the input > still being marked as integer here, as said, otherwise we can change it > to boolean and see if anybody notices (which for this I'd risk) or maybe > try using oneOf. > > src/PVE/API2/Firewall/Cluster.pm | 10 +++++++++- > src/PVE/Firewall.pm | 12 ++++++++---- > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm > index 51d0e43..b8d1d23 100644 > --- a/src/PVE/API2/Firewall/Cluster.pm > +++ b/src/PVE/API2/Firewall/Cluster.pm > @@ -76,7 +76,7 @@ my $add_option_properties = sub { > my ($properties) = @_; > > foreach my $k (keys %$option_properties) { > - $properties->{$k} = $option_properties->{$k}; > + $properties->{$k} = $option_properties->{$k} if !defined($properties->{$k}); > } > > return $properties; > @@ -119,6 +119,14 @@ __PACKAGE__->register_method({ > parameters => { > additionalProperties => 0, > properties => &$add_option_properties({ > + # TODO: remove override for enable with PVE 10. This was only done for backward compat, > + # but the timestamp is nowhere used and our UI only sets booleans anyway. > + enable => { > + description => "Enable or disable the firewall cluster wide.", > + type => 'integer', > + minimum => 0, > + optional => 1, > + }, > delete => { > type => 'string', > format => 'pve-configid-list', > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm > index ec9c9ae..999d206 100644 > --- a/src/PVE/Firewall.pm > +++ b/src/PVE/Firewall.pm > @@ -1283,8 +1283,7 @@ sub copy_list_with_digest { > our $cluster_option_properties = { > enable => { > description => "Enable or disable the firewall cluster wide.", > - type => 'integer', > - minimum => 0, > + type => 'boolean', > default => 0, > optional => 1, > }, > @@ -3321,9 +3320,14 @@ sub parse_clusterfw_option { > if ($line =~ m/^(enable):\s*(\d+)\s*$/i) { > $opt = lc($1); > $value = int($2); > - if (($value > 1) && ((time() - $value) > 60)) { > - $value = 0; > + if ($value > 1) { > + # TODO: remove this with PVE 10 > + warn "deprecated auto-disable timestamp format used for 'enable' cluster firewall" > + . " config property; support for this will be removed in PVE 10\n"; > + > + $value = 0 if (time() - $value) > 60; > } > + $value = $value ? 1 : 0; > } elsif ($line =~ m/^(ebtables):\s*(0|1)\s*$/i) { > $opt = lc($1); > $value = int($2); _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel