search

netbox-community / netbox

The premier source of truth powering network automation. Open source under Apache 2. Public demo: https://demo.netbox.dev
http://netboxlabs.com/oss/netbox/
Apache License 2.0
15.68k stars 2.53k forks source link

importing netbox.navigation.menu disables all plugin menus #16937

Closed Kek5chen closed 1 month ago

Kek5chen commented 1 month ago

Deployment Type

Self-hosted

NetBox Version

v4.0.7

Python Version

3.11

Steps to Reproduce

  1. Set up a basic plugin
  2. Anywhere, (for example the plugin __init__.py in the plugins ready() method), import from netbox.navigation.menu import MENUS or just import netbox.navigation.menu (untested but should be same behavior)

Expected Behavior

You can just import this module without side effects and for example, read the default menus from it.

Observed Behavior

Just importing this, will disable all plugin menus due to their respective initialization code being ran at the time of this import inside of this module.

Kek5chen commented 1 month ago

I have a PR with the fix ready once this is triaged.

arthanson commented 1 month ago

@Kek5chen I've assigned to you.

jeremystretch commented 1 month ago
  1. Anywhere, (for example the plugin __init__.py in the plugins ready() method), import from netbox.navigation.menu import MENUS or just import netbox.navigation.menu (untested but should be same behavior)

@Kek5chen why would you need to import this? The registration process for plugin menus does not require this.

Kek5chen commented 1 month ago

It's pretty simple, when you want to check the Navigation Menu items, for whatever reason, even just as to read from them, you have to import from the menu.py file. When you import this in a plug-in, it will try to unasked and prematurely register the plug-in navigation menus, at a too early state and make them inaccessible later when they should actually load. It's an obvious behavioural bug. We have a use case where this is important for us, and we stumbled upon this, and would like it fixed officially. Many thanks in advance.

Kek5chen commented 1 month ago

Even looking behind this use case, a plug-in should not be able to break the loading process of the whole environment with valid code.

Kek5chen commented 1 month ago

The problem is also that it appears fine, until you see that all your custom plug-in menus and the plug-in button are missing even though the plug-ins tried to register them. All just because of this one import.

jeremystretch commented 1 month ago

for whatever reason

For what reason, specifically?

Kek5chen commented 1 month ago

We have an internal plug-in that parses the menu, its links and grouping into our own internal data structure on plug-in ready.

This has to be done on ready as some data is mangled later due to the plug-in menus loading after ready and since we are only interested in the default, raw menu as it comes boxed with the specific netbox version that is currently latest stable. These parsed entries we use, so that we can then, in our own normal vanilla plug-in menu (created through the usual process) make references to these pages and urls as well as export these links to outside sources, as for example top desk, MS Teams and so on.

We also add our own query parameters to these links and mangle them ourselves, so it's important. We also internally group some things, where we put these links, after the NetBox layout in the same manner and the menu changing without our plug-in would lead to many, many dead / invalid links hovering around. (Though I have to say the grouping is unnecessary, but one of our team leads is stone set on this approach..)

We've been using NetBox for so long that we've adapted processes to be ready for changes in the navigation menu, and their links and grouping. We use these for monitoring and alerting but up until now we could only read from these menus through obscure ways of not importing the menu file (as this issue covers why that is a problem), but using a parser and basically forcing these entries out, having to worry about in no way, ever, importing this one specific file as to the plug-in menu load on import. It would end up breaking a LOT of our other plug-in registrations that will stop showing up without this fix.

This here fix would stabilise this part of our pipeline significantly and also make the internal plug-in code much more clean and readable as we go from a few thousand lines to just around 1-2 hundred. Though we're trying to slowly merge all our internal patches that we are working on, so that we might be able to run a vanilla release with our plug-ins at some point, as long as there are no updates enabling some of these features or us revisiting these patches and finding a better way of implementation.

We also just try to push those patches that we deem might benefit others. And this is most likely one of them.

Sorry if this is a little crazy, but what can you do. Again, we'd be grateful if you could merge this since it'd save some engineers some hours and probably a lot of future maintenance.

Best regards

jeremystretch commented 1 month ago

I'm sorry but what you're attempting to do is not supported functionality. It's simply not a design goal of the dynamic navigation system. You'll need to modify your code to import the menu code only after the application has been fully initialized. Django's AppConfig.ready() method may be helpful in this regard.

Kek5chen commented 1 month ago

You'll need to modify your code to import the menu code only after the application has been fully initialized.

If you would've attentively read my explanation, then you would've noticed that this is not an option, because plug-in menus and their deluded actions are going to mess with the navigation menu to produce a non-default state.

I don't understand why this is not a fix you can just merge, as it even was previously accepted and triaged by one of your senior developers.

This is a bug fix for something that shouldn't be a bug in the first place!

I don't know why your lack of allowance of good things in your own world has to affect us and others.

Just because our use case is not supported, doesn't mean it's invalid, or that importing this file for whatever other reason doesn't have its validity!

Neither did you give me any hints as how to improve this PR to make it fit more into the NetBox ideology, nor did you even try to understand our case.

This is simply unacceptable for FOSS.

You are a very closeted man, Jeremy, and it's no wonder that people are frustrated by NetBox and the team behind it.

jeremystretch commented 1 month ago

You're certainly entitled to your opinion, but my decision stands for the reason cited. Of course, you're always free to fork the NetBox code and make whatever changes you like. Best of luck.