From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8979598434 for ; Fri, 14 Apr 2023 08:19:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6EBD2222B7 for ; Fri, 14 Apr 2023 08:19:23 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 14 Apr 2023 08:19:16 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 97F3B41DCE for ; Fri, 14 Apr 2023 08:19:10 +0200 (CEST) Message-ID: <6b4d19b3-fc1c-0f30-be4a-22bc9b866fbe@proxmox.com> Date: Fri, 14 Apr 2023 08:19:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Lukas Wagner References: <20230327151857.495565-1-l.wagner@proxmox.com> Content-Language: de-AT, en-GB From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20230327151857.495565-1-l.wagner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.099 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record T_SPF_TEMPERROR 0.01 SPF: test of record failed (temperror) Subject: Re: [pve-devel] [PATCH cluster/manager/ha-manager/proxmox{, -perl-rs} 00/18] fix #4156: introduce new notification module X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Apr 2023 06:19:54 -0000 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 imple= mented or discussed. > A later patch could mark individual mail recipients in vzdump jobs as > being deprecated in favor of the new notification endpoints.=20 Hmmm, this is something that I did not think to closely about when drafti= ng the (very rough) design for the notification overhaul, but it has some me= rit to be able to configure this per job =E2=80=93 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 vi= rtual guests and one for production virtual guests. FWIW this could also be han= dled 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 o= ther notification emitting type) which is then also exposed when editing said = job directly. Otherwise, as we have now a pretty generic datacenter wide conf= ig for almost all relevant jobs Proxmox VE can handle, we could also add a p= roperty there that allows to match some notification policy/filter/config =E2=80=93= that might be cleaner as it avoids having to read/handle two configs in one API call= =2E > 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. >=20 > 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 silenc= e 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. =20 > 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 that has a defined structur= e 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/pla= in and text/html multiparts again. >=20 >=20 > 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 >=20 >=20 > 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 >=20 > 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: >=20 > # Global context > my =3D PVE::Notification::register({ > 'id' =3D> 'backup-failed', > 'severity' =3D> 'error', > 'properties' =3D> ['host', 'vmlist', 'logs'], > 'title' =3D> '{{ host }}: Backup failed' > 'body' =3D> <<'EOF' > A backup has failed for the following VMs: {{ vmlist }} >=20 > {{ 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 li= ke it would be rust (i.e., the notifiy send call itself _is_ the registry) w= ould be even nicer =E2=80=93 but might be a tad too complex, as Wolfgang rejec= t implementing a Perl parser in rust already :( >=20 > # Later, to send the notification: > PVE::Notification::send(->instantiate({ > 'host' =3D> 'earth', > 'vmlist' =3D> ... , > 'logs' =3D> ... , > })); >=20 > 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. >=20 > - 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) >=20 > - Maybe add some CLI so that admins can send notifications in > scripts (an API endpoint callable via pvesh might be enough for a > start) >=20 > - Add more notification events >=20 > - Add other endpoints, e.g. webhook, a generic SMTP, etc. >=20 > - Integrate the new module into the other products >=20 > [1] https://gotify.net/ > [2] https://bugzilla.proxmox.com/show_bug.cgi?id=3D4526 all in all it doesn't sounds to bad, from a higer level the structured da= ta to be able to keep special formats like html tables in backup notifiactio= ns 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 t= wo higher level issues we should IMO tackle before initial integration can b= e merged. >=20 >=20 > Summary over all repositories: > 34 files changed, 1464 insertions(+), 140 deletions(-) >=20 > Generated by murpp v0.1.0 huh, what's this? some patch handling tool I don't know about?