Fix automation expressions for device rules

[gvde]

I have been wondering why my perl regexp like 'Aruba.*Switch' in the "System Description Match" field of the automation templates don't work, until I have found this:

https://github.com/Cacti/cacti/blob/master/lib/api_automation.php#L3071-L3073

Syntax of MariaDB REGEXP according to https://mariadb.com/kb/en/regexp/ is expr REGEXP pat or expr RLIKE pat, i.e. the pattern is after the REGEXP keyword not before, similar to the SQL LIKE.

Thus sysDescr REGEXP "(' . preg_quote($qsysDescr) . ')" actually matches whatever has been entered into the match field with the regexp in the text read into the snmp description. Obviously that doesn't match. So basically parameters to the REGEXP are swapped. They should be in the same order as for the LIKE part: first the text to be checked, then the pattern to be used.

Running 1.2.25 from EPEL9 on AlmaLinux 9.

[gvde]

This has also the effect that systems which deliver an empty string as sysDescr will always match a device rule as an empty pattern matches anything.

[gvde]

The SQL LIKE isn't really doing what is expected either, e.g.: LIKE CONCAT("%", sysOid, "%")

For the default device rules like 'ACME' there is no sysobjectid match set, thus this results in a LIKE "%%" which again matches anything.

The whole thing is pretty flawed...

[TheWitness]

@gvde That last bit is actually the bug. Waiting on @netniV on if the fix can make it for 1.2.27. Thanks for the bug report.

[gvde]

@gvde That last bit is actually the bug. Waiting on @netniV on if the fix can make it for 1.2.27. Thanks for the bug report.

The REGEXP is a bug, too. It's swapped. The patterns are stored in the database, the snmp values from the device are passed via variables into the function.

[gvde]

After a lot of thinking and testing I've ended up with this change:

original:

       $sql_where .= trim($sysDescr)  != '' ? 'WHERE (sysDescr REGEXP "(' . preg_quote($qsysDescr) . ')" OR ' . db_qstr($sysDescr) . ' LIKE CONCAT("%", sysDescr, "%"))':'';
       $sql_where .= trim($sysObject) != '' ? ($sql_where != '' ? ' AND':'WHERE') . ' (sysOID REGEXP "(' . preg_quote($qsysObject) . ')" OR ' . db_qstr($sysObject) . ' LIKE CONCAT("%", sysOid, "%"))':'';
       $sql_where .= trim($sysName)   != '' ? ($sql_where != '' ? ' AND':'WHERE') . ' (sysName REGEXP "(' . preg_quote($qsysName) . ')" OR ' . db_qstr($sysName) . ' LIKE CONCAT("%", sysName, "%"))':'';

now:

        $sql_where .= 'WHERE (sysDescr = "" OR sysDescr <> "" AND (' . db_qstr($sysDescr) . ' REGEXP sysDescr OR ' . db_qstr($sysDescr) . ' LIKE CONCAT("%", sysDescr, "%")))';
        $sql_where .= 'AND (sysOid = "" OR sysOid <> "" AND (' . db_qstr($sysObject) . ' REGEXP sysOid OR ' . db_qstr($sysObject) . ' LIKE CONCAT("%", sysOid, "%")))';
        $sql_where .= 'AND (sysName = "" OR sysName <> "" AND (' . db_qstr($sysName) . ' REGEXP sysName OR ' . db_qstr($sysName) . ' LIKE CONCAT("%", sysName, "%")))';

1. I have removed the initial condition trim($sys...) != '' in each line. $sys... contains the value from the device. If the value from the device is empty, the match still must be checked. If I have 'Aruba.*Switch' as match in the database for sysDescr, I expect that to match the value from the device. If the snmp sysDescr value from the device is empty IMHO that should not match the device rule.

2. If either of the matches in the device rule is empty, is always accepted by = "" otherwise it checks the match as REGEXP and LIKE.

I've intentionally did it this way to clarify the distinction, even though it's not necessary as the empty string would always match, thus I could rewrite

'AND (sysName = "" OR sysName <> "" AND (' . db_qstr($sysName) . ' REGEXP sysName OR ' . db_qstr($sysName) . ' LIKE CONCAT("%", sysName, "%")))'

simply to

'AND (' . db_qstr($sysName) . ' REGEXP sysName OR ' . db_qstr($sysName) . ' LIKE CONCAT("%", sysName, "%"))'

But for clarity I've put it this way so that people don't have to think about what it means to match against and empty pattern.

3. I have swapped the REGEXP arguments into the correct order, matching the content of the snmp values against the pattern stored in the database.

This works fine for me and how I expect it. Device rules only apply if the match(es) defined really match against the snmp values.

Although, I think for general usability it would be easier to understand if the matches defined would be considered as full match and not partials as it is at the moment. The user then can always use the % or .* to match anything at the beginning or end, i.e. (' . db_qstr($sysName) . ' REGEXP CONCAT("^", sysName, "$") OR ' . db_qstr($sysName) . ' LIKE sysName).

Of course, that is a breaking change as it would require to rewrite the current matches, e.g. for the Net-SNMP Device from Linux to %Linux% or for Cisco Router from (Cisco Internetwork Operating System Software|IOS) to .*(Cisco Internetwork Operating System Software|IOS).*.

And maybe even distinguish between REGEXP and LIKE arguments by syntax, e.g. requiring a slash to indicate that it is a regular expression and not a like match, i.e. /(Cisco Internetwork Operating System Software|IOS)/'.

But the last bits where just some thoughts about what would be the "best" way do configured this so that it's easy but clear to use...

[TheWitness]

I created a pull request on this one.

[TheWitness]

I thing the regexp is right. The pattern is what is stored in the automation database, the expression is the devices snmp_xxx data. So, the order is correct, the empty string is where the error occurs.

[TheWitness]

But the LIKE bit has me worried too. It's like we need more testing.

[gvde]

I thing the regexp is right. The pattern is what is stored in the automation database, the expression is the devices snmp_xxx data. So, the order is correct, the empty string is where the error occurs.

The REGEXP is not correct. Please check the documentation https://mariadb.com/kb/en/regexp/

It's expr REGEXP pat. You have ' (sysDescr REGEXP "(' . preg_quote($qsysDescr) . ')" OR ' in the code. The first sysDescr references the field in the database which contains the match pattern. $qsysDescr contains the string from the device's sysDescr snmp value.

To verify:

MariaDB [cacti]> select "(Cisco Internetwork Operating System Software|IOS)" REGEXP "Cisco IOS 10";
+----------------------------------------------------------------------------+
| "(Cisco Internetwork Operating System Software|IOS)" REGEXP "Cisco IOS 10" |
+----------------------------------------------------------------------------+
|                                                                          0 |
+----------------------------------------------------------------------------+
1 row in set (0.001 sec)

MariaDB [cacti]> select  "Cisco IOS 10" REGEXP "(Cisco Internetwork Operating System Software|IOS)";
+----------------------------------------------------------------------------+
| "Cisco IOS 10" REGEXP "(Cisco Internetwork Operating System Software|IOS)" |
+----------------------------------------------------------------------------+
|                                                                          1 |
+----------------------------------------------------------------------------+
1 row in set (0.000 sec)

[gvde]

After looking into the git blame and the docs for mysql and mariadb, I do wonder if the REGEXP ever worked in the last two years since this: af253ae

I don't find any indication in the docs, current or older version, that REGEXP ever excepted the swapped parameters and not the same order like RLIKE.

I'll prepare a pull request. As is seems it didn't work for at least almost two years, I strongly tend to distinguish the REGEXP from the LIKE matches by syntax of the match string to avoid confusion. In particular, if I am matching an OID "1.3.1.2.3" and simply use that as REGEXP and LIKE match, there could be easily some confusion, as it matches "1.3.123.3" as REGEXP but not as LIKE and the match string itself doesn't indicate what it is supposed to mean...

[gvde]

O.K. I have committed this: gvde@2b80238

I haven't done the pull request, yet. I am unclear about the localization as the popup text should be changed to indicate that perl re start with an /. Also about the ports into the other branches. Can anyone point me where, how, what??

Of course, if that "breaking" change with the '/' in the beginning is not acceptable, I could change that. However, as it didn't seem to work for some years now, I don't think it's so much of a breaking change anyway.