From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 665501FF13F for ; Thu, 09 Apr 2026 14:07:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6A710353B; Thu, 9 Apr 2026 14:08:04 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 09 Apr 2026 14:07:53 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager v2] Add notifications backend From: "Lukas Wagner" To: "Shannon Sterz" , "Arthur Bied-Charreton" , X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260409045819.19858-1-a.bied-charreton@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775736405042 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.053 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. [webhook.rs,defines.mk,matchers.rs,notifications.rs,smtp.rs,sendmail.rs,proxmox.com,gotify.rs,mod.rs,targets.rs,context.rs,lib.rs,pdm.rs] Message-ID-Hash: M5Y7XIDUPBFYSCCVVTNQWWXZITKMHSAJ X-Message-ID-Hash: M5Y7XIDUPBFYSCCVVTNQWWXZITKMHSAJ X-MailFrom: l.wagner@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 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 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_Com= mit_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 >> --- >> 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, ApiLock= Guard}; >> +use proxmox_sys::fs::file_read_optional_string; >> + >> +/// Configuration file location for notification targets/matchers. >> +pub const NOTIFICATION_CONFIG_PATH: &str =3D configdir!("/notifications= .cfg"); >> + >> +/// Private configuration file location for secrets - only readable by = `root`. >> +pub const NOTIFICATION_PRIV_CONFIG_PATH: &str =3D configdir!("/notifica= tions-priv.cfg"); >> + >> +/// Lockfile to prevent concurrent write access. >> +pub const NOTIFICATION_LOCK_FILE: &str =3D 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 { >> + open_api_lockfile(NOTIFICATION_LOCK_FILE, None, true) >> +} >> + >> +/// Load notification config. >> +pub fn config() -> Result { >> + let content =3D file_read_optional_string(NOTIFICATION_CONFIG_PATH)= ?.unwrap_or_default(); >> + >> + let priv_content =3D >> + file_read_optional_string(NOTIFICATION_PRIV_CONFIG_PATH)?.unwra= p_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=20 let config =3D 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) =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. > 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 =3D true >> proxmox-ldap.workspace =3D true >> proxmox-log.workspace =3D true >> proxmox-login.workspace =3D true >> +proxmox-notify.workspace =3D true >> proxmox-openid.workspace =3D true >> proxmox-rest-server =3D { workspace =3D true, features =3D [ "templates= " ] } >> proxmox-router =3D { workspace =3D true, features =3D [ "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 =3D &sorted!([ >> ("acme", &acme::ROUTER), >> ("certificate", &certificate::ROUTER), >> ("notes", ¬es::ROUTER), >> + ("notifications", ¬ifications::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, Gotify= PrivateConfig, >> + 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_DIG= EST_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, Error> { >> + let config =3D pdm_config::notifications::config()?; >> + >> + let endpoints =3D proxmox_notify::api::gotify::get_endpoints(&confi= g)?; >> + >> + 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) -> R= esult { >> + let config =3D pdm_config::notifications::config()?; >> + let endpoint =3D proxmox_notify::api::gotify::get_endpoint(&config,= &name)?; >> + >> + rpcenv["digest"] =3D hex::encode(config.digest()).into(); > > > if you return the digest as described above this could become: > > let (config, digest) =3D pdm_config::notifications::config()?; > let endpoint =3D proxmox_notify::api::gotify::get_endpoint(&config, &= name)?; > > rpcenv["digest"] =3D 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 =3D pdm_config::notifications::lock_config()?; >> + let mut config =3D 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) =3D 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 > 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 =3D GotifyPrivateConfig { >> + name: endpoint.name.clone(), >> + token, >> + }; >> + >> + proxmox_notify::api::gotify::add_endpoint(&mut config, endpoint, pr= ivate_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, >> + delete: Option>, >> + digest: Option, >> + _rpcenv: &mut dyn RpcEnvironment, >> +) -> Result<(), Error> { >> + let _lock =3D pdm_config::notifications::lock_config()?; >> + let mut config =3D pdm_config::notifications::config()?; >> + let digest =3D 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 =3D pdm_config::notifications::lock_config()?; >> + let mut config =3D pdm_config::notifications::config()?; >> + proxmox_notify::api::gotify::delete_gotify_endpoint(&mut config, &n= ame)?; >> + > > 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 =3D PDMContext; >> + >> +impl Context for PDMContext { >> + fn lookup_email_for_user(&self, user: &str) -> Option { >> + let (config, _digest) =3D match proxmox_access_control::user::c= onfig() { >> + Ok(c) =3D> c, >> + Err(err) =3D> { >> + error!("failed to read user config: {err}"); >> + return None; >> + } >> + }; >> + >> + match config.lookup::("user", user) { >> + Ok(user) =3D> user.email, >> + Err(_) =3D> None, >> + } >> + } >> + >> + fn default_sendmail_author(&self) -> String { >> + format!("Proxmox Datacenter Manager - {}", proxmox_sys::nodenam= e()) >> + } >> + >> + fn default_sendmail_from(&self) -> String { >> + let content =3D 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 { >> + let content =3D attempt_file_read(PDM_NODE_CFG_FILENAME); >> + content.and_then(|content| lookup_datacenter_config_key(&conten= t, "http-proxy")) >> + } >> + >> + fn default_config(&self) -> &'static str { >> + DEFAULT_CONFIG >> + } >> + >> + fn lookup_template( >> + &self, >> + filename: &str, >> + namespace: Option<&str>, >> + source: TemplateSource, >> + ) -> Result, Error> { >> + let base =3D match source { >> + TemplateSource::Vendor =3D> "/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 =3D> configdir!("/notification-tem= plates"), > > if notification related config files are moved to their own sub-folder > this should probably become `notifications/templates` or similar > +1