* [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module
@ 2023-03-27 15:18 Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 01/18] add proxmox-notification crate Lukas Wagner
` (18 more replies)
0 siblings, 19 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
The purpose of this patch series is to overhaul the existing mail
notification infrastructure in Proxmox VE.
The series replaces calls to 'sendmail' with calls to a
new, configurable notification module. The module was designed to
support multiple notification endpoints, 'sendmail' using the system's
sendmail command being the first one. As a proof of the extensibility
of the current approach, the 'gotify' [1] plugin was also implemented
in this series.
The series also includes a first draft of how notification filtering
based on severity and other properties could look like (more about
properties will follow later in this cover letter).
One thing that should be avoided is breaking existing mail
notifications when these changes hit the repositories. This is a bit
tricky, since currently, we send mails to 'root' (replication, HA - is
forwarded by the mail forwarder to root@pam's mail address),
root@pam's email address (APT), as well as to freely chooseable email
addresses (vzdump). In this version of the patch series, a seamless
transition is ensured by adding an 'ephemeral' sendmail notification
endpoint right before sending a notification.
A later patch could mark individual mail recipients in vzdump jobs as
being deprecated in favor of the new notification endpoints. The
drawback of this approach is that we might send a notification twice
in case a user has created another sendmail endpoint with the same
recipient. However, this is still better than not sending a
notification at all. However, I'm open for suggestions for other
approaches for maintaining compatibility.
Alternative approaches that came to my mind are:
- Explicitly break existing mail notifications with a major
release, maybe create a default sendmail endpoint where *all*
notifications are sent to root@pam via a sendmail endpoint.
In this variant, the custom mail recipients for vzdump jobs would
be removed
- On update, create endpoints in the new configuration file for all
possible email addresses, along with appropriate filters so that
the behavior for every component currently sending mails remains
unchanged. I don't really like this approach, as it might lead to
a pretty messy 'notifications.cfg' with lots of endpoints/filters, if
the user has lots of different backup jobs configured.
Side effects of the changes:
- vzdump loses the HTML table which lists VMs/CTs. Since different
endpoints support different markup styles for formatting, it
does not really make sense to support this in the notification API, at
least not without 'cross-rendering' (e.g. markdown --> HTML)
Short summary of the introduced changes per repo:
- proxmox:
Add new proxmox-notification crate, including configuration file
parsing, two endpoints (sendmail, gotify) and per-endpoint
filtering
- proxmox-perl-rs:
Add bindings for proxmox-notification, so that we can use it from
perl. Also configure logging in such a way that log messages from
proxmox-notification integrate nicely in task logs.
- proxmox-cluster:
Register new configuration file, 'notifications.cfg'
- pve-manager:
Add some more glue code, so that the notification functions are
callable in a nice way. This is needed since we need to read the
configuration file and feed the config to the rust bindings as a
string.
Replace calls to 'sendmail' in vzdump,
apt, and replication with calls to the new notification module.
- pve-ha-manager:
Also replace calls to 'sendmail' with the new notification module
Follow-up work (in no particular order):
- Add CRUD API routes for managing notification endpoints and
filters - right now, this can only be done by editing
'notifications.cfg'
- Add a GUI and CLI using this API
- Right now, the notification API is very simple. Sending a
notification can be as simple as
PVE::Notification::info("Title", "Body")
In the future, the API might be changed/extended so that supports
"registering" notifications. This allows us to a.) generate a
list of all possible notification sources in the system b.) allows
users to easily create filters for specific notification events.
In my head, using the notification module could look like this
then:
# Global context
my = PVE::Notification::register({
'id' => 'backup-failed',
'severity' => 'error',
'properties' => ['host', 'vmlist', 'logs'],
'title' => '{{ host }}: Backup failed'
'body' => <<'EOF'
A backup has failed for the following VMs: {{ vmlist }}
{{ logs }}
EOF
});
# Later, to send the notification:
PVE::Notification::send(->instantiate({
'host' => 'earth',
'vmlist' => ... ,
'logs' => ... ,
}));
Title and body effectively become templates that can be
instantiated with arbitrary properties. This has the following
benefits:
- This ensures that notifications could be easily translated in
the future
- Allows us to add functionality that allows users to customize
notification text, as wished in [2].
- Properties can be used in filters (e.g. exact match, regex,
etc.)
- Properties can be listed in the list of notifications
mentioned above, making it easier to create filters.
- proxmox-mail-forward could be integrated as well. This would feed
e.g. zfs-zed events into our notification infrastructure. Special
care must be taken to not create recursive notification loops
(e.g. zed sends to root, forwarder uses notification module, a
configured sendmail endpoint sends to root, forwarder uses module
--> loop)
- Maybe add some CLI so that admins can send notifications in
scripts (an API endpoint callable via pvesh might be enough for a
start)
- Add more notification events
- Add other endpoints, e.g. webhook, a generic SMTP, etc.
- Integrate the new module into the other products
[1] https://gotify.net/
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=4526
proxmox:
Lukas Wagner (6):
add proxmox-notification crate
notification: implement sendmail endpoint
notification: add notification filter mechanism
notification: implement gotify endpoint
notification: allow adding new sendmail endpoints / filters
notification: add debian packaging
Cargo.toml | 2 +
proxmox-notification/Cargo.toml | 22 +
proxmox-notification/debian/changelog | 5 +
proxmox-notification/debian/control | 59 +++
proxmox-notification/debian/copyright | 16 +
proxmox-notification/debian/debcargo.toml | 7 +
proxmox-notification/src/config.rs | 71 +++
proxmox-notification/src/endpoints/gotify.rs | 80 ++++
proxmox-notification/src/endpoints/mod.rs | 2 +
.../src/endpoints/sendmail.rs | 78 ++++
proxmox-notification/src/filter.rs | 427 ++++++++++++++++++
proxmox-notification/src/lib.rs | 307 +++++++++++++
proxmox-notification/src/methods.rs | 55 +++
13 files changed, 1131 insertions(+)
create mode 100644 proxmox-notification/Cargo.toml
create mode 100644 proxmox-notification/debian/changelog
create mode 100644 proxmox-notification/debian/control
create mode 100644 proxmox-notification/debian/copyright
create mode 100644 proxmox-notification/debian/debcargo.toml
create mode 100644 proxmox-notification/src/config.rs
create mode 100644 proxmox-notification/src/endpoints/gotify.rs
create mode 100644 proxmox-notification/src/endpoints/mod.rs
create mode 100644 proxmox-notification/src/endpoints/sendmail.rs
create mode 100644 proxmox-notification/src/filter.rs
create mode 100644 proxmox-notification/src/lib.rs
create mode 100644 proxmox-notification/src/methods.rs
proxmox-perl-rs:
Lukas Wagner (2):
log: set default log level to 'info', add product specfig logging env
var1
add basic bindings for the proxmox_notification crate
Cargo.toml | 2 +
common/pkg/Makefile | 1 +
common/src/logger.rs | 12 +++-
pmg-rs/src/lib.rs | 2 +-
pve-rs/Cargo.toml | 1 +
pve-rs/src/lib.rs | 3 +-
pve-rs/src/notification.rs | 128 +++++++++++++++++++++++++++++++++++++
7 files changed, 145 insertions(+), 4 deletions(-)
create mode 100644 pve-rs/src/notification.rs
pve-cluster:
Lukas Wagner (1):
cluster files: add notifications.cfg
data/PVE/Cluster.pm | 1 +
data/src/status.c | 1 +
2 files changed, 2 insertions(+)
pve-manager:
Lukas Wagner (7):
test: fix names of .PHONY targets
add PVE::Notification module
vzdump: send notifications via new notification module
vzdump: rename 'sendmail' sub to 'send_notification'
test: rename mail_test.pl to vzdump_notification_test.pl
api: apt: send notification via new notification module
api: replication: send notifications via new notification module
PVE/API2/APT.pm | 55 +++++---
PVE/API2/Replication.pm | 13 +-
PVE/API2/VZDump.pm | 2 +-
PVE/Makefile | 1 +
PVE/Notification.pm | 60 +++++++++
PVE/VZDump.pm | 124 +++++-------------
test/Makefile | 16 ++-
...il_test.pl => vzdump_notification_test.pl} | 28 ++--
8 files changed, 169 insertions(+), 130 deletions(-)
create mode 100644 PVE/Notification.pm
rename test/{mail_test.pl => vzdump_notification_test.pl} (73%)
pve-ha-manager:
Lukas Wagner (2):
manager: send notifications via new notification module
manager: rename 'sendmail' --> 'send_notification'
src/PVE/HA/Env.pm | 4 ++--
src/PVE/HA/Env/PVE2.pm | 15 +++++++++++++--
src/PVE/HA/NodeStatus.pm | 2 +-
src/PVE/HA/Sim/Env.pm | 2 +-
4 files changed, 17 insertions(+), 6 deletions(-)
Summary over all repositories:
34 files changed, 1464 insertions(+), 140 deletions(-)
Generated by murpp v0.1.0
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 01/18] add proxmox-notification crate
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 02/18] notification: implement sendmail endpoint Lukas Wagner
` (17 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Cargo.toml | 1 +
proxmox-notification/Cargo.toml | 10 ++++++++++
proxmox-notification/src/lib.rs | 0
3 files changed, 11 insertions(+)
create mode 100644 proxmox-notification/Cargo.toml
create mode 100644 proxmox-notification/src/lib.rs
diff --git a/Cargo.toml b/Cargo.toml
index 9123af5..9ccfa1a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -10,6 +10,7 @@ members = [
"proxmox-lang",
"proxmox-ldap",
"proxmox-metrics",
+ "proxmox-notification",
"proxmox-rest-server",
"proxmox-router",
"proxmox-schema",
diff --git a/proxmox-notification/Cargo.toml b/proxmox-notification/Cargo.toml
new file mode 100644
index 0000000..47c85a3
--- /dev/null
+++ b/proxmox-notification/Cargo.toml
@@ -0,0 +1,10 @@
+[package]
+name = "proxmox-notification"
+version = "0.1.0"
+authors.workspace = true
+edition.workspace = true
+license.workspace = true
+repository.workspace = true
+exclude.workspace = true
+
+[dependencies]
diff --git a/proxmox-notification/src/lib.rs b/proxmox-notification/src/lib.rs
new file mode 100644
index 0000000..e69de29
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 02/18] notification: implement sendmail endpoint
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 01/18] add proxmox-notification crate Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 03/18] notification: add notification filter mechanism Lukas Wagner
` (16 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Add everything needed for a simple endpoint of type 'sendmail', which
uses the 'sendmail' binary on the system to send emails to users.
The current implementation makes it easy to implement other notification
providers with minimal changes to the code. All one has to do is to
implement the 'Endpoint' trait and register the plugin in the configuration
parser.
A notification contains the following data: title, body, severity,
and a map of arbitrary metadata fields. The metadata fields will
later be useful for filtering notifications. I chose the 'dynamic'
metadata map approach over static fields to be more flexible: This
allows us to have different kind of metadata for different products,
without the proxmox-notification crate being aware of this.
'Type safety' for metadata properties can be enforced in the glue code
for any given product.
In terms of configuration: The configuration for notification endpoints
and filters will live in a new configuration file, `notifications.cfg`.
However, since the crate should be product-agnostic, we process the
configuration as a raw string. Loading/storing of the configuration
file will happen in product-specific code.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Cargo.toml | 1 +
proxmox-notification/Cargo.toml | 12 ++
proxmox-notification/src/config.rs | 53 ++++++
proxmox-notification/src/endpoints/mod.rs | 1 +
.../src/endpoints/sendmail.rs | 71 +++++++
proxmox-notification/src/lib.rs | 178 ++++++++++++++++++
6 files changed, 316 insertions(+)
create mode 100644 proxmox-notification/src/config.rs
create mode 100644 proxmox-notification/src/endpoints/mod.rs
create mode 100644 proxmox-notification/src/endpoints/sendmail.rs
diff --git a/Cargo.toml b/Cargo.toml
index 9ccfa1a..d167c73 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -89,6 +89,7 @@ proxmox-lang = { version = "1.1", path = "proxmox-lang" }
proxmox-rest-server = { version = "0.3.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.4.2", path = "proxmox-sys" }
diff --git a/proxmox-notification/Cargo.toml b/proxmox-notification/Cargo.toml
index 47c85a3..0286c8f 100644
--- a/proxmox-notification/Cargo.toml
+++ b/proxmox-notification/Cargo.toml
@@ -8,3 +8,15 @@ repository.workspace = true
exclude.workspace = true
[dependencies]
+anyhow.workspace = true
+lazy_static.workspace = true
+log.workspace = true
+openssl.workspace = true
+proxmox-http = { workspace = true, features = ["client-sync"]}
+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
+handlebars.workspace = true
diff --git a/proxmox-notification/src/config.rs b/proxmox-notification/src/config.rs
new file mode 100644
index 0000000..58c79d4
--- /dev/null
+++ b/proxmox-notification/src/config.rs
@@ -0,0 +1,53 @@
+use anyhow::Error;
+use lazy_static::lazy_static;
+use proxmox_schema::{const_regex, ApiStringFormat, ApiType, ObjectSchema, Schema, StringSchema};
+use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
+
+use crate::endpoints::sendmail::SendmailConfig;
+use crate::endpoints::sendmail::SENDMAIL_TYPENAME;
+
+// Copied from PBS
+#[rustfmt::skip]
+#[macro_export]
+macro_rules! PROXMOX_SAFE_ID_REGEX_STR { () => { r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)" }; }
+
+const_regex! {
+
+ pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", PROXMOX_SAFE_ID_REGEX_STR!(), r"$");
+}
+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();
+
+lazy_static! {
+ pub static ref CONFIG: SectionConfig = init();
+}
+
+fn init() -> SectionConfig {
+ let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA);
+
+ const SENDMAIL_SCHEMA: &ObjectSchema = SendmailConfig::API_SCHEMA.unwrap_object_schema();
+
+ config.register_plugin(SectionConfigPlugin::new(
+ SENDMAIL_TYPENAME.to_string(),
+ Some(String::from("name")),
+ SENDMAIL_SCHEMA,
+ ));
+
+ 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)?;
+ Ok((data, digest))
+}
+
+pub fn write(config: &SectionConfigData) -> Result<String, Error> {
+ CONFIG.write("notifications.cfg", config)
+}
diff --git a/proxmox-notification/src/endpoints/mod.rs b/proxmox-notification/src/endpoints/mod.rs
new file mode 100644
index 0000000..2d7b9ba
--- /dev/null
+++ b/proxmox-notification/src/endpoints/mod.rs
@@ -0,0 +1 @@
+pub(crate) mod sendmail;
diff --git a/proxmox-notification/src/endpoints/sendmail.rs b/proxmox-notification/src/endpoints/sendmail.rs
new file mode 100644
index 0000000..2c43ab1
--- /dev/null
+++ b/proxmox-notification/src/endpoints/sendmail.rs
@@ -0,0 +1,71 @@
+use crate::{Endpoint, Notification};
+use anyhow::Error;
+
+use proxmox_schema::{api, const_regex, ApiStringFormat, Schema, StringSchema, Updater};
+use serde::{Deserialize, Serialize};
+
+// Copied from PBS
+const_regex! {
+ pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";
+}
+const SINGLE_LINE_COMMENT_FORMAT: ApiStringFormat =
+ ApiStringFormat::Pattern(&SINGLE_LINE_COMMENT_REGEX);
+const EMAIL_SCHEMA: Schema = StringSchema::new("E-Mail Address.")
+ .format(&SINGLE_LINE_COMMENT_FORMAT)
+ .min_length(2)
+ .max_length(64)
+ .schema();
+
+pub(crate) const SENDMAIL_TYPENAME: &str = "sendmail";
+
+#[api(
+ properties: {
+ recipient: {
+ type: Array,
+ items: {
+ schema: EMAIL_SCHEMA,
+ },
+ },
+ },
+)]
+#[derive(Debug, Serialize, Deserialize, Updater)]
+#[serde(rename_all = "kebab-case")]
+/// Config for Sendmail notification endpoints
+pub struct SendmailConfig {
+ /// Name of the endpoint
+ pub name: String,
+ /// Mail recipients
+ pub recipient: Vec<String>,
+ /// `From` address for the mail
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub from_address: Option<String>,
+ /// Author of the mail
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub author: Option<String>,
+}
+
+impl Endpoint for SendmailConfig {
+ fn send(&self, notification: &Notification) -> Result<(), Error> {
+ let recipients: Vec<&str> = self.recipient.iter().map(String::as_str).collect();
+
+ // Note: OX has serious problems displaying text mails,
+ // so we include html as well
+ let html = format!(
+ "<html><body><pre>\n{}\n<pre>",
+ handlebars::html_escape(¬ification.body)
+ );
+
+ proxmox_sys::email::sendmail(
+ &recipients,
+ ¬ification.title,
+ Some(¬ification.body),
+ Some(&html),
+ self.from_address.as_deref(),
+ self.author.as_deref(),
+ )
+ }
+
+ fn name(&self) -> &str {
+ &self.name
+ }
+}
diff --git a/proxmox-notification/src/lib.rs b/proxmox-notification/src/lib.rs
index e69de29..f076c88 100644
--- a/proxmox-notification/src/lib.rs
+++ b/proxmox-notification/src/lib.rs
@@ -0,0 +1,178 @@
+use std::collections::HashMap;
+
+use anyhow::Error;
+
+use endpoints::sendmail::SendmailConfig;
+use endpoints::sendmail::SENDMAIL_TYPENAME;
+use proxmox_schema::api;
+use proxmox_section_config::SectionConfigData;
+use serde::{Deserialize, Serialize};
+
+mod config;
+mod endpoints;
+
+#[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 documention
+ fn send(&self, notification: &Notification) -> Result<(), Error>;
+
+ /// The name/identifier for this endpoint
+ fn name(&self) -> &str;
+}
+
+#[derive(Debug, Clone)]
+/// A sendable notifiction
+pub struct Notification {
+ /// The title of the notification
+ pub title: String,
+ /// Notification text
+ pub body: String,
+ /// Notification severity
+ pub severity: Severity,
+ /// Additional metadata for the notification
+ pub properties: HashMap<String, String>,
+}
+
+/// Notification configuration
+pub struct Config(SectionConfigData);
+
+impl Clone for Config {
+ fn clone(&self) -> Self {
+ Self(SectionConfigData {
+ sections: self.0.sections.clone(),
+ order: self.0.order.clone(),
+ })
+ }
+}
+
+impl Config {
+ /// Parse raw config
+ pub fn new(raw_config: &str) -> Result<Self, Error> {
+ // TODO: save and compare digest? Do we need this here?
+ let (config, _digest) = crate::config::config(raw_config)?;
+
+ Ok(Self(config))
+ }
+
+ /// Serialize config
+ pub fn write(&self) -> Result<String, Error> {
+ config::write(&self.0)
+ }
+
+ /// 'Instantiate' all notification endpoints from their configuration
+ pub fn instantiate(&self) -> Result<Bus, Error> {
+ let mut endpoints = Vec::new();
+
+ let sendmail_endpoints: Vec<SendmailConfig> =
+ self.0.convert_to_typed_array(SENDMAIL_TYPENAME)?;
+
+ endpoints.extend(
+ sendmail_endpoints
+ .into_iter()
+ .map(Box::new)
+ .map(|e| e as Box<dyn Endpoint>),
+ );
+
+ Ok(Bus { endpoints })
+ }
+}
+
+/// 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: Vec<Box<dyn Endpoint>>,
+}
+
+impl Bus {
+ pub fn add_endpoint(&mut self, endpoint: Box<dyn Endpoint>) {
+ self.endpoints.push(endpoint);
+ }
+
+ /// Send a notification to all registered endpoints
+ pub fn send(&self, notification: &Notification) -> Result<(), Error> {
+ log::info!(
+ "sending notification with title '{title}'",
+ title = notification.title
+ );
+
+ for endpoint in &self.endpoints {
+ endpoint.send(notification).unwrap_or_else(|e| {
+ log::error!(
+ "could not notfiy via endpoint `{name}`: {e}",
+ name = endpoint.name()
+ )
+ })
+ }
+
+ Ok(())
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use std::{cell::RefCell, rc::Rc};
+
+ use anyhow::Error;
+
+ use super::*;
+
+ #[derive(Default, Clone)]
+ struct MockEndpoint {
+ messages: Rc<RefCell<Vec<Notification>>>,
+ }
+
+ impl Endpoint for MockEndpoint {
+ fn send(&self, message: &Notification) -> Result<(), Error> {
+ self.messages.borrow_mut().push(message.clone());
+
+ Ok(())
+ }
+
+ fn name(&self) -> &str {
+ "mock-endpoint"
+ }
+ }
+
+ impl MockEndpoint {
+ fn messages(&self) -> Vec<Notification> {
+ 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(&Notification {
+ title: "Title".into(),
+ body: "Body".into(),
+ severity: Severity::Info,
+ properties: Default::default(),
+ })?;
+ let messages = mock.messages();
+ assert_eq!(messages.len(), 1);
+
+ Ok(())
+ }
+}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 03/18] notification: add notification filter mechanism
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 01/18] add proxmox-notification crate Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 02/18] notification: implement sendmail endpoint Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 04/18] notification: implement gotify endpoint Lukas Wagner
` (15 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
This commit adds a way to filter notifications based on a.) severity and
b.) arbitrary metadata property fields. For better demonstration, an example
configuration file follows:
sendmail: mail
recipient root@example.org
filter only-certain-vms-and-errors
filter: only-certain-vms-or-errors
mode or
min-severity error
sub-filter only-certain-vms
sub-filter all-but-one-ct
filter: only-certain-vms
mode and
match-property object_type=vm
sub-filter vm-ids
filter: vm-ids
mode or
match-property object_id=103
match-property object_id=104
filter: all-but-one-ct
mode and
invert-match true
match-property object_type=ct
match-property object_id=110
In plain English, this translates to: "Send mails for all errors, as
well as all events related to VM with the IDs 103 and 104, and also
all events for any container except the one with ID 110".
The example demonstrates how sub-filters and and/or/not operators can be
used to construct filters with high granularity.
Filters are lazily evaluated, and at most once, in case multiple
endpoints/filters use the same (sub-)filter. Furthermore, there
are checks in place so that recursive sub-filter definitions are
detected.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notification/src/config.rs | 9 +
.../src/endpoints/sendmail.rs | 7 +
proxmox-notification/src/filter.rs | 426 ++++++++++++++++++
proxmox-notification/src/lib.rs | 131 +++++-
4 files changed, 566 insertions(+), 7 deletions(-)
create mode 100644 proxmox-notification/src/filter.rs
diff --git a/proxmox-notification/src/config.rs b/proxmox-notification/src/config.rs
index 58c79d4..939bdb6 100644
--- a/proxmox-notification/src/config.rs
+++ b/proxmox-notification/src/config.rs
@@ -5,6 +5,7 @@ use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlug
use crate::endpoints::sendmail::SendmailConfig;
use crate::endpoints::sendmail::SENDMAIL_TYPENAME;
+use crate::filter::{FilterConfig, FILTER_TYPENAME};
// Copied from PBS
#[rustfmt::skip]
@@ -33,12 +34,20 @@ fn init() -> SectionConfig {
const SENDMAIL_SCHEMA: &ObjectSchema = SendmailConfig::API_SCHEMA.unwrap_object_schema();
+ const FILTER_SCHEMA: &ObjectSchema = FilterConfig::API_SCHEMA.unwrap_object_schema();
+
config.register_plugin(SectionConfigPlugin::new(
SENDMAIL_TYPENAME.to_string(),
Some(String::from("name")),
SENDMAIL_SCHEMA,
));
+ config.register_plugin(SectionConfigPlugin::new(
+ FILTER_TYPENAME.to_string(),
+ Some(String::from("name")),
+ FILTER_SCHEMA,
+ ));
+
config
}
diff --git a/proxmox-notification/src/endpoints/sendmail.rs b/proxmox-notification/src/endpoints/sendmail.rs
index 2c43ab1..7f29b48 100644
--- a/proxmox-notification/src/endpoints/sendmail.rs
+++ b/proxmox-notification/src/endpoints/sendmail.rs
@@ -42,6 +42,9 @@ pub struct SendmailConfig {
/// Author of the mail
#[serde(skip_serializing_if = "Option::is_none")]
pub author: Option<String>,
+ /// Filter to apply
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub filter: Option<String>,
}
impl Endpoint for SendmailConfig {
@@ -68,4 +71,8 @@ impl Endpoint for SendmailConfig {
fn name(&self) -> &str {
&self.name
}
+
+ fn filter(&self) -> Option<&str> {
+ self.filter.as_deref()
+ }
}
diff --git a/proxmox-notification/src/filter.rs b/proxmox-notification/src/filter.rs
new file mode 100644
index 0000000..9846e93
--- /dev/null
+++ b/proxmox-notification/src/filter.rs
@@ -0,0 +1,426 @@
+use std::collections::{HashMap, HashSet};
+
+use anyhow::{bail, Context, Error};
+use proxmox_schema::{api, property_string::PropertyIterator, Updater};
+use serde::{Deserialize, Serialize};
+
+use crate::{Notification, Severity};
+
+pub const FILTER_TYPENAME: &str = "filter";
+
+#[api]
+#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy)]
+#[serde(rename_all = "kebab-case")]
+pub enum FilterModeOperator {
+ /// All filter properties have to match (AND)
+ #[default]
+ And,
+ /// At least one filter property has to match (OR)
+ Or,
+}
+
+impl FilterModeOperator {
+ /// Apply the mode operator to two bools, lhs and rhs
+ fn apply(&self, lhs: bool, rhs: bool) -> bool {
+ match self {
+ FilterModeOperator::And => lhs && rhs,
+ FilterModeOperator::Or => lhs || rhs,
+ }
+ }
+
+ fn neutral_element(&self) -> bool {
+ match self {
+ FilterModeOperator::And => true,
+ FilterModeOperator::Or => false,
+ }
+ }
+
+ /// Check if we need to evaluate any other properties, or if we can return early, since
+ /// false AND (...) = false
+ /// true OR (...) = true
+ fn short_circuit_return_possible(&self, value: bool) -> bool {
+ matches!(
+ (self, value),
+ (FilterModeOperator::And, false) | (FilterModeOperator::Or, true)
+ )
+ }
+}
+
+#[api(
+ properties: {
+ "sub-filter": {
+ optional: true,
+ type: Array,
+ items: {
+ description: "Name of the subfilter",
+ type: String,
+ },
+ },
+ "match-property": {
+ optional: true,
+ type: Array,
+ items: {
+ description: "Notification properties to match",
+ type: String,
+ },
+ },
+ },
+)]
+#[derive(Debug, Serialize, Deserialize, Updater)]
+#[serde(rename_all = "kebab-case")]
+/// Config for Sendmail notification endpoints
+pub struct FilterConfig {
+ /// Name of the filter
+ pub name: String,
+
+ /// Minimum severity to match
+ pub min_severity: Option<Severity>,
+
+ /// Subfilter, allows arbitrary nesting (no recursion allowed)
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub sub_filter: Option<Vec<String>>,
+
+ /// Choose between 'and' and 'or' for when multiple properties are specified
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub mode: Option<FilterModeOperator>,
+
+ /// Notification properties to match.
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub match_property: Option<Vec<String>>,
+
+ /// Invert match of the whole filter
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub invert_match: Option<bool>,
+}
+
+/// A caching, lazily-evaluating notification filter. Parameterized with the notification itself,
+/// since there are usually multiple filters to check for a single notification that is to be sent.
+pub(crate) struct FilterMatcher<'a> {
+ filters: HashMap<&'a str, &'a FilterConfig>,
+ cached_results: HashMap<&'a str, bool>,
+ notification: &'a Notification,
+}
+
+impl<'a> FilterMatcher<'a> {
+ pub(crate) fn new(filters: &'a [FilterConfig], notification: &'a Notification) -> Self {
+ let filters = filters.iter().map(|f| (f.name.as_str(), f)).collect();
+
+ Self {
+ filters,
+ cached_results: Default::default(),
+ notification,
+ }
+ }
+
+ /// Check if the notification that was used to instatiate Self matches a given filter
+ pub(crate) fn check_filter_match(&mut self, filter_name: &str) -> Result<bool, Error> {
+ let mut visited = HashSet::new();
+
+ self.do_check_filter(filter_name, &mut visited)
+ }
+
+ fn do_check_filter(
+ &mut self,
+ filter_name: &str,
+ visited: &mut HashSet<String>,
+ ) -> Result<bool, Error> {
+ if visited.contains(filter_name) {
+ bail!("recursive filter definition: {filter_name}");
+ }
+
+ if let Some(is_match) = self.cached_results.get(filter_name) {
+ return Ok(*is_match);
+ }
+
+ visited.insert(filter_name.into());
+
+ let filter_config = self
+ .filters
+ .get(filter_name)
+ .copied()
+ .context("no filter with name {filter_name} defined")?;
+
+ let mode_operator = filter_config.mode.unwrap_or_default();
+
+ let mut notification_matches = mode_operator.neutral_element();
+
+ notification_matches = mode_operator.apply(
+ notification_matches,
+ self.check_severity_match(filter_config, mode_operator),
+ );
+
+ notification_matches = mode_operator.apply(
+ notification_matches,
+ self.check_property_match(filter_config, mode_operator)?,
+ );
+
+ // ...then check the sub-filters
+ if let Some(sub_filters) = &filter_config.sub_filter {
+ for filter in sub_filters {
+ let is_match = self.do_check_filter(filter, visited)?;
+
+ self.cached_results.insert(filter.as_str(), is_match);
+
+ notification_matches = mode_operator.apply(notification_matches, is_match);
+
+ if mode_operator.short_circuit_return_possible(notification_matches) {
+ return Ok(notification_matches);
+ }
+ }
+ }
+
+ if filter_config.invert_match.unwrap_or_default() {
+ notification_matches = !notification_matches;
+ }
+
+ Ok(notification_matches)
+ }
+
+ fn check_property_match(
+ &self,
+ filter_config: &FilterConfig,
+ mode_operator: FilterModeOperator,
+ ) -> Result<bool, Error> {
+ let mut notification_matches = mode_operator.neutral_element();
+
+ if let Some(match_property_operators) = filter_config.match_property.as_ref() {
+ for op in match_property_operators {
+ for prop in PropertyIterator::new(op) {
+ let prop = prop?;
+
+ if let (Some(key), expected_value) = prop {
+ let is_match = if let Some(value) = self.notification.properties.get(key) {
+ value.as_str() == expected_value
+ } else {
+ // If the metadata field is not present, we don't match
+ false
+ };
+
+ notification_matches = mode_operator.apply(notification_matches, is_match);
+
+ if mode_operator.short_circuit_return_possible(notification_matches) {
+ return Ok(notification_matches);
+ }
+ }
+ }
+ }
+ }
+
+ Ok(notification_matches)
+ }
+
+ fn check_severity_match(
+ &self,
+ filter_config: &FilterConfig,
+ mode_operator: FilterModeOperator,
+ ) -> bool {
+ if let Some(min_severity) = filter_config.min_severity {
+ self.notification.severity >= min_severity
+ } else {
+ mode_operator.neutral_element()
+ }
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::config;
+
+ #[test]
+ fn test_filter_config_parses_correctly() -> Result<(), Error> {
+ let (c, _) = config::config(
+ r"
+filter: foo
+ min-severity info
+ match-property object_type=vm
+ match-property object_id=103
+ invert-match true
+ mode and
+
+filter: bar
+ min-severity warning
+ match-property object_type=ct,object_id=104
+ sub-filter foo
+ mode or
+",
+ )?;
+
+ let filters: Vec<FilterConfig> = c.convert_to_typed_array("filter")?;
+
+ assert_eq!(filters.len(), 2);
+
+ Ok(())
+ }
+
+ fn parse_filters(config: &str) -> Result<Vec<FilterConfig>, Error> {
+ let (config, _) = config::config(config)?;
+ Ok(config.convert_to_typed_array("filter")?)
+ }
+
+ fn empty_notification_with_severity(severity: Severity) -> Notification {
+ Notification {
+ title: String::new(),
+ body: String::new(),
+ severity,
+ properties: HashMap::new(),
+ }
+ }
+
+ fn empty_notification_with_metadata(metadata: &[(&str, &str)]) -> Notification {
+ let metadata = HashMap::from_iter(
+ metadata
+ .into_iter()
+ .map(|e| (e.0.to_string(), e.1.to_string())),
+ );
+
+ Notification {
+ title: String::new(),
+ body: String::new(),
+ severity: Severity::Error,
+ properties: metadata,
+ }
+ }
+
+ #[test]
+ fn test_trivial_severity_filters() -> Result<(), Error> {
+ let config = "
+filter: test
+ min-severity warning
+";
+
+ let filters = parse_filters(config)?;
+
+ let is_match = |severity| {
+ let notifiction = empty_notification_with_severity(severity);
+ let mut results = FilterMatcher::new(&filters, ¬ifiction);
+ results.check_filter_match("test")
+ };
+
+ assert!(is_match(Severity::Warning)?);
+ assert!(!is_match(Severity::Notice)?);
+ assert!(is_match(Severity::Error)?);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_recursive_filter_loop() -> Result<(), Error> {
+ let config = "
+filter: direct-a
+ sub-filter direct-b
+
+filter: direct-b
+ sub-filter direct-a
+
+filter: indirect-c
+ sub-filter indirect-d
+
+filter: indirect-d
+ sub-filter indirect-e
+
+filter: indirect-e
+ sub-filter indirect-c
+";
+
+ let filters = parse_filters(config)?;
+
+ let notifiction = empty_notification_with_severity(Severity::Info);
+ let mut results = FilterMatcher::new(&filters, ¬ifiction);
+ assert!(results.check_filter_match("direct-a").is_err());
+ assert!(results.check_filter_match("indirect-c").is_err());
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_property_matches() -> Result<(), Error> {
+ let config = "
+filter: test
+ match-property object_type=vm
+
+filter: multiple-and
+ mode and
+ match-property a=foo,b=bar
+ match-property c=lorem,d=ipsum
+
+filter: multiple-or
+ mode or
+ match-property a=foo,b=bar
+ match-property c=lorem,d=ipsum
+";
+ let filters = parse_filters(config)?;
+
+ let is_match = |filter, metadata| -> Result<bool, Error> {
+ let notifiction = empty_notification_with_metadata(metadata);
+ let mut results = FilterMatcher::new(&filters, ¬ifiction);
+ results.check_filter_match(filter)
+ };
+
+ assert!(is_match("test", &[("object_type", "vm")])?);
+ assert!(!is_match("test", &[("object_type", "ct")])?);
+ assert!(is_match(
+ "multiple-and",
+ &[("a", "foo"), ("b", "bar"), ("c", "lorem"), ("d", "ipsum")],
+ )?);
+ assert!(!is_match(
+ "multiple-and",
+ &[
+ ("a", "invalid"),
+ ("b", "bar"),
+ ("c", "lorem"),
+ ("d", "ipsum")
+ ],
+ )?);
+ assert!(!is_match("multiple-and", &[("a", "foo"), ("b", "bar")],)?);
+ assert!(is_match("multiple-or", &[("a", "foo"),])?);
+ assert!(is_match("multiple-or", &[("b", "bar"),])?);
+ assert!(is_match("multiple-or", &[("d", "ipsum"),])?);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_invert_match() -> Result<(), Error> {
+ let config = "
+filter: test
+ match-property object_type=vm
+ invert-match true
+";
+ let filters = parse_filters(config)?;
+
+ let notifiction = empty_notification_with_metadata(&[("object_type", "vm")]);
+ let mut results = FilterMatcher::new(&filters, ¬ifiction);
+ assert!(!results.check_filter_match("test")?);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_subfilter_matches() -> Result<(), Error> {
+ let config = "
+filter: test
+ match-property object_type=vm
+ sub-filter vm-ids
+
+filter: vm-ids
+ mode or
+ match-property object_id=100
+ match-property object_id=101
+";
+ let filters = parse_filters(config)?;
+
+ let is_match = |metadata| -> Result<bool, Error> {
+ let notifiction = empty_notification_with_metadata(metadata);
+ let mut results = FilterMatcher::new(&filters, ¬ifiction);
+ results.check_filter_match("test")
+ };
+
+ assert!(is_match(&[("object_type", "vm"), ("object_id", "100")])?);
+ assert!(is_match(&[("object_type", "vm"), ("object_id", "101")])?);
+ assert!(!is_match(&[("object_type", "ct"), ("object_id", "101")])?);
+ assert!(!is_match(&[("object_type", "vm"), ("object_id", "111")])?);
+
+ Ok(())
+ }
+}
diff --git a/proxmox-notification/src/lib.rs b/proxmox-notification/src/lib.rs
index f076c88..c688a10 100644
--- a/proxmox-notification/src/lib.rs
+++ b/proxmox-notification/src/lib.rs
@@ -4,12 +4,14 @@ use anyhow::Error;
use endpoints::sendmail::SendmailConfig;
use endpoints::sendmail::SENDMAIL_TYPENAME;
+use filter::{FilterConfig, FilterMatcher, FILTER_TYPENAME};
use proxmox_schema::api;
use proxmox_section_config::SectionConfigData;
use serde::{Deserialize, Serialize};
mod config;
mod endpoints;
+mod filter;
#[api()]
#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
@@ -33,6 +35,9 @@ pub trait Endpoint {
/// The name/identifier for this endpoint
fn name(&self) -> &str;
+
+ /// The name of the filter to use
+ fn filter(&self) -> Option<&str>;
}
#[derive(Debug, Clone)]
@@ -88,7 +93,9 @@ impl Config {
.map(|e| e as Box<dyn Endpoint>),
);
- Ok(Bus { endpoints })
+ let filters = self.0.convert_to_typed_array(FILTER_TYPENAME)?;
+
+ Ok(Bus { endpoints, filters })
}
}
@@ -98,6 +105,7 @@ impl Config {
#[derive(Default)]
pub struct Bus {
endpoints: Vec<Box<dyn Endpoint>>,
+ filters: Vec<FilterConfig>,
}
impl Bus {
@@ -105,20 +113,52 @@ impl Bus {
self.endpoints.push(endpoint);
}
+ pub fn add_filter(&mut self, filter: FilterConfig) {
+ self.filters.push(filter)
+ }
+
/// Send a notification to all registered endpoints
pub fn send(&self, notification: &Notification) -> Result<(), Error> {
log::info!(
- "sending notification with title '{title}'",
+ "sending notification with title `{title}`",
title = notification.title
);
+ let mut notification_filter = FilterMatcher::new(&self.filters, notification);
+
for endpoint in &self.endpoints {
- endpoint.send(notification).unwrap_or_else(|e| {
- log::error!(
- "could not notfiy via endpoint `{name}`: {e}",
- name = endpoint.name()
+ let should_notify = if let Some(filter) = endpoint.filter() {
+ notification_filter
+ .check_filter_match(filter)
+ .unwrap_or_else(|e| {
+ log::error!(
+ "could not apply filter `{filter}` for endpoint `{name}: {e}`",
+ name = endpoint.name()
+ );
+ // If the filter is somehow erroneous, we send a notification by default,
+ // so no events are missed
+ true
+ })
+ } else {
+ true
+ };
+
+ if should_notify {
+ if let Err(e) = endpoint.send(notification) {
+ log::error!(
+ "could not notfiy via endpoint `{name}`: {e}",
+ name = endpoint.name()
+ )
+ } else {
+ log::info!("notified via endpoint `{name}`", name = endpoint.name())
+ }
+ } else {
+ log::info!(
+ "skipped endpoint `{name}`, filter `{filter}` did not match",
+ name = endpoint.name(),
+ filter = endpoint.filter().unwrap_or_default()
)
- })
+ }
}
Ok(())
@@ -136,6 +176,7 @@ mod tests {
#[derive(Default, Clone)]
struct MockEndpoint {
messages: Rc<RefCell<Vec<Notification>>>,
+ filter: Option<String>,
}
impl Endpoint for MockEndpoint {
@@ -148,9 +189,20 @@ mod tests {
fn name(&self) -> &str {
"mock-endpoint"
}
+
+ fn filter(&self) -> Option<&str> {
+ self.filter.as_deref()
+ }
}
impl MockEndpoint {
+ fn new(filter: Option<String>) -> Self {
+ Self {
+ filter,
+ ..Default::default()
+ }
+ }
+
fn messages(&self) -> Vec<Notification> {
self.messages.borrow().clone()
}
@@ -175,4 +227,69 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_severity_ordering() {
+ // Not intended to be exhaustive, just a quick
+ // sanity check ;)
+
+ assert!(Severity::Info < Severity::Notice);
+ assert!(Severity::Info < Severity::Warning);
+ assert!(Severity::Info < Severity::Error);
+ assert!(Severity::Error > Severity::Warning);
+ assert!(Severity::Warning > Severity::Notice);
+ }
+
+ #[test]
+ fn test_multiple_endpoints_with_different_filters() -> Result<(), Error> {
+ let endpoint1 = MockEndpoint::new(Some("filter1".into()));
+ let endpoint2 = MockEndpoint::new(Some("filter2".into()));
+
+ let mut bus = Bus::default();
+
+ bus.add_endpoint(Box::new(endpoint1.clone()));
+ bus.add_endpoint(Box::new(endpoint2.clone()));
+
+ bus.add_filter(FilterConfig {
+ name: "filter1".into(),
+ min_severity: Some(Severity::Warning),
+ sub_filter: None,
+ mode: None,
+ match_property: None,
+ invert_match: None,
+ });
+
+ bus.add_filter(FilterConfig {
+ name: "filter2".into(),
+ min_severity: Some(Severity::Error),
+ sub_filter: None,
+ mode: None,
+ match_property: None,
+ invert_match: None,
+ });
+
+ let send_with_severity = |severity| {
+ bus.send(&Notification {
+ title: "Title".into(),
+ body: "Body".into(),
+ severity,
+ properties: Default::default(),
+ })
+ .unwrap();
+ };
+
+ send_with_severity(Severity::Info);
+ assert_eq!(endpoint1.messages().len(), 0);
+ assert_eq!(endpoint2.messages().len(), 0);
+
+ send_with_severity(Severity::Warning);
+ assert_eq!(endpoint1.messages().len(), 1);
+ assert_eq!(endpoint2.messages().len(), 0);
+
+ send_with_severity(Severity::Error);
+ assert_eq!(endpoint1.messages().len(), 2);
+ assert_eq!(endpoint2.messages().len(), 1);
+
+ Ok(())
+ }
}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 04/18] notification: implement gotify endpoint
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (2 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 03/18] notification: add notification filter mechanism Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 05/18] notification: allow adding new sendmail endpoints / filters Lukas Wagner
` (14 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Add an endpoint for Gotify [1], showing the how easy it is to add new
endpoint implementations.
[1] https://gotify.net/
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notification/src/config.rs | 9 +++
proxmox-notification/src/endpoints/gotify.rs | 80 ++++++++++++++++++++
proxmox-notification/src/endpoints/mod.rs | 1 +
proxmox-notification/src/filter.rs | 1 +
proxmox-notification/src/lib.rs | 13 +++-
5 files changed, 103 insertions(+), 1 deletion(-)
create mode 100644 proxmox-notification/src/endpoints/gotify.rs
diff --git a/proxmox-notification/src/config.rs b/proxmox-notification/src/config.rs
index 939bdb6..e2454b6 100644
--- a/proxmox-notification/src/config.rs
+++ b/proxmox-notification/src/config.rs
@@ -3,6 +3,8 @@ use lazy_static::lazy_static;
use proxmox_schema::{const_regex, ApiStringFormat, ApiType, ObjectSchema, Schema, StringSchema};
use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
+use crate::endpoints::gotify::GotifyConfig;
+use crate::endpoints::gotify::GOTIFY_TYPENAME;
use crate::endpoints::sendmail::SendmailConfig;
use crate::endpoints::sendmail::SENDMAIL_TYPENAME;
use crate::filter::{FilterConfig, FILTER_TYPENAME};
@@ -33,6 +35,7 @@ fn init() -> SectionConfig {
let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA);
const SENDMAIL_SCHEMA: &ObjectSchema = SendmailConfig::API_SCHEMA.unwrap_object_schema();
+ const GOTIFY_SCHEMA: &ObjectSchema = GotifyConfig::API_SCHEMA.unwrap_object_schema();
const FILTER_SCHEMA: &ObjectSchema = FilterConfig::API_SCHEMA.unwrap_object_schema();
@@ -42,6 +45,12 @@ fn init() -> SectionConfig {
SENDMAIL_SCHEMA,
));
+ config.register_plugin(SectionConfigPlugin::new(
+ GOTIFY_TYPENAME.to_string(),
+ Some(String::from("name")),
+ GOTIFY_SCHEMA,
+ ));
+
config.register_plugin(SectionConfigPlugin::new(
FILTER_TYPENAME.to_string(),
Some(String::from("name")),
diff --git a/proxmox-notification/src/endpoints/gotify.rs b/proxmox-notification/src/endpoints/gotify.rs
new file mode 100644
index 0000000..3972bfe
--- /dev/null
+++ b/proxmox-notification/src/endpoints/gotify.rs
@@ -0,0 +1,80 @@
+use std::collections::HashMap;
+
+use crate::{Endpoint, Notification, Severity};
+use anyhow::Error;
+
+use proxmox_schema::{api, Updater};
+use serde::{Deserialize, Serialize};
+
+use proxmox_http::client::sync::Client;
+use proxmox_http::{HttpClient, HttpOptions};
+
+#[derive(Serialize)]
+struct GotifyMessageBody<'a> {
+ title: &'a str,
+ message: &'a str,
+ priority: u32,
+}
+
+fn severity_to_priority(level: Severity) -> u32 {
+ match level {
+ Severity::Info => 1,
+ Severity::Notice => 3,
+ Severity::Warning => 5,
+ Severity::Error => 9,
+ }
+}
+
+pub(crate) const GOTIFY_TYPENAME: &str = "gotify";
+
+#[api()]
+#[derive(Serialize, Deserialize, Updater)]
+#[serde(rename_all = "kebab-case")]
+/// Config for Gotify notification endpoints
+pub struct GotifyConfig {
+ /// Name of the endpoint
+ pub name: String,
+ /// Gotify Server URL
+ pub server: String,
+ /// Authentication token
+ pub token: String,
+ /// Filter to apply
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub filter: Option<String>,
+}
+
+impl Endpoint for GotifyConfig {
+ fn send(&self, notification: &Notification) -> Result<(), Error> {
+ // TODO: What about proxy configuration?
+ let client = Client::new(HttpOptions::default());
+
+ let uri = format!("{}/message", self.server);
+
+ let body = GotifyMessageBody {
+ title: ¬ification.title,
+ message: ¬ification.body,
+ priority: severity_to_priority(notification.severity),
+ };
+
+ let body = serde_json::to_vec(&body)?;
+ let extra_headers =
+ HashMap::from([("Authorization".into(), format!("Bearer {}", self.token))]);
+
+ client.post(
+ &uri,
+ Some(body.as_slice()),
+ Some("application/json"),
+ Some(&extra_headers),
+ )?;
+
+ Ok(())
+ }
+
+ fn name(&self) -> &str {
+ &self.name
+ }
+
+ fn filter(&self) -> Option<&str> {
+ self.filter.as_deref()
+ }
+}
diff --git a/proxmox-notification/src/endpoints/mod.rs b/proxmox-notification/src/endpoints/mod.rs
index 2d7b9ba..481d75b 100644
--- a/proxmox-notification/src/endpoints/mod.rs
+++ b/proxmox-notification/src/endpoints/mod.rs
@@ -1 +1,2 @@
+pub(crate) mod gotify;
pub(crate) mod sendmail;
diff --git a/proxmox-notification/src/filter.rs b/proxmox-notification/src/filter.rs
index 9846e93..3b7b19d 100644
--- a/proxmox-notification/src/filter.rs
+++ b/proxmox-notification/src/filter.rs
@@ -74,6 +74,7 @@ pub struct FilterConfig {
pub name: String,
/// Minimum severity to match
+ #[serde(skip_serializing_if = "Option::is_none")]
pub min_severity: Option<Severity>,
/// Subfilter, allows arbitrary nesting (no recursion allowed)
diff --git a/proxmox-notification/src/lib.rs b/proxmox-notification/src/lib.rs
index c688a10..fc724dd 100644
--- a/proxmox-notification/src/lib.rs
+++ b/proxmox-notification/src/lib.rs
@@ -2,6 +2,8 @@ use std::collections::HashMap;
use anyhow::Error;
+use endpoints::gotify::GotifyConfig;
+use endpoints::gotify::GOTIFY_TYPENAME;
use endpoints::sendmail::SendmailConfig;
use endpoints::sendmail::SENDMAIL_TYPENAME;
use filter::{FilterConfig, FilterMatcher, FILTER_TYPENAME};
@@ -11,7 +13,7 @@ use serde::{Deserialize, Serialize};
mod config;
mod endpoints;
-mod filter;
+pub mod filter;
#[api()]
#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
@@ -86,6 +88,8 @@ impl Config {
let sendmail_endpoints: Vec<SendmailConfig> =
self.0.convert_to_typed_array(SENDMAIL_TYPENAME)?;
+ let gotify_endpoints: Vec<GotifyConfig> = self.0.convert_to_typed_array(GOTIFY_TYPENAME)?;
+
endpoints.extend(
sendmail_endpoints
.into_iter()
@@ -93,6 +97,13 @@ impl Config {
.map(|e| e as Box<dyn Endpoint>),
);
+ endpoints.extend(
+ gotify_endpoints
+ .into_iter()
+ .map(Box::new)
+ .map(|e| e as Box<dyn Endpoint>),
+ );
+
let filters = self.0.convert_to_typed_array(FILTER_TYPENAME)?;
Ok(Bus { endpoints, filters })
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 05/18] notification: allow adding new sendmail endpoints / filters
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (3 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 04/18] notification: implement gotify endpoint Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 06/18] notification: add debian packaging Lukas Wagner
` (13 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
This is needed to migrate the PVE codebase from sendmail to the new
notification module - there, every place in the code that currently
sends out mails will add an 'ephemeral' sendmail endpoint, and also a
simple severity filter if needed. This ensures that there is a smooth
migration to the new notification module, without any events going
unnoticed.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notification/src/lib.rs | 1 +
proxmox-notification/src/methods.rs | 55 +++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
create mode 100644 proxmox-notification/src/methods.rs
diff --git a/proxmox-notification/src/lib.rs b/proxmox-notification/src/lib.rs
index fc724dd..24429e4 100644
--- a/proxmox-notification/src/lib.rs
+++ b/proxmox-notification/src/lib.rs
@@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize};
mod config;
mod endpoints;
pub mod filter;
+pub mod methods;
#[api()]
#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
diff --git a/proxmox-notification/src/methods.rs b/proxmox-notification/src/methods.rs
new file mode 100644
index 0000000..4f841e3
--- /dev/null
+++ b/proxmox-notification/src/methods.rs
@@ -0,0 +1,55 @@
+use anyhow::Error;
+
+use crate::{
+ endpoints::sendmail::{SendmailConfig, SENDMAIL_TYPENAME},
+ filter::{FilterConfig, FilterModeOperator, FILTER_TYPENAME},
+ Config, Severity,
+};
+
+pub fn add_sendmail_endpoint(
+ config: &mut Config,
+ name: &str,
+ recipient: Vec<String>,
+ from_address: Option<String>,
+ author: Option<String>,
+ filter: Option<String>,
+) -> Result<(), Error> {
+ config.0.set_data(
+ name,
+ SENDMAIL_TYPENAME,
+ &SendmailConfig {
+ name: name.into(),
+ recipient,
+ from_address,
+ author,
+ filter,
+ },
+ )?;
+
+ Ok(())
+}
+
+pub fn add_filter(
+ config: &mut Config,
+ name: &str,
+ min_severity: Option<Severity>,
+ sub_filter: Option<Vec<String>>,
+ mode: Option<FilterModeOperator>,
+ match_property: Option<Vec<String>>,
+ invert_match: Option<bool>,
+) -> Result<(), Error> {
+ config.0.set_data(
+ name,
+ FILTER_TYPENAME,
+ &FilterConfig {
+ name: name.into(),
+ min_severity,
+ sub_filter,
+ mode,
+ match_property,
+ invert_match,
+ },
+ )?;
+
+ Ok(())
+}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox 06/18] notification: add debian packaging
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (4 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 05/18] notification: allow adding new sendmail endpoints / filters Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1 Lukas Wagner
` (12 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notification/debian/changelog | 5 ++
proxmox-notification/debian/control | 59 +++++++++++++++++++++++
proxmox-notification/debian/copyright | 16 ++++++
proxmox-notification/debian/debcargo.toml | 7 +++
4 files changed, 87 insertions(+)
create mode 100644 proxmox-notification/debian/changelog
create mode 100644 proxmox-notification/debian/control
create mode 100644 proxmox-notification/debian/copyright
create mode 100644 proxmox-notification/debian/debcargo.toml
diff --git a/proxmox-notification/debian/changelog b/proxmox-notification/debian/changelog
new file mode 100644
index 0000000..3d4f9af
--- /dev/null
+++ b/proxmox-notification/debian/changelog
@@ -0,0 +1,5 @@
+rust-proxmox-notification (0.1.0-1) stable; urgency=medium
+
+ * Initial release.
+
+ -- Proxmox Support Team <support@proxmox.com> Thu, 12 Jan 2023 11:42:11 +0200
diff --git a/proxmox-notification/debian/control b/proxmox-notification/debian/control
new file mode 100644
index 0000000..42f80ee
--- /dev/null
+++ b/proxmox-notification/debian/control
@@ -0,0 +1,59 @@
+Source: rust-proxmox-notification
+Section: rust
+Priority: optional
+Build-Depends: debhelper (>= 12),
+ dh-cargo (>= 25),
+ cargo:native <!nocheck>,
+ rustc:native <!nocheck>,
+ libstd-rust-dev <!nocheck>,
+ librust-anyhow-1+default-dev <!nocheck>,
+ librust-handlebars-3+default-dev <!nocheck>,
+ librust-lazy-static-1+default-dev (>= 1.4-~~) <!nocheck>,
+ librust-log-0.4+default-dev (>= 0.4.17-~~) <!nocheck>,
+ librust-openssl-0.10+default-dev <!nocheck>,
+ librust-proxmox-http-0.8+client-sync-dev <!nocheck>,
+ librust-proxmox-http-0.8+default-dev <!nocheck>,
+ librust-proxmox-schema-1+api-macro-dev (>= 1.3.7-~~) <!nocheck>,
+ librust-proxmox-schema-1+default-dev (>= 1.3.7-~~) <!nocheck>,
+ librust-proxmox-section-config-1+default-dev (>= 1.0.2-~~) <!nocheck>,
+ librust-proxmox-sys-0.4+default-dev (>= 0.4.2-~~) <!nocheck>,
+ librust-regex-1+default-dev (>= 1.5-~~) <!nocheck>,
+ librust-serde-1+default-dev <!nocheck>,
+ librust-serde-json-1+default-dev <!nocheck>
+Maintainer: Proxmox Support Team <support@proxmox.com>
+Standards-Version: 4.6.1
+Vcs-Git: git://git.proxmox.com/git/proxmox.git
+Vcs-Browser: https://git.proxmox.com/?p=proxmox.git
+X-Cargo-Crate: proxmox-notification
+Rules-Requires-Root: no
+
+Package: librust-proxmox-notification-dev
+Architecture: any
+Multi-Arch: same
+Depends:
+ ${misc:Depends},
+ librust-anyhow-1+default-dev,
+ librust-handlebars-3+default-dev,
+ librust-lazy-static-1+default-dev (>= 1.4-~~),
+ librust-log-0.4+default-dev (>= 0.4.17-~~),
+ librust-openssl-0.10+default-dev,
+ librust-proxmox-http-0.8+client-sync-dev,
+ librust-proxmox-http-0.8+default-dev,
+ librust-proxmox-schema-1+api-macro-dev (>= 1.3.7-~~),
+ librust-proxmox-schema-1+default-dev (>= 1.3.7-~~),
+ librust-proxmox-section-config-1+default-dev (>= 1.0.2-~~),
+ librust-proxmox-sys-0.4+default-dev (>= 0.4.2-~~),
+ librust-regex-1+default-dev (>= 1.5-~~),
+ librust-serde-1+default-dev,
+ librust-serde-json-1+default-dev
+Provides:
+ librust-proxmox-notification+default-dev (= ${binary:Version}),
+ librust-proxmox-notification-0-dev (= ${binary:Version}),
+ librust-proxmox-notification-0+default-dev (= ${binary:Version}),
+ librust-proxmox-notification-0.1-dev (= ${binary:Version}),
+ librust-proxmox-notification-0.1+default-dev (= ${binary:Version}),
+ librust-proxmox-notification-0.1.0-dev (= ${binary:Version}),
+ librust-proxmox-notification-0.1.0+default-dev (= ${binary:Version})
+Description: Rust crate "proxmox-notification" - Rust source code
+ This package contains the source for the Rust proxmox-notification crate,
+ packaged by debcargo for use with cargo and dh-cargo.
diff --git a/proxmox-notification/debian/copyright b/proxmox-notification/debian/copyright
new file mode 100644
index 0000000..4fce23a
--- /dev/null
+++ b/proxmox-notification/debian/copyright
@@ -0,0 +1,16 @@
+Copyright (C) 2023 Proxmox Server Solutions GmbH
+
+This software is written by Proxmox Server Solutions GmbH <support@proxmox.com>
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU Affero General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU Affero General Public License for more details.
+
+You should have received a copy of the GNU Affero General Public License
+along with this program. If not, see <http://www.gnu.org/licenses/>.
diff --git a/proxmox-notification/debian/debcargo.toml b/proxmox-notification/debian/debcargo.toml
new file mode 100644
index 0000000..b7864cd
--- /dev/null
+++ b/proxmox-notification/debian/debcargo.toml
@@ -0,0 +1,7 @@
+overlay = "."
+crate_src_path = ".."
+maintainer = "Proxmox Support Team <support@proxmox.com>"
+
+[source]
+vcs_git = "git://git.proxmox.com/git/proxmox.git"
+vcs_browser = "https://git.proxmox.com/?p=proxmox.git"
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (5 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 06/18] notification: add debian packaging Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-31 9:17 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 08/18] add basic bindings for the proxmox_notification crate Lukas Wagner
` (11 subsequent siblings)
18 siblings, 1 reply; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Logging behaviour can be overridden by the {PMG,PVE}_LOG environment
variable.
This commit also disables styled output and timestamps in log messages,
since we usually log to the journal anyway. The log output is configured
to match with other log messages in task logs.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
common/src/logger.rs | 12 ++++++++++--
pmg-rs/src/lib.rs | 2 +-
pve-rs/src/lib.rs | 2 +-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/common/src/logger.rs b/common/src/logger.rs
index 36dc856..3c9a075 100644
--- a/common/src/logger.rs
+++ b/common/src/logger.rs
@@ -1,6 +1,14 @@
+use env_logger::{Builder, Env};
+use std::io::Write;
+
/// Initialize logging. Should only be called once
-pub fn init() {
- if let Err(e) = env_logger::try_init() {
+pub fn init(env_var_name: &str, default_log_level: &str) {
+ if let Err(e) = Builder::from_env(Env::new().filter_or(env_var_name, default_log_level))
+ .format(|buf, record| writeln!(buf, "{}: {}", record.level(), record.args()))
+ .write_style(env_logger::WriteStyle::Never)
+ .format_timestamp(None)
+ .try_init()
+ {
eprintln!("could not set up env_logger: {e}");
}
}
diff --git a/pmg-rs/src/lib.rs b/pmg-rs/src/lib.rs
index 5914bc9..d5c1080 100644
--- a/pmg-rs/src/lib.rs
+++ b/pmg-rs/src/lib.rs
@@ -12,6 +12,6 @@ mod export {
#[export]
pub fn init() {
- common::logger::init();
+ common::logger::init("PMG_LOG", "info");
}
}
diff --git a/pve-rs/src/lib.rs b/pve-rs/src/lib.rs
index 671aad0..af3076e 100644
--- a/pve-rs/src/lib.rs
+++ b/pve-rs/src/lib.rs
@@ -14,6 +14,6 @@ mod export {
#[export]
pub fn init() {
- common::logger::init();
+ common::logger::init("PVE_LOG", "info");
}
}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs 08/18] add basic bindings for the proxmox_notification crate
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (6 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1 Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-cluster 09/18] cluster files: add notifications.cfg Lukas Wagner
` (10 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Notes:
notifications.rs should be moved to common/src at some point.
Currently it resides in pve-rs/src, since rust-analyzer breaks for me
for all code in common/src.
Cargo.toml | 2 +
common/pkg/Makefile | 1 +
pve-rs/Cargo.toml | 1 +
pve-rs/src/lib.rs | 1 +
pve-rs/src/notification.rs | 128 +++++++++++++++++++++++++++++++++++++
5 files changed, 133 insertions(+)
create mode 100644 pve-rs/src/notification.rs
diff --git a/Cargo.toml b/Cargo.toml
index 94e29d5..fc0f7c1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -25,6 +25,7 @@ perlmod = { version = "0.13", features = [ "exporter" ] }
proxmox-acme-rs = { version = "0.4", features = ["client"] }
proxmox-apt = "0.9"
proxmox-http = { version = "0.8", features = ["client-sync", "client-trait"] }
+proxmox-notification = "0.1"
proxmox-openid = "0.9.8"
proxmox-resource-scheduling = "0.2.1"
proxmox-subscription = "0.3"
@@ -37,3 +38,4 @@ proxmox-time = "1.1.3"
#proxmox-tfa = { path = "../proxmox/proxmox-tfa" }
#proxmox-time = { path = "../proxmox/proxmox-time" }
#proxmox-uuid = { path = "../proxmox/proxmox-uuid" }
+#proxmox-notification = { path = "../proxmox/proxmox-notification" }
diff --git a/common/pkg/Makefile b/common/pkg/Makefile
index bf22a7b..658d336 100644
--- a/common/pkg/Makefile
+++ b/common/pkg/Makefile
@@ -17,6 +17,7 @@ Proxmox/RS/CalendarEvent.pm: ../scripts/genpackage.pl
perl ../scripts/genpackage.pl Common \
Proxmox::RS::APT::Repositories \
Proxmox::RS::CalendarEvent \
+ Proxmox::RS::Notification \
Proxmox::RS::Subscription
all: Proxmox/RS/CalendarEvent.pm
diff --git a/pve-rs/Cargo.toml b/pve-rs/Cargo.toml
index 0108eb2..9eafc25 100644
--- a/pve-rs/Cargo.toml
+++ b/pve-rs/Cargo.toml
@@ -33,6 +33,7 @@ perlmod.workspace = true
proxmox-apt.workspace = true
proxmox-http.workspace = true
+proxmox-notification.workspace = true
proxmox-openid.workspace = true
proxmox-resource-scheduling.workspace = true
proxmox-subscription.workspace = true
diff --git a/pve-rs/src/lib.rs b/pve-rs/src/lib.rs
index af3076e..7126e25 100644
--- a/pve-rs/src/lib.rs
+++ b/pve-rs/src/lib.rs
@@ -4,6 +4,7 @@
pub mod common;
pub mod apt;
+pub mod notification;
pub mod openid;
pub mod resource_scheduling;
pub mod tfa;
diff --git a/pve-rs/src/notification.rs b/pve-rs/src/notification.rs
new file mode 100644
index 0000000..ab325a8
--- /dev/null
+++ b/pve-rs/src/notification.rs
@@ -0,0 +1,128 @@
+#[perlmod::package(name = "Proxmox::RS::Notification")]
+mod export {
+ use anyhow::{bail, Error};
+ use perlmod::Value;
+ use std::sync::Mutex;
+
+ use std::collections::HashMap;
+
+ use proxmox_notification::{
+ filter::FilterModeOperator, methods, Config, Notification, Severity,
+ };
+
+ pub struct NotificationConfig {
+ config: Mutex<Config>,
+ }
+
+ perlmod::declare_magic!(Box<NotificationConfig> : &NotificationConfig as "Proxmox::RS::Notification");
+
+ /// Support `dclone` so this can be put into the `ccache` of `PVE::Cluster`.
+ #[export(name = "STORABLE_freeze", raw_return)]
+ fn storable_freeze(
+ #[try_from_ref] this: &NotificationConfig,
+ cloning: bool,
+ ) -> Result<Value, Error> {
+ if !cloning {
+ bail!("freezing Notification config not supported!");
+ }
+
+ let mut cloned = Box::new(NotificationConfig {
+ config: Mutex::new(this.config.lock().unwrap().clone()),
+ });
+ let value = Value::new_pointer::<NotificationConfig>(&mut *cloned);
+ let _perl = Box::leak(cloned);
+ Ok(value)
+ }
+
+ /// Instead of `thaw` we implement `attach` for `dclone`.
+ #[export(name = "STORABLE_attach", raw_return)]
+ fn storable_attach(
+ #[raw] class: Value,
+ cloning: bool,
+ #[raw] serialized: Value,
+ ) -> Result<Value, Error> {
+ if !cloning {
+ bail!("STORABLE_attach called with cloning=false");
+ }
+ let data = unsafe { Box::from_raw(serialized.pv_raw::<NotificationConfig>()?) };
+ Ok(perlmod::instantiate_magic!(&class, MAGIC => data))
+ }
+
+ #[export(raw_return)]
+ fn new(#[raw] class: Value, raw_config: &str) -> Result<Value, Error> {
+ Ok(perlmod::instantiate_magic!(&class, MAGIC => Box::new(
+ NotificationConfig {
+ config: Mutex::new(Config::new(raw_config)?)
+ }
+ )))
+ }
+
+ #[export]
+ fn write(#[try_from_ref] this: &NotificationConfig) -> Result<String, Error> {
+ this.config.lock().unwrap().write()
+ }
+
+ #[export]
+ fn send(
+ #[try_from_ref] this: &NotificationConfig,
+ severity: Severity,
+ title: &str,
+ message: &str,
+ properties: Option<HashMap<String, String>>,
+ ) -> Result<(), Error> {
+ let config = this.config.lock().unwrap();
+
+ let notification_bus = config.instantiate()?;
+
+ let notification = Notification {
+ title: title.into(),
+ body: message.into(),
+ severity,
+ properties: properties.unwrap_or_default(),
+ };
+
+ notification_bus.send(¬ification)?;
+
+ Ok(())
+ }
+
+ #[export]
+ fn add_sendmail_endpoint(
+ #[try_from_ref] this: &NotificationConfig,
+ name: &str,
+ recipient: Vec<String>,
+ from_address: Option<String>,
+ author: Option<String>,
+ filter: Option<String>,
+ ) -> Result<(), Error> {
+ let mut config = this.config.lock().unwrap();
+ methods::add_sendmail_endpoint(&mut config, name, recipient, from_address, author, filter)?;
+
+ Ok(())
+ }
+
+ #[export]
+ fn add_filter(
+ #[try_from_ref] this: &NotificationConfig,
+ name: &str,
+ min_severity: Option<Severity>,
+ sub_filter: Option<Vec<String>>,
+ mode: Option<FilterModeOperator>,
+ match_properties: Option<Vec<String>>,
+ invert_match: Option<bool>,
+ ) -> Result<(), Error> {
+ let mut config = this.config.lock().unwrap();
+
+ methods::add_filter(
+ &mut config,
+ name,
+ min_severity,
+ sub_filter,
+ mode,
+ match_properties,
+ invert_match,
+ )?;
+
+ Ok(())
+ }
+}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-cluster 09/18] cluster files: add notifications.cfg
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (7 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 08/18] add basic bindings for the proxmox_notification crate Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 10/18] test: fix names of .PHONY targets Lukas Wagner
` (9 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
data/PVE/Cluster.pm | 1 +
data/src/status.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index efca58f..1a1f9e8 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -55,6 +55,7 @@ my $observed = {
'firewall/cluster.fw' => 1,
'user.cfg' => 1,
'domains.cfg' => 1,
+ 'notifications.cfg' => 1,
'priv/shadow.cfg' => 1,
'priv/tfa.cfg' => 1,
'priv/token.cfg' => 1,
diff --git a/data/src/status.c b/data/src/status.c
index 9e520a5..b39cde0 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -82,6 +82,7 @@ static memdb_change_t memdb_change_array[] = {
{ .path = "storage.cfg" },
{ .path = "user.cfg" },
{ .path = "domains.cfg" },
+ { .path = "notifications.cfg"},
{ .path = "priv/shadow.cfg" },
{ .path = "priv/acme/plugins.cfg" },
{ .path = "priv/tfa.cfg" },
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 10/18] test: fix names of .PHONY targets
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (8 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-cluster 09/18] cluster files: add notifications.cfg Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 11/18] add PVE::Notification module Lukas Wagner
` (8 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
They need to have the same name as the target.
Took the opportunity to move the .PHONY right next to the target recipe,
so that mistakes like these are hopefully easier caught.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
test/Makefile | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/test/Makefile b/test/Makefile
index 670a3611..cccdc1c9 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -4,29 +4,35 @@ all:
export PERLLIB=..
-.PHONY: check balloon-test replication-test mail-test vzdump-test
+.PHONY: check
check: test-replication test-balloon test-mail test-vzdump test-osd
+.PHONY: test-balloon
test-balloon:
./balloontest.pl
+.PHONY: test-replication
test-replication: replication1.t replication2.t replication3.t replication4.t replication5.t replication6.t
replication%.t: replication_test%.pl
./$<
+.PHONY: test-mail
test-mail:
./mail_test.pl
+.PHONY: test-vzdump
test-vzdump: test-vzdump-guest-included test-vzdump-new
-.PHONY: test-vzdump-guest-included test-vzdump-new
+.PHONY: test-vzdump-guest-included
test-vzdump-guest-included:
./vzdump_guest_included_test.pl
+.PHONY: test-vzdump-new
test-vzdump-new:
./vzdump_new_test.pl
+.PHONY: test-osd
test-osd:
./OSD_test.pl
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 11/18] add PVE::Notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (9 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 10/18] test: fix names of .PHONY targets Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 12/18] vzdump: send notifications via new notification module Lukas Wagner
` (7 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
The PVE::Notification is a very thin wrapper around the
Proxmox::RS::Notification module, feeding the configuration from the
new 'notifications.cfg' file into it as a raw string.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
PVE/Makefile | 1 +
PVE/Notification.pm | 60 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
create mode 100644 PVE/Notification.pm
diff --git a/PVE/Makefile b/PVE/Makefile
index 48b85d33..0501b204 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -13,6 +13,7 @@ PERLSOURCE = \
HTTPServer.pm \
Jobs.pm \
NodeConfig.pm \
+ Notification.pm \
Report.pm \
VZDump.pm
diff --git a/PVE/Notification.pm b/PVE/Notification.pm
new file mode 100644
index 00000000..9933c4de
--- /dev/null
+++ b/PVE/Notification.pm
@@ -0,0 +1,60 @@
+package PVE::Notification;
+
+use strict;
+use warnings;
+
+use PVE::Cluster;
+use Proxmox::RS::Notification;
+
+PVE::Cluster::cfs_register_file(
+ 'notifications.cfg',
+ \&parse_notification_config,
+ \&write_notification_config,
+);
+
+sub parse_notification_config {
+ my ($filename, $raw) = @_;
+
+ $raw = '' if !defined($raw);
+ my $config = Proxmox::RS::Notification->new($raw);
+
+ return $config;
+}
+
+sub write_notification_config {
+ my ($filename, $config) = @_;
+
+ return $config->write();
+}
+
+sub config {
+ return PVE::Cluster::cfs_read_file('notifications.cfg');
+}
+
+sub send_notification {
+ my ($severity, $title, $message, $properties, $config) = @_;
+ $config = config() if !defined($config);
+ $config->send($severity, $title, $message, $properties);
+}
+
+sub info {
+ my ($title, $message, $config) = @_;
+ send_notification("info", $title, $message, undef, $config);
+}
+
+sub notice {
+ my ($title, $message, $config) = @_;
+ send_notification("notice", $title, $message, undef, $config);
+}
+
+sub warning {
+ my ($title, $message, $config) = @_;
+ send_notification("warning", $title, $message, undef, $config);
+}
+
+sub error {
+ my ($title, $message, $config) = @_;
+ send_notification("error", $title, $message, undef, $config);
+}
+
+1;
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 12/18] vzdump: send notifications via new notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (10 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 11/18] add PVE::Notification module Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 13/18] vzdump: rename 'sendmail' sub to 'send_notification' Lukas Wagner
` (6 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
... instead of using sendmail directly.
If the 'mailto' property is set set for a backup, an ephemeral
notification endpoint of type 'sendmail' will be added to the
notification backend. Also, a fitting notification filter will be added,
with the 'min-severity' property set to either 'info' or 'error',
depending on whether 'notify always' or 'on failure only' is set for a
backup job. This approach allows us to add the new notification backend
without breaking any existing mail notifications.
The notification backend only supports plain text for message bodies,
thus the HTML table generation has been removed.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
PVE/VZDump.pm | 116 +++++++++++++---------------------------------
test/mail_test.pl | 22 +++++----
2 files changed, 44 insertions(+), 94 deletions(-)
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a04837e7..c0971220 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -19,6 +19,7 @@ use PVE::Exception qw(raise_param_exc);
use PVE::HA::Config;
use PVE::HA::Env::PVE2;
use PVE::JSONSchema qw(get_standard_option);
+use PVE::Notification;
use PVE::RPCEnvironment;
use PVE::Storage;
use PVE::VZDump::Common;
@@ -308,7 +309,7 @@ sub sendmail {
my $mailto = $opts->{mailto};
- return if !($mailto && scalar(@$mailto));
+ # return if !($mailto && scalar(@$mailto));
my $cmdline = $self->{cmdline};
@@ -326,10 +327,10 @@ sub sendmail {
}
}
- my $notify = $opts->{mailnotification} || 'always';
- return if (!$ecount && !$err && ($notify eq 'failure'));
+ my $failed = ($ecount || $err);
+
+ my $stat = $failed ? 'backup failed' : 'backup successful';
- my $stat = ($ecount || $err) ? 'backup failed' : 'backup successful';
if ($err) {
if ($err =~ /\n/) {
$stat .= ": multiple problems";
@@ -391,90 +392,13 @@ sub sendmail {
}
$text_log_part .= $detail_post if defined($detail_post);
- # html part
- my $html = "<html><body>\n";
- $html .= "<p>" . (escape_html($err) =~ s/\n/<br>/gr) . "</p>\n" if $err;
- $html .= "<table border=1 cellpadding=3>\n";
- $html .= "<tr><td>VMID<td>NAME<td>STATUS<td>TIME<td>SIZE<td>FILENAME</tr>\n";
-
- my $ssize = 0;
- foreach my $task (@$tasklist) {
- my $vmid = $task->{vmid};
- my $name = $task->{hostname};
-
- if ($task->{state} eq 'ok') {
- $ssize += $task->{size};
-
- $html .= sprintf (
- "<tr><td>%s<td>%s<td>OK<td>%s<td align=right>%s<td>%s</tr>\n",
- $vmid,
- $name,
- format_time($task->{backuptime}),
- format_size ($task->{size}),
- escape_html ($task->{target}),
- );
- } else {
- $html .= sprintf (
- "<tr><td>%s<td>%s<td><font color=red>FAILED<td>%s<td colspan=2>%s</tr>\n",
- $vmid,
- $name,
- format_time($task->{backuptime}),
- escape_html ($task->{msg}),
- );
- }
- }
-
- $html .= sprintf ("<tr><td align=left colspan=3>TOTAL<td>%s<td>%s<td></tr>",
- format_time ($totaltime), format_size ($ssize));
-
- $html .= "\n</table><br><br>\n";
- my $html_log_part;
- $html_log_part .= "Detailed backup logs:<br /><br />\n";
- $html_log_part .= "<pre>\n";
- $html_log_part .= escape_html($cmdline) . "\n\n";
-
- $html_log_part .= escape_html($detail_pre) . "\n" if defined($detail_pre);
- foreach my $task (@$tasklist) {
- my $vmid = $task->{vmid};
- my $log = $task->{tmplog};
- if (!$log) {
- $html_log_part .= "$vmid: no log available\n\n";
- next;
- }
- if (open (my $TMP, '<', "$log")) {
- while (my $line = <$TMP>) {
- next if $line =~ /^status: \d+/; # not useful in mails
- if ($line =~ m/^\S+\s\d+\s+\d+:\d+:\d+\s+(ERROR|WARN):/) {
- $html_log_part .= encode8bit ("$vmid: <font color=red>".
- escape_html ($line) . "</font>");
- } else {
- $html_log_part .= encode8bit ("$vmid: " . escape_html ($line));
- }
- }
- close ($TMP);
- } else {
- $html_log_part .= "$vmid: Could not open log file\n\n";
- }
- $html_log_part .= "\n";
- }
- $html_log_part .= escape_html($detail_post) if defined($detail_post);
- $html_log_part .= "</pre>";
- my $html_end = "\n</body></html>\n";
- # end html part
-
- if (length($text) + length($text_log_part) +
- length($html) + length($html_log_part) +
- length($html_end) < MAX_MAIL_SIZE)
+ if (length($text) + length($text_log_part) < MAX_MAIL_SIZE)
{
- $html .= $html_log_part;
- $html .= $html_end;
$text .= $text_log_part;
} else {
- my $msg = "Log output was too long to be sent by mail. ".
+ my $msg = "Log output was too long to be sent. ".
"See Task History for details!\n";
$text .= $msg;
- $html .= "<p>$msg</p>";
- $html .= $html_end;
}
my $subject = "vzdump backup status ($hostname) : $stat";
@@ -482,7 +406,31 @@ sub sendmail {
my $dcconf = PVE::Cluster::cfs_read_file('datacenter.cfg');
my $mailfrom = $dcconf->{email_from} || "root";
- PVE::Tools::sendmail($mailto, $subject, $text, $html, $mailfrom, "vzdump backup tool");
+
+ my $notify = $opts->{mailnotification} || 'always';
+ my $min_severity = $notify eq "failure" ? "error" : "info";
+
+ my $config = PVE::Notification::config();
+
+ if ($mailto && scalar(@$mailto)) {
+ # add ephemeral sendmail endpoint for the existing vzdump-job,
+ # in order to not break existing mail notifications. The changed
+ # config is not persisted.
+ # TODO: Remove in PVE x.y ?
+ $config->add_filter("legacy-vzdump-sendmail-filter", $min_severity);
+ $config->add_sendmail_endpoint(
+ "legacy-vzdump-sendmail",
+ $mailto,
+ $mailfrom,
+ "vzdump backup tool",
+ "legacy-vzdump-sendmail-filter");
+ }
+
+ if ($failed) {
+ PVE::Notification::error($subject, $text, $config);
+ } else {
+ PVE::Notification::info($subject, $text, $config);
+ }
};
sub new {
diff --git a/test/mail_test.pl b/test/mail_test.pl
index d0114441..c77c1aae 100755
--- a/test/mail_test.pl
+++ b/test/mail_test.pl
@@ -5,7 +5,7 @@ use warnings;
use lib '..';
-use Test::More tests => 5;
+use Test::More tests => 3;
use Test::MockModule;
use PVE::VZDump;
@@ -29,18 +29,22 @@ sub prepare_mail_with_status {
sub prepare_long_mail {
open(TEST_FILE, '>', $TEST_FILE_PATH); # Removes previous content
# 0.5 MB * 2 parts + the overview tables gives more than 1 MB mail
- print TEST_FILE "a" x (1024*1024/2);
+ print TEST_FILE "a" x (1024*1024);
close(TEST_FILE);
}
-my ($result_text, $result_html);
+my $result_text;
-my $mock_tools_module = Test::MockModule->new('PVE::Tools');
-$mock_tools_module->mock('sendmail', sub {
- my (undef, undef, $text, $html, undef, undef) = @_;
+my $mock_notification_module = Test::MockModule->new('PVE::Notification');
+$mock_notification_module->mock('send_notification', sub {
+ my (undef, undef, $text, undef, undef) = @_;
$result_text = $text;
- $result_html = $html;
});
+$mock_notification_module->mock('config', sub {
+ # Return empty config
+ return Proxmox::RS::Notification->new("");
+});
+
my $mock_cluster_module = Test::MockModule->new('PVE::Cluster');
$mock_cluster_module->mock('cfs_read_file', sub {
@@ -62,7 +66,7 @@ my $SELF = {
my $task = { state => 'ok', vmid => '100', };
my $tasklist;
sub prepare_test {
- $result_text = $result_html = undef;
+ $result_text = undef;
$task->{tmplog} = shift;
$tasklist = [ $task ];
}
@@ -77,13 +81,11 @@ sub prepare_test {
prepare_mail_with_status();
PVE::VZDump::sendmail($SELF, $tasklist, 0, undef, undef, undef);
unlike($result_text, $STATUS, "Status are not in text part of mails");
- unlike($result_html, $STATUS, "Status are not in HTML part of mails");
}
{
prepare_test($TEST_FILE_PATH);
prepare_long_mail();
PVE::VZDump::sendmail($SELF, $tasklist, 0, undef, undef, undef);
like($result_text, $LOG_TOO_LONG, "Text part of mails gets shortened");
- like($result_html, $LOG_TOO_LONG, "HTML part of mails gets shortened");
}
unlink $TEST_FILE_PATH;
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 13/18] vzdump: rename 'sendmail' sub to 'send_notification'
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (11 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 12/18] vzdump: send notifications via new notification module Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 14/18] test: rename mail_test.pl to vzdump_notification_test.pl Lukas Wagner
` (5 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
PVE/API2/VZDump.pm | 2 +-
PVE/VZDump.pm | 10 ++++------
test/mail_test.pl | 6 +++---
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 8e873c05..83772e73 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -126,7 +126,7 @@ __PACKAGE__->register_method ({
$vzdump->getlock($upid); # only one process allowed
};
if (my $err = $@) {
- $vzdump->sendmail([], 0, $err);
+ $vzdump->send_notification([], 0, $err);
exit(-1);
}
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index c0971220..f1a0a5bd 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -302,15 +302,13 @@ sub read_vzdump_defaults {
}
use constant MAX_MAIL_SIZE => 1024*1024;
-sub sendmail {
+sub send_notification {
my ($self, $tasklist, $totaltime, $err, $detail_pre, $detail_post) = @_;
my $opts = $self->{opts};
my $mailto = $opts->{mailto};
- # return if !($mailto && scalar(@$mailto));
-
my $cmdline = $self->{cmdline};
my $ecount = 0;
@@ -563,7 +561,7 @@ sub new {
}
if ($errors) {
- eval { $self->sendmail([], 0, $errors); };
+ eval { $self->send_notification([], 0, $errors); };
debugmsg ('err', $@) if $@;
die "$errors\n";
}
@@ -1253,11 +1251,11 @@ sub exec_backup {
my $totaltime = time() - $starttime;
eval {
- # otherwise $self->sendmail() will interpret it as multiple problems
+ # otherwise $self->send_notification() will interpret it as multiple problems
my $chomped_err = $err;
chomp($chomped_err) if $chomped_err;
- $self->sendmail(
+ $self->send_notification(
$tasklist,
$totaltime,
$chomped_err,
diff --git a/test/mail_test.pl b/test/mail_test.pl
index c77c1aae..2dfbf067 100755
--- a/test/mail_test.pl
+++ b/test/mail_test.pl
@@ -73,19 +73,19 @@ sub prepare_test {
{
prepare_test($TEST_FILE_WRONG_PATH);
- PVE::VZDump::sendmail($SELF, $tasklist, 0, undef, undef, undef);
+ PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
like($result_text, $NO_LOGFILE, "Missing logfile is detected");
}
{
prepare_test($TEST_FILE_PATH);
prepare_mail_with_status();
- PVE::VZDump::sendmail($SELF, $tasklist, 0, undef, undef, undef);
+ PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
unlike($result_text, $STATUS, "Status are not in text part of mails");
}
{
prepare_test($TEST_FILE_PATH);
prepare_long_mail();
- PVE::VZDump::sendmail($SELF, $tasklist, 0, undef, undef, undef);
+ PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
like($result_text, $LOG_TOO_LONG, "Text part of mails gets shortened");
}
unlink $TEST_FILE_PATH;
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 14/18] test: rename mail_test.pl to vzdump_notification_test.pl
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (12 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 13/18] vzdump: rename 'sendmail' sub to 'send_notification' Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 15/18] api: apt: send notification via new notification module Lukas Wagner
` (4 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
test/Makefile | 8 ++++----
test/{mail_test.pl => vzdump_notification_test.pl} | 0
2 files changed, 4 insertions(+), 4 deletions(-)
rename test/{mail_test.pl => vzdump_notification_test.pl} (100%)
diff --git a/test/Makefile b/test/Makefile
index cccdc1c9..62d75050 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
export PERLLIB=..
.PHONY: check
-check: test-replication test-balloon test-mail test-vzdump test-osd
+check: test-replication test-balloon test-vzdump-notification test-vzdump test-osd
.PHONY: test-balloon
test-balloon:
@@ -17,9 +17,9 @@ test-replication: replication1.t replication2.t replication3.t replication4.t re
replication%.t: replication_test%.pl
./$<
-.PHONY: test-mail
-test-mail:
- ./mail_test.pl
+.PHONY: test-vzdump-notification
+test-vzdump-notification:
+ ./vzdump_notification_test.pl
.PHONY: test-vzdump
test-vzdump: test-vzdump-guest-included test-vzdump-new
diff --git a/test/mail_test.pl b/test/vzdump_notification_test.pl
similarity index 100%
rename from test/mail_test.pl
rename to test/vzdump_notification_test.pl
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 15/18] api: apt: send notification via new notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (13 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 14/18] test: rename mail_test.pl to vzdump_notification_test.pl Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 16/18] api: replication: send notifications " Lukas Wagner
` (3 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
... instead of using sendmail directly
As with the VZDump patch, the changes were implemented in such a way
that existing mail notifications continue to work as before. Here this
means that if root@pam has an email address configured, we add an
ephemeral sendmail endpoint for that email address.
Potential side effect: If there is another sendmail endpoint configured
in notifications.cfg which also sends to root@pam, root may receive
two emails.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
PVE/API2/APT.pm | 55 ++++++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 6694dbeb..543037de 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -19,6 +19,7 @@ use PVE::DataCenterConfig;
use PVE::SafeSyslog;
use PVE::INotify;
use PVE::Exception;
+use PVE::Notification;
use PVE::RESTHandler;
use PVE::RPCEnvironment;
use PVE::API2Tools;
@@ -341,34 +342,46 @@ __PACKAGE__->register_method({
my $rootcfg = $usercfg->{users}->{'root@pam'} || {};
my $mailto = $rootcfg->{email};
+ my $config = PVE::Notification::config();
+
if ($mailto) {
- my $hostname = `hostname -f` || PVE::INotify::nodename();
- chomp $hostname;
my $mailfrom = $dcconf->{email_from} || "root";
- my $subject = "New software packages available ($hostname)";
-
- my $data = "The following updates are available:\n\n";
-
- my $count = 0;
- foreach my $p (sort {$a->{Package} cmp $b->{Package} } @$pkglist) {
- next if $p->{NotifyStatus} && $p->{NotifyStatus} eq $p->{Version};
- $count++;
- if ($p->{OldVersion}) {
- $data .= "$p->{Package}: $p->{OldVersion} ==> $p->{Version}\n";
- } else {
- $data .= "$p->{Package}: $p->{Version} (new)\n";
- }
- }
+ # add ephemeral sendmail endpoint, in order to not break existing
+ # mail notifications. The changed config is not persisted.
+ # TODO: Remove in PVE x.y ?
+ $config->add_sendmail_endpoint(
+ "legacy-apt-sendmail",
+ [$mailto],
+ $mailfrom,
+ "");
+
+ }
- return if !$count;
+ my $hostname = `hostname -f` || PVE::INotify::nodename();
+ chomp $hostname;
+ my $subject = "New software packages available ($hostname)";
- PVE::Tools::sendmail($mailto, $subject, $data, undef, $mailfrom, '');
+ my $data = "The following updates are available:\n\n";
- foreach my $pi (@$pkglist) {
- $pi->{NotifyStatus} = $pi->{Version};
+ my $count = 0;
+ foreach my $p (sort {$a->{Package} cmp $b->{Package} } @$pkglist) {
+ next if $p->{NotifyStatus} && $p->{NotifyStatus} eq $p->{Version};
+ $count++;
+ if ($p->{OldVersion}) {
+ $data .= "$p->{Package}: $p->{OldVersion} ==> $p->{Version}\n";
+ } else {
+ $data .= "$p->{Package}: $p->{Version} (new)\n";
}
- PVE::Tools::file_set_contents($pve_pkgstatus_fn, encode_json($pkglist));
}
+
+ return if !$count;
+
+ PVE::Notification::info($subject, $data, $config);
+
+ foreach my $pi (@$pkglist) {
+ $pi->{NotifyStatus} = $pi->{Version};
+ }
+ PVE::Tools::file_set_contents($pve_pkgstatus_fn, encode_json($pkglist));
}
return;
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-manager 16/18] api: replication: send notifications via new notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (14 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 15/18] api: apt: send notification via new notification module Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 17/18] manager: " Lukas Wagner
` (2 subsequent siblings)
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
PVE/API2/Replication.pm | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index d70b4607..e3566a71 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -15,6 +15,7 @@ use PVE::QemuConfig;
use PVE::QemuServer;
use PVE::LXC::Config;
use PVE::LXC;
+use PVE::Notification;
use PVE::RESTHandler;
@@ -129,7 +130,17 @@ my sub _handle_job_err {
$msg .= "\nError:\n$err";
eval {
- PVE::Tools::sendmail('root', "Replication Job: $job->{id} failed", $msg)
+ my $config = PVE::Notification::config();
+ # For backward compatibility, add ephemeral sendmail endpoint,
+ # so that root receives a notification even
+ # if there is no endpoint defined.
+ $config->add_sendmail_endpoint(
+ "legacy-replication-sendmail",
+ ["root"],
+ undef, # TODO: read $mailfrom from datacenter.conf?
+ "");
+
+ PVE::Notification::error("Replication Job: $job->{id} failed", $msg, $config);
};
warn ": $@" if $@;
}
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-ha-manager 17/18] manager: send notifications via new notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (15 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 16/18] api: replication: send notifications " Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 18/18] manager: rename 'sendmail' --> 'send_notification' Lukas Wagner
2023-04-14 6:19 ` [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Thomas Lamprecht
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
... instead of using sendmail directly
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
src/PVE/HA/Env/PVE2.pm | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index f6ebfeb..a3b30b5 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -13,6 +13,7 @@ use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file
use PVE::DataCenterConfig;
use PVE::INotify;
use PVE::RPCEnvironment;
+use PVE::Notification;
use PVE::HA::Tools ':exit_codes';
use PVE::HA::Env;
@@ -228,7 +229,17 @@ sub sendmail {
# mail to the address configured in the datacenter
my $mailto = 'root';
- PVE::Tools::sendmail($mailto, $subject, $text, undef, $mailfrom);
+ my $config = PVE::Notification::config();
+
+ # Add ephemeral sendmail endpoint for backwards compatibility
+ $config->add_sendmail_endpoint(
+ "legacy-ha-manager-sendmail",
+ [$mailto],
+ $mailfrom,
+ undef);
+
+
+ PVE::Notification::warning($subject, $text, $config);
}
my $last_lock_status_hash = {};
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [pve-devel] [PATCH pve-ha-manager 18/18] manager: rename 'sendmail' --> 'send_notification'
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (16 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 17/18] manager: " Lukas Wagner
@ 2023-03-27 15:18 ` Lukas Wagner
2023-04-14 6:19 ` [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Thomas Lamprecht
18 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-27 15:18 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
src/PVE/HA/Env.pm | 4 ++--
src/PVE/HA/Env/PVE2.pm | 2 +-
src/PVE/HA/NodeStatus.pm | 2 +-
src/PVE/HA/Sim/Env.pm | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/PVE/HA/Env.pm b/src/PVE/HA/Env.pm
index 16603ec..98aeb09 100644
--- a/src/PVE/HA/Env.pm
+++ b/src/PVE/HA/Env.pm
@@ -144,10 +144,10 @@ sub log {
return $self->{plug}->log($level, @args);
}
-sub sendmail {
+sub send_notification {
my ($self, $subject, $text) = @_;
- return $self->{plug}->sendmail($subject, $text);
+ return $self->{plug}->send_notification($subject, $text);
}
# acquire a cluster wide manager lock
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index a3b30b5..064c892 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -220,7 +220,7 @@ sub log {
syslog($level, $msg);
}
-sub sendmail {
+sub send_notification {
my ($self, $subject, $text) = @_;
# Leave it to postfix to append the correct hostname
diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm
index ee5be8e..a281dfa 100644
--- a/src/PVE/HA/NodeStatus.pm
+++ b/src/PVE/HA/NodeStatus.pm
@@ -216,7 +216,7 @@ EOF
$mail_text .= to_json($data, { pretty => 1, canonical => 1});
- $haenv->sendmail($mail_subject, $mail_text);
+ $haenv->send_notification($mail_subject, $mail_text);
};
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index c6ea73c..c4a4872 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -288,7 +288,7 @@ sub log {
printf("%-5s %5d %12s: $msg\n", $level, $time, "$self->{nodename}/$self->{log_id}");
}
-sub sendmail {
+sub send_notification {
my ($self, $subject, $text) = @_;
# only log subject, do not spam the logs
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1 Lukas Wagner
@ 2023-03-31 9:17 ` Lukas Wagner
0 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-03-31 9:17 UTC (permalink / raw)
To: pve-devel
Whoop, not sure what happened here with the commit message - already fixed locally, will be included in v2 ;)
On 3/27/23 17:18, Lukas Wagner wrote:
> Logging behaviour can be overridden by the {PMG,PVE}_LOG environment
> variable.
>
> This commit also disables styled output and timestamps in log messages,
> since we usually log to the journal anyway. The log output is configured
> to match with other log messages in task logs.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
--
- Lukas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
` (17 preceding siblings ...)
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 18/18] manager: rename 'sendmail' --> 'send_notification' Lukas Wagner
@ 2023-04-14 6:19 ` Thomas Lamprecht
2023-04-14 9:47 ` Lukas Wagner
18 siblings, 1 reply; 22+ messages in thread
From: Thomas Lamprecht @ 2023-04-14 6:19 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 27/03/2023 um 17:18 schrieb Lukas Wagner:
> The purpose of this patch series is to overhaul the existing mail
> notification infrastructure in Proxmox VE.
nice! Some high level review, but I didn't checked the code basically at all,
so sorry if some comments of it are moot, as they're either already implemented
or discussed.
> A later patch could mark individual mail recipients in vzdump jobs as
> being deprecated in favor of the new notification endpoints.
Hmmm, this is something that I did not think to closely about when drafting
the (very rough) design for the notification overhaul, but it has some merit
to be able to configure this per job – but naturally it also adds some
redundancy with a global filtering system. IOW., we still need something
not all too compley for handling a job for, say, not so important test virtual
guests and one for production virtual guests. FWIW this could also be handled
by having a fixed mapping for some overides and jobs, i.e., the global
notification config could hold stanzas that match to a specific job (or other
notification emitting type) which is then also exposed when editing said job
directly. Otherwise, as we have now a pretty generic datacenter wide config
for almost all relevant jobs Proxmox VE can handle, we could also add a property
there that allows to match some notification policy/filter/config – that might
be cleaner as it avoids having to read/handle two configs in one API call.
> The
> drawback of this approach is that we might send a notification twice
> in case a user has created another sendmail endpoint with the same
> recipient. However, this is still better than not sending a
> notification at all. However, I'm open for suggestions for other
> approaches for maintaining compatibility.
>
> Alternative approaches that came to my mind are:
> - Explicitly break existing mail notifications with a major
> release, maybe create a default sendmail endpoint where *all*
> notifications are sent to root@pam via a sendmail endpoint.
> In this variant, the custom mail recipients for vzdump jobs would
> be removed
> - On update, create endpoints in the new configuration file for all
> possible email addresses, along with appropriate filters so that
> the behavior for every component currently sending mails remains
> unchanged. I don't really like this approach, as it might lead to
> a pretty messy 'notifications.cfg' with lots of endpoints/filters, if
> the user has lots of different backup jobs configured.
If we go that way we could do the ephermal sendpoint only heuristically,
i.e., check if there's another (email) notifaction endpoint that matches
ours and the `root@localhost || $configured_root_at_pam_email` and silence
it then, as that means that the admin switched over to the new mechanism.
Or, much simpler and transparent, some node/cluster global flag in e.g.,
vzdump.conf.
> Side effects of the changes:
> - vzdump loses the HTML table which lists VMs/CTs. Since different
> endpoints support different markup styles for formatting, it
> does not really make sense to support this in the notification API, at
> least not without 'cross-rendering' (e.g. markdown --> HTML)
This I'd think should be avoided if just anyhow possible, as that is
quite the nice feature. For keeping that possible we could add a optional
strucutred data object that can be passed to the notifaction system and
that (some) plugins can then use to show some more structured data.
Simplest might be a Option<serde_json::Value> that has a defined structure
like for example (psuedo json):
{
schema: {
type: "table", // or object
rows: ["vmid", "job", "..."],
// ... ?
},
data: [
{ vmid: 100, job: "foo", "...": "..."},
...
],
}
The mail plugin could then produce the two tables again, for the text/plain
and text/html multiparts again.
>
>
> Short summary of the introduced changes per repo:
> - proxmox:
> Add new proxmox-notification crate, including configuration file
> parsing, two endpoints (sendmail, gotify) and per-endpoint
> filtering
> - proxmox-perl-rs:
> Add bindings for proxmox-notification, so that we can use it from
> perl. Also configure logging in such a way that log messages from
> proxmox-notification integrate nicely in task logs.
> - proxmox-cluster:
> Register new configuration file, 'notifications.cfg'
> - pve-manager:
> Add some more glue code, so that the notification functions are
> callable in a nice way. This is needed since we need to read the
> configuration file and feed the config to the rust bindings as a
> string.
> Replace calls to 'sendmail' in vzdump,
> apt, and replication with calls to the new notification module.
> - pve-ha-manager:
> Also replace calls to 'sendmail' with the new notification module
>
>
> Follow-up work (in no particular order):
> - Add CRUD API routes for managing notification endpoints and
> filters - right now, this can only be done by editing
> 'notifications.cfg'
> - Add a GUI and CLI using this API
> - Right now, the notification API is very simple. Sending a
> notification can be as simple as
> PVE::Notification::info("Title", "Body")
nit, might use the verb form for this to be slightly shorter, i.e.:
PVE::Notify::info
>
> In the future, the API might be changed/extended so that supports
> "registering" notifications. This allows us to a.) generate a
> list of all possible notification sources in the system b.) allows
> users to easily create filters for specific notification events.
> In my head, using the notification module could look like this
> then:
>
> # Global context
> my = PVE::Notification::register({
> 'id' => 'backup-failed',
> 'severity' => 'error',
> 'properties' => ['host', 'vmlist', 'logs'],
> 'title' => '{{ host }}: Backup failed'
> 'body' => <<'EOF'
> A backup has failed for the following VMs: {{ vmlist }}
>
> {{ logs }}
> EOF
> });
hmm, yeah having that in module space so that its called on load like our
API endpoints is certainly the straight forward way, but having it act like
it would be rust (i.e., the notifiy send call itself _is_ the registry) would
be even nicer – but might be a tad too complex, as Wolfgang reject implementing
a Perl parser in rust already :(
>
> # Later, to send the notification:
> PVE::Notification::send(->instantiate({
> 'host' => 'earth',
> 'vmlist' => ... ,
> 'logs' => ... ,
> }));
>
> Title and body effectively become templates that can be
> instantiated with arbitrary properties. This has the following
> benefits:
> - This ensures that notifications could be easily translated in
> the future
> - Allows us to add functionality that allows users to customize
> notification text, as wished in [2].
> - Properties can be used in filters (e.g. exact match, regex,
> etc.)
> - Properties can be listed in the list of notifications
> mentioned above, making it easier to create filters.
Yes that would be nice and they could also have access to the structured
data, if passed as considered above.
>
> - proxmox-mail-forward could be integrated as well. This would feed
> e.g. zfs-zed events into our notification infrastructure. Special
> care must be taken to not create recursive notification loops
> (e.g. zed sends to root, forwarder uses notification module, a
> configured sendmail endpoint sends to root, forwarder uses module
> --> loop)
>
> - Maybe add some CLI so that admins can send notifications in
> scripts (an API endpoint callable via pvesh might be enough for a
> start)
>
> - Add more notification events
>
> - Add other endpoints, e.g. webhook, a generic SMTP, etc.
>
> - Integrate the new module into the other products
>
> [1] https://gotify.net/
> [2] https://bugzilla.proxmox.com/show_bug.cgi?id=4526
all in all it doesn't sounds to bad, from a higer level the structured data
to be able to keep special formats like html tables in backup notifiactions
and the how to switch over, or better said, how to also have some control in
the job-specific config about how/if notifications are emitted, are the two
higher level issues we should IMO tackle before initial integration can be
merged.
>
>
> Summary over all repositories:
> 34 files changed, 1464 insertions(+), 140 deletions(-)
>
> Generated by murpp v0.1.0
huh, what's this? some patch handling tool I don't know about?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module
2023-04-14 6:19 ` [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Thomas Lamprecht
@ 2023-04-14 9:47 ` Lukas Wagner
0 siblings, 0 replies; 22+ messages in thread
From: Lukas Wagner @ 2023-04-14 9:47 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
First of all, thanks for the feedback!
On 4/14/23 08:19, Thomas Lamprecht wrote:
>
> Hmmm, this is something that I did not think to closely about when drafting
> the (very rough) design for the notification overhaul, but it has some merit
> to be able to configure this per job – but naturally it also adds some
> redundancy with a global filtering system. IOW., we still need something
> not all too compley for handling a job for, say, not so important test virtual
> guests and one for production virtual guests. FWIW this could also be handled
> by having a fixed mapping for some overides and jobs, i.e., the global
> notification config could hold stanzas that match to a specific job (or other
> notification emitting type) which is then also exposed when editing said job
> directly. Otherwise, as we have now a pretty generic datacenter wide config
> for almost all relevant jobs Proxmox VE can handle, we could also add a property
> there that allows to match some notification policy/filter/config – that might
> be cleaner as it avoids having to read/handle two configs in one API call.
In an earlier on-paper draft of the whole notification overhaul I planned but then discarded
the concept of 'notification channels'. A notification channel would be ... well ... a channel
where jobs etc. can emit notifications to. In the notification configuration, one would then
be able to configure which endpoints are used for each channel. Jobs can configure which,
if at all, channel they emit notifications to. For some use cases, this would eliminate the
need for filtering (maybe except for severity) completely.
In this approach, the whole 'ephemeral sendmail endpoint' for VZDump jobs with configured email
notifications would/could be promoted to be an 'ephemeral channel', covering two use cases:
a) graceful switch-over to the new notification backend, without breaking anything
b) a 'short cut' for those users who don't really care about anything other than emails
The major difference from the user's perspective between 'ephemeral channel' and 'ephemeral endpoint'
would be that the channel sends *only* to the manually set address, without bothering about any other
endpoints. This would also solve the 'problem' where a notification might be sent twice when a job has a
the mailto property set *and* there is a sendmail endpoint with the same address.
To illustrate the concept, a short example. Let's assume we have some important production VMs.
The backup job for these could notify via the 'notify-prod' channel, which will ultimately fire out
notifications on several, high-priority endpoints (e.g. mail *and* some form of push notification).
Other, less important VMs could be backed up by a job that emits notifications to a different, low prio
channel (e.g. 'notify-non-prod'). For yet some other VMs, a specific person needs to be notified, so
there the admin chooses the 'direct email, without bells and whistles'-option, where they can
simply add the mail address in the VZDump job dialog.
I hope I could illustrate the idea, what do you think?
>
>> The
>> drawback of this approach is that we might send a notification twice
>> in case a user has created another sendmail endpoint with the same
>> recipient. However, this is still better than not sending a
>> notification at all. However, I'm open for suggestions for other
>> approaches for maintaining compatibility.
>>
>> Alternative approaches that came to my mind are:
>> - Explicitly break existing mail notifications with a major
>> release, maybe create a default sendmail endpoint where *all*
>> notifications are sent to root@pam via a sendmail endpoint.
>> In this variant, the custom mail recipients for vzdump jobs would
>> be removed
>> - On update, create endpoints in the new configuration file for all
>> possible email addresses, along with appropriate filters so that
>> the behavior for every component currently sending mails remains
>> unchanged. I don't really like this approach, as it might lead to
>> a pretty messy 'notifications.cfg' with lots of endpoints/filters, if
>> the user has lots of different backup jobs configured.
>
>
> If we go that way we could do the ephermal sendpoint only heuristically,
> i.e., check if there's another (email) notifaction endpoint that matches
> ours and the `root@localhost || $configured_root_at_pam_email` and silence
> it then, as that means that the admin switched over to the new mechanism.
>
> Or, much simpler and transparent, some node/cluster global flag in e.g.,
> vzdump.conf.
>
Good idea! However, as mentioned, this would no longer be necessary with the
approach described above.
>
>> Side effects of the changes:
>> - vzdump loses the HTML table which lists VMs/CTs. Since different
>> endpoints support different markup styles for formatting, it
>> does not really make sense to support this in the notification API, at
>> least not without 'cross-rendering' (e.g. markdown --> HTML)
>
>
> This I'd think should be avoided if just anyhow possible, as that is
> quite the nice feature. For keeping that possible we could add a optional
> strucutred data object that can be passed to the notifaction system and
> that (some) plugins can then use to show some more structured data.
>
> Simplest might be a Option<serde_json::Value> that has a defined structure
> like for example (psuedo json):
>
> {
> schema: {
> type: "table", // or object
> rows: ["vmid", "job", "..."],
> // ... ?
> },
> data: [
> { vmid: 100, job: "foo", "...": "..."},
> ...
> ],
> }
>
> The mail plugin could then produce the two tables again, for the text/plain
> and text/html multiparts again.
>
I've thought about this before, but ultimately discarded the idea since I was not sure
whether it was worth the effort. The HTML table was looking rather crude before, so I
did personally not see the benefit of it over a nice plain-text table.
I'll give this approach a try for v2. I'll have to think how this will play
together with the mentioned registered/templated notifications.
Having the data structured in the way you propose will also be beneficial once other
notification endpoints are implemented, e.g. webhook with JSON payload. There,
a plain text table does not make that much sense as a payload ;) .
Also, having HTML rendering in a single place will make it easy to add fancier styling later.
> nit, might use the verb form for this to be slightly shorter, i.e.:
> PVE::Notify::info
Good point, I'll rename it.
>
>>
>> In the future, the API might be changed/extended so that supports
>> "registering" notifications. This allows us to a.) generate a
>> list of all possible notification sources in the system b.) allows
>> users to easily create filters for specific notification events.
>> In my head, using the notification module could look like this
>> then:
>>
>> # Global context
>> my = PVE::Notification::register({
>> 'id' => 'backup-failed',
>> 'severity' => 'error',
>> 'properties' => ['host', 'vmlist', 'logs'],
>> 'title' => '{{ host }}: Backup failed'
>> 'body' => <<'EOF'
>> A backup has failed for the following VMs: {{ vmlist }}
>>
>> {{ logs }}
>> EOF
>> });
>
> hmm, yeah having that in module space so that its called on load like our
> API endpoints is certainly the straight forward way, but having it act like
> it would be rust (i.e., the notifiy send call itself _is_ the registry) would
> be even nicer – but might be a tad too complex, as Wolfgang reject implementing
> a Perl parser in rust already :(
>
I haven't really spent much thought yet on how the notification API would look in our
Rust-based products, but how could the `send` call fill the registry?
I assume this is only possible with some (procedural?) macro magic? Do we have similar
prior work somewhere?
>>
>> # Later, to send the notification:
>> PVE::Notification::send(->instantiate({
>> 'host' => 'earth',
>> 'vmlist' => ... ,
>> 'logs' => ... ,
>> }));
>>
>> Title and body effectively become templates that can be
>> instantiated with arbitrary properties. This has the following
>> benefits:
>> - This ensures that notifications could be easily translated in
>> the future
>> - Allows us to add functionality that allows users to customize
>> notification text, as wished in [2].
>> - Properties can be used in filters (e.g. exact match, regex,
>> etc.)
>> - Properties can be listed in the list of notifications
>> mentioned above, making it easier to create filters.
>
> Yes that would be nice and they could also have access to the structured
> data, if passed as considered above.
>
Agreed.
> all in all it doesn't sounds to bad, from a higer level the structured data
> to be able to keep special formats like html tables in backup notifiactions
> and the how to switch over, or better said, how to also have some control in
> the job-specific config about how/if notifications are emitted, are the two
> higher level issues we should IMO tackle before initial integration can be
> merged.
All right, thanks again for the feedback. I'll take your suggestions into account and
send a v2 once ready.
>>
>>
>> Summary over all repositories:
>> 34 files changed, 1464 insertions(+), 140 deletions(-)
>>
>> Generated by murpp v0.1.0
>
> huh, what's this? some patch handling tool I don't know about?
>
>
It's something that I've been building on the side. I was kind of annoyed by
manually assembling cover letters, especially when the patch series spanned multiple
repos and when there were multiple iterations of the same patch series.
It's a small Rust (tm) tool that pretty much fully automates preparing patches for series
like these. You can write the text for the cover letter, title, series version, and included
repos/branches in a single configuration file (YAML for now, as this allows me to include the
text for the body in a multi-line string, but I'm looking for alternatives).
The tool will then generate all patches with consecutive patch numbering and generate the final
cover letter, including all diff-stats and a final summary.
This series is the first time I've used the tool and I'm pretty happy so far.
Apart from writing the cover letter, I did not have to put in any manual work to assemble this series;
a single `murpp generate` was enough to have this patch series ready for `git send-email`.
I've created a staff repo for it, in case anybody wants to try it out.
--
- Lukas
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-04-14 9:47 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 15:18 [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 01/18] add proxmox-notification crate Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 02/18] notification: implement sendmail endpoint Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 03/18] notification: add notification filter mechanism Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 04/18] notification: implement gotify endpoint Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 05/18] notification: allow adding new sendmail endpoints / filters Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox 06/18] notification: add debian packaging Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 07/18] log: set default log level to 'info', add product specfig logging env var1 Lukas Wagner
2023-03-31 9:17 ` Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH proxmox-perl-rs 08/18] add basic bindings for the proxmox_notification crate Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-cluster 09/18] cluster files: add notifications.cfg Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 10/18] test: fix names of .PHONY targets Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 11/18] add PVE::Notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 12/18] vzdump: send notifications via new notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 13/18] vzdump: rename 'sendmail' sub to 'send_notification' Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 14/18] test: rename mail_test.pl to vzdump_notification_test.pl Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 15/18] api: apt: send notification via new notification module Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-manager 16/18] api: replication: send notifications " Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 17/18] manager: " Lukas Wagner
2023-03-27 15:18 ` [pve-devel] [PATCH pve-ha-manager 18/18] manager: rename 'sendmail' --> 'send_notification' Lukas Wagner
2023-04-14 6:19 ` [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module Thomas Lamprecht
2023-04-14 9:47 ` Lukas Wagner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox