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

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

On Thu Apr 9, 2026 at 12:18 PM CEST, Arthur Bied-Charreton wrote:
> 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.

i think updating the code to how we'd write it these days is still
reasonable, though. we'll have to eventually do that anyway when
factoring it out. as long as nothing changes on a functional level, this
shouldn't be to big of an issue.

-->8 snip 8<--

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

that's fair they were pretty repetitive anyway. imo this is something we
should encourage earlier rather than later. updating pbs to support this
shouldn't be too much work once we do factor this out either imo.

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

unless necessary i'd default to stricter permissions from the outset.
that would follow the principle of least privilege.

the remotes.cfg is currently set to more lenient permissions because we
lack a mechanism to contact remotes in a more privileged context. this
is not the case for notifications, so keeping the stricter permissions
is preferred.

hope that makes sense :)

> [...]
>> > +/// 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();

-->8 snip 8<--