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 8D96E1FF136 for ; Mon, 23 Mar 2026 13:27:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DAF471615E; Mon, 23 Mar 2026 13:26:53 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Mon, 23 Mar 2026 13:26:17 +0100 Message-Id: Subject: Re: [PATCH proxmox 7/7] notify (smtp): Add logging and state-related error types From: "Lukas Wagner" To: "Arthur Bied-Charreton" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260213160415.609868-1-a.bied-charreton@proxmox.com> <20260213160415.609868-8-a.bied-charreton@proxmox.com> In-Reply-To: <20260213160415.609868-8-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774268732051 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 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: S6IZ5YBGOLJQS5YDENAUXVTHGZ56LYVS X-Message-ID-Hash: S6IZ5YBGOLJQS5YDENAUXVTHGZ56LYVS 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: On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote: > Log load/store events in SMTP state management, and add error types to ma= ke errors clearer (previously used Config(De)?Serialization errors, which l= ed to confusing error messages for state-related errors) Commit message should be properly wrapped :) Also, I think this commit should be split, the error-part and the logging part rather seem to be separate things. Both could also be probably folded into previous commits (e.g. add the persistence errors when you introduce the state handling), but no hard feelings about that, I'm also fine with having this in a separate commit. > > Also add logs for OAuth2 token exchange events. > > Signed-off-by: Arthur Bied-Charreton > --- > proxmox-notify/src/endpoints/smtp.rs | 12 +++++++++++ > proxmox-notify/src/endpoints/smtp/state.rs | 25 +++++++++++++++------- > proxmox-notify/src/lib.rs | 12 +++++++++++ > 3 files changed, 41 insertions(+), 8 deletions(-) > > diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/en= dpoints/smtp.rs > index 4364bd11..0ae1ac9f 100644 > --- a/proxmox-notify/src/endpoints/smtp.rs > +++ b/proxmox-notify/src/endpoints/smtp.rs > @@ -12,6 +12,7 @@ use oauth2::{ClientId, ClientSecret, RefreshToken}; > =20 > use proxmox_schema::api_types::COMMENT_SCHEMA; > use proxmox_schema::{api, Updater}; > +use tracing::info; nit: usually we try to group imports like: - std - 3rd party crates - our on crates - crate-level imports > =20 > use crate::context::context; > use crate::endpoints::common::mail; > @@ -301,6 +302,12 @@ impl SmtpEndpoint { > self.name().into(), > Box::new(Error::Generic("no refresh token fo= und".into())), > ))?; > + > + info!( > + "requesting OAuth2 access token for endpoint '{}'", > + self.config.name > + ); > + Seeing this in practise, I think I'd rather use debug! here... Could be considered a bit noisy otherwise, as this line would appear e.g. in task logs for backup jobs. > let token_exchange_result =3D self.get_access_token(&ref= resh_token, method)?; > =20 > state > @@ -313,6 +320,11 @@ impl SmtpEndpoint { > .set_last_refreshed(proxmox_time::epoch_i64()) > .store(self.name())?; > =20 > + info!( > + "OAuth2 token exchange successful for endpoint '{}'"= , > + self.config.name > + ); > + Same here > transport_builder > .credentials(Credentials::new( > self.config.from_address.to_owned(), > diff --git a/proxmox-notify/src/endpoints/smtp/state.rs b/proxmox-notify/= src/endpoints/smtp/state.rs > index 60bef590..45d0db93 100644 > --- a/proxmox-notify/src/endpoints/smtp/state.rs > +++ b/proxmox-notify/src/endpoints/smtp/state.rs > @@ -1,4 +1,5 @@ > use serde::{Deserialize, Serialize}; > +use tracing::debug; > =20 > use crate::{context::context, Error}; > =20 > @@ -25,13 +26,19 @@ impl State { > /// # Errors > /// An [`Error`] is returned if deserialization of the state object = fails. > pub(crate) fn load(name: &str) -> Result { > - match proxmox_sys::fs::file_get_optional_contents(context().stat= e_file_path(name)) > - .map_err(|e| Error::ConfigDeserialization(e.into()))? > + let path =3D context().state_file_path(name); > + > + match proxmox_sys::fs::file_get_optional_contents(&path) > + .map_err(|e| Error::StateRetrieval(path.to_owned(), e.into()= ))? > { > Some(bytes) =3D> { > - serde_json::from_slice(&bytes).map_err(|e| Error::Config= Deserialization(e.into())) > + debug!("loaded state file for endpoint '{name}' from {pa= th}"); > + serde_json::from_slice(&bytes).map_err(|e| Error::StateR= etrieval(path, e.into())) > + } > + None =3D> { > + debug!("no existing state file found for endpoint '{name= }' at {path}, creating empty state"); > + Ok(State::default()) > } > - None =3D> Ok(State::default()), > } > } > =20 > @@ -43,14 +50,16 @@ impl State { > let path =3D context().state_file_path(name); > let parent =3D std::path::Path::new(&path).parent().unwrap(); > =20 > + debug!("attempting to persist state for endpoint '{name}' at {pa= th}"); > + > proxmox_sys::fs::ensure_dir_exists(parent, &context().secret_cre= ate_options(), false) > - .map_err(|e| Error::ConfigSerialization(e.into()))?; > + .map_err(|e| Error::StatePersistence(path.to_owned(), e.into= ()))?; > =20 > let s =3D serde_json::to_string_pretty(&self) > - .map_err(|e| Error::ConfigSerialization(e.into()))?; > + .map_err(|e| Error::StatePersistence(path.to_owned(), e.into= ()))?; > =20 > - proxmox_sys::fs::replace_file(path, s.as_bytes(), context().secr= et_create_options(), true) > - .map_err(|e| Error::ConfigSerialization(e.into())) > + proxmox_sys::fs::replace_file(&path, s.as_bytes(), context().sec= ret_create_options(), true) > + .map_err(|e| Error::StatePersistence(path, e.into())) > } > =20 > /// Set `last_refreshed`. > diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs > index 996393c2..37237fc0 100644 > --- a/proxmox-notify/src/lib.rs > +++ b/proxmox-notify/src/lib.rs > @@ -41,6 +41,10 @@ pub enum Error { > FilterFailed(String), > /// The notification's template string could not be rendered > RenderError(Box), > + /// The state for an endpoint could not be persisted > + StatePersistence(String, Box), > + /// The state for an endpoint could not be retrieved > + StateRetrieval(String, Box), > /// Generic error for anything else > Generic(String), > } > @@ -70,6 +74,12 @@ impl Display for Error { > Error::FilterFailed(message) =3D> { > write!(f, "could not apply filter: {message}") > } > + Error::StatePersistence(path, err) =3D> { > + write!(f, "could not persist state at {path}: {err}") > + } > + Error::StateRetrieval(path, err) =3D> { > + write!(f, "could not retrieve state from {path}: {err}") > + } > Error::RenderError(err) =3D> write!(f, "could not render not= ification template: {err}"), > Error::Generic(message) =3D> f.write_str(message), > } > @@ -86,6 +96,8 @@ impl StdError for Error { > Error::TargetTestFailed(errs) =3D> Some(&*errs[0]), > Error::FilterFailed(_) =3D> None, > Error::RenderError(err) =3D> Some(&**err), > + Error::StatePersistence(_, err) =3D> Some(&**err), > + Error::StateRetrieval(_, err) =3D> Some(&**err), > Error::Generic(_) =3D> None, > } > }