[pbs-devel] [PATCH proxmox 2/2] notify: matcher: API: update methods for nested matcher support
Lukas Wagner
l.wagner at proxmox.com
Wed May 21 16:23:07 CEST 2025
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 at 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
More information about the pbs-devel
mailing list