* [pbs-devel] [PATCH proxmox 1/2] notify: matcher: allow to evaluate other matchers via `eval-matcher`
2025-05-21 14:23 [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers Lukas Wagner
@ 2025-05-21 14:23 ` Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox 2/2] notify: matcher: API: update methods for nested matcher support Lukas Wagner
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2025-05-21 14:23 UTC (permalink / raw)
To: pbs-devel
This commit adds support for nested matchers, allowing to build
arbitrary matching 'formulas'.
A matcher can now have one or more `eval-matcher` directives. These
take the name of another matcher as a parameter. When the
matcher is evaluated, we first check any referenced nested matcher and
use their results to compute the final verdict for the matcher.
If one wants to use a matcher via `eval-matcher`, the matcher
must be marked as nested. Any nested matcher is only evaluated when
another matcher references it. Any configured targets for a nested
matcher are ignored, only the 'top-most' matcher's targets are
notified if the notification matches.
Direct/indirect recursion is not allowed (A -> A, A -> B -> A)
an raises an error.
A simple cache is introduced to make sure that we don't have to evaluate
any nested matcher more than once.
Simple config example:
matcher: top-level
target mail-to-root
mode any
eval-matcher a
eval-matcher b
matcher: a
nested true
mode all
match-field exact:datastore=store
match-field exact:type=gc
match-severity error
matcher: b
nested true
mode all
match-field exact:datastore=store
match-field exact:type=prune
match-severity error
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/lib.rs | 20 ++-
proxmox-notify/src/matcher.rs | 309 ++++++++++++++++++++++++++++++++--
2 files changed, 304 insertions(+), 25 deletions(-)
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 12e59474..bb5bb605 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -376,7 +376,7 @@ impl Config {
#[derive(Default)]
pub struct Bus {
endpoints: HashMap<String, Box<dyn Endpoint>>,
- matchers: Vec<MatcherConfig>,
+ matchers: HashMap<String, MatcherConfig>,
}
#[allow(unused_macros)]
@@ -514,10 +514,14 @@ impl Bus {
);
}
- let matchers = config
- .config
- .convert_to_typed_array(MATCHER_TYPENAME)
- .map_err(|err| Error::ConfigDeserialization(err.into()))?;
+ let matchers = HashMap::from_iter(
+ config
+ .config
+ .convert_to_typed_array(MATCHER_TYPENAME)
+ .map_err(|err| Error::ConfigDeserialization(err.into()))?
+ .into_iter()
+ .map(|e: MatcherConfig| (e.name.clone(), e)),
+ );
Ok(Bus {
endpoints,
@@ -531,8 +535,8 @@ impl Bus {
}
#[cfg(test)]
- pub fn add_matcher(&mut self, filter: MatcherConfig) {
- self.matchers.push(filter)
+ pub fn add_matcher(&mut self, matcher: MatcherConfig) {
+ self.matchers.insert(matcher.name.clone(), matcher);
}
/// Send a notification. Notification matchers will determine which targets will receive
@@ -540,7 +544,7 @@ impl Bus {
///
/// Any errors will not be returned but only logged.
pub fn send(&self, notification: &Notification) {
- let targets = matcher::check_matches(self.matchers.as_slice(), notification);
+ let targets = matcher::check_matches(&self.matchers, notification);
for target in targets {
if let Some(endpoint) = self.endpoints.get(target) {
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index 083c2dbd..3cc0189a 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -1,4 +1,4 @@
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
use std::fmt;
use std::fmt::Debug;
use std::str::FromStr;
@@ -98,6 +98,13 @@ pub const MATCH_FIELD_ENTRY_SCHEMA: Schema = StringSchema::new("Match metadata f
},
optional: true,
},
+ "eval-matcher": {
+ type: Array,
+ items: {
+ schema: ENTITY_NAME_SCHEMA,
+ },
+ optional: true,
+ },
"target": {
type: Array,
items: {
@@ -106,7 +113,7 @@ pub const MATCH_FIELD_ENTRY_SCHEMA: Schema = StringSchema::new("Match metadata f
optional: true,
},
})]
-#[derive(Debug, Serialize, Deserialize, Updater, Default)]
+#[derive(Clone, Debug, Serialize, Deserialize, Updater, Default)]
#[serde(rename_all = "kebab-case")]
/// Config for Sendmail notification endpoints
pub struct MatcherConfig {
@@ -136,6 +143,11 @@ pub struct MatcherConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub invert_match: Option<bool>,
+ /// List of nested matchers to evaluate.
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub eval_matcher: Vec<String>,
+
/// Targets to notify.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
#[updater(serde(skip_serializing_if = "Option::is_none"))]
@@ -149,6 +161,13 @@ pub struct MatcherConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub disable: Option<bool>,
+ /// Determine if this matcher can be used as a nested matcher.
+ /// Nested matchers are only evaluated if they are referenced by
+ /// another matcher. Any configured targets of a nested matcher
+ /// are ignored.
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub nested: Option<bool>,
+
/// Origin of this config entry.
#[serde(skip_serializing_if = "Option::is_none")]
#[updater(skip)]
@@ -282,7 +301,30 @@ impl FromStr for FieldMatcher {
}
impl MatcherConfig {
- pub fn matches(&self, notification: &Notification) -> Result<Option<&[String]>, Error> {
+ pub fn matches(
+ &self,
+ notification: &Notification,
+ all_matchers: &HashMap<String, MatcherConfig>,
+ cache: &mut HashMap<String, bool>,
+ ) -> Result<Option<&[String]>, Error> {
+ let mut trace = HashSet::new();
+ trace.insert(self.name.clone());
+ let result = self.matches_impl(notification, all_matchers, &mut trace, cache)?;
+
+ if result {
+ Ok(Some(&self.target))
+ } else {
+ Ok(None)
+ }
+ }
+
+ fn matches_impl(
+ &self,
+ notification: &Notification,
+ all_matchers: &HashMap<String, MatcherConfig>,
+ trace: &mut HashSet<String>,
+ cache: &mut HashMap<String, bool>,
+ ) -> Result<bool, Error> {
let mode = self.mode.unwrap_or_default();
let mut is_match = mode.neutral_element();
@@ -311,13 +353,52 @@ impl MatcherConfig {
);
}
- let invert_match = self.invert_match.unwrap_or_default();
+ for matcher_name in &self.eval_matcher {
+ no_matchers = false;
+ match all_matchers.get(matcher_name) {
+ Some(matcher) if matcher.nested.unwrap_or_default() => {
+ if trace.contains(matcher_name) {
+ return Err(Error::FilterFailed(
+ "recursive sub-matcher definition".into(),
+ ));
+ }
- Ok(if is_match != invert_match || no_matchers {
- Some(&self.target)
- } else {
- None
- })
+ if matcher.disable.unwrap_or_default() {
+ continue;
+ }
+
+ trace.insert(matcher.name.clone());
+
+ if let Some(cached_result) = cache.get(&matcher.name) {
+ is_match = mode.apply(is_match, *cached_result);
+ } else {
+ is_match = mode.apply(
+ is_match,
+ matcher.matches_impl(notification, all_matchers, trace, cache)?,
+ );
+ }
+
+ trace.remove(matcher_name);
+ }
+ Some(_) => {
+ return Err(Error::FilterFailed(
+ "referenced matcher is not declared as sub-matcher".into(),
+ ));
+ }
+ None => {
+ return Err(Error::FilterFailed(
+ "referenced sub-matcher does not exist".into(),
+ ));
+ }
+ }
+ }
+
+ let invert_match = self.invert_match.unwrap_or_default();
+ is_match = is_match != invert_match || no_matchers;
+
+ cache.insert(self.name.clone(), is_match);
+
+ Ok(is_match)
}
/// Check if given `MatchDirectives` match a notification.
@@ -438,24 +519,31 @@ pub enum DeleteableMatcherProperty {
}
pub fn check_matches<'a>(
- matchers: &'a [MatcherConfig],
+ matchers: &'a HashMap<String, MatcherConfig>,
notification: &Notification,
) -> HashSet<&'a str> {
let mut targets = HashSet::new();
+ let mut cache = HashMap::new();
- for matcher in matchers {
- if matcher.disable.unwrap_or_default() {
- // Skip this matcher if it is disabled
- info!("skipping disabled matcher '{name}'", name = matcher.name);
+ for (name, matcher) in matchers {
+ if matcher.nested.unwrap_or_default() {
+ // Matchers which are declared are only evaluated if a
+ // top-level, non-nested matcher references it.
continue;
}
- match matcher.matches(notification) {
+ if matcher.disable.unwrap_or_default() {
+ // Skip this matcher if it is disabled
+ info!("skipping disabled matcher '{name}'");
+ continue;
+ }
+
+ match matcher.matches(notification, matchers, &mut cache) {
Ok(t) => {
let t = t.unwrap_or_default();
targets.extend(t.iter().map(|s| s.as_str()));
}
- Err(err) => error!("matcher '{matcher}' failed: {err}", matcher = matcher.name),
+ Err(err) => error!("matcher '{name}' failed: {err}"),
}
}
@@ -505,6 +593,7 @@ mod tests {
assert!("regex:'3=b.*".parse::<FieldMatcher>().is_err());
assert!("invalid:'bar=b.*".parse::<FieldMatcher>().is_err());
}
+
#[test]
fn test_severities() {
let notification =
@@ -526,7 +615,193 @@ mod tests {
..Default::default()
};
- assert!(config.matches(¬ification).unwrap().is_some())
+ let mut all_matchers = HashMap::new();
+ all_matchers.insert("default".to_string(), config.clone());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .unwrap()
+ .is_some())
}
}
+
+ #[test]
+ fn test_submatcher() {
+ let mut all_matchers = HashMap::new();
+
+ let config = MatcherConfig {
+ name: "sub-1".to_string(),
+ nested: Some(true),
+ mode: Some(MatchModeOperator::All),
+ match_severity: vec!["error".parse().unwrap()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "sub-2".to_string(),
+ nested: Some(true),
+ mode: Some(MatchModeOperator::All),
+ match_field: vec!["exact:datastore=backups".parse().unwrap()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "top".to_string(),
+ eval_matcher: vec!["sub-1".into(), "sub-2".into()],
+ mode: Some(MatchModeOperator::Any),
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let notification = Notification::from_template(
+ Severity::Notice,
+ "test",
+ Value::Null,
+ HashMap::from_iter([("datastore".into(), "backups".into())]),
+ );
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .unwrap()
+ .is_some());
+
+ let notification = Notification::from_template(
+ Severity::Error,
+ "test",
+ Value::Null,
+ HashMap::from_iter([("datastore".into(), "other".into())]),
+ );
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .unwrap()
+ .is_some());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .unwrap()
+ .is_some());
+
+ let notification = Notification::from_template(
+ Severity::Warning,
+ "test",
+ Value::Null,
+ HashMap::from_iter([("datastore".into(), "other".into())]),
+ );
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .unwrap()
+ .is_none());
+ }
+
+ #[test]
+ fn test_submatcher_recursion_direct() {
+ let mut all_matchers = HashMap::new();
+
+ let config = MatcherConfig {
+ name: "sub-1".to_string(),
+ nested: Some(true),
+ eval_matcher: vec!["sub-1".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "top".to_string(),
+ eval_matcher: vec!["sub-1".into()],
+ mode: Some(MatchModeOperator::Any),
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, HashMap::new());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .is_err());
+ }
+
+ #[test]
+ fn test_submatcher_recursion_indirect() {
+ let mut all_matchers = HashMap::new();
+
+ let config = MatcherConfig {
+ name: "sub-1".to_string(),
+ nested: Some(true),
+ eval_matcher: vec!["sub-2".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "sub-2".to_string(),
+ nested: Some(true),
+ eval_matcher: vec!["sub-1".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "top".to_string(),
+ eval_matcher: vec!["sub-1".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, HashMap::new());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .is_err());
+ }
+
+ #[test]
+ fn test_submatcher_does_not_exist() {
+ let mut all_matchers = HashMap::new();
+
+ let config = MatcherConfig {
+ name: "top".to_string(),
+ eval_matcher: vec!["doesntexist".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, HashMap::new());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .is_err());
+ }
+
+ #[test]
+ fn test_submatcher_does_not_declared_as_submatcher() {
+ let mut all_matchers = HashMap::new();
+
+ let config = MatcherConfig {
+ name: "sub-1".to_string(),
+ nested: Some(true),
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let config = MatcherConfig {
+ name: "top".to_string(),
+ eval_matcher: vec!["doesntexist".into()],
+ ..Default::default()
+ };
+ all_matchers.insert(config.name.clone(), config.clone());
+
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, HashMap::new());
+
+ assert!(config
+ .matches(¬ification, &all_matchers, &mut HashMap::new())
+ .is_err());
+ }
}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pbs-devel] [PATCH proxmox 2/2] notify: matcher: API: update methods for nested matcher support
2025-05-21 14:23 [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox 1/2] notify: matcher: allow to evaluate other matchers via `eval-matcher` Lukas Wagner
@ 2025-05-21 14:23 ` Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox-widget-toolkit 1/1] notifications: matchers: add " Lukas Wagner
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2025-05-21 14:23 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [pbs-devel] [PATCH proxmox-widget-toolkit 1/1] notifications: matchers: add nested matcher support
2025-05-21 14:23 [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox 1/2] notify: matcher: allow to evaluate other matchers via `eval-matcher` Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox 2/2] notify: matcher: API: update methods for nested matcher support Lukas Wagner
@ 2025-05-21 14:23 ` Lukas Wagner
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox-backup 1/1] docs: notifications: add section about nested matchers Lukas Wagner
2025-06-02 14:35 ` [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for " Lukas Wagner
4 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2025-05-21 14:23 UTC (permalink / raw)
To: pbs-devel
Adds a new checkbox to declare a matcher is 'nested', as well as a
new node type in the rule tree 'Evaluate nested matcher'. The latter
allows one to select other matcher which is declared as 'nested'.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
src/data/model/NotificationConfig.js | 8 +-
src/panel/NotificationConfigView.js | 6 +
src/window/NotificationMatcherEdit.js | 220 +++++++++++++++++++++++++-
3 files changed, 232 insertions(+), 2 deletions(-)
diff --git a/src/data/model/NotificationConfig.js b/src/data/model/NotificationConfig.js
index 03cf317..e1f7058 100644
--- a/src/data/model/NotificationConfig.js
+++ b/src/data/model/NotificationConfig.js
@@ -9,7 +9,13 @@ Ext.define('proxmox-notification-endpoints', {
Ext.define('proxmox-notification-matchers', {
extend: 'Ext.data.Model',
- fields: ['name', 'comment', 'disable', 'origin'],
+ fields: [
+ 'name',
+ 'comment',
+ 'disable',
+ 'origin',
+ 'nested',
+ ],
proxy: {
type: 'proxmox',
},
diff --git a/src/panel/NotificationConfigView.js b/src/panel/NotificationConfigView.js
index 9505eb7..994ff9d 100644
--- a/src/panel/NotificationConfigView.js
+++ b/src/panel/NotificationConfigView.js
@@ -326,6 +326,12 @@ Ext.define('Proxmox.panel.NotificationMatcherView', {
renderer: (disable) => Proxmox.Utils.renderEnabledIcon(!disable),
align: 'center',
},
+ {
+ dataIndex: 'nested',
+ text: gettext('Nested'),
+ renderer: (nested) => Proxmox.Utils.renderEnabledIcon(nested),
+ align: 'center',
+ },
{
dataIndex: 'name',
text: gettext('Matcher Name'),
diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
index 83c09ea..06597d5 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -21,6 +21,21 @@ Ext.define('Proxmox.panel.NotificationMatcherGeneralPanel', {
allowBlank: false,
checked: true,
},
+ {
+ xtype: 'proxmoxcheckbox',
+ name: 'nested',
+ fieldLabel: gettext('Nested matcher'),
+ allowBlank: false,
+ checked: false,
+ bind: '{isNestedMatcher}',
+ autoEl: {
+ tag: 'div',
+ 'data-qtip': gettext('Nested matchers can be used by other matchers to create sophisticated matching rules.'),
+ },
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ },
+ },
{
xtype: 'proxmoxtextfield',
name: 'comment',
@@ -64,9 +79,24 @@ Ext.define('Proxmox.panel.NotificationMatcherTargetPanel', {
{
xtype: 'pmxNotificationTargetSelector',
name: 'target',
- allowBlank: false,
+ allowBlank: true,
},
],
+
+ onGetValues: function(values) {
+ let me = this;
+
+ if (Ext.isArray(values.target)) {
+ if (values.target.length === 0) {
+ delete values.target;
+ if (!me.isCreate) {
+ Proxmox.Utils.assemble_field_data(values, { 'delete': 'target' });
+ }
+ }
+ }
+
+ return values;
+ },
});
Ext.define('Proxmox.window.NotificationMatcherEdit', {
@@ -81,6 +111,12 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
width: 800,
+ viewModel: {
+ data: {
+ isNestedMatcher: false,
+ },
+ },
+
initComponent: function() {
let me = this;
@@ -130,6 +166,9 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
xtype: 'pmxNotificationMatcherTargetPanel',
isCreate: me.isCreate,
baseUrl: me.baseUrl,
+ bind: {
+ disabled: '{isNestedMatcher}',
+ },
},
],
},
@@ -357,6 +396,11 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
value: '',
};
break;
+ case 'eval-matcher':
+ data = {
+ matcher: '',
+ };
+ break;
}
let node = {
@@ -424,6 +468,7 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
xtype: 'pmxNotificationMatchRuleSettings',
cbind: {
baseUrl: '{baseUrl}',
+ name: '{name}',
},
},
@@ -445,6 +490,7 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
deleteArrayIfEmtpy('match-field');
deleteArrayIfEmtpy('match-severity');
deleteArrayIfEmtpy('match-calendar');
+ deleteArrayIfEmtpy('eval-matcher');
return values;
},
@@ -506,6 +552,14 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
iconCls = 'fa fa-filter';
break;
+ case 'eval-matcher': {
+ let v = data.matcher;
+ text = Ext.String.format(gettext("Evaluate nested matcher: {0}"), v);
+ iconCls = 'fa fa-filter';
+ if (!v) {
+ iconCls += ' internal-error';
+ }
+ } break;
}
return [text, iconCls];
@@ -632,12 +686,29 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
deleteEmpty: !me.isCreate,
});
+ let realEvalMatcher = Ext.create({
+ xtype: 'hiddenfield',
+ name: 'eval-matcher',
+ setValue: function(value) {
+ this.value = value;
+ this.checkChange();
+ },
+ getValue: function() {
+ return this.value;
+ },
+ getSubmitValue: function() {
+ let value = this.value;
+ return value;
+ },
+ });
+
let storeChanged = function(store) {
store.suspendEvent('datachanged');
let matchFieldStmts = [];
let matchSeverityStmts = [];
let matchCalendarStmts = [];
+ let evalMatcherStmts = [];
let modeStmt = 'all';
let invertMatchStmt = false;
@@ -663,6 +734,9 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
modeStmt = data.value;
invertMatchStmt = data.invert;
break;
+ case 'eval-matcher':
+ evalMatcherStmts.push(data.matcher);
+ break;
}
let [text, iconCls] = me.getNodeTextAndIcon(type, data);
@@ -692,6 +766,10 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
realMatchSeverity.setValue(matchSeverityStmts);
realMatchSeverity.resumeEvent('change');
+ realEvalMatcher.suspendEvent('change');
+ realEvalMatcher.setValue(evalMatcherStmts);
+ realEvalMatcher.resumeEvent('change');
+
store.resumeEvent('datachanged');
};
@@ -800,6 +878,30 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
});
});
+ realEvalMatcher.addListener('change', function(field, value) {
+ let parseEvalMatcher = function(matcher) {
+ return {
+ type: 'eval-matcher',
+ data: {
+ matcher: matcher,
+ },
+ leaf: true,
+ };
+ };
+
+ for (let node of treeStore.queryBy(
+ record => record.get('type') === 'eval-matcher').getRange()) {
+ node.remove(true);
+ }
+
+ let records = value.map(parseEvalMatcher);
+ let rootNode = treeStore.getRootNode();
+
+ for (let record of records) {
+ rootNode.appendChild(record);
+ }
+ });
+
treeStore.addListener('datachanged', storeChanged);
let treePanel = Ext.create({
@@ -844,6 +946,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
realMatchSeverity,
realInvertMatch,
realMatchCalendar,
+ realEvalMatcher,
treePanel,
{
xtype: 'button',
@@ -913,6 +1016,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
['match-field', gettext('Match Field')],
['match-severity', gettext('Match Severity')],
['match-calendar', gettext('Match Calendar')],
+ ['eval-matcher', gettext('Evaluate nested matcher')],
],
},
{
@@ -927,6 +1031,13 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
{
xtype: 'pmxNotificationMatchCalendarSettings',
},
+ {
+ xtype: 'pmxNotificationEvalMatcherSettings',
+ cbind: {
+ baseUrl: '{baseUrl}',
+ name: '{name}',
+ },
+ },
],
});
@@ -1372,3 +1483,110 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
me.callParent();
},
});
+
+Ext.define('Proxmox.panel.EvalMatcherSettings', {
+ extend: 'Ext.panel.Panel',
+ xtype: 'pmxNotificationEvalMatcherSettings',
+ border: false,
+ layout: 'anchor',
+ // Hide initially to avoid glitches when opening the window
+ hidden: true,
+ bind: {
+ hidden: '{!typeIsEvalMatcher}',
+ },
+ viewModel: {
+ // parent is set in `initComponents`
+ formulas: {
+ typeIsEvalMatcher: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ get: function(record) {
+ return record?.get('type') === 'eval-matcher';
+ },
+ },
+ evalMatcherValue: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ set: function(value) {
+ let record = this.get('selectedRecord');
+ let currentData = record.get('data');
+ record.set({
+ data: {
+ ...currentData,
+ matcher: value,
+ },
+ });
+ },
+ get: function(record) {
+ return record?.get('data')?.matcher;
+ },
+ },
+ },
+ },
+
+ initComponent: function() {
+ let me = this;
+
+ let valueStore = Ext.create('Ext.data.Store', {
+ model: 'proxmox-notification-matchers',
+ autoLoad: true,
+ proxy: {
+ type: 'proxmox',
+ url: `/api2/json/${me.baseUrl}/matchers`,
+ },
+ filters: [
+ {
+ property: 'nested',
+ value: true,
+ },
+ (item) => item.get('name') !== me.name,
+ ],
+ });
+
+ Ext.apply(me.viewModel, {
+ parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+ });
+ Ext.apply(me, {
+ items: [
+ {
+ fieldLabel: gettext('Matcher'),
+ xtype: 'proxmoxComboGrid',
+ autoSelect: false,
+ editable: false,
+ isFormField: false,
+ submitValue: false,
+ allowBlank: false,
+ showClearTrigger: true,
+ field: 'name',
+ store: valueStore,
+ valueField: 'name',
+ displayField: 'name',
+ notFoundIsValid: false,
+ multiSelect: false,
+ bind: {
+ value: '{evalMatcherValue}',
+ },
+ listConfig: {
+ columns: [
+ {
+ header: gettext('Name'),
+ dataIndex: 'name',
+ flex: 1,
+ },
+ {
+ header: gettext('Comment'),
+ dataIndex: 'comment',
+ flex: 2,
+ },
+ ],
+ },
+ },
+ ],
+ });
+ me.callParent();
+ },
+});
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [pbs-devel] [PATCH proxmox-backup 1/1] docs: notifications: add section about nested matchers
2025-05-21 14:23 [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers Lukas Wagner
` (2 preceding siblings ...)
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox-widget-toolkit 1/1] notifications: matchers: add " Lukas Wagner
@ 2025-05-21 14:23 ` Lukas Wagner
2025-06-02 14:35 ` [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for " Lukas Wagner
4 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2025-05-21 14:23 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
docs/notifications.rst | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/docs/notifications.rst b/docs/notifications.rst
index 5669dab0..5b990394 100644
--- a/docs/notifications.rst
+++ b/docs/notifications.rst
@@ -258,6 +258,29 @@ Examples:
The following severities are in use:
``info``, ``notice``, ``warning``, ``error``, ``unknown``.
+Nested Matcher Rules
+^^^^^^^^^^^^^^^^^^^^
+
+By using ``eval-matcher <other-matcher-name>`` rules, a matcher can evaluate
+other matchers that were marked as a nested matcher. This allows to build
+arbitrarily complex matching rules.
+
+Any matcher that shall be used as a nested matcher must have the ``nested``
+flag set. Nested matchers do not have any associated notification targets. They
+are only evaluated if they are referenced by another matcher's ``eval-matcher``
+rule. If a nested matcher is disabled, it will not be evaluated even if
+referenced by another matcher.
+
+A nested matcher itself can evaluate other nested matchers. Direct (a matcher
+evaluates itself) and indirect (two or more matchers evaluate each other)
+recursion is not allowed.
+
+Examples:
+
+* ``eval-matcher sub-1``: Evaluates the ``sub-1`` matcher and uses its result
+ to compute the result of the current matcher.
+
+
.. _notification_events:
Notification Events
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers
2025-05-21 14:23 [pbs-devel] [PATCH proxmox{, -widget-toolkit, -backup} 0/4] notifications: add support for nested matchers Lukas Wagner
` (3 preceding siblings ...)
2025-05-21 14:23 ` [pbs-devel] [PATCH proxmox-backup 1/1] docs: notifications: add section about nested matchers Lukas Wagner
@ 2025-06-02 14:35 ` Lukas Wagner
4 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2025-06-02 14:35 UTC (permalink / raw)
To: pbs-devel
On 2025-05-21 16:23, Lukas Wagner wrote:
> This series adds support for nested notification matchers. A new match rule is
> added, 'eval-matcher', which allows to evaluate other matchers and use their
> result in the current matcher.
>
> Any matcher that shall be used as a nested matcher must have the (new) `nested`
> flag set to true. These matchers are only evaluated if they are referenced
> by another matcher. Any target configured for a nested matcher is ignored,
> only the 'top-level'/'outermost' matcher decides which targets to notify.
>
> This patch series includes:
> - patches for proxmox-notify, which add the new config entries for matchers,
> API methods and nested matcher evaluation
> - patches for proxmox-backup, which contain documentation for the new
> feature
> - patches for proxmox-widget-toolkit, containing the GUI changes needed
> for this feature.
>
> Small config example:
>
> matcher: top-level
> target mail-to-root
> mode any
> eval-matcher a
> eval-matcher b
>
> matcher: a
> nested true
> mode all
> match-field exact:datastore=store
> match-field exact:type=gc
> match-severity error
>
> matcher: b
> nested true
> mode all
> match-field exact:datastore=store
> match-field exact:type=prune
> match-severity error
>
I think I might need to NACK my own patch series here. After some
experimentation and more careful testing, I've become quite unhappy with the
overall UX of these new nested matchers.
For context, my original plan when I first created the notification stack was
to eventually allow to create nested rules by adding new "any/all" sub-nodes in
the match rule tree of a *single* matcher. Each of these new nodes could then
have further 'match-*' rules as their children, etc.
In this series however, I proposed a different approach. Here you could
reference *other* matchers from a matcher's rule tree; the nested matcher
must be created first from the regular matcher overview.
The reason why I went with the current approach were technical
limitations of our tech stack. Neither section-config nor the our POST/PUT
x-www-form-urlencoded data work particularly well for highly nested data
structures, like match rule nodes of a tree.
The current approach was the most straight-forward one to implement,
without requiring major changes to any component in the stack,
neither the UI, nor the API, nor the config format.
I see the following problems with my current approach:
- The matcher overview becomes cluttered quite quickly. For example,
implementing the (legacy) notification policy of a single datastore
(always, never, error) x (GC, Prune, Verify, Sync) results in up to 5
matchers (1 top-level matching 'any' of 4 nested matches, one for each
event type/severity combination). Having multiple datastores with different
policies (e.g. different admins, or production/non-production), makes it
even more cluttered and requires conscious effort by the admin to keep
things easy to understand (e.g. good naming/comments for nested matchers)
- A nested matcher must already exist when being added to a top-level
matcher. Creating matchers 'bottom-up' feels unnatural and might force
users to jump around different matcher config windows to produce the
desired policy.
- Understanding a nested matcher structure requires opening multiple modal
dialogs, hampering usability. There is no way to see the full matcher
hierarchy at once.
I think this should be implemented in a way that aligns more with my
original idea. The core takeaway of these UX problems is that these (nested)
rules should be edited from a single place in the UI, such as the match rule
tree that already exists in the matcher edit window.
I think the most realistic option, keeping the limitations of our API and
section config in mind, is to find a backwards-compatible way to flatten these
nested rules from the tree and store them all in a single 'matcher' section.
Alternatively, we could keep the proposed nested/eval-matcher structure in
the config file and then provide some API to update multiple
matchers in one go (either multiple POST/PUT wrapped in a transaction, or by
having a single endpoint which then 'de-multiplexes' into multiple
matcher sections.
I really wish we could just send a JSON body in the POST/PUT and store the
rules in a config format with proper nesting, this would make all of this
*much* easier, both for API consumers as well as the API/config
implementation.
--
- Lukas
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread