From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 4BDF31FF13F for ; Thu, 09 Apr 2026 13:13:02 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D0FCA1F747; Thu, 9 Apr 2026 13:13:45 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 09 Apr 2026 13:13:41 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager v2] Add notifications backend To: "Arthur Bied-Charreton" X-Mailer: aerc 0.20.0 References: <20260409045819.19858-1-a.bied-charreton@proxmox.com> In-Reply-To: From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775733153417 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.125 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: VPKMOXNLIRRUUEQYZTRCMVSVNVGIJBPS X-Message-ID-Hash: VPKMOXNLIRRUUEQYZTRCMVSVNVGIJBPS X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pdm-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 o= f > 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) =3D 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 { >> > + let config =3D pdm_config::notifications::config()?; >> > + let endpoint =3D proxmox_notify::api::gotify::get_endpoint(&confi= g, &name)?; >> > + >> > + rpcenv["digest"] =3D hex::encode(config.digest()).into(); -->8 snip 8<--