From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6576CE254 for ; Tue, 18 Jul 2023 09:20:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 40F80167E5 for ; Tue, 18 Jul 2023 09:19:58 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 18 Jul 2023 09:19:56 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8100842F8B for ; Tue, 18 Jul 2023 09:19:56 +0200 (CEST) Message-ID: <4844941f-43e0-121c-0038-4fe577f4da98@proxmox.com> Date: Tue, 18 Jul 2023 09:19:54 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: de-AT To: Maximiliano Sandoval , Proxmox VE development discussion References: <20230717150051.710464-1-l.wagner@proxmox.com> <20230717150051.710464-3-l.wagner@proxmox.com> From: Lukas Wagner In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.104 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [config.rs, p.name, mod.rs, config.name, lib.rs, schema.rs] Subject: Re: [pve-devel] [PATCH v3 proxmox 02/66] notify: preparation for the first endpoint plugin X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jul 2023 07:20:28 -0000 Thanks for the review! On 7/17/23 17:48, Maximiliano Sandoval wrote: > > Lukas Wagner writes: > >> Signed-off-by: Lukas Wagner >> --- >> Cargo.toml | 1 + >> proxmox-notify/Cargo.toml | 9 + >> proxmox-notify/src/config.rs | 51 +++++ >> proxmox-notify/src/endpoints/mod.rs | 0 >> proxmox-notify/src/lib.rs | 311 ++++++++++++++++++++++++++++ >> proxmox-notify/src/schema.rs | 43 ++++ >> 6 files changed, 415 insertions(+) >> create mode 100644 proxmox-notify/src/config.rs >> create mode 100644 proxmox-notify/src/endpoints/mod.rs >> create mode 100644 proxmox-notify/src/schema.rs >> >> diff --git a/Cargo.toml b/Cargo.toml >> index 317593f0..ef8a050a 100644 >> --- a/Cargo.toml >> +++ b/Cargo.toml >> @@ -93,6 +93,7 @@ proxmox-lang = { version = "1.1", path = "proxmox-lang" } >> proxmox-rest-server = { version = "0.4.0", path = "proxmox-rest-server" } >> proxmox-router = { version = "1.3.1", path = "proxmox-router" } >> proxmox-schema = { version = "1.3.7", path = "proxmox-schema" } >> +proxmox-section-config = { version = "1.0.2", path = "proxmox-section-config" } >> proxmox-serde = { version = "0.1.1", path = "proxmox-serde", features = [ "serde_json" ] } >> proxmox-sortable-macro = { version = "0.1.2", path = "proxmox-sortable-macro" } >> proxmox-sys = { version = "0.5.0", path = "proxmox-sys" } >> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml >> index 2e69d5b0..37d175f0 100644 >> --- a/proxmox-notify/Cargo.toml >> +++ b/proxmox-notify/Cargo.toml >> @@ -8,3 +8,12 @@ repository.workspace = true >> exclude.workspace = true >> >> [dependencies] >> +lazy_static.workspace = true >> +log.workspace = true >> +openssl.workspace = true >> +proxmox-schema = { workspace = true, features = ["api-macro"]} >> +proxmox-section-config = { workspace = true } >> +proxmox-sys.workspace = true >> +regex.workspace = true >> +serde.workspace = true >> +serde_json.workspace = true >> diff --git a/proxmox-notify/src/config.rs b/proxmox-notify/src/config.rs >> new file mode 100644 >> index 00000000..362ca0fc >> --- /dev/null >> +++ b/proxmox-notify/src/config.rs >> @@ -0,0 +1,51 @@ >> +use lazy_static::lazy_static; >> +use proxmox_schema::{ApiType, ObjectSchema}; >> +use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin}; >> + >> +use crate::schema::BACKEND_NAME_SCHEMA; >> +use crate::Error; >> + >> +lazy_static! { > > Ideally this uses once_cell::sync::Lazy. Noted, I'll do that if the need for a v4 arises or as a separate follow-up series. > >> + pub static ref CONFIG: SectionConfig = config_init(); >> + pub static ref PRIVATE_CONFIG: SectionConfig = private_config_init(); >> +} >> + >> +fn config_init() -> SectionConfig { >> + let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA); > > unneeded mut keyword This was 'intentional'... the following sendmail/gotify patches will insert their config init code here and originally I wanted to have it in a way where gotify could be applied without sendmail and vice versa. So this commit sets the base for both commits, if that makes any sense. > >> + >> + config >> +} >> + >> +fn private_config_init() -> SectionConfig { >> + let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA); > > Ditto Same here. > >> + >> + config >> +} >> + >> +pub fn config(raw_config: &str) -> Result<(SectionConfigData, [u8; 32]), Error> { >> + let digest = openssl::sha::sha256(raw_config.as_bytes()); >> + let data = CONFIG >> + .parse("notifications.cfg", raw_config) >> + .map_err(|err| Error::ConfigDeserialization(err.into()))?; >> + Ok((data, digest)) >> +} >> + >> +pub fn private_config(raw_config: &str) -> Result<(SectionConfigData, [u8; 32]), Error> { >> + let digest = openssl::sha::sha256(raw_config.as_bytes()); >> + let data = PRIVATE_CONFIG >> + .parse("priv/notifications.cfg", raw_config) >> + .map_err(|err| Error::ConfigDeserialization(err.into()))?; >> + Ok((data, digest)) >> +} >> + >> +pub fn write(config: &SectionConfigData) -> Result { >> + CONFIG >> + .write("notifications.cfg", config) >> + .map_err(|err| Error::ConfigSerialization(err.into())) >> +} >> + >> +pub fn write_private(config: &SectionConfigData) -> Result { >> + PRIVATE_CONFIG >> + .write("priv/notifications.cfg", config) >> + .map_err(|err| Error::ConfigSerialization(err.into())) >> +} >> diff --git a/proxmox-notify/src/endpoints/mod.rs b/proxmox-notify/src/endpoints/mod.rs >> new file mode 100644 >> index 00000000..e69de29b >> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs >> index e69de29b..7b90ee15 100644 >> --- a/proxmox-notify/src/lib.rs >> +++ b/proxmox-notify/src/lib.rs >> @@ -0,0 +1,311 @@ >> +use std::collections::HashMap; >> +use std::fmt::Display; >> + >> +use proxmox_schema::api; >> +use proxmox_section_config::SectionConfigData; >> +use serde::{Deserialize, Serialize}; >> +use serde_json::json; >> +use serde_json::Value; >> + >> +use std::error::Error as StdError; >> + >> +mod config; >> +pub mod endpoints; >> +pub mod schema; >> + >> +#[derive(Debug)] >> +pub enum Error { >> + ConfigSerialization(Box), >> + ConfigDeserialization(Box), >> + NotifyFailed(String, Box), >> + TargetDoesNotExist(String), >> +} >> + >> +impl Display for Error { >> + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { >> + match self { >> + Error::ConfigSerialization(err) => { >> + write!(f, "could not serialize configuration: {err}") >> + } >> + Error::ConfigDeserialization(err) => { >> + write!(f, "could not deserialize configuration: {err}") >> + } >> + Error::NotifyFailed(endpoint, err) => { >> + write!(f, "could not notify via endpoint(s): {endpoint}: {err}") >> + } >> + Error::TargetDoesNotExist(target) => { >> + write!(f, "notification target '{target}' does not exist") >> + } >> + } >> + } >> +} >> + >> +impl StdError for Error { >> + fn source(&self) -> Option<&(dyn StdError + 'static)> { >> + match self { >> + Error::ConfigSerialization(err) => Some(&**err), > > Does this really need the double deref? Yeah, does not seem to work any way. Though I'm not sure I can fully explain why. Copied the approach from (I think) the TFA crate. > >> + Error::ConfigDeserialization(err) => Some(&**err), >> + Error::NotifyFailed(_, err) => Some(&**err), >> + Error::TargetDoesNotExist(_) => None, >> + } >> + } >> +} >> + >> +#[api()] >> +#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd)] >> +#[serde(rename_all = "kebab-case")] >> +/// Severity of a notification >> +pub enum Severity { >> + /// General information >> + Info, >> + /// A noteworthy event >> + Notice, >> + /// Warning >> + Warning, >> + /// Error >> + Error, >> +} >> + >> +/// Notification endpoint trait, implemented by all endpoint plugins >> +pub trait Endpoint { >> + /// Send a documentation >> + fn send(&self, notification: &Notification) -> Result<(), Error>; >> + >> + /// The name/identifier for this endpoint >> + fn name(&self) -> &str; >> +} >> + >> +#[derive(Debug, Clone)] >> +/// Notification which can be sent >> +pub struct Notification { >> + /// Notification severity >> + pub severity: Severity, >> + /// The title of the notification >> + pub title: String, >> + /// Notification text >> + pub body: String, >> + /// Additional metadata for the notification >> + pub properties: Option, >> +} >> + >> +/// Notification configuration >> +pub struct Config { > > Might be good to derive Debug here. I mostly only add Debug once I need it. Do you always add it? > >> + config: SectionConfigData, >> + private_config: SectionConfigData, >> + digest: [u8; 32], >> + private_digest: [u8; 32], >> +} >> + >> +impl Clone for Config { > > You can just derive Clone on Config, in general if a Copy type does the correct > thing they will be copied when clone() is called. True, forgot about that. Originally I did not derive Clone since I did not want to touch another sub-crate from this repo, as this makes deploying quite a bite more cumbersome. Will be fixed in a follow-up or in a v4. > >> + fn clone(&self) -> Self { >> + Self { >> + config: SectionConfigData { >> + sections: self.config.sections.clone(), >> + order: self.config.order.clone(), >> + }, >> + private_config: SectionConfigData { >> + sections: self.private_config.sections.clone(), >> + order: self.private_config.order.clone(), >> + }, >> + digest: self.digest, >> + private_digest: self.private_digest, >> + } >> + } >> +} >> + >> +impl Config { >> + /// Parse raw config >> + pub fn new(raw_config: &str, raw_private_config: &str) -> Result { >> + let (config, digest) = config::config(raw_config)?; >> + let (private_config, private_digest) = config::private_config(raw_private_config)?; >> + >> + Ok(Self { >> + config, >> + digest, >> + private_config, >> + private_digest, >> + }) >> + } >> + >> + /// Serialize config >> + pub fn write(&self) -> Result<(String, String), Error> { >> + Ok(( >> + config::write(&self.config)?, >> + config::write_private(&self.private_config)?, >> + )) >> + } >> + >> + /// Returns the SHA256 digest of the configuration. >> + /// The digest is only computed once when the configuration deserialized. >> + pub fn digest(&self) -> &[u8; 32] { >> + &self.digest >> + } >> +} >> + >> +/// Notification bus - distributes notifications to all registered endpoints >> +// The reason for the split between `Config` and this struct is to make testing with mocked >> +// endpoints a bit easier. >> +#[derive(Default)] >> +pub struct Bus { >> + endpoints: HashMap>, >> +} >> + >> +#[allow(unused_macros)] >> +macro_rules! parse_endpoints_with_private_config { >> + ($config:ident, $public_config:ty, $private_config:ty, $endpoint_type:ident, $type_name:expr) => { >> + (|| -> Result>, Error> { >> + let mut endpoints: Vec> = Vec::new(); >> + >> + let configs: Vec<$public_config> = $config >> + .config >> + .convert_to_typed_array($type_name) >> + .map_err(|err| Error::ConfigDeserialization(err.into()))?; >> + >> + let private_configs: Vec<$private_config> = $config >> + .private_config >> + .convert_to_typed_array($type_name) >> + .map_err(|err| Error::ConfigDeserialization(err.into()))?; >> + >> + for config in configs { >> + if let Some(private_config) = private_configs.iter().find(|p| p.name == config.name) >> + { >> + endpoints.push(Box::new($endpoint_type { >> + config, >> + private_config: private_config.clone(), >> + })); >> + } else { >> + log::error!( >> + "Could not instantiate endpoint '{name}': private config does not exist", >> + name = config.name >> + ); >> + } >> + } >> + >> + Ok(endpoints) >> + })() >> + }; >> +} >> + >> +#[allow(unused_macros)] >> +macro_rules! parse_endpoints_without_private_config { >> + ($config:ident, $public_config:ty, $endpoint_type:ident, $type_name:expr) => { >> + (|| -> Result>, Error> { >> + let mut endpoints: Vec> = Vec::new(); >> + >> + let configs: Vec<$public_config> = $config >> + .config >> + .convert_to_typed_array($type_name) >> + .map_err(|err| Error::ConfigDeserialization(err.into()))?; >> + >> + for config in configs { >> + endpoints.push(Box::new($endpoint_type { config })); >> + } >> + >> + Ok(endpoints) >> + })() >> + }; >> +} >> + >> +impl Bus { >> + /// Instantiate notification bus from a given configuration. >> + pub fn from_config(config: &Config) -> Result { > > We are not using the config here? underscore config -> `_config`. Same reason as above, fragment from splitting commits. > >> + let mut endpoints = HashMap::new(); > > Remove this mut. Same reason as above, fragment from splitting commits. > >> + >> + Ok(Bus { endpoints }) >> + } >> + >> + #[cfg(test)] >> + pub fn add_endpoint(&mut self, endpoint: Box) { >> + self.endpoints.insert(endpoint.name().to_string(), endpoint); >> + } >> + >> + pub fn send(&self, target: &str, notification: &Notification) -> Result<(), Error> { >> + log::info!( >> + "sending notification with title '{title}'", >> + title = notification.title >> + ); >> + >> + let endpoint = self >> + .endpoints >> + .get(target) >> + .ok_or(Error::TargetDoesNotExist(target.into()))?; > > Clippy: Use ok_or_else here 📎 Noted, thanks! > >> + >> + endpoint.send(notification).unwrap_or_else(|e| { >> + log::error!( >> + "could not notfiy via endpoint `{name}`: {e}", > > typo notfiy. Noted, thanks! > >> + name = endpoint.name() >> + ) >> + }); >> + >> + Ok(()) >> + } >> + >> + pub fn test_target(&self, target: &str) -> Result<(), Error> { >> + let endpoint = self >> + .endpoints >> + .get(target) >> + .ok_or(Error::TargetDoesNotExist(target.into()))?; > > ok_or_else 📎 Ack > >> + >> + endpoint.send(&Notification { >> + severity: Severity::Info, >> + title: "Test notification".into(), >> + body: "This is a test of the notification target '{{ target }}'".into(), >> + properties: Some(json!({ "target": target })), >> + })?; >> + >> + Ok(()) >> + } >> +} >> + >> +#[cfg(test)] >> +mod tests { >> + use std::{cell::RefCell, rc::Rc}; >> + >> + use super::*; >> + >> + #[derive(Default, Clone)] >> + struct MockEndpoint { >> + messages: Rc>>, >> + } >> + >> + impl Endpoint for MockEndpoint { >> + fn send(&self, message: &Notification) -> Result<(), Error> { >> + self.messages.borrow_mut().push(message.clone()); >> + >> + Ok(()) >> + } >> + >> + fn name(&self) -> &str { > > nit, maybe this trait method should return a static str instead. > Nah, this will not work once actual endpoint plugins are implemenented. They store the name as a string inside the struct and thus return a &str >> + "mock-endpoint" >> + } >> + } >> + >> + impl MockEndpoint { >> + fn messages(&self) -> Vec { >> + self.messages.borrow().clone() >> + } >> + } >> + >> + #[test] >> + fn test_add_mock_endpoint() -> Result<(), Error> { >> + let mock = MockEndpoint::default(); >> + >> + let mut bus = Bus::default(); >> + >> + bus.add_endpoint(Box::new(mock.clone())); >> + >> + bus.send( >> + "mock-endpoint", >> + &Notification { >> + title: "Title".into(), >> + body: "Body".into(), >> + severity: Severity::Info, >> + properties: Default::default(), >> + }, >> + )?; >> + let messages = mock.messages(); >> + assert_eq!(messages.len(), 1); >> + >> + Ok(()) >> + } >> +} >> diff --git a/proxmox-notify/src/schema.rs b/proxmox-notify/src/schema.rs >> new file mode 100644 >> index 00000000..68f11959 >> --- /dev/null >> +++ b/proxmox-notify/src/schema.rs >> @@ -0,0 +1,43 @@ >> +use proxmox_schema::{const_regex, ApiStringFormat, Schema, StringSchema}; >> + >> +// Copied from PBS >> +macro_rules! proxmox_safe_id_regex_str { >> + () => { >> + r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)" >> + }; >> +} >> + >> +const_regex! { >> + pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$"; >> + pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", proxmox_safe_id_regex_str!(), r"$"); >> +} >> + >> +const SINGLE_LINE_COMMENT_FORMAT: ApiStringFormat = >> + ApiStringFormat::Pattern(&SINGLE_LINE_COMMENT_REGEX); >> + >> +pub const COMMENT_SCHEMA: Schema = StringSchema::new("Comment.") >> + .format(&SINGLE_LINE_COMMENT_FORMAT) >> + .max_length(128) >> + .schema(); >> + >> +pub const EMAIL_SCHEMA: Schema = StringSchema::new("E-Mail Address.") >> + .format(&SINGLE_LINE_COMMENT_FORMAT) >> + .min_length(2) >> + .max_length(64) >> + .schema(); >> + >> +pub const PROXMOX_SAFE_ID_FORMAT: ApiStringFormat = >> + ApiStringFormat::Pattern(&PROXMOX_SAFE_ID_REGEX); >> + >> +pub const BACKEND_NAME_SCHEMA: Schema = StringSchema::new("Notification backend name.") >> + .format(&PROXMOX_SAFE_ID_FORMAT) >> + .min_length(3) >> + .max_length(32) >> + .schema(); >> + >> +pub const ENTITY_NAME_SCHEMA: Schema = >> + StringSchema::new("Name schema for endpoints, filters and groups") >> + .format(&PROXMOX_SAFE_ID_FORMAT) >> + .min_length(2) >> + .max_length(32) >> + .schema(); > > nit, maybe these could go on the top. These are in a separate file, maybe you missed that? Or what exactly do you mean? -- - Lukas