[pbs-devel] [PATCH proxmox 2/2] notify: matcher: API: update methods for nested matcher support

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

This commit adds support for setting the two new configuration keys for
matchers, `eval-matcher` and `nested`. Some checks are added to prevent
invalid configs which would lead to an error when a matcher is evaluated
(e.g. recursion).

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/api/matcher.rs | 229 ++++++++++++++++++++++++++++--
 proxmox-notify/src/api/mod.rs     |   4 +
 proxmox-notify/src/matcher.rs     |   4 +
 3 files changed, 228 insertions(+), 9 deletions(-)

diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index f5605acb..4ab837c5 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -1,10 +1,13 @@
+use std::collections::HashMap;
+use std::ops::Deref;
+
 use proxmox_http_error::HttpError;
 
 use crate::api::http_err;
 use crate::matcher::{
     DeleteableMatcherProperty, MatcherConfig, MatcherConfigUpdater, MATCHER_TYPENAME,
 };
-use crate::Config;
+use crate::{http_bail, Config};
 
 /// Get a list of all matchers
 ///
@@ -39,6 +42,7 @@ pub fn get_matcher(config: &Config, name: &str) -> Result<MatcherConfig, HttpErr
 pub fn add_matcher(config: &mut Config, matcher_config: MatcherConfig) -> Result<(), HttpError> {
     super::ensure_unique(config, &matcher_config.name)?;
     super::ensure_endpoints_exist(config, &matcher_config.target)?;
+    ensure_no_recursion(config, &matcher_config)?;
 
     config
         .config
@@ -83,43 +87,73 @@ pub fn update_matcher(
                 DeleteableMatcherProperty::InvertMatch => matcher.invert_match = None,
                 DeleteableMatcherProperty::Comment => matcher.comment = None,
                 DeleteableMatcherProperty::Disable => matcher.disable = None,
+                DeleteableMatcherProperty::EvalMatcher => matcher.eval_matcher.clear(),
+                DeleteableMatcherProperty::Nested => matcher.nested = None,
             }
         }
     }
 
-    if let Some(match_severity) = matcher_updater.match_severity {
+    let MatcherConfigUpdater {
+        match_severity,
+        match_field,
+        match_calendar,
+        mode,
+        invert_match,
+        nested,
+        eval_matcher,
+        comment,
+        disable,
+        target,
+    } = matcher_updater;
+
+    if let Some(match_severity) = match_severity {
         matcher.match_severity = match_severity;
     }
 
-    if let Some(match_field) = matcher_updater.match_field {
+    if let Some(match_field) = match_field {
         matcher.match_field = match_field;
     }
 
-    if let Some(match_calendar) = matcher_updater.match_calendar {
+    if let Some(match_calendar) = match_calendar {
         matcher.match_calendar = match_calendar;
     }
 
-    if let Some(mode) = matcher_updater.mode {
+    if let Some(mode) = mode {
         matcher.mode = Some(mode);
     }
 
-    if let Some(invert_match) = matcher_updater.invert_match {
+    if let Some(invert_match) = invert_match {
         matcher.invert_match = Some(invert_match);
     }
 
-    if let Some(comment) = matcher_updater.comment {
+    if let Some(comment) = comment {
         matcher.comment = Some(comment);
     }
 
-    if let Some(disable) = matcher_updater.disable {
+    if let Some(disable) = disable {
         matcher.disable = Some(disable);
     }
 
-    if let Some(target) = matcher_updater.target {
+    if let Some(nested) = nested {
+        matcher.nested = Some(nested);
+    }
+
+    if let Some(eval_matcher) = eval_matcher {
+        ensure_matchers_exist(config, eval_matcher.iter().map(Deref::deref))?;
+        matcher.eval_matcher = eval_matcher;
+    }
+
+    if let Some(target) = target {
         super::ensure_endpoints_exist(config, target.as_slice())?;
         matcher.target = target;
     }
 
+    ensure_no_recursion(config, &matcher)?;
+
+    if !matcher.nested.unwrap_or_default() {
+        ensure_not_referenced_by_other_matcher(config, &matcher.name)?;
+    }
+
     config
         .config
         .set_data(name, MATCHER_TYPENAME, &matcher)
@@ -142,12 +176,92 @@ pub fn update_matcher(
 pub fn delete_matcher(config: &mut Config, name: &str) -> Result<(), HttpError> {
     // Check if the matcher exists
     let _ = get_matcher(config, name)?;
+    super::ensure_safe_to_delete(config, name)?;
 
     config.config.sections.remove(name);
 
     Ok(())
 }
 
+/// Ensure that a matcher, identified by it's name, is not referenced by another matcher's
+/// `eval-matcher` statement.
+fn ensure_not_referenced_by_other_matcher(config: &Config, name: &str) -> Result<(), HttpError> {
+    let referrers = super::get_referrers(config, name)?;
+
+    if !referrers.is_empty() {
+        let used_by = referrers.into_iter().collect::<Vec<_>>().join(", ");
+        http_bail!(
+            BAD_REQUEST,
+            "can't remove 'nested' flag, matcher is referenced by: {used_by}"
+        );
+    }
+
+    Ok(())
+}
+
+/// Ensure that adding the matcher in `new_entry` to `config` would not create
+/// any direct or indirect recursion.
+fn ensure_no_recursion(config: &Config, new_entry: &MatcherConfig) -> Result<(), HttpError> {
+    let all_matchers = get_matchers(config)?;
+
+    let mut map: HashMap<String, &MatcherConfig> =
+        HashMap::from_iter(all_matchers.iter().map(|i| (i.name.clone(), i)));
+    map.insert(new_entry.name.clone(), new_entry);
+
+    for matcher in map.values() {
+        let mut trace = Vec::new();
+        traverse(matcher, &map, &mut trace)?;
+    }
+
+    Ok(())
+}
+
+/// Traverse matcher graph and check for any recursion.
+fn traverse<'a>(
+    matcher: &'a MatcherConfig,
+    matchers: &'a HashMap<String, &MatcherConfig>,
+    trace: &mut Vec<&'a String>,
+) -> Result<(), HttpError> {
+    // Recursion safety: Either we have a DAG and will terminate after visiting all
+    // nodes in the graph, or we quit early because we detected a loop by having the same
+    // matcher name twice in the trace.
+
+    if trace.contains(&&matcher.name) {
+        let mut trace_str = String::new();
+        for item in trace {
+            trace_str.push_str(item);
+            trace_str.push_str(" → ");
+        }
+
+        trace_str.push_str(&matcher.name);
+        http_bail!(BAD_REQUEST, "matcher recursion detected: {trace_str}");
+    }
+
+    trace.push(&matcher.name);
+
+    for next_name in &matcher.eval_matcher {
+        if let Some(next) = matchers.get(next_name) {
+            traverse(next, matchers, trace)?;
+        }
+    }
+
+    trace.pop();
+
+    Ok(())
+}
+
+/// Ensure that `config` contains all matchers in `matchers`.
+fn ensure_matchers_exist<'a>(
+    config: &'a Config,
+    matchers: impl Iterator<Item = &'a str>,
+) -> Result<(), HttpError> {
+    for name in matchers {
+        get_matcher(config, name)?;
+    }
+
+    Ok(())
+}
+
 #[cfg(all(test, feature = "sendmail"))]
 mod tests {
     use super::*;
@@ -259,4 +373,101 @@ matcher: matcher2
 
         Ok(())
     }
+
+    #[test]
+    fn test_matcher_delete_referenced_matcher_fails() {
+        let mut config = Config::new(
+            "
+matcher: matcher1
+    eval-matcher matcher2
+
+matcher: matcher2
+    nested true
+",
+            "",
+        )
+        .unwrap();
+
+        assert!(delete_matcher(&mut config, "matcher2").is_err());
+    }
+
+    #[test]
+    fn test_matcher_update_would_create_indirect_recursion() {
+        let mut config = Config::new(
+            "
+matcher: matcher1
+    nested true
+    eval-matcher matcher2
+
+matcher: matcher2
+    nested true
+",
+            "",
+        )
+        .unwrap();
+
+        assert!(update_matcher(
+            &mut config,
+            "matcher2",
+            MatcherConfigUpdater {
+                eval_matcher: Some(vec!["matcher1".into()]),
+                ..Default::default()
+            },
+            None,
+            None,
+        )
+        .is_err());
+    }
+
+    #[test]
+    fn test_matcher_update_would_create_direct_recursion() {
+        let mut config = Config::new(
+            "
+matcher: matcher1
+    nested true
+",
+            "",
+        )
+        .unwrap();
+
+        assert!(update_matcher(
+            &mut config,
+            "matcher1",
+            MatcherConfigUpdater {
+                eval_matcher: Some(vec!["matcher1".into()]),
+                ..Default::default()
+            },
+            None,
+            None,
+        )
+        .is_err());
+    }
+
+    #[test]
+    fn test_remove_nested_for_referenced_matcher() {
+        let mut config = Config::new(
+            "
+matcher: matcher1
+    nested true
+    eval-matcher matcher2
+
+matcher: matcher2
+    nested true
+",
+            "",
+        )
+        .unwrap();
+
+        assert!(update_matcher(
+            &mut config,
+            "matcher2",
+            MatcherConfigUpdater {
+                nested: Some(false),
+                ..Default::default()
+            },
+            None,
+            None,
+        )
+        .is_err());
+    }
 }
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index 7f823bc7..ee9bb2b9 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -202,6 +202,10 @@ fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, HttpE
         if matcher.target.iter().any(|target| target == entity) {
             referrers.insert(matcher.name.clone());
         }
+
+        if matcher.eval_matcher.iter().any(|target| target == entity) {
+            referrers.insert(matcher.name.clone());
+        }
     }
 
     Ok(referrers)
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index 3cc0189a..40867ab9 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -516,6 +516,10 @@ pub enum DeleteableMatcherProperty {
     Mode,
     /// Delete `target`
     Target,
+    /// Delete `nested`.
+    Nested,
+    /// Delete `eval-matcher`.
+    EvalMatcher,
 }
 
 pub fn check_matches<'a>(
-- 
2.39.5



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