Re: [PATCH datacenter-manager v2] Add notifications backend

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Thu, Apr 09, 2026 at 11:20:22AM +0200, Shannon Sterz wrote:

Hey, thanks for the feedback! As a general answer, here is my reasoning 
as to why I did not make any improvemenets when porting the
notifications stack from PBS to PDM.

A possible goal Lukas and I discussed would be for proxmox-notify to 
include the router, to prevent the ~1000 lines of code duplication from 
spreading across 3 products.

This is currently not doable nicely because PVE defines its router on
the Perl side, meaning we would still need to have 2 different entry
points. Keeping this in mind though, I wanted to avoid the PDM router
implementation diverging from the PBS one too much in order to make an
eventual migration less painful.

I understand that this might be thinking too far into a future that will
possibly never happen, so I am of course happy to incorporate the 
changes you proposed.

> On Thu Apr 9, 2026 at 6:57 AM CEST, Arthur Bied-Charreton wrote:
> 
> nit: imo a commit message like "server: add notification backend" would
> be more appropriate. usually it is recommended to try add a tag prefix
> of what your code touches [1].
> 
> [1]: https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages
> 
> > Ideally, the whole router should be in proxmox-notify. However, after
> > discussing this quite extensively off-list with Lukas, we came to the
> > conclusion that doing so would require a large refactor that is
> > especially tricky given the fact that PVE would still need a different
> > entry point into proxmox-notify than PBS and PDM, since it defines its
> > router on the Perl side.
> 
> nit: usually mentions of offline discussions would be more appropriate
> in the notes of a patch or a cover letter, than the commit message.
Noted, thanks!
> generally, it may make sense to split this up a little and add a cover
> letter imo.
> 
Yes, since this is currently 1:1 copy-pasted from PBS I figured it might
make sense to have it all in the same commit, but if we iterate on this,
I will of course split it up.

> > For this reason, factoring the routing logic out into proxmox-notify is
> > offset to a future where PVE also has a Rust-based router, and for now
> > it is just copied over from PBS (commit: fbf8d1b2) with the adaptations
> > described below.
> >
> > The `proxmox_notify::context::Context` trait implementation is added to
> > PDM directly instead of creating a pdm.rs module in proxmox-notify to
> > avoid adding even more product-specific logic to it. It uses
> > `proxmox-access-control`, which is pulled in by PDM anyway, for getting
> > the user config instead of reading the file directly.
> >
> > The `matcher-fields` and `matcher-field-values` endpoints currently
> > return empty vecs, they will be updated in a future commit introducing
> > actual notifications. The same applies for the notification templates.
> 
> any reason to not go ahead and add at least one notification in this
> series too? being able to test that notifications function, but not
> having anything to be notified about seems a little odd to me. one easy
> notification you could add is the `send_certificate_renewal_mail` in
> `api::nodes::certificates::spawn_certificate_worker()`. that should
> essentially be identical to pbs anyway.
> 
Yes that would have been better, I just wanted to get this out of the
way while I worked on adding notifications. Will add that in v2. 

[...]
> > +use anyhow::Error;
> > +
> > +use proxmox_notify::Config;
> > +
> > +use pdm_buildcfg::configdir;
> > +use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
> > +use proxmox_sys::fs::file_read_optional_string;
> > +
> > +/// Configuration file location for notification targets/matchers.
> > +pub const NOTIFICATION_CONFIG_PATH: &str = configdir!("/notifications.cfg");
> > +
> > +/// Private configuration file location for secrets - only readable by `root`.
> > +pub const NOTIFICATION_PRIV_CONFIG_PATH: &str = configdir!("/notifications-priv.cfg");
> > +
> > +/// Lockfile to prevent concurrent write access.
> > +pub const NOTIFICATION_LOCK_FILE: &str = configdir!("/.notifications.lck");
> > +
> 
> you might want to move these to their own "notifications" sub-folder or
> similar (maybe just "notify"?). in pdm we tend to put config files in
> topically fitting sub-folders (e.g. access, acme, auth)
> 
ACK, notify sounds good to me

> > +/// Get exclusive lock for `notifications.cfg`.
> > +pub fn lock_config() -> Result<ApiLockGuard, Error> {
> > +    open_api_lockfile(NOTIFICATION_LOCK_FILE, None, true)
> > +}
> > +
> > +/// Load notification config.
> > +pub fn config() -> Result<Config, Error> {
> > +    let content = file_read_optional_string(NOTIFICATION_CONFIG_PATH)?.unwrap_or_default();
> > +
> > +    let priv_content =
> > +        file_read_optional_string(NOTIFICATION_PRIV_CONFIG_PATH)?.unwrap_or_default();
> > +
> > +    Ok(Config::new(&content, &priv_content)?)
> 
> you might want to consider returning a `ConfigDigest` here too.
> returning that encourages validating the digest as its less easily
> forgotten.
> 
As written above, I made no improvements while porting this over from
PBS. I agree this would be better though. If we are okay diverging from
PBS I will be happy to add this to v2. (Note that I did not answer all
ConfigDigest-related comments on the handlers directly, please consider 
those all acknowledged, if we go with this I will of course address all of 
them).

> > +}
> > +
> > +/// Save notification config.
> > +pub fn save_config(config: Config) -> Result<(), Error> {
> > +    let (cfg, priv_cfg) = config.write()?;
> > +    replace_config(NOTIFICATION_CONFIG_PATH, cfg.as_bytes())?;
> > +    replace_config(NOTIFICATION_PRIV_CONFIG_PATH, priv_cfg.as_bytes())?;
> 
> the privileged config should probably be saved with higher permission
> than the general config. consider using `replace_secret_config` here.
> 
Yes that's what I initially had, but the remotes.cfg (which also stores
sensitive data like the remote access tokens) is also stored with 0640
and www-data:www-data, so I figured it would make sense to do the same
here, what do you think?

[...]
> > +/// Get a gotify endpoint.
> > +pub fn get_endpoint(name: String, rpcenv: &mut dyn RpcEnvironment) -> Result<GotifyConfig, Error> {
> > +    let config = pdm_config::notifications::config()?;
> > +    let endpoint = proxmox_notify::api::gotify::get_endpoint(&config, &name)?;
> > +
> > +    rpcenv["digest"] = hex::encode(config.digest()).into();
> 
> 
> if you return the digest as described above this could become:
> 
>     let (config, digest) = pdm_config::notifications::config()?;
>     let endpoint = proxmox_notify::api::gotify::get_endpoint(&config, &name)?;
> 
>     rpcenv["digest"] = digest.to_hex().into();
> 
ACK, see general answer on ConfigDigest above

[...]
> > +/// Add a new gotify endpoint.
> > +pub fn add_endpoint(
> > +    endpoint: GotifyConfig,
> > +    token: String,
> > +    _rpcenv: &mut dyn RpcEnvironment,
> > +) -> Result<(), Error> {
> > +    let _lock = pdm_config::notifications::lock_config()?;
> > +    let mut config = pdm_config::notifications::config()?;
> 
> this should probably verify the digest. if you return a config
> digest as described above this could become:
> 
>     let mut (config, expected_digest) = domains::config()?;
>     expected_digest.detect_modification(digest.as_ref())?;
> 
> assuming that you take the digest as an input parameter to this api call
> with:
> 
>     digest: {
>         optional: true,
>         type: ConfigDigest,
>     }
> 
> and:
> 
>     digest: Option<ConfigDigest>
> 
ACK, see general answer on ConfigDigest above

[...]
> > +#[api(
> > +    input: {
> > +        properties: {}
> > +    },
> > +    returns: {
> > +        description: "List of known metadata fields.",
> > +        type: Array,
> > +        items: {type: MatchableField},
> > +    },
> > +    access: {permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_AUDIT, false)},
> > +)]
> > +/// Get all known metadata fields.
> > +pub fn get_fields() -> Result<Vec<MatchableField>, Error> {
> > +    Ok(vec![])
> 
> if you decide to add the acme notification here in a next version, you
> can add hostname here too imo.
>
ACK, will do
[...]
> > +        };
> > +
> > +        let path = Path::new(base)
> > +            .join(namespace.unwrap_or("default"))
> > +            .join(filename);
> > +
> > +        error!("{path:?}");
> 
> is this intentional? i don't think unconditionally emitting an error
> event here makes sense.
> 
No, I used that for debugging and forgot to remove it, sorry about
that...
> > +
> > +        proxmox_sys::fs::file_read_optional_string(path)
> > +            .map_err(|err| Error::Generic(format!("could not load template: {err}")))
> > +    }
> > +}
> > +
> > +/// Initialize `proxmox-notify` by registering the PDM context.
> > +pub fn init() {
> > +    proxmox_notify::context::set_context(&PDM_CONTEXT);
> > +}
> > diff --git a/templates/Makefile b/templates/Makefile
> > new file mode 100644
> > index 0000000..294fa7b
> > --- /dev/null
> > +++ b/templates/Makefile
> > @@ -0,0 +1,14 @@
> > +include ../defines.mk
> > +
> > +NOTIFICATION_TEMPLATES=						\
> > +	default/test-body.txt.hbs				\
> > +	default/test-subject.txt.hbs			\
> > +
> > +all:
> > +
> > +clean:
> > +
> > +install:
> > +	install -dm755 $(DESTDIR)$(DATAROOTDIR)/proxmox-datacenter-manager/templates/default
> > +	$(foreach i,$(NOTIFICATION_TEMPLATES), \
> > +	    install -m644 $(i) $(DESTDIR)$(DATAROOTDIR)/proxmox-datacenter-manager/templates/$(i) ;)
> > diff --git a/templates/default/test-body.txt.hbs b/templates/default/test-body.txt.hbs
> > new file mode 100644
> > index 0000000..2445443
> > --- /dev/null
> > +++ b/templates/default/test-body.txt.hbs
> > @@ -0,0 +1 @@
> > +This is a test of the notification target '{{target}}'.
> > diff --git a/templates/default/test-subject.txt.hbs b/templates/default/test-subject.txt.hbs
> > new file mode 100644
> > index 0000000..cb8e132
> > --- /dev/null
> > +++ b/templates/default/test-subject.txt.hbs
> > @@ -0,0 +1 @@
> > +Test notification
>