On Fri Jul 12, 2024 at 1:27 PM CEST, Lukas Wagner wrote: > Sending as an RFC because I don't want this merged yet; that being > said, the feature should be mostly finished at this point, I'd > appreciate any reviews and feedback. > > This series adds support for webhook notification targets to PVE > and PBS. > > A webhook is a HTTP API route provided by a third-party service that > can be used to inform the third-party about an event. In our case, > we can easily interact with various third-party notification/messaging > systems and send PVE/PBS notifications via this service. > The changes were tested against ntfy.sh, Discord and Slack. > > The configuration of webhook targets allows one to configure: > - The URL > - The HTTP method (GET/POST/PUT) > - HTTP Headers > - Body > > One can use handlebar templating to inject notification text and metadata > in the url, headers and body. > > One challenge is the handling of sensitve tokens and other secrets. > Since the endpoint is completely generic, we cannot know in advance > whether the body/header/url contains sensitive values. > Thus we add 'secrets' which are stored in the protected config only > accessible by root (e.g. /etc/pve/priv/notifications.cfg). These > secrets are accessible in URLs/headers/body via templating: > > Url: https://example.com/{{ secrets.token }} > > Secrets can only be set and updated, but never retrieved via the API. > In the UI, secrets are handled like other secret tokens/passwords. > > Bumps for PVE: > - libpve-rs-perl needs proxmox-notify bumped > - pve-manager needs bumped proxmox-widget-toolkit and libpve-rs-perl bumped > - proxmox-mail-forward needs proxmox-notify bumped > > Bumps for PBS: > - proxmox-backup needs proxmox-notify bumped > - proxmox-mail-forward needs proxmox-notify bumped
Since this is an RFC, I mainly just did some proofreading; I haven't really spotted anything out of the ordinary, apart from a few *very small* things I commented on inline. I like the overall idea of adding webhooks, so this looks pretty solid to me. At first I thought that this might be a bit of a niche use case, but I feel like it might actually be quite interesting for orgs that are e.g. on Slack: You could e.g. just "route" all notifications via a webhook to Slack, and Slack then sends a push notification to one's phone. The same can obviously done with other applications / services as well. So, pretty cool stuff :) Not sure if this has been discussed somewhere already (off list etc.), but could you elaborate on why you don't want this merged yet? The patches look pretty solid to me, IMHO. Then again, I haven't really tested them yet due to all the required package bumps, so take this with a grain of salt. If you want to have this RFC tested, I can of course give it a shot - do let me know if that's the case :) > > > Changes v1 -> v2: > - Rebase proxmox-notify changes > > proxmox: > > Lukas Wagner (2): > notify: implement webhook targets > notify: add api for webhook targets > > proxmox-notify/Cargo.toml | 9 +- > proxmox-notify/src/api/mod.rs | 20 + > proxmox-notify/src/api/webhook.rs | 406 +++++++++++++++++++ > proxmox-notify/src/config.rs | 23 ++ > proxmox-notify/src/endpoints/mod.rs | 2 + > proxmox-notify/src/endpoints/webhook.rs | 509 ++++++++++++++++++++++++ > proxmox-notify/src/lib.rs | 17 + > 7 files changed, 983 insertions(+), 3 deletions(-) > create mode 100644 proxmox-notify/src/api/webhook.rs > create mode 100644 proxmox-notify/src/endpoints/webhook.rs > > > proxmox-perl-rs: > > Lukas Wagner (2): > common: notify: add bindings for webhook API routes > common: notify: add bindings for get_targets > > common/src/notify.rs | 72 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > > proxmox-widget-toolkit: > > Lukas Wagner (1): > notification: add UI for adding/updating webhook targets > > src/Makefile | 1 + > src/Schema.js | 5 + > src/panel/WebhookEditPanel.js | 417 ++++++++++++++++++++++++++++++++++ > 3 files changed, 423 insertions(+) > create mode 100644 src/panel/WebhookEditPanel.js > > > pve-manager: > > Lukas Wagner (2): > api: notifications: use get_targets impl from proxmox-notify > api: add routes for webhook notification endpoints > > PVE/API2/Cluster/Notifications.pm | 297 ++++++++++++++++++++++++++---- > 1 file changed, 263 insertions(+), 34 deletions(-) > > > pve-docs: > > Lukas Wagner (1): > notification: add documentation for webhook target endpoints. > > notifications.adoc | 93 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > > proxmox-backup: > > Lukas Wagner (3): > api: notification: add API routes for webhook targets > ui: utils: enable webhook edit window > docs: notification: add webhook endpoint documentation > > docs/notifications.rst | 100 +++++++++++++ > src/api2/config/notifications/mod.rs | 2 + > src/api2/config/notifications/webhook.rs | 175 +++++++++++++++++++++++ > www/Utils.js | 5 + > 4 files changed, 282 insertions(+) > create mode 100644 src/api2/config/notifications/webhook.rs > > > proxmox-mail-forward: > > Lukas Wagner (1): > bump proxmox-notify dependency > > Cargo.toml | 2 +- > debian/control | 8 ++++---- > 2 files changed, 5 insertions(+), 5 deletions(-) > > > Summary over all repositories: > 19 files changed, 2121 insertions(+), 42 deletions(-) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
