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 6DB761FF139 for ; Tue, 10 Feb 2026 16:51:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0A8EC7F96; Tue, 10 Feb 2026 16:52:35 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Feb 2026 16:51:59 +0100 Message-Id: To: "Arthur Bied-Charreton" , Subject: Re: [PATCH proxmox 2/5] notify: Add state file handling From: "Lukas Wagner" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-3-a.bied-charreton@proxmox.com> In-Reply-To: <20260204161354.458814-3-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770738634846 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.036 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 Message-ID-Hash: 74G6TQKA2Z3NFA5IXFLVOOENM5GRB6UW X-Message-ID-Hash: 74G6TQKA2Z3NFA5IXFLVOOENM5GRB6UW 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 VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hey Arthur, great work so far. Some comments inline. On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote: > Add State struct abstracting state file deserialization, updates and > persistence, as well as an EndpointState marker trait stateful endpoints > may implement. > > Also add a state_file_path method to the crate's Context trait, which > allows > tests to build their own context instead of depending on statics. The context trait is more about being able to use proxmox-notify in different products than making it testable, the latter is rather a (great) side-effect. > > As far as SMTP endpoints are concerned, file locks are not necessary. > Old Microsoft tokens stay valid for 90 days after refreshes [1], and > Google > tokens' lifetime is just extended at every use [2], so concurrent reads > should not > be an issue here. Since this state is now pretty general purpose and *could* be used by other endpoints for something different than OAuth tokens, it still would make sense to think about potential race conditions and potential solutions for them. > > [1] > https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#= token-lifetime > [2] > https://stackoverflow.com/questions/8953983/do-google-refresh-tokens-expi= re > > Signed-off-by: Arthur Bied-Charreton > --- > proxmox-notify/Cargo.toml | 1 + > proxmox-notify/debian/control | 2 + > proxmox-notify/src/context/mod.rs | 2 + > proxmox-notify/src/context/pbs.rs | 4 ++ > proxmox-notify/src/context/pve.rs | 4 ++ > proxmox-notify/src/context/test.rs | 4 ++ > proxmox-notify/src/lib.rs | 60 ++++++++++++++++++++++++++++++ > 7 files changed, 77 insertions(+) > > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml > index 52493ef7..daa10296 100644 > --- a/proxmox-notify/Cargo.toml > +++ b/proxmox-notify/Cargo.toml > @@ -40,6 +40,7 @@ proxmox-sendmail =3D { workspace =3D true, optional =3D= true } > proxmox-sys =3D { workspace =3D true, optional =3D true } > proxmox-time.workspace =3D true > proxmox-uuid =3D { workspace =3D true, features =3D ["serde"] } > +nix.workspace =3D true > =20 > [features] > default =3D ["sendmail", "gotify", "smtp", "webhook"] > diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/contro= l > index 7770f5ee..76b8a1fa 100644 > --- a/proxmox-notify/debian/control > +++ b/proxmox-notify/debian/control > @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native , > librust-handlebars-5+default-dev , > librust-http-1+default-dev , > librust-lettre-0.11+default-dev (>=3D 0.11.1-~~) , > + librust-nix-0.29+default-dev , > librust-oauth2-5+default-dev , > librust-openssl-0.10+default-dev , > librust-percent-encoding-2+default-dev (>=3D 2.1-~~) , > @@ -52,6 +53,7 @@ Depends: > librust-anyhow-1+default-dev, > librust-const-format-0.2+default-dev, > librust-handlebars-5+default-dev, > + librust-nix-0.29+default-dev, > librust-oauth2-5+default-dev, > librust-openssl-0.10+default-dev, > librust-proxmox-http-error-1+default-dev, > diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/conte= xt/mod.rs > index 8b6e2c43..86130409 100644 > --- a/proxmox-notify/src/context/mod.rs > +++ b/proxmox-notify/src/context/mod.rs > @@ -32,6 +32,8 @@ pub trait Context: Send + Sync + Debug { > namespace: Option<&str>, > source: TemplateSource, > ) -> Result, Error>; > + /// Return the state file, or None if no state file exists for this = context. This does not return an Option, so I guess the doc comment is not correct here? > + fn state_file_path(&self) -> &'static str; > } > =20 > #[cfg(not(test))] > diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/conte= xt/pbs.rs > index 3e5da59c..67010060 100644 > --- a/proxmox-notify/src/context/pbs.rs > +++ b/proxmox-notify/src/context/pbs.rs > @@ -125,6 +125,10 @@ impl Context for PBSContext { > .map_err(|err| Error::Generic(format!("could not load templa= te: {err}")))?; > Ok(template_string) > } > + > + fn state_file_path(&self) -> &'static str { > + "/etc/proxmox-backup/notifications.state.json" > + } Since we don't have a cluster filesystem in PBS, it probably would be good to place this file in an actual state directory, such as /var/lib/proxmox-backup/. > } > =20 > #[cfg(test)] > diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/conte= xt/pve.rs > index a97cce26..0dffbb11 100644 > --- a/proxmox-notify/src/context/pve.rs > +++ b/proxmox-notify/src/context/pve.rs > @@ -74,6 +74,10 @@ impl Context for PVEContext { > .map_err(|err| Error::Generic(format!("could not load templa= te: {err}")))?; > Ok(template_string) > } > + > + fn state_file_path(&self) -> &'static str { > + "/etc/pve/priv/notifications.state.json" > + } > } > =20 > pub static PVE_CONTEXT: PVEContext =3D PVEContext; > diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/cont= ext/test.rs > index 2c236b4c..e0236b9c 100644 > --- a/proxmox-notify/src/context/test.rs > +++ b/proxmox-notify/src/context/test.rs > @@ -40,4 +40,8 @@ impl Context for TestContext { > ) -> Result, Error> { > Ok(Some(String::new())) > } > + > + fn state_file_path(&self) -> &'static str { > + "/tmp/notifications.state.json" > + } > } > diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs > index 1134027c..a40342cc 100644 > --- a/proxmox-notify/src/lib.rs > +++ b/proxmox-notify/src/lib.rs > @@ -6,6 +6,7 @@ use std::fmt::Display; > use std::str::FromStr; > =20 > use context::context; > +use serde::de::DeserializeOwned; > use serde::{Deserialize, Serialize}; > use serde_json::json; > use serde_json::Value; > @@ -272,6 +273,65 @@ impl Notification { > } > } > =20 > +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)] > +pub struct State { > + #[serde(flatten)] > + pub sections: HashMap, > +} > + > +impl FromStr for State { > + type Err =3D Error; > + > + fn from_str(s: &str) -> Result { > + serde_json::from_str(s).map_err(|e| Error::ConfigDeserialization= (e.into())) > + } > +} > + > +/// Marker trait to be implemented by the state structs for stateful end= points. > +pub trait EndpointState: Serialize + DeserializeOwned + Default {} > + > +impl State { > + pub fn from_path>(path: P) -> Result { > + let contents =3D proxmox_sys::fs::file_read_string(path) > + .map_err(|e| Error::ConfigDeserialization(e.into()))?; > + Self::from_str(&contents) Btw, you could use proxmox_sys::file_get_contents and then deserialize using serde_json::from_slice; this should be a bit more efficient since it does not need to check if it is valid utf-8 at all. There is also promxox_sys::file_get_optional_contents -- see the next patch for the context why I mention this. > + } > + > + pub fn persist>(&self, path: P) -> Result<= (), Error> { > + let state_str =3D > + serde_json::to_string_pretty(self).map_err(|e| Error::Config= Serialization(e.into()))?; > + > + let mode =3D nix::sys::stat::Mode::from_bits_truncate(0o600); > + let options =3D proxmox_sys::fs::CreateOptions::new().perm(mode)= ; I think it might be better to provide the CreateOptions also via some method in Context - then the application has full control over the permissions, user and group for the file. Since you also provide the path via a parameter, I guess the caller of this function would retrieve it from the context and then pass it along with the path. I think I'd use something generic like "secret_create_options" (name taken from `proxmox-product-config`), indicating that we want the CreateOptions for *something* secret/sensitive. Also you don't need the `nix` crate then, I think. > + > + proxmox_sys::fs::replace_file(path, state_str.as_bytes(), option= s, true) > + .map_err(|e| Error::ConfigSerialization(e.into())) > + } > + > + pub fn get(&self, name: &str) -> Result,= Error> { > + match self.sections.get(name) { > + Some(v) =3D> Ok(Some( > + S::deserialize(v).map_err(|e| Error::ConfigDeserializati= on(e.into()))?, > + )), > + None =3D> Ok(None), > + } > + } > + > + pub fn get_or_default(&self, name: &str) -> Result= { > + Ok(self.get(name)?.unwrap_or_default()) > + } > + > + pub fn set(&mut self, name: &str, state: &S) -> Re= sult<(), Error> { > + let v =3D serde_json::to_value(state).map_err(|e| Error::ConfigS= erialization(e.into()))?; > + self.sections.insert(name.to_string(), v); > + Ok(()) > + } > + > + pub fn remove(&mut self, name: &str) { > + self.sections.remove(name); > + } > +} Also here, same as for the other patch, consider adding doc-comments - and `pub(crate)` might also be a good fit here. > + > /// Notification configuration > #[derive(Debug, Clone)] > pub struct Config {