sol1 / icingaweb2-module-netbox

Netbox importer for director, and integration with netbox
28 stars 6 forks source link

Uncaught TypeError with Unamed devices #16

Closed xeiss closed 2 years ago

xeiss commented 2 years ago

In Netbox it is possible to create a device without a name. In Netbox it will be shown as "Unnamed device". But when I try to import devices with such a device I get the following error:

Uncaught TypeError: Argument 1 passed to Icinga\Module\Netbox\Netbox::keymaker() must be of the type string, null given, called in /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php on line 122 and defined in /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php:100
Stack trace:
#0 /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php(122): Icinga\Module\Netbox\Netbox->keymaker()
#1 /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php(266): Icinga\Module\Netbox\Netbox->makeHelperKeys()
#2 /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php(342): Icinga\Module\Netbox\Netbox->transform()
#3 /usr/share/icingaweb2/modules/netbox/library/Netbox/Netbox.php(415): Icinga\Module\Netbox\Netbox->get_netbox()
#4 /usr/share/icingaweb2/modules/netbox/library/Netbox/ProvidedHook/Director/ImportSource.php(305): Icinga\Module\Netbox\Netbox->devices()
#5 /usr/share/icingaweb2/modules/director/library/Director/Import/Import.php(168): Icinga\Module\Netbox\ProvidedHook\Director\I (Netbox.php:100)

#0 [internal function]: Icinga\Application\Web->Icinga\Application\{closure}()
#1 {main}

The problemetic line in "library/Netbox/Netbox.php":

if (property_exists($row, 'name')) {
  $row->keyid = $this->keymaker($row->name);
}

The property exists, but it is null. Quote from PHP Help: As opposed with isset(), property_exists() returns true even if the property has the value null.

So I changed line 121 and 167 from "property_exists($row, 'name')" to isset($row->name) which fixed the preview. But there is still the problem that, the imported object don't have a name. So it can't sync to icinga, which needs a unique name. So maybe the best fix, would be to ignore this items, because without a hostname they are useless for network monitoring? Possible with following line after 113 (foreach): if (!(isset($row->name))) { continue; }

In my case it is a radio amplifier which does not have any ip / hostname / network port, but it uses rack space and so it have to be in Netbox. I don't need the device in Icinga, but the import should not fail because of this device.

Netbox v3.3.2 icingaweb2-module-netbox Version: master@06.09.2022 (v3.1.16.5) Icinga Director: 1.9.1 Icinga: r2.13.5-1

davekempe commented 2 years ago

ah yes, for this reason we added validation to our Netbox which prevented devices from not having a name. I prefer this - devices in racks should have a label with a name so people know what they are. There is a way to negate choices in the filter in the import source. In the search filter we can do something like this: tag=sol1-servers&tag__n=sol1-firewalls&status=active This only returns devices that are tagged with 'sol1-servers' NOT with 'sol1-firewalls' and are status=active. You may be able to filter out devices without names like this, or more specifically, only import devices that meet the right criteria using this syntax.

sol1-matt commented 2 years ago

Hi @xeiss, thanks for the report, I'd never have found this on my own.

Filters The reason for this is one of the things I do for my setups is add a custom field in Netbox for devices and virtual machines called monitoring_source, I make this custom field type selection and have a list of choices with the first one being Do not monitor and the second Default and depending on the setup I require I make one of these the default value.

When I import data I use the custom field in the filter field so anything I don't want to monitor, such as rack RU blanks/brushes and other stuff that doesn't need any monitoring can easily be included or excluded.

I'm not sure off the top of my head if you can filter on not null with Netbox, at least of name. If these things with no name have a specific role you can exclude that. Exclude filters in Netbox are __n, eg: https://netbox.example.com/dcim/devices/?role__n=patch-panelwould exclude the rolePatch panelwhich has a slugpatch-panel, this translates in the importerfiltervalue torole__n=patch-panel`.

Key column name Another solution would be to use something other than keyid or name as the Key column name value (and thus object name). However this bug would likely be triggered even if you did that.

So I think this means I need to fix error being thrown when the value is null but at the same time leave the data in place as somebody may want to use a different column for Key column name and object name.

For now I'd advise you to look into using a filter to exclude the data you don't want from the import, it is a better option anyway as it is less data you need to request from Netbox and I find the less I need to do in Icinga to massage the data into place the more robust and stable my automation is.

sol1-matt commented 2 years ago

@xeiss I have a fix, a6a2a35, on the dev branch. If you'd like to checkout this branch and let me know if this fixes your use case that would be great. I don't have a Netbox available that is setup to allow empty names on devices.

The solution returns null on null for the keymaker, this solves the problems

xeiss commented 2 years ago

@davekempe Yeah, to force the name would be also possible. But this is a default behavior from Netbox, so the plugin should can handle this.

Filtering would be possible, but the fix is a better solution, so you don't have to debug the source to find the reason why the import "crashes". Sure you then have entries with name: null, which can not deploy to icinga, but that problem is findable/filterable in GUI.

@sol1-matt thank you for that really fast fix. I tested it out, and it works as it should. The difference beetween my fix and your fix, is that the value keyid doesn't exists in the preview json, but with your fix it exisits with the value null. So I think your solution is more beautiful, especially that every call is fixed.

sol1-matt commented 2 years ago

@xeiss no problem. I agree the import shouldn't crash, doesn't mean it won't fail though but it should be clear why it is failing so you can fix it.

Director import really wants consistent data regardless of what you return now or in the future, having columns pop in or out is something I learn't was bad the hard way.

For Netbox filtering out "not null" I've found to be difficult. For this and other reasons I've settled on using monitoring_source custom field as a long term stable solution to ensuring you can explicitly include/exclude devices.

I'll push this change to master and turn it into a release. I'll probably bundle it with #13, the FHRP Groups addition which I'm still testing.

sol1-matt commented 2 years ago

Can be found in release https://github.com/sol1/icingaweb2-module-netbox/tree/v3.1.16.6