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: https://github.com/Cacti/cacti/commit/af253aeef16577e485674ebba6f95375bde97967

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: https://github.com/gvde/cacti/commit/2b80238bfaa1f908df9ee15efa3173287557fa5e

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.

[TheWitness]

So, I cleared my head on this. You are right. Let me review your update.

[TheWitness]

Test the lib/api_automation.php from my Repo and provide feedback.

[TheWitness]

Been a busy week at the office.

[TheWitness]

Once you confirm, we can release 1.2.27.

[gvde]

Test the lib/api_automation.php from my Repo and provide feedback.

I have commented on your last change in your pull request (I think, github sometimes confuses me...)

Did you look at my suggested commit https://github.com/gvde/cacti/commit/4558f772d94b2b7fd1ef23faccb5dc3859f1e445

This is much shorter and works as I think the user expects it to work (as least I do). I have distinguished strings for REGEXP and LIKE by the '/' at the beginning. That's not necessary. If you insist on simply checking the string as REGEXP and LIKE I can change that.

However, as I have mentioned before, I have some concern as it does have unpredictable consequences as I have pointed out before https://github.com/Cacti/cacti/issues/5743#issuecomment-2099829972 I would generally prefer if it is clearly decided whether the string is considered a Perl RE or a SQL LIKE match as their syntaxes is completely different.

[TheWitness]

First, I like the idea of using the prepared statement. But the assumption that the user will use a slash in their pregs is a bad one.

What I would do on that subject, maybe in 1.3 is to allow the user to define either like or preg as a drop-down option, and then be specific about the query form.

[TheWitness]

Let me update to prepared statement for now.

[TheWitness]

Take another look. I think this one will work better.

[TheWitness]

Test it though.

[TheWitness]

It did not work for me. More debugging required.

[TheWitness]

Okay, latest update is now correct.

[gvde]

First, I like the idea of using the prepared statement. But the assumption that the user will use a slash in their pregs is a bad one.

What I would do on that subject, maybe in 1.3 is to allow the user to define either like or preg as a drop-down option, and then be specific about the query form.

O.K. That would be better. I didn't want to make too many changes, but as the regexp is currently not working and didn't for some years now I felt it would hurt.

Either way, I still don't understand why you want to accept devices with empty strings for all rules. If I enter "Cisco" for the match in sysDescr I expect that string to be in there. If the device has no description I expect it not to match.

Let's assume you have three devices which have the following texts set for sysDescr:

dev1: sysDescr "Aruba JL076A 3810M-40G-8SR-PoE+-1-slot Switch" dev2: sysDescr "ArubaOS" dev3: sysDescr ""

Now I enter a device rule with description "ArubaOS" and empty sysObject and empty sysName match. Your code matches dev2 and dev3.

Then I enter a device rule with description "Aruba.*Switch" and empty sysObject and empty sysName match. Your code matches dev1 and dev3.

A device with empty sysDescr will always match because you don't evaluate the condition. A device which present empty strings for sysDescr, sysName, and sysOid will actually match all device rules defined.

I find that extremely confusing and it makes it impossible to prevent devices with empty strings to match arbitrary device rules...

I'll prepare a commit with what I think it should look like. I think you are focusing on the wrong parts making it overly complicated...

[gvde]

Okay, latest update is now correct.

I have just saw that. I think it's easier if you omit the IF altogether. If any of the matches in the database is the empty string it matches always anyway. expr REGEXP "" is always true. expr LIKE "%%" is also always true.

[TheWitness]

Good point. I guess it 6 one way, half a dozen the other. Also, I guess we don't need the trims in you revised update. Can you remove them and update your pull?

[gvde]

I just created a pull request https://github.com/Cacti/cacti/pull/5747 showing the way, I would do it.