net-mgmt/zabbix-agent: add Zabbix Agent to plugins #97

fraenki · 2017-03-23T17:05:32Z

From the authors of Zabbix:

Zabbix is the ultimate enterprise-level software designed for real-time monitoring of millions of metrics collected from tens of thousands of servers, virtual machines and network devices.
Zabbix is Open Source and comes at no cost.

This will add the Zabbix Agent to our growing list of plugins, effectively allowing OPNsense to be monitored with Zabbix.

This version of the plugin is ready for use, but not feature complete (from my point of view). Thus I've set the version to 0.1 for now.

The wishlist for version 1.0 (or later):

allow to create UserParameter entries (arbitrary shell commands, run as Zabbix user)
provide a set of pre-defined OPNsense checks (pf stats, IPsec status, configd commands, OPNsense API calls)
automatically generate firewall rules from ListenIP/ListenPort and Servers/ActiveServers
fix the log file in GUI (see security/acme-client: logfile is scrambled in the GUI #69)

fabianfrz

Added some comments to the code.

fabianfrz · 2017-03-23T18:07:33Z

net-mgmt/zabbix-agent/src/etc/inc/plugins.inc.d/zabbixagent.inc

+    $result['section'] = 'OPNsense.zabbixagent';
+    $result['description'] = gettext('Enterprise-class open source distributed monitoring agent');
+    return array($result);
+}


Seems to be not final.

yes, it's a warning to myself and others what should be considered before enabling sync :)

fabianfrz · 2017-03-23T18:12:08Z

net-mgmt/zabbix-agent/src/opnsense/service/templates/OPNsense/ZabbixAgent/zabbix_agentd.conf

+# Default:
+# TLSPSKFile=
+
+{% endif %}


In a generated configuration file, documentation comments are bloat.

I think it is a good habit to keep comments in the sample configuration to give an idea about a certain value and it's usefulness. It's a very common thing to do.

fabianfrz · 2017-03-23T18:15:44Z

net-mgmt/zabbix-agent/src/opnsense/service/templates/OPNsense/ZabbixAgent/zabbix_agentd.conf

+{% if OPNsense.ZabbixAgent.settings.features.enableActiveChecks == '1' and OPNsense.ZabbixAgent.settings.features.activeCheckServers|default("") != "" %}
+ServerActive={{OPNsense.ZabbixAgent.settings.features.activeCheckServers}}
+{% else %}
+# NOTE: Active checks are disabled


useless control path

fabianfrz · 2017-03-23T18:19:11Z

net-mgmt/zabbix-agent/src/www/diag_logs_zabbixagent.php

+$logfile = '/var/log/zabbix/zabbix_agentd.log';
+$logclog = false;
+
+require_once 'diag_logs_template.inc';


probably should be integrated into MVC, so the log can be downloaded via the API.

there is a PR open if anyone wants to touch that.....

I think this is outside the scope of this plugin.

fabianfrz · 2017-03-23T18:22:09Z

net-mgmt/zabbix-agent/src/opnsense/mvc/app/models/OPNsense/ZabbixAgent/ZabbixAgent.xml

+                <activeCheckServers type="CSVListField">
+                    <Required>N</Required>
+                    <multiple>Y</multiple>
+                    <mask>/^([a-zA-Z0-9\.:\[\]\s\-]*?,)*([a-zA-Z0-9\.:\[\]\s\-]*)$/</mask>


whitespace (\s) in host:port combinations?

Removed \s.

fabianfrz · 2017-03-23T18:23:50Z

net-mgmt/zabbix-agent/src/opnsense/mvc/app/models/OPNsense/ZabbixAgent/ZabbixAgent.xml

+                        <val_2>2 - error information</val_2>
+                        <val_3>3 - warnings [default]</val_3>
+                        <val_4>4 - debugging</val_4>
+                        <val_5>5 - extended debugging</val_5>


probably remove the numbers as the user receives the strings. It is probably better to use round brackets because it would fit better into the rest of the system.

Well, in Zabbix the number is actually the debug level. The text is just a vague description. I'm not against removing it, it's just that this is the most precise information...

Regarding the brackets, do you have some examples? I've not seen other GUI elements where the default value was highlighted in any way.

it is only one entry:
3 - warnings [default] -> 3 - warnings (default)
or even better:
warnings (default)

The most readable is:

"Warnings (3, default)"
"Debugging (4)"
"Extended debugging (5)"

Numbers don't have to be omitted, but we can't assume users know / want to know what numbers correspond to what meaning. It also makes translations clearer.

@fabianfrz Yes, I know what entry you mean... I just wanted to see some examples why round brackets would be better suited. I know at least one example where round brackets would look really strange. And since I couldn't find any other highlighting for default values I've started using this variation for my other plugins already.

@fichtner Nice proposal, I'll change it accordingly.

fabianfrz · 2017-03-23T18:28:53Z

net-mgmt/zabbix-agent/src/opnsense/mvc/app/controllers/OPNsense/ZabbixAgent/IndexController.php

+    public function indexAction()
+    {
+        // set page title
+        $this->view->title = "Zabbix Agent Settings";


Fixed. And I will fix this for the "hello world" plugin too ;)

fabianfrz · 2017-03-23T18:29:48Z

net-mgmt/zabbix-agent/Makefile

+PLUGIN_VERSION=		0.1
+PLUGIN_COMMENT=		Enterprise-class open source distributed monitoring agent
+PLUGIN_DEPENDS=     zabbix32-agent
+PLUGIN_MAINTAINER=	opnsense@moov.de


maybe align the values

Fixed. Tabs are evil.

fabianfrz · 2017-03-24T20:32:00Z

please add a package description (same directory as the makefile). This will be shown to the user in the future if he wants more information about the plugin.

fichtner

good work, just being picky

fichtner · 2017-03-24T20:45:21Z

net-mgmt/zabbix-agent/Makefile

+PLUGIN_COMMENT=		Enterprise-class open source distributed monitoring agent
+PLUGIN_DEPENDS=     zabbix32-agent
+PLUGIN_MAINTAINER=	opnsense@moov.de
+


PLUGIN_DEVEL=yes

fichtner · 2017-03-24T20:46:00Z

net-mgmt/zabbix-agent/Makefile

+PLUGIN_NAME=		zabbix-agent
+PLUGIN_VERSION=		0.1
+PLUGIN_COMMENT=		Enterprise-class open source distributed monitoring agent
+PLUGIN_DEPENDS=     zabbix32-agent


is this the actual package that we use? https://github.com/opnsense/tools/blob/master/config/17.1/ports.conf#L88

There's no point using such an outdated version anymore. I strongly suggest to use 3.2 (or at a minimum 3.0) by now.

I misread 32 as 23... ok, will update tools.git

opnsense/tools@d2bafa3e0

Maybe also add a upgrade warning to the release notes, just in case someone is already using Zabbix Agent on OPNsense without a GUI.

the package name changes, no problem until they install the plugin, but I'll try to think of something.

fichtner · 2017-03-24T20:49:36Z

net-mgmt/zabbix-agent/src/etc/inc/plugins.inc.d/zabbixagent.inc

+ */
+
+/**
+XXX: needs investigation, hostname must be unique/replaced/excluded on secondary node(s)


yes, this is really bad for sync. why not use the system hostname instead to avoid the clash?

We can't just use the hostname. It must match the hostname in the Zabbix Server, otherwise many checks will not work. That's why it must be configurable.

fichtner · 2017-03-24T20:50:50Z

...zabbix-agent/src/opnsense/mvc/app/controllers/OPNsense/ZabbixAgent/Api/ServiceController.php

+        $response = $backend->configdRun("zabbixagent status");
+
+        if (strpos($response, "not running") > 0) {
+            if ($mdlAgent->settings->main->enabled->__toString() == 1) {


although just boilerplate, it's quirky: __toString() == "1"

fichtner · 2017-03-24T20:50:59Z

...zabbix-agent/src/opnsense/mvc/app/controllers/OPNsense/ZabbixAgent/Api/ServiceController.php

+            }
+        } elseif (strpos($response, "is running") > 0) {
+            $status = "running";
+        } elseif ($mdlAgent->settings->main->enabled->__toString() == 0) {


fichtner · 2017-03-24T21:00:22Z

net-mgmt/zabbix-agent/src/opnsense/scripts/OPNsense/ZabbixAgent/setup.sh

@@ -0,0 +1,11 @@
+#!/bin/sh
+
+AGENT_DIRS="/var/run/zabbix /var/log/zabbix /usr/local/etc/zabbix_agentd.d"


have you double checked with pkg-plist files? they can have other important dirs.

note this is also mostly for /var if MFS was enabled as /usr/local is persistent and likely created by the package install

https://github.com/opnsense/ports/tree/47aa039c8c8f361193e18a636760392971ef57ae/net-mgmt/zabbix24-server

Well, in theory the package creates %%ETCDIR%%/zabbix_agentd.conf.d, but it doesn't work for me (it's not being created on package install). I've changed the code to use this (FreeBSD) default directory instead, but I let setup.sh create the directory.

fichtner · 2017-03-24T21:00:57Z

net-mgmt/zabbix-agent/src/opnsense/service/conf/actions.d/actions_zabbixagent.conf

@@ -0,0 +1,34 @@
+[setup]


is this called separately? if not can zap

fichtner · 2017-03-24T21:02:02Z

net-mgmt/zabbix-agent/src/opnsense/service/conf/actions.d/actions_zabbixagent.conf

+message:restarting zabbix_agentd
+
+[status]
+command:/usr/local/etc/rc.d/zabbix_agentd status || exit 0


doesn't this return the status if you let it? not as script_output, then check for error

Fixed. Was using this most likely because of an old bug (in HAProxy plugin).

fichtner · 2017-03-24T21:03:27Z

net-mgmt/zabbix-agent/src/opnsense/service/templates/OPNsense/ZabbixAgent/rc.conf.d

+{% else %}
+zabbix_agentd_enable=NO
+{% endif %}
+zabbix_agentd_config=/usr/local/etc/zabbix_agentd.conf


into enable block, setup invoke is also missing:

https://github.com/opnsense/core/blob/master/src/opnsense/service/templates/OPNsense/Proxy/rc.conf.d#L3

Oh nice! I wasn't aware of this. Added.

fichtner · 2017-03-24T21:04:19Z

net-mgmt/zabbix-agent/src/www/diag_logs_zabbixagent.php

+$logfile = '/var/log/zabbix/zabbix_agentd.log';
+$logclog = false;
+
+require_once 'diag_logs_template.inc';


there is a PR open if anyone wants to touch that.....

fraenki · 2017-03-27T12:29:09Z

@fabianfrz @fichtner Thanks for your feedback! I've incorporated all changes. Let me know if more should be changed/fixed.

fabianfrz · 2017-03-27T13:17:50Z

looks OK to me.

fraenki · 2017-03-27T13:45:47Z

fichtner · 2017-03-28T05:05:03Z

@fraenki FYI, I had to do this opnsense/tools@54a0e5616337 due to the bump to 3.x

net-mgmt/zabbix-agent: add Zabbix Agent to plugins

ee0bb8e

fraenki self-assigned this Mar 23, 2017

fraenki requested a review from fichtner March 23, 2017 17:05

fabianfrz reviewed Mar 23, 2017

View reviewed changes

fichtner reviewed Mar 24, 2017

View reviewed changes

fraenki added a commit to fraenki/plugins that referenced this pull request Mar 27, 2017

net-mgmt/zabbix-agent: incorporate changes & fixes (opnsense#97)

d0c7d16

net-mgmt/zabbix-agent: incorporate changes & fixes (opnsense#97)

76233e1

fraenki force-pushed the zabbix_agent branch from d0c7d16 to 76233e1 Compare March 27, 2017 12:31

fichtner approved these changes Mar 27, 2017

View reviewed changes

fraenki merged commit 158f230 into opnsense:master Mar 27, 2017

		@@ -0,0 +1,11 @@
		#!/bin/sh

		AGENT_DIRS="/var/run/zabbix /var/log/zabbix /usr/local/etc/zabbix_agentd.d"

net-mgmt/zabbix-agent: add Zabbix Agent to plugins #97

net-mgmt/zabbix-agent: add Zabbix Agent to plugins #97

Conversation

fraenki commented Mar 23, 2017 • edited Loading

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraenki Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraenki Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz commented Mar 24, 2017

fichtner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraenki Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraenki commented Mar 27, 2017

fabianfrz commented Mar 27, 2017

fraenki commented Mar 27, 2017

fichtner commented Mar 28, 2017

fraenki commented Mar 23, 2017 •

edited

Loading

fraenki Mar 27, 2017 •

edited

Loading

fraenki Mar 27, 2017 •

edited

Loading

fraenki Mar 27, 2017 •

edited

Loading