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

* [pve-devel] [PATCH proxmox] fix #6143: notify: allow overriding notification templates
@ 2025-03-21 13:33 Alexander Zeidler
  2025-03-28 13:27 ` Lukas Wagner
  0 siblings, 1 reply; 2+ messages in thread
From: Alexander Zeidler @ 2025-03-21 13:33 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>
---
This patch was previously sent as RFC and has now all suggestions from
Lukas Wagner implemented:
https://lore.proxmox.com/pve-devel/20250313151734.258337-1-a.zeidler@proxmox.com/

This patch should not be merged until the existing PVE and PBS
templates have been audited. For PVE this is currently being done, for
PBS a patch is already sent:
https://lore.proxmox.com/pbs-devel/20250321122521.198725-1-l.wagner@proxmox.com/

The documentation changes for PVE and PBS (steps for override template
creation, list of variables and helpers) will be sent in separate
patches after the above mentioned audit is completed.


 proxmox-notify/src/context/mod.rs  |   4 +-
 proxmox-notify/src/context/pbs.rs  |   9 +-
 proxmox-notify/src/context/pve.rs  |  10 +-
 proxmox-notify/src/context/test.rs |   2 +
 proxmox-notify/src/renderer/mod.rs | 154 ++++++++++++++++++++---------
 5 files changed, 129 insertions(+), 50 deletions(-)

diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
index c0a5a13b..8b6e2c43 100644
--- a/proxmox-notify/src/context/mod.rs
+++ b/proxmox-notify/src/context/mod.rs
@@ -1,6 +1,7 @@
 use std::fmt::Debug;
 use std::sync::Mutex;
 
+use crate::renderer::TemplateSource;
 use crate::Error;
 
 #[cfg(any(feature = "pve-context", feature = "pbs-context"))]
@@ -24,11 +25,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
+    /// Return the path of `filename` from `source` and a certain (optional) `namespace`
     fn lookup_template(
         &self,
         filename: &str,
         namespace: Option<&str>,
+        source: TemplateSource,
     ) -> Result<Option<String>, Error>;
 }
 
diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
index e8106c57..3e5da59c 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -7,6 +7,7 @@ use proxmox_schema::{ObjectSchema, Schema, StringSchema};
 use proxmox_section_config::{SectionConfig, SectionConfigPlugin};
 
 use crate::context::{common, Context};
+use crate::renderer::TemplateSource;
 use crate::Error;
 
 const PBS_USER_CFG_FILENAME: &str = "/etc/proxmox-backup/user.cfg";
@@ -109,8 +110,14 @@ impl Context for PBSContext {
         &self,
         filename: &str,
         namespace: Option<&str>,
+        source: TemplateSource,
     ) -> Result<Option<String>, Error> {
-        let path = Path::new("/usr/share/proxmox-backup/templates")
+        let path = match source {
+            TemplateSource::Vendor => "/usr/share/proxmox-backup/templates",
+            TemplateSource::Override => "/etc/proxmox-backup/notification-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..a97cce26 100644
--- a/proxmox-notify/src/context/pve.rs
+++ b/proxmox-notify/src/context/pve.rs
@@ -1,4 +1,5 @@
 use crate::context::{common, Context};
+use crate::renderer::TemplateSource;
 use crate::Error;
 use std::path::Path;
 
@@ -58,10 +59,17 @@ impl Context for PVEContext {
         &self,
         filename: &str,
         namespace: Option<&str>,
+        source: TemplateSource,
     ) -> Result<Option<String>, Error> {
-        let path = Path::new("/usr/share/pve-manager/templates")
+        let path = match source {
+            TemplateSource::Vendor => "/usr/share/pve-manager/templates",
+            TemplateSource::Override => "/etc/pve/notification-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..13ef6eae 100644
--- a/proxmox-notify/src/context/test.rs
+++ b/proxmox-notify/src/context/test.rs
@@ -1,4 +1,5 @@
 use crate::context::Context;
+use crate::renderer::TemplateSource;
 use crate::Error;
 
 #[derive(Debug)]
@@ -29,6 +30,7 @@ impl Context for TestContext {
         &self,
         _filename: &str,
         _namespace: Option<&str>,
+        _source: TemplateSource,
     ) -> 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..b37cf438 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -1,6 +1,6 @@
 //! Module for rendering notification templates.
 
-use std::time::Duration;
+use std::{fmt::Display, time::Duration};
 
 use handlebars::{
     Context, Handlebars, Helper, HelperDef, HelperResult, Output, RenderContext,
@@ -190,11 +190,29 @@ impl ValueRenderFunction {
     }
 }
 
-/// Available template types
+/// Choose between the provided `vendor` template or its by the user optionally created `override`
 #[derive(Copy, Clone)]
+pub enum TemplateSource {
+    Vendor,
+    Override,
+}
+
+impl Display for TemplateSource {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            TemplateSource::Vendor => f.write_str("vendor"),
+            TemplateSource::Override => f.write_str("override"),
+        }
+    }
+}
+
+/// Available template types
+#[derive(Copy, Clone, PartialEq)]
 pub enum TemplateType {
     /// HTML body template
     HtmlBody,
+    /// Fallback HTML body, based on the `PlaintextBody` template
+    HtmlBodyFromPlaintext,
     /// Plaintext body template
     PlaintextBody,
     /// Subject template
@@ -205,14 +223,24 @@ impl TemplateType {
     fn file_suffix(&self) -> &'static str {
         match self {
             TemplateType::HtmlBody => "body.html.hbs",
+            TemplateType::HtmlBodyFromPlaintext => "body.txt.hbs",
             TemplateType::PlaintextBody => "body.txt.hbs",
             TemplateType::Subject => "subject.txt.hbs",
         }
     }
 
     fn postprocess(&self, mut rendered: String) -> String {
-        if let Self::Subject = self {
-            rendered = rendered.replace('\n', " ");
+        match self {
+            TemplateType::HtmlBodyFromPlaintext => {
+                rendered = format!(
+                    "<html><body><pre>{}</pre></body></html>",
+                    handlebars::html_escape(&rendered)
+                )
+            }
+            TemplateType::Subject => {
+                rendered = rendered.replace('\n', " ");
+            }
+            _ => {}
         }
 
         rendered
@@ -221,6 +249,7 @@ impl TemplateType {
     fn block_render_fns(&self) -> BlockRenderFunctions {
         match self {
             TemplateType::HtmlBody => html::block_render_functions(),
+            TemplateType::HtmlBodyFromPlaintext => plaintext::block_render_functions(),
             TemplateType::Subject => plaintext::block_render_functions(),
             TemplateType::PlaintextBody => plaintext::block_render_functions(),
         }
@@ -231,6 +260,7 @@ impl TemplateType {
             TemplateType::PlaintextBody => handlebars::no_escape,
             TemplateType::Subject => handlebars::no_escape,
             TemplateType::HtmlBody => handlebars::html_escape,
+            TemplateType::HtmlBodyFromPlaintext => handlebars::no_escape,
         }
     }
 }
@@ -250,70 +280,100 @@ 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,
+    source: TemplateSource,
+) -> Result<Option<String>, Error> {
+    let template_string = context::context().lookup_template(&filename, None, source)?;
 
-    let block_render_fns = renderer.block_render_fns();
-    block_render_fns.register_helpers(&mut handlebars);
+    if let Some(template_string) = template_string {
+        let mut handlebars = Handlebars::new();
+        handlebars.register_escape_fn(renderer.escape_fn());
 
-    ValueRenderFunction::register_helpers(&mut handlebars);
+        let block_render_fns = renderer.block_render_fns();
+        block_render_fns.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, data)
-        .map_err(|err| Error::RenderError(err.into()))?;
+        handlebars.register_helper(
+            "relative-percentage",
+            Box::new(handlebars_relative_percentage_helper),
+        );
+
+        let rendered_template = handlebars
+            .render_template(&template_string, data)
+            .map_err(|err| Error::RenderError(err.into()))?;
+
+        let rendered_template = renderer.postprocess(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 is chosen via the `ty` parameter (see [TemplateType] for
+/// available options). If an override template is found and renderable, it is
+/// used instead of the vendor one. If the [TemplateType] is `HtmlBody` but no
+/// HTML template is found or renderable, it falls back to use a plaintext
+/// template encapsulated in a pre-formatted HTML block (<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 mut source = TemplateSource::Override;
+
+    loop {
+        let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+        let result = render_template_impl(data, ty, &filename, source);
+
+        match result {
+            Ok(Some(s)) => {
+                return Ok(s);
+            }
+            Ok(None) => {}
+            Err(err) => {
+                tracing::error!("failed to render {source} template '{filename}': {err}");
+            }
+        }
 
-    let (template_string, fallback) = match (template_string, ty) {
-        (None, TemplateType::HtmlBody) => {
-            ty = TemplateType::PlaintextBody;
-            let plaintext_filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+        match (ty, source) {
+            (
+                TemplateType::HtmlBody
+                | TemplateType::HtmlBodyFromPlaintext
+                | TemplateType::PlaintextBody
+                | TemplateType::Subject,
+                TemplateSource::Override,
+            ) => {
+                // Override template not found or renderable, try the vendor one instead
+                source = TemplateSource::Vendor;
+            }
+            (TemplateType::HtmlBody, TemplateSource::Vendor) => {
+                // Override and vendor HTML templates not found or renderable,
+                // try next the override plaintext as fallback
+                ty = TemplateType::HtmlBodyFromPlaintext;
+                source = TemplateSource::Override;
+            }
             (
-                context::context().lookup_template(&plaintext_filename, None)?,
-                true,
-            )
+                TemplateType::HtmlBodyFromPlaintext
+                | TemplateType::PlaintextBody
+                | TemplateType::Subject,
+                TemplateSource::Vendor,
+            ) => {
+                // Return error, no suitable templates found or renderable
+                break;
+            }
         }
-        (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)
-        );
     }
 
-    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