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

[Hannes Laimer <h.laimer@proxmox.com>]

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