[PATCH manager v7 39/41] api: notification: reject matchers with expression if not supported by all nodes

[Lukas Wagner <l.wagner@proxmox.com>]

The new matcher UI will automatically migrate any existing matcher to
one with an 'expression', and new matchers will also only use this new
parameter. If any of the cluster nodes uses a version of pve-manager
that does not yet support the new parameter, any matcher written by an
updated node will break notifications on the older nodes.

To avoid this, any time before writing the notification config after
creating/updating a matcher, we walk the matchers and check if any of
them has 'expression' set. If it is set, we check published pve-manager
version of all cluster nodes and reject the update to the config if any
node is too old.

While this still leaves the possibility of an outdated, offline node
coming back online and then failing to parse the migrated notification
config, it would be unreasonable to disallow editing the notification
configuration if any node is offline.

Also record the dependency on the newester libpve-common-perl in
d/control.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---

Notes:
    New in v3.
    
    Note to maintainers: Please change the version guard to the version
    of pve-manager that actually includes the change.
    
    Change since v3:
      - ran `make tidy`
      - Instead of only when creating/updating matchers, *all* calls that
        potentially write the notification config check for the new
        parameter. This is due to 'expression' being used in the new builtin
        'default-matcher' implementation, which will be written to the
        config on *any* change.
        In v3, if there was no config file there before, creating *any*
        new config entity would cause 'default-matcher' to be serialized,
        breaking notifications on outdated nodes.
    
    Changes since v5:
      - The 'default-matcher' remains now unchanged (without an expression),
        so we only need to check when creating/updating matchers, not one
        every single write of the notification config.
    
    Changes since v6:
      - Use pve_cfg_min_version from PVE::Tools

 PVE/API2/Cluster/Notifications.pm | 40 +++++++++++++++++++++++++++++++
 debian/control                    |  2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
index bd3c86cc..cbb04d2b 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -11,6 +11,7 @@ use PVE::Tools qw(extract_param);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
 use PVE::Notify;
+use PVE::SafeSyslog;
 
 use base qw(PVE::RESTHandler);
 
@@ -56,6 +57,43 @@ sub raise_api_error {
     die $exc;
 }
 
+my $matcher_expressions_version = "9.2.25";
+my $matcher_expressions_version_array = [9, 2, 25];
+
+my sub matcher_expression_supported_by_all_nodes {
+    my $version_info = PVE::Cluster::get_node_kv('version-info');
+    for my $node (sort keys $version_info->%*) {
+        my $node_info = eval { JSON::decode_json($version_info->{$node}); };
+        if (my $err = $@) { 
+            # warn, but continue.
+            # Rejecting writes to notifications.cfg if a cluster node is
+            # not available or if has not yet published its version through
+            # pvestatd would be unreasonable.
+            syslog('warn', "cannot parse version info for $node as JSON - $err");
+        } elsif (!PVE::Tools::pvecfg_min_version(
+            $node_info->{version},
+            @$matcher_expressions_version_array,
+        )) {
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
+# FIXME: MAJOR VERSION: This check can be dropped at the next major version bump.
+my sub ensure_expression_supported_if_used {
+    my ($config) = @_;
+
+    my $matchers = $config->get_matchers();
+    my $has_expression = grep { defined($_->{expression}) } $matchers->@*;
+
+    if ($has_expression && !matcher_expression_supported_by_all_nodes()) {
+        die "At least one cluster node does not support matcher expressions."
+            . " Please upgrade pve-manager to at least $matcher_expressions_version.";
+    }
+}
+
 __PACKAGE__->register_method({
     name => 'index',
     path => '',
@@ -1666,6 +1704,7 @@ __PACKAGE__->register_method({
 
                 $config->add_matcher($param);
 
+                ensure_expression_supported_if_used($config);
                 PVE::Notify::write_config($config);
             });
         };
@@ -1716,6 +1755,7 @@ __PACKAGE__->register_method({
                     $name, $param, $delete, $digest,
                 );
 
+                ensure_expression_supported_if_used($config);
                 PVE::Notify::write_config($config);
             });
         };
diff --git a/debian/control b/debian/control
index 2255af19..199987d5 100644
--- a/debian/control
+++ b/debian/control
@@ -62,7 +62,7 @@ Depends: apt (>= 1.5~),
          libpve-access-control (>= 9.2.0~),
          libpve-cluster-api-perl (>= 9.1.8),
          libpve-cluster-perl (>= 9.1.5),
-         libpve-common-perl (>= 9.2.6),
+         libpve-common-perl (>= 9.2.7),
          libpve-guest-common-perl (>= 5.1.4),
          libpve-http-server-perl (>= 6.0.3),
          libpve-network-api-perl (>= 1.6.0),
-- 
2.47.3