[pve-devel] [RFC proxmox] fix #6143: notify: allow overriding notification templates

* [pve-devel] [RFC proxmox] fix #6143: notify: allow overriding notification templates
@ 2025-03-13 15:17 Alexander Zeidler
  2025-03-17 13:43 ` Lukas Wagner
  0 siblings, 1 reply; 2+ messages in thread
From: Alexander Zeidler @ 2025-03-13 15:17 UTC (permalink / raw)
  To: pve-devel

Previously, notification templates could be modified by the user, but
these were overwritten again with installing newer package versions of
pve-manager and proxmox-backup.

Now override templates can be created cluster-wide in the path
“/etc/{pve,proxmox-backup}/notification-templates/{namespace}”, which
are used with priority. The folder structure has to be created and
populated manually (e.g. /etc/pve/notification-templates/default/).

If override templates are not existing or their rendering fails, the
vendor templates in
"/usr/share/{pve-manager,proxmox-backup}/templates/default/" are used.

Sequence: [override html -> vendor html ->] override txt -> vendor txt

An error is only returned if none of the template candidates could be
used. Using an override template gets not logged.

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---

Questions:
- Is there anything against the chosen override path?

Upcoming (from bugzilla report):
- Lukas Wagner: Existing templates/variables/helpers have been
  reviewed to make sure that they are OK to be public API
- The steps needed to override the template are added to the
  notification docs
- Template variables and helpers are documented


 proxmox-notify/src/context/mod.rs  |   3 +-
 proxmox-notify/src/context/pbs.rs  |   9 ++-
 proxmox-notify/src/context/pve.rs  |  10 ++-
 proxmox-notify/src/context/test.rs |   1 +
 proxmox-notify/src/renderer/mod.rs | 124 ++++++++++++++++++-----------
 5 files changed, 99 insertions(+), 48 deletions(-)

diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
index c0a5a13b..c4649d91 100644
--- a/proxmox-notify/src/context/mod.rs
+++ b/proxmox-notify/src/context/mod.rs
@@ -24,11 +24,12 @@ pub trait Context: Send + Sync + Debug {
     fn http_proxy_config(&self) -> Option<String>;
     /// Return default config for built-in targets/matchers.
     fn default_config(&self) -> &'static str;
-    /// Lookup a template in a certain (optional) namespace
+    /// Lookup a template (or its override) in a certain (optional) namespace
     fn lookup_template(
         &self,
         filename: &str,
         namespace: Option<&str>,
+        use_override: bool,
     ) -> Result<Option<String>, Error>;
 }
 
diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
index e8106c57..da4d9ba0 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -109,8 +109,15 @@ impl Context for PBSContext {
         &self,
         filename: &str,
         namespace: Option<&str>,
+        use_override: bool,
     ) -> Result<Option<String>, Error> {
-        let path = Path::new("/usr/share/proxmox-backup/templates")
+        let path = if use_override {
+            "/etc/proxmox-backup/notification-templates"
+        } else {
+            "/usr/share/proxmox-backup/templates"
+        };
+
+        let path = Path::new(&path)
             .join(namespace.unwrap_or("default"))
             .join(filename);
 
diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
index d49ab27c..05b1b5b7 100644
--- a/proxmox-notify/src/context/pve.rs
+++ b/proxmox-notify/src/context/pve.rs
@@ -58,10 +58,18 @@ impl Context for PVEContext {
         &self,
         filename: &str,
         namespace: Option<&str>,
+        use_override: bool,
     ) -> Result<Option<String>, Error> {
-        let path = Path::new("/usr/share/pve-manager/templates")
+        let path = if use_override {
+            "/etc/pve/notification-templates"
+        } else {
+            "/usr/share/pve-manager/templates"
+        };
+
+        let path = Path::new(&path)
             .join(namespace.unwrap_or("default"))
             .join(filename);
+
         let template_string = proxmox_sys::fs::file_read_optional_string(path)
             .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
         Ok(template_string)
diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
index 5df25d05..6efd84c5 100644
--- a/proxmox-notify/src/context/test.rs
+++ b/proxmox-notify/src/context/test.rs
@@ -29,6 +29,7 @@ impl Context for TestContext {
         &self,
         _filename: &str,
         _namespace: Option<&str>,
+        _use_override: bool,
     ) -> Result<Option<String>, Error> {
         Ok(Some(String::new()))
     }
diff --git a/proxmox-notify/src/renderer/mod.rs b/proxmox-notify/src/renderer/mod.rs
index e058ea22..ac519bee 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -191,7 +191,7 @@ impl ValueRenderFunction {
 }
 
 /// Available template types
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, PartialEq)]
 pub enum TemplateType {
     /// HTML body template
     HtmlBody,
@@ -250,70 +250,104 @@ impl BlockRenderFunctions {
 }
 
 fn render_template_impl(
-    template: &str,
     data: &Value,
     renderer: TemplateType,
-) -> Result<String, Error> {
-    let mut handlebars = Handlebars::new();
-    handlebars.register_escape_fn(renderer.escape_fn());
+    filename: &str,
+    use_override: bool,
+    html_fallback: bool,
+) -> Result<Option<String>, Error> {
+    let template_string = context::context().lookup_template(&filename, None, use_override)?;
+
+    if let Some(template_string) = template_string {
+        let mut handlebars = Handlebars::new();
+        handlebars.register_escape_fn(renderer.escape_fn());
+
+        let block_render_fns = renderer.block_render_fns();
+        block_render_fns.register_helpers(&mut handlebars);
 
-    let block_render_fns = renderer.block_render_fns();
-    block_render_fns.register_helpers(&mut handlebars);
+        ValueRenderFunction::register_helpers(&mut handlebars);
+
+        handlebars.register_helper(
+            "relative-percentage",
+            Box::new(handlebars_relative_percentage_helper),
+        );
 
-    ValueRenderFunction::register_helpers(&mut handlebars);
+        let rendered_template = handlebars
+            .render_template(&template_string, data)
+            .map_err(|err| Error::RenderError(err.into()))?;
 
-    handlebars.register_helper(
-        "relative-percentage",
-        Box::new(handlebars_relative_percentage_helper),
-    );
+        let mut rendered_template = renderer.postprocess(rendered_template);
 
-    let rendered_template = handlebars
-        .render_template(template, data)
-        .map_err(|err| Error::RenderError(err.into()))?;
+        if html_fallback {
+            rendered_template = format!(
+                "<html><body><pre>{}</pre></body></html>",
+                handlebars::html_escape(&rendered_template)
+            );
+        }
 
-    Ok(rendered_template)
+        Ok(Some(rendered_template))
+    } else {
+        Ok(None)
+    }
 }
 
 /// Render a template string.
 ///
-/// The output format can be chosen via the `renderer` parameter (see [TemplateType]
-/// for available options).
+/// The output format can be chosen via the `ty` parameter (see [TemplateType]
+/// for available options). Use an override template if existing and renderable,
+/// otherwise fall back to original template. If [TemplateType] is `HtmlBody` but no
+/// HTML template could be used, fall back to plaintext templates and add the tags
+/// `<html><body><pre>`.
 pub fn render_template(
     mut ty: TemplateType,
     template: &str,
     data: &Value,
 ) -> Result<String, Error> {
-    let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
-
-    let template_string = context::context().lookup_template(&filename, None)?;
-
-    let (template_string, fallback) = match (template_string, ty) {
-        (None, TemplateType::HtmlBody) => {
-            ty = TemplateType::PlaintextBody;
-            let plaintext_filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
-            (
-                context::context().lookup_template(&plaintext_filename, None)?,
-                true,
-            )
+    let mut use_override = true;
+    let mut html_fallback = false;
+
+    loop {
+        let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+        let result = render_template_impl(data, ty, &filename, use_override, html_fallback);
+
+        match result {
+            Ok(Some(s)) => {
+                return Ok(s);
+            }
+            Ok(None) => {}
+            Err(err) => {
+                let source = if use_override { "override" } else { "vendor" };
+                tracing::error!("failed to render {source} template '{filename}': {err}");
+            }
         }
-        (template_string, _) => (template_string, false),
-    };
-
-    let template_string = template_string.ok_or(Error::Generic(format!(
-        "could not load template '{template}'"
-    )))?;
 
-    let mut rendered = render_template_impl(&template_string, data, ty)?;
-    rendered = ty.postprocess(rendered);
-
-    if fallback {
-        rendered = format!(
-            "<html><body><pre>{}</pre></body></html>",
-            handlebars::html_escape(&rendered)
-        );
+        match (ty, use_override) {
+            (TemplateType::HtmlBody, true) => {
+                use_override = false;
+            }
+            (TemplateType::HtmlBody, false) => {
+                ty = TemplateType::PlaintextBody;
+                use_override = true;
+                html_fallback = true;
+            }
+            (TemplateType::PlaintextBody, true) => {
+                use_override = false;
+            }
+            (TemplateType::PlaintextBody, false) => {
+                // return error, no other options
+                break;
+            }
+            (TemplateType::Subject, true) => {
+                use_override = false;
+            }
+            (TemplateType::Subject, false) => break,
+        }
     }
 
-    Ok(rendered)
+    Err(Error::Generic(
+        "failed to render notification template, all template candidates are erroneous or missing"
+            .into(),
+    ))
 }
 
 #[cfg(test)]
-- 
2.39.5



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

^ permalink raw reply	[flat|nested] 2+ messages in thread