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 5FAF81FF13F for ; Thu, 09 Apr 2026 12:18:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BAD6E1E6D2; Thu, 9 Apr 2026 12:19:27 +0200 (CEST) Date: Thu, 9 Apr 2026 12:18:51 +0200 From: Arthur Bied-Charreton To: Shannon Sterz Subject: Re: [PATCH datacenter-manager v2] Add notifications backend Message-ID: References: <20260409045819.19858-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775729864138 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.802 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com,pdm.rs,defines.mk] Message-ID-Hash: JJAAUZDP7UWIIIUJEJXHY4ZQTDT7SG7G X-Message-ID-Hash: JJAAUZDP7UWIIIUJEJXHY4ZQTDT7SG7G X-MailFrom: a.bied-charreton@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 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 { > > + open_api_lockfile(NOTIFICATION_LOCK_FILE, None, true) > > +} > > + > > +/// Load notification config. > > +pub fn config() -> Result { > > + 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 { > > + 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 > 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, 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 >