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

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

Thanks a lot for this patch, much appreciated!

On  2025-03-13 16:17, Alexander Zeidler wrote:
> 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?

IMO they are fine, but somebody else should double-check.

> 
> Upcoming (from bugzilla report):
> - Lukas Wagner: Existing templates/variables/helpers have been
>   reviewed to make sure that they are OK to be public API

I'll revisit the existing templates in the next days and see if there is anything
we should fix up before exposing this to the user. This patch
should not be merged until the existing templates have been audited.

> - 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,

The `TemplateSource` type I mention below would also be used here
instead of a 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,

I think the imperative mood/voice is a bit confusing to describe the functionality of a function.
It reads like you are giving a command to the reader.

How about something like:

"The function attempts to render an override template if it exist and is renderable, otherwise
it uses the original template. If the [TemplateType] is `HtmlBody` but no HTML template could be
found or rendered, it falls back to the plaintext template, encapsulating the content in
a pre-formatted HTML block (<pre>)."

> +/// 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;
> +            }

Btw, you can match on multiple variants at the same time using the | operator.
           (TemplateType::HtmlBody | TemplateType::PlaintextBody | TemplateType::Subject, true) => {
               use_override = false;
           }

I think this would be nice to use in this case, there are actually only
3 distinct bodies of the match arms:
  - setting override to false
  - breaking from the loop
  - setting ty for HTML-from-plain fallback, resetting override to true

> +            (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)]


The only thing I don't like that much is the two boolean parameters for
`render_template_impl`... In this case where the function is called
in only one place (and it will probably stay that way) it is not that
bad, but boolean parameter always have the problem that they can be a
bit difficult to understand (you have to look at the definition of the
function to get the meaning) and can be easily mixed up (e.g. you could switch
the order of both parameters; this would be hard to spot in a review and the
compiler also does not help at all in this case.

I think what we could do here is the following. Have a 

pub enum TemplateSource {
    Vendor,
    Override
}

and change the 'use_override' parameter to a 'source' parameter.
You may want to derive `Display` for the `TemplateSource` enum
so that you can use in the the `tracing::error!` statement.

Also, we could make the HTML-falling-back-to-plaintext case a separate
variant in `TemplateType` e.g. TemplateType::HtmlFromPlaintext.
In the match statement you can then just set the `ty` and skip
setting `html_fallback` alltogether. Adding the `<html><pre>`
stuff can be moved to the `postprocess` function, similar
to how `Subject` is already postprocessed.


-- 
- Lukas



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