[pve-devel] [PATCH firewall] cluster config: soft-deprecate auto-disable timestamp support

* [pve-devel] [PATCH firewall] cluster config: soft-deprecate auto-disable timestamp support
@ 2025-10-07 13:09 Thomas Lamprecht
  2025-10-08  7:35 ` Hannes Laimer
  0 siblings, 1 reply; 2+ messages in thread
From: Thomas Lamprecht @ 2025-10-07 13:09 UTC (permalink / raw)
  To: pve-devel

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 <h.laimer@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

@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);
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread