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

Alexander Zeidler a.zeidler at proxmox.com
Fri Mar 21 14:36:00 CET 2025


On Mon Mar 17, 2025 at 2:43 PM CET, Lukas Wagner wrote:
> Thanks a lot for this patch, much appreciated!
Thank you! All of your comments are implemented in the new patch:
https://lore.proxmox.com/pve-devel/20250321133341.151340-1-a.zeidler@proxmox.com/

>
> 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 at 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.
Implemented in new patch.

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

Implemented in new patch:
> 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)]
>
>

Implemented in new patch:
> 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.
>

Implemented in new patch:
> 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.





More information about the pve-devel mailing list