Has REGEX/VALUE support been removed from spine?

[eriks]

TLDR: Spine does result validation checks before regex's defined in data queries are applied to snmp results causing otherwise good results to be rejected before the regex can even be applied.

---

I recently upgraded to 1.2.9, after working out a few other issues such as https://github.com/Cacti/cacti/issues/3350 I thought I was done. Cacti was running great and all the graphs were updating. Well that was up until a colleuge came to me and said "why aren't these two graphs working"?

So I started digging into this one. Basic debugging showed pretty quickly that spine was coming back with "U" aka Undefined for the OIDs in question. These graphs and the data query in general worked fine in the previous version of cacti 1.1.39. The only thing that changed was the upgrade

Code: Select all

2020/03/27 23:50:24 - SPINE: Poller[1] PID[28173] Device[808] HT[1] DS[12339] SNMP: v2: <redacted>, dsname: upcAtten, oid: .1.3.6.1.4.1.31637.1.1.9.12.0, value: U
2020/03/27 23:50:24 - SPINE: Poller[1] PID[28173] Device[808] HT[1] DS[12338] SNMP: v2: <redacted>, dsname: peakBeaconSample, oid: .1.3.6.1.4.1.31637.1.1.9.6.0, value: U
2020/03/27 23:50:24 - SPINE: Poller[1] PID[28173] Device[808] HT[1] DS[12338] SNMP: v2: <redacted>, dsname: peakBeaconClearSky, oid: .1.3.6.1.4.1.31637.1.1.9.5.0, value: U
2020/03/27 23:50:24 - SPINE: Poller[1] PID[28173] Device[808] HT[1] Total Time: 5.8 Seconds

The device in question uses a fairly simple snmp data query I wrote up, the device returns all the values in a rather painful way which requires regular expressions to sort out - not particularly complex ones mind you. Using just one reading here as an example the device returns the result as a string. The string consists of a leading space - for whatever reason, followed by the number and the units (dB)

Code: Select all

snmpget -v2c -c <redacted> -On <redacted> .1.3.6.1.4.1.31637.1.1.9.6.0
.1.3.6.1.4.1.31637.1.1.9.6.0 = STRING: " -68.5 dB"

So the regex in the XML is pretty straight forward, knock off the leading space, select the value and ignore the unit:

Code: Select all

                <upcSampledBeaconLevel>
                        <name>Sampled Beacon Level</name>
                        <method>walk</method>
                        <source>VALUE/REGEXP:\s*([+-]?[0-9]*\.[0-9]) dB</source>
                        <direction>output</direction>
                        <oid>.1.3.6.1.4.1.31637.1.1.9.6</oid>
                </upcSampledBeaconLevel>

I started going though the spine source code trying to find out why it was returning undefined and I started by looking for where the regex was applied. I found nothing, as far as I can tell spine does not apply the regex anywhere. That's not really a problem if it's applied at some point later in the process but what *IS* a problem is that spine seems to run a bunch of checks to validate the result:

Code: Select all

 if (!validate_result(snmp_oids[j].result)) {
                                                if (set.spine_log_level == 2) {
                                                        SPINE_LOG(("WARNING: Invalid Response, Device[%i] HT[%i] DS[%i] SNMP: v%i: %s, dsname: %s, oid: %s, value: %s",
                                                                host_id, host_thread, poller_items[snmp_oids[j].array_position].local_data_id, host->snmp_version,
                                                                host->hostname, poller_items[snmp_oids[j].array_position].rrd_name,
                                                                poller_items[snmp_oids[j].array_position].arg1, snmp_oids[j].result));
                                                } else if (set.spine_log_level == 1) {
                                                        errors++;
                                                        buffer_output_errors(error_string, buf_size, buf_errors, host_id, host_thread, poller_items[snmp_oids[j].array_position].local_data_id, false);
                                                }

                                                SET_UNDEFINED(snmp_oids[j].result);
                                        }
                                }

In this snippet from the spine sources we have it calling validate_result, which in turn checks if the value is numeric (by calling is_numeric), and the answer is no because of the trailing "dB". It also calls a function called is_multipart_output() which seems to check if it is a key-value pair delimited by a "!" or a ":" neither of which is the present here so that check fails as well.

Suffice to say then due to both checks failing validate_result() returns false which causes spine to call the SET_UNDEFINED macro which overwrites the otherwise good result with "U' and you get what we have above in the log.

So my questions are:
1. Was regex support just removed from spine intentionally? I can't see how regexes in snmp data queries could possibly work with these sort of validation checks in spine before the regex is actually applied to the polled value
2. If the answer to 1 is yes, then was it done with the intent that I should now write a script to perform such queries?

Thanks,
Erik

[Osiris]

Can you go through the commits and find where strip_alpha() was removed. Let us know the commit link. That'll help.

[eriks]

May have been this one?

Code: Select all

commit fb9e4dd12d8a5c370412ae8bf94154470d1d370e
Author: cigamit <jimmy@sqmail.org>
Date:   Mon Jan 2 08:46:48 2017 -0600

    Don't strip alpha on raw snmp_get() requests

For certain in poller.c strip_alpha seems to only be called if the result is from a script, not from snmp.

[eriks]

So if I make the following patch to poller.c then it works:

Code: Select all

diff --git a/poller.c b/poller.c
index 4a2bbf2..3cf38d2 100644
--- a/poller.c
+++ b/poller.c
@@ -1474,7 +1474,7 @@ void poll_host(int host_id, int host_thread, int last_host_thread, int host_data
                                        /* is valid output, continue */
                                } else {
                                        /* remove double or single quotes from string */
-                                       snprintf(temp_result, RESULTS_BUFFER, "%s", snmp_oids[j].result);
+                                       snprintf(temp_result, RESULTS_BUFFER, "%s", strip_alpha(trim(snmp_oids[j].result)));
                                        snprintf(snmp_oids[j].result , RESULTS_BUFFER, "%s", temp_result);

                                        /* detect erroneous non-numeric result */

I added both trim and strip_alpha() here similar to what is done for the script output. This resolves the problem because this section of the code (outside of the switch statement) is where these particular OIDS get processed. BUT this is not a good fix, if this same change is made to the section under "case POLLER_ACTION_SNMP:" spine will crash. It seems that strip_alpha can cause a crash with a certain result, but I have not worked on it any further to try to determine which result triggers the crash.

As an unrelated side note the compiler throws a warning with the most recent version of the 1.2.x branch:

Code: Select all

ping.c: In function ‘get_address_type’:
ping.c:878:3: warning: implicit declaration of function ‘inet_ntop’; did you mean ‘init_snmp’? [-Wimplicit-function-declaration]
   inet_ntop(res->ai_family, res->ai_addr->sa_data, addrstr, 100);
   ^~~~~~~~~
   init_snmp

Including the header which declares inet_ntop() will sort this out:

Code: Select all

--- common.h.orig       2020-03-28 11:22:59.092000000 -0400
+++ common.h    2020-03-28 12:07:12.568000000 -0400
@@ -93,6 +93,7 @@
 #include <stdio.h>
 #include <syslog.h>
 #include <stdbool.h>
+#include <arpa/inet.h>

 #if HAVE_STDINT_H
 #  include <stdint.h>

[Osiris]

Can you open a spine issue on GitHub?

[TheWitness]

I got it.

[eriks]

Oh sorry I was going to create one I really was - I was just looking at it further this morning. The issue with the poller stopping isn't so much a crash as it is one of the system calls returning EAGAIN. But even with the patch in place I've been having problems reproducing it this morning to narrow it down further.