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

["Shannon Sterz" <s.sterz@proxmox.com>]

On Thu Apr 9, 2026 at 2:07 PM CEST, Lukas Wagner wrote:
> 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:

-->8 snip 8<--

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

ack, was not aware of that.

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

sounds fine to me, i mostly suggested "notify" because it's shorter.

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

yes that's true for this crate, but from what i can tell returning a
config alongside its digest is a common pattern. but you are righ, in
this case we enforce it through `proxmox_notify`, so should be fine.

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

ack, was not aware that we want to switch to this approach here. but in
hindsight it makes sense why `test_target` wasn't marked as protected.

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

hm yeah i guess you are right, it wouldn't add too much. guess im just a
little paranoid about modifying on top of an unknown state. but should
probably be fine in these cases.

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

yep hence why i think switching to it would make sense now.

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

see above.

-->8 snip 8<--