Re: [PATCH 1/5] Initialize async zabbix sender from zabbix_utils

[Michael Tremer <michael.tremer@ipfire.org>]

Hello,

Just as a warning, I am going to be picky :)

> On 30 Jul 2026, at 20:15, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> When Zabbix is enabled in new config section [zabbix], and the zabbix_utils 
> python module is available, a zabbix AsyncSender object will be initialized 
> for sending alerts to Zabbix using parameters from the new config section.
> 
> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> ---
> src/reporter.conf.in     | 23 +++++++++++++++++++++++
> src/suricata-reporter.in | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
> 
> diff --git a/src/reporter.conf.in b/src/reporter.conf.in
> index 5943006..bab01b6 100644
> --- a/src/reporter.conf.in
> +++ b/src/reporter.conf.in
> @@ -45,3 +45,26 @@
> ; 3 = Low Severity
> ; 4 = Informational
> ;severity = 3
> +
> +[zabbix]
> +; Enable sending alerts to Zabbix
> +;enabled = false
> +
> +; Path to the Zabbix agent configuration file
> +;zabbix_agentd_config = /etc/zabbix_agentd/zabbix_agentd.conf
> +
> +; Zabbix server ip or hostname (required if zabbix_agentd_config is not set)
> +;zabbix_server_host = 127.0.0.1
> +
> +; Zabbix server port (defaults to 10051 if not set)
> +;zabbix_server_port = 10051

You seem to like loooong variable names which I would shorten.

You are already in the [zabbix] section of the file, so you could simply call it “host” and “port”. Simple names.

> +
> +; Hostname as defined in Zabbix server to send alerts to (defaults to either the
> +; Hostname directive in Zabbix Agent config or system hostname)
> +;alert_item_hostname = IPFire
> +
> +; Zabbix item key to send alerts to
> +;alert_item_key = ipfire.suricata.event.get
> +
> +; Max age (seconds) to retry sending alerts to Zabbix
> +;alert_max_age = 3600
> \ No newline at end of file
> diff --git a/src/suricata-reporter.in b/src/suricata-reporter.in
> index 28b55bc..f9da7b4 100644
> --- a/src/suricata-reporter.in
> +++ b/src/suricata-reporter.in
> @@ -37,6 +37,13 @@ import socket
> import sqlite3
> import sys
> 
> +# Load zabbix_utils module if available 
> +zabbix_utils_available = True
> +try:
> +    from zabbix_utils import AsyncSender, ItemValue
> +except ImportError:
> +    zabbix_utils_available = False
> +

To keep the logic in one block, you could add the “True” statement to the else: clause of the block.

> # Fetch the hostname
> HOSTNAME = socket.gethostname()
> 
> @@ -75,6 +82,10 @@ class Reporter(object):
> # Remember the last time the database was cleaned
> self.last_cleanup_at = None
> 
> + # Initialize Zabbix sender
> + self.zabbix_sender = None
> + self.init_zabbix_sender()

I would have the function just return the sender object. That way, you can make it one line here and you don’t have to remember the name of the class variable in the function below.

> +
> # Register any signals
> for signo in (signal.SIGINT, signal.SIGTERM):
> self.loop.add_signal_handler(signo, self.terminate)
> @@ -97,6 +108,32 @@ class Reporter(object):
> 
> return config
> 
> + def init_zabbix_sender(self):
> + """
> + Initialize the Zabbix async sender if configured
> + """
> + if not self.config.getboolean('zabbix', 'enabled', fallback=False):
> + return
> +
> + if not zabbix_utils_available:
> + log.error("zabbix-utils is not installed. Zabbix alerts will not be sent.")
> + return
> +
> + zabbix_config = self.config.get('zabbix', 'zabbix_agentd_config', fallback='')
> + zabbix_server_host = self.config.get('zabbix', 'zabbix_server_host', fallback='')
> + zabbix_server_port = self.config.getint('zabbix', 'zabbix_server_port', fallback=10051)

Here you are being bitten by the long names again. This would be much shorter:

 host = self.config.get('zabbix', 'host', fallback=‘')

Oh, and you seem to be mixing “ and ‘ a lot in the code :) Both work, but just “ would be fine for me.

> +
> + if zabbix_config:
> + if not os.path.isfile(zabbix_config):
> + log.error(f"Zabbix agent config file {zabbix_config} does not exist.")
> + return
> + self.zabbix_sender = AsyncSender(use_config=True, config_path=zabbix_config)
> + else:
> + if not zabbix_server_host:
> + log.error("zabbix_server_host must be specified when zabbix_agentd_config is not provided.")
> + return
> + self.zabbix_sender = AsyncSender(server=zabbix_server_host, port=zabbix_server_port)
> +
> def _open_database(self):
> """
> Opens the database
> -- 
> 2.54.0
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
> 
>