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

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

On Thu Apr 9, 2026 at 11:20 AM CEST, Shannon Sterz wrote:
> 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
>

+1

>> 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.
> generally, it may make sense to split this up a little and add a cover
> letter imo.
>

There is truly no single 'correct' way to split up such a change, but
this would probably be how I'd split it up:

 - pdm-config part as a separate commit
 - commit for defining/setting up notification context
 - commit for the 'test' notification template (since that one is needed
   by the 'test' route)
 - a commit for moving in the routes from PBS, as close to the original
   as possible (just to make it compile, e.g. replacing references to
   pbs_config with pdm_config, etc.)
 - if there are any improvements that are added on top of the PBS code,
   add them as commits on top (so that it is easier to backport them to
   PBS, if appropriate)

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

This was coordinated with me, I told him that it would be okay to send
this part early. He can either build on top of this series and include
more patches (e.g. adding the GUI, notification events, docs, etc.) in
future versions, or if it makes sense we can also apply certain parts
early.


>> The endpoints are made unprotected and the configuration files
>> read/writable by `www-data`, like for `remotes.cfg`.
>>
>> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
>> ---
>>  Cargo.toml                                    |   1 +
>>  Makefile                                      |   3 +-
>>  debian/proxmox-datacenter-manager.install     |   2 +
>>  defines.mk                                    |   1 +
>>  lib/pdm-config/Cargo.toml                     |   1 +
>>  lib/pdm-config/src/lib.rs                     |   1 +
>>  lib/pdm-config/src/notifications.rs           |  39 ++++
>>  server/Cargo.toml                             |   1 +
>>  server/src/api/config/mod.rs                  |   2 +
>>  server/src/api/config/notifications/gotify.rs | 185 +++++++++++++++++
>>  .../src/api/config/notifications/matchers.rs  | 165 ++++++++++++++++
>>  server/src/api/config/notifications/mod.rs    | 102 ++++++++++
>>  .../src/api/config/notifications/sendmail.rs  | 173 ++++++++++++++++
>>  server/src/api/config/notifications/smtp.rs   | 186 ++++++++++++++++++
>>  .../src/api/config/notifications/targets.rs   |  61 ++++++
>>  .../src/api/config/notifications/webhook.rs   | 170 ++++++++++++++++
>>  server/src/context.rs                         |   2 +
>>  server/src/lib.rs                             |   1 +
>>  server/src/notifications/mod.rs               | 115 +++++++++++
>>  templates/Makefile                            |  14 ++
>>  templates/default/test-body.txt.hbs           |   1 +
>>  templates/default/test-subject.txt.hbs        |   1 +
>>  22 files changed, 1226 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/pdm-config/src/notifications.rs
>>  create mode 100644 server/src/api/config/notifications/gotify.rs
>>  create mode 100644 server/src/api/config/notifications/matchers.rs
>>  create mode 100644 server/src/api/config/notifications/mod.rs
>>  create mode 100644 server/src/api/config/notifications/sendmail.rs
>>  create mode 100644 server/src/api/config/notifications/smtp.rs
>>  create mode 100644 server/src/api/config/notifications/targets.rs
>>  create mode 100644 server/src/api/config/notifications/webhook.rs
>>  create mode 100644 server/src/notifications/mod.rs
>>  create mode 100644 templates/Makefile
>>  create mode 100644 templates/default/test-body.txt.hbs
>>  create mode 100644 templates/default/test-subject.txt.hbs
>>

[...]

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

Would be okay for me, but I'd prefer `notifications` for consistency
with what we do on other products.

>> +/// 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.

digest-checking is handled in the API handler implementations in
proxmox_notify::api where it seemed useful (see my later comments), it
cannot really be forgotten since it is a required parameter for these
functions.

I guess we could do a 

	let config = Config::new(&content, &priv_content)?;

	Ok((config, config.digest()))

here, but I'm not sure it gains us much?

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

Unlike PBS, the notification system will run in the unprivileged process
in PDM, so that webhook/smtp stuff will not run as root. This means that
we also need to store the secrets with less strict permissions. We
already do the same for remotes.cfg and remotes.shadow, so this should
be fine here as well, I think.
Of course, we could also use the same approach as in PBS (notification
code runs as root, secrets are 0600 root:root), but I prefer the
approach used here.

>> +    Ok(())
>> +}
>> diff --git a/server/Cargo.toml b/server/Cargo.toml
>> index 6969549..a4f7bbd 100644
>> --- a/server/Cargo.toml
>> +++ b/server/Cargo.toml
>> @@ -46,6 +46,7 @@ proxmox-lang.workspace = true
>>  proxmox-ldap.workspace = true
>>  proxmox-log.workspace = true
>>  proxmox-login.workspace = true
>> +proxmox-notify.workspace = true
>>  proxmox-openid.workspace = true
>>  proxmox-rest-server = { workspace = true, features = [ "templates" ] }
>>  proxmox-router = { workspace = true, features = [ "cli", "server"] }
>> diff --git a/server/src/api/config/mod.rs b/server/src/api/config/mod.rs
>> index 8f646c1..a465219 100644
>> --- a/server/src/api/config/mod.rs
>> +++ b/server/src/api/config/mod.rs
>> @@ -6,6 +6,7 @@ pub mod access;
>>  pub mod acme;
>>  pub mod certificate;
>>  pub mod notes;
>> +pub mod notifications;
>>  pub mod views;
>>
>>  #[sortable]
>> @@ -14,6 +15,7 @@ const SUBDIRS: SubdirMap = &sorted!([
>>      ("acme", &acme::ROUTER),
>>      ("certificate", &certificate::ROUTER),
>>      ("notes", &notes::ROUTER),
>> +    ("notifications", &notifications::ROUTER),
>>      ("views", &views::ROUTER)
>>  ]);
>>
>> diff --git a/server/src/api/config/notifications/gotify.rs b/server/src/api/config/notifications/gotify.rs
>> new file mode 100644
>> index 0000000..a281245
>> --- /dev/null
>> +++ b/server/src/api/config/notifications/gotify.rs
>> @@ -0,0 +1,185 @@
>> +use anyhow::Error;
>> +use serde_json::Value;
>> +
>> +use proxmox_notify::endpoints::gotify::{
>> +    DeleteableGotifyProperty, GotifyConfig, GotifyConfigUpdater, GotifyPrivateConfig,
>> +    GotifyPrivateConfigUpdater,
>> +};
>> +use proxmox_notify::schema::ENTITY_NAME_SCHEMA;
>> +use proxmox_router::{Permission, Router, RpcEnvironment};
>> +use proxmox_schema::api;
>> +
>> +use pbs_api_types::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA};
>> +
>> +#[api(
>> +    input: {
>> +        properties: {},
>> +    },
>> +    returns: {
>> +        description: "List of gotify endpoints.",
>> +        type: Array,
>> +        items: { type: GotifyConfig },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_AUDIT, false),
>> +    },
>> +)]
>> +/// List all gotify endpoints.
>> +pub fn list_endpoints(
>> +    _param: Value,
>> +    _rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<Vec<GotifyConfig>, Error> {
>> +    let config = pdm_config::notifications::config()?;
>> +
>> +    let endpoints = proxmox_notify::api::gotify::get_endpoints(&config)?;
>> +
>> +    Ok(endpoints)
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: ENTITY_NAME_SCHEMA,
>> +            }
>> +        },
>> +    },
>> +    returns: { type: GotifyConfig },
>> +    access: {
>> +        permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_AUDIT, false),
>> +    },
>> +)]
>> +/// 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();
>

>> +
>> +    Ok(endpoint)
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            endpoint: {
>> +                type: GotifyConfig,
>> +                flatten: true,
>> +            },
>> +            token: {
>> +                description: "Authentication token",
>> +            }
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_MODIFY, false),
>> +    },
>> +)]
>> +/// 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>
>

We lock the config anyway, so it does not really matter (I think?) if
somebody else modified some other entity? And if somebody had added an
entity with the same id, then we fail anyways... So I'm not sure if
using the digest here gains us anything?

I quickly checked a couple add_* handlers in PBS, we don't
really check the digest when adding new entities there as well.

Generally, with regards to using the ConfigDigest type: +1
I think this type was introduced well after the notification API was
added to PBS, which might be the reason I did not use them there.

>> +    let private_endpoint_config = GotifyPrivateConfig {
>> +        name: endpoint.name.clone(),
>> +        token,
>> +    };
>> +
>> +    proxmox_notify::api::gotify::add_endpoint(&mut config, endpoint, private_endpoint_config)?;
>> +
>> +    pdm_config::notifications::save_config(config)?;
>> +    Ok(())
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: ENTITY_NAME_SCHEMA,
>> +            },
>> +            updater: {
>> +                type: GotifyConfigUpdater,
>> +                flatten: true,
>> +            },
>> +            token: {
>> +                description: "Authentication token",
>> +                optional: true,
>> +            },
>> +            delete: {
>> +                description: "List of properties to delete.",
>> +                type: Array,
>> +                optional: true,
>> +                items: {
>> +                    type: DeleteableGotifyProperty,
>> +                }
>> +            },
>> +            digest: {
>> +                optional: true,
>> +                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
>> +            },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_MODIFY, false),
>> +    },
>> +)]
>> +/// Update gotify endpoint.
>> +pub fn update_endpoint(
>> +    name: String,
>> +    updater: GotifyConfigUpdater,
>> +    token: Option<String>,
>> +    delete: Option<Vec<DeleteableGotifyProperty>>,
>> +    digest: Option<String>,
>> +    _rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<(), Error> {
>> +    let _lock = pdm_config::notifications::lock_config()?;
>> +    let mut config = pdm_config::notifications::config()?;
>> +    let digest = digest.map(hex::decode).transpose()?;
>
> using the `ConfigDigest` trait instead of `String` above you could drop
> this line if im not mistaken.
>
>> +
>> +    proxmox_notify::api::gotify::update_endpoint(
>> +        &mut config,
>> +        &name,
>> +        updater,
>> +        GotifyPrivateConfigUpdater { token },
>> +        delete.as_deref(),
>> +        digest.as_deref(),
>> +    )?;
>> +
>> +    pdm_config::notifications::save_config(config)?;
>> +    Ok(())
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: ENTITY_NAME_SCHEMA,
>> +            }
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["system", "notifications"], PRIV_SYS_MODIFY, false),
>> +    },
>> +)]
>> +/// Delete gotify endpoint.
>> +pub fn delete_endpoint(name: String, _rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
>> +    let _lock = pdm_config::notifications::lock_config()?;
>> +    let mut config = pdm_config::notifications::config()?;
>> +    proxmox_notify::api::gotify::delete_gotify_endpoint(&mut config, &name)?;
>> +
>
> this would probably also benefit from checking the config digest.
>

Same here, I'm not really sure what the benefit would be here?

IMO there are a couple cases to consider for concurrent modifications:
  - somebody modified entity A, we delete B -> should be fine
  - somebody modified entity A, we delete A -> does not matter, we want
    to delete it anyways
  - we deleted A, somebody else modifies A -> update fails anyways due
    to the wrong config digest or the entity missing already
  - both try to delete A -> fails for one of both due to the already
    deleted entity (HTTP 404)

Did I miss something?

[...]


>> +
>> +#[derive(Debug)]
>> +struct PDMContext;
>> +
>> +static PDM_CONTEXT: PDMContext = PDMContext;
>> +
>> +impl Context for PDMContext {
>> +    fn lookup_email_for_user(&self, user: &str) -> Option<String> {
>> +        let (config, _digest) = match proxmox_access_control::user::config() {
>> +            Ok(c) => c,
>> +            Err(err) => {
>> +                error!("failed to read user config: {err}");
>> +                return None;
>> +            }
>> +        };
>> +
>> +        match config.lookup::<User>("user", user) {
>> +            Ok(user) => user.email,
>> +            Err(_) => None,
>> +        }
>> +    }
>> +
>> +    fn default_sendmail_author(&self) -> String {
>> +        format!("Proxmox Datacenter Manager - {}", proxmox_sys::nodename())
>> +    }
>> +
>> +    fn default_sendmail_from(&self) -> String {
>> +        let content = attempt_file_read(PDM_NODE_CFG_FILENAME);
>> +        content
>> +            .and_then(|content| lookup_datacenter_config_key(&content, "email-from"))
>> +            .unwrap_or_else(|| String::from("root"))
>> +    }
>> +
>> +    fn http_proxy_config(&self) -> Option<String> {
>> +        let content = attempt_file_read(PDM_NODE_CFG_FILENAME);
>> +        content.and_then(|content| lookup_datacenter_config_key(&content, "http-proxy"))
>> +    }
>> +
>> +    fn default_config(&self) -> &'static str {
>> +        DEFAULT_CONFIG
>> +    }
>> +
>> +    fn lookup_template(
>> +        &self,
>> +        filename: &str,
>> +        namespace: Option<&str>,
>> +        source: TemplateSource,
>> +    ) -> Result<Option<String>, Error> {
>> +        let base = match source {
>> +            TemplateSource::Vendor => "/usr/share/proxmox-datacenter-manager/templates",

I've always regretted calling it just 'templates' and considered sending
a patch to rectify it for the other products. How about:

   /usr/share/proxmox-datacenter-manager/notifications/templates

Then it's also somewhat consistent with the proposal below.


>> +            TemplateSource::Override => configdir!("/notification-templates"),
>
> if notification related config files are moved to their own sub-folder
> this should probably become `notifications/templates` or similar
>

+1