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 <[email protected]> Signed-off-by: Thomas Lamprecht <[email protected]> --- @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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
