howardjones / network-weathermap

Network Weathermap draws diagrams from data
http://www.network-weathermap.com/
MIT License
426 stars 95 forks source link

Cacti Plugin is creating duplicate records in plugin_realms #133

Closed netniV closed 6 years ago

netniV commented 6 years ago

Hey @howardjones,

For some reason, your plugin is creating many plugin_realm entries. I am not sure if this is a bug with the cacti realm code or your method of creating the realm but if you select the plugini_realm table, it has a few copies of your plugin within it.

select * from plugin_realms

image

As per https://github.com/Cacti/cacti/issues/1388

pautiina commented 6 years ago

image

netniV commented 6 years ago

I think this may be, because the call to api_plugin_register_realm is occurring often to make sure it's registered. Plus a potential bug in the base code:

register_realm should really only be called during install, so may want to double check that (I haven't looked at your sources to verify that).

netniV commented 6 years ago

Confirmed, bug with api_plugin_register_realm() introduce to develop code. The check to create the realm had become inverted somehow.

netniV commented 6 years ago

Actually, I'm going to reopen this as my logs are being spammed. I added a simple cacti_log() to see when the plugin realms function was being called, I then went to the site logged in an enabled quicktree....

2018/02/22 09:27:09 - CMDPHP api_plugin_register_realm('quicktree', 'quicktree.php', 'Plugin -> QuickTree: Access', 1)
--
2018/02/22 09:27:09 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:27:09 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:27:09 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:27:08 - CMDPHP api_plugin_register_realm('quicktree', 'quicktree.php', 'Plugin -> QuickTree: Access', 1)
2018/02/22 09:27:08 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:27:08 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:27:08 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:27:07 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:27:07 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:27:07 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:26:59 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:26:59 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:26:59 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:26:56 - AUTH LOGIN: User 'admin' Authenticated
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:26:56 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:26:51 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:26:51 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:26:51 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:25:03 - THOLDLISTS STATS: Time:0.01 Imports:1 Imported:0
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-editor.php', 'Weathermap: Edit Maps', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin-mgmt.php', 'Weathermap: Configure/Manage', 1)
2018/02/22 09:25:03 - CMDPHP api_plugin_register_realm('weathermap', 'weathermap-cacti10-plugin.php', 'Weathermap: View', 1)

As you can see, before I even went to login I've been spammed with realm creations (presumably by the poller).

howardjones commented 6 years ago

If ever there was a place for a composite database key...

I don't get this on my local Cacti 1.1.29 install, incidentally.

howardjones commented 6 years ago

When weathermap sees 'dev' in the version number, it runs setup_table() on every page, effectively. When it doesn't it compares the code version with the version number in the database, and only runs setup_table() if they are different. setup_table() includes the api_plugin_register_realm() calls.

But again, I don't get this behaviour on my 1.1.29 install, so it seems like a Cacti bug.

netniV commented 6 years ago

The duplication of realm is a cacti bug, but interesting to know it's only a dev thing for the repeated plugin_realm call. The thing is there is also a change now to realm_register where it will automatically assign the current user as having permission... so that change will have an impact here too.

https://github.com/Cacti/cacti/issues/1388#issuecomment-367613980 https://github.com/Cacti/cacti/commit/160a42c1e6ca35f006bdf24c7ae4c81a16c732e4 https://github.com/Cacti/cacti/issues/1384

In theory, it shouldn't cause too much hassle once we fix the recently introduced bug since the realm should already have been created by that point.

howardjones commented 6 years ago

The database will stop the duplicates for you with a properly defined composite key, regardless of what check is done in the php code.

I don't really see why automatically adding the current user has any particular impact, once the bug is fixed.

netniV commented 6 years ago

I'd have to check what happens with realms if the plugin exists but is not installed yet. I think somewhere the files still get included... thus if you have the dev version and it's registering the realm, the first person who hits the register_realm will be the one to get permission.

Thus, it may be a NON admin that does if it's not just done via the install code and then they get the permission - but again that is more a cacti bug if it happens. Something to check and be aware of more than suggesting it's completely wrong.

howardjones commented 6 years ago

OK, I see what you mean. I can move it to the place where the hooks are registered easily enough.

howardjones commented 6 years ago

So I just checked, and actuallly, the api_plugin_register_realm calls are in the config_arrays hook (because they used to updating Cacti arrays). That shouldn't be called for a non-installed plugin.

Anyway, I've moved them in next to the hook registration function calls.

netniV commented 6 years ago

Hmm, another thing to test out. I still think plugin code gets called somewhere in the base before it's installed. Your change should at least reduce the DB hit even though it was minor lol.

howardjones commented 6 years ago

It definitely did in early 1.x versions, because I could break the Cacti installer just by having code in the plugins directory. That should never happen. (and doesn't anymore)

netniV commented 6 years ago

Take a guess at what this bad baby is designed to do: image

howardjones commented 6 years ago

Unit tests. The more you can test automatically, the less stuff like this slips through.

netniV commented 6 years ago

Totally agree. Problem is, most of cacti is still based loosely around the 0.8.8 code... which means in page functions! The API though, yeah should have unit tests.

howardjones commented 6 years ago

Definitely recommend this talk... I only found it recently, but it's essentially what I've been through with Weathermap over the last couple of years, and a couple of work projects, to varying degrees (just getting to the DI stuff now)

https://leanpub.com/mlaphp

netniV commented 6 years ago

https://github.com/Cacti/cacti/issues/1389