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

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

[Thomas Lamprecht]

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

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

[Hannes Laimer]

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 <h.laimer@proxmox.com>

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



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