Set napalm connection options if they exist in netbox

[floatingstatic]

It would be beneficial to load napalm optional arguments as connection options based the platform a host is associated with within netbox.

I have a test group called ros (mikrotik) as follows:

With this change we can do something like the following to automatically load these into host['connection_options']['napalm'] based on the device platform:

>>> from pprint import pprint
>>> from nornir import InitNornir
>>>
>>> nr = InitNornir(
...     inventory={
...         "plugin": "nornir.plugins.inventory.netbox.NBInventory",
...             "options": {
...                 "nb_url": "https://localhost:8080",
...                 "nb_token": "0123456789abcdef0123456789abcdef01234567",
...                 "ssl_verify": True,
...                 "filter_parameters": {"role": "router", "status": "active"},
...             },
...     },
...     core={"num_workers": 20, "raise_on_error": False},
...     logging={"enabled": False, "level": "error"},
... )
>>>
>>> test_device = nr.filter(name="MA-SAV-CHA-RTR-01")
>>> pprint(test_device.inventory.get_inventory_dict())
{'defaults': {'connection_options': {},
              'data': {},
              'hostname': None,
              'password': None,
              'platform': None,
              'port': None,
              'username': None},
 'groups': {},
 'hosts': {'MA-SAV-CHA-RTR-01': {'connection_options': {'napalm': {'extras': {'optional_args': {'login_methods': ['login_plain']},
                                                                              'timeout': 10},
                                                                   'hostname': None,
                                                                   'password': None,
                                                                   'platform': None,
                                                                   'port': None,
                                                                   'username': None}},
                                 'data': {'asset_tag': None,
                                          'model': 'ccr1036-8g-2s',
                                          'role': 'router',
                                          'serial': 'A7FB0A06CBF7',
                                          'site': '1000_ma-sav-cha',
                                          'vendor': 'Mikrotik'},
                                 'groups': [],
                                 'hostname': '10.10.0.1',
                                 'password': None,
                                 'platform': 'ros',
                                 'port': None,
                                 'username': None}}}

[dbarrosop]

Thanks for the PR!

@wvandeun @clay584 would you mind reviewing this?

Due to nornir3 release, you will also need to propose the change here: https://github.com/wvandeun/nornir_netbox

You can check https://nornir.tech/2020/06/18/nornir-3-beta/ for more details.

Thanks again!

[wvandeun]

I like the idea, but it also has an issue in my point of view.

The idea of that field in NetBox is to specify optional_args for the Napalm driver for NetBox internal usage. The json data is effectively directly passed on to the Napalm driver optional_args attribute.
https://github.com/netbox-community/netbox/blob/fd564f09d1eb1ceaf738f91c8d8236d6a86c4247/netbox/dcim/api/views.py#L421

What you implemented works, but could lead to situation where you have to configure the same parameter twice, if you use both the build-in Napalm support for NetBox and Nornir.

For example let's say we want to set the login_method for a particular driver. We want that to work for both the NetBox build-in Napalm support & for Nornir.

We would have to come up with the following structure in NetBox for that to work:

{
    "login_methods": ["login_plain"],
    "extras": {
	"optional_args": {
	    "login_methods": ["login_plain"]
	}
    }
}

This doesn't really make sense to me and is going to be very confusing.
Also it's quite limiting since we would only be able to use this for the Napalm connection plugin.

At some point I had the idea of making a blend of NBInventory & SimpleInventory. The idea is that we would could create a groups.yaml file where you could specify for example connection parameters, based on some attribute.
A group could be a NetBox platform, site, region, ...

The advantage is that we could support all connection plugins with this and we don't have to misuse the Napalm attributes field of NetBox. The yaml files can be put in version control, you can use a proper edit to create them (linters, ... ) compared to a simple webform in a browser...

[dbarrosop]

Couple of comments/ideas :)

Given that those are meant to be napalm arguments, why not add them as they are meant to be so nb can consume them and have the nornir plugin adapt it for nornir's consumption?

For instance, you define:

{    "login_methods": ["login_plain"] }

so it works out of the box in napalm/netbox integration and then in the plugin you assign it under host -> connection options -> napalm

That way it'd work as intended and it'd work with nornir as well thanks to the plugin.

In addition, you could also abuse that field to pass connection_options (for any driver, based on a dictionary like in SimpleInventory) and add a flag to nb plugin to use this behavior (the one described before being the default behavior).

[wvandeun]

Sounds good to me.
@floatingstatic do you have some time to implement it as suggested above?

[floatingstatic]

Thank you for the clarification regarding how that field is intended to work with Netbox. Let me revise it per @dbarrosop suggestions. I will also open a similar PR under https://github.com/wvandeun/nornir_netbox for the nornir 3 release as well if all looks good here.

[floatingstatic]

I revised my platform NAPALM args in netbox to look like:

{"login_methods": ["login_plain"]}

The end result looks the same as before but only connection_options.napalm.extras.optional_args gets the contents of that field now. I do not use the NAPALM functionality in netbox myself which is why I did not catch this behavior previously (thanks again)

>>> from pprint import pprint
>>> from nornir import InitNornir
>>>
>>> nr = InitNornir(
...     inventory={
...         "plugin": "nornir.plugins.inventory.netbox.NBInventory",
...             "options": {
...                 "nb_url": "https://localhost:8080",
...                 "nb_token": "0123456789abcdef0123456789abcdef01234567",
...                 "ssl_verify": True,
...                 "filter_parameters": {"role": "router", "status": "active"},
...             },
...     },
... )
>>>
>>> test_device = nr.filter(name="MA-SAV-CHA-RTR-01")
>>> pprint(test_device.inventory.get_inventory_dict())
{'defaults': {'connection_options': {},
              'data': {},
              'hostname': None,
              'password': None,
              'platform': None,
              'port': None,
              'username': None},
 'groups': {},
 'hosts': {'MA-SAV-CHA-RTR-01': {'connection_options': {'napalm': {'extras': {'optional_args': {'login_methods': ['login_plain']},
                                                                              'timeout': 10},
                                                                   'hostname': None,
                                                                   'password': None,
                                                                   'platform': None,
                                                                   'port': None,
                                                                   'username': None}},
                                 'data': {'asset_tag': None,
                                          'model': 'ccr1036-8g-2s',
                                          'role': 'router',
                                          'serial': 'A7FB0A06CBF7',
                                          'site': '1000_ma-sav-cha',
                                          'vendor': 'Mikrotik'},
                                 'groups': [],
                                 'hostname': '10.10.0.1',
                                 'password': None,
                                 'platform': 'ros',
                                 'port': None,
                                 'username': None}}}

[wvandeun]

Some other points:

• Something is not quite right with the tests. You have not changed the expected results (see expected.json), yet the test succeeds. Can you have a look at that?

• We need to introduce a flag (argument) to enable the suggested feature, which should be disabled by default (use_platform_connection_options?)

• Can you have a look at how we could implement the same feature for other connection plugins as suggested above? If you don't want to (or don't have time) that's fine too

• We may want to setup some documentation that explains this feature

[floatingstatic]

@wvandeun I think I have the test fixed up and I ended up adding two additional flags that enable this feature as well as another one that "abuses" the netbox napalm args field and passes the results directly to connection_options. As far as additional documentation I am happy to add whatever may be needed beyond the docstrings. Thanks again for your reviews. I squashed my commits on the off chance this is good to go.

[floatingstatic]

@wvandeun Reverted to the previous host platform behavior, however I made a modification. Newer versions of netbox have a nested dictionary in the platform field, something like:

              'platform': {'id': 2,
                           'name': 'ros',
                           'slug': 'ros',
                           'url': 'http://netbox:8080/api/dcim/platforms/2/'},

So I made a modification that uses isinstance() to check if this field is a dictionary and that should be compatible with older releases where this may not be the case. Sadly I do not have an instance of an older release spun up for testing so I am just using mocked 2.3.5 data in the tests.

[wvandeun]

👍🏻 we may want to add a test case with mock data for a NetBox version that uses that platform data model

Why would we introduce 2 flags? One flag to enable both the Napalm and other connection methods should be sufficient I think?

[dbarrosop]

One flag to enable both the Napalm and other connection methods should be sufficient I think?

You could also use a constant or enumeration or whatever instead. Some flag that if you set to USE_AS_ORIGINALLY_INTENDED work one way, if you set as USE_AS_GENERIC_NORNIR some other, etc.... I agree two flags makes things a bit more complex for the user but I also think trying to be smart and infer things is how bugs and strong opinions are born :)

[floatingstatic]

We need to introduce a flag (argument) to enable the suggested feature, which should be disabled by default (use_platform_connection_options?)
Can you have a look at how we could implement the same feature for other connection plugins as suggested above? If you don't want to (or don't have time) that's fine too

My read of your previous comment was that you did not want platform options to be pulled from Netbox at all by default, which is why I have two flags here. I guess I misunderstood your request.

Personally I would rather just use platform napalm args if they are available (not gated by any flag) then use one flag ( use_platform_connection_options ) which would allow us to abuse/override the intended use of the field in netbox so it works with other connection types. Does that seem ok to you or is it more in line with your original thinking @wvandeun ?

I will also add some additional mock data formatted as we see it newer versions of Netbox.

[wvandeun]

I agree two flags makes things a bit more complex for the user but I also think trying to be smart and infer things is how bugs and strong opinions are born :)

So is abusing fields in applications for things they were not supposed to be used for :)

Are we suggesting the flags would be mutual exclusive then? If not, how would we handle the case where we pass napalm arguments as intended and when they are defined in the generic nornir way?

[floatingstatic]

@wvandeun / @dbarrosop can you have another look? I dropped the second flag and opted to use an environment variable since we require this for setting the base netbox url and api key anyway. If one passes use_platform_args set to True we use the netbox platform args as intended (imported as napalm options). If we pass this flag and set NB_PLATFORM_AS_CONN_OPTS=1 as an environment variable we do the same but import this netbox field into connection_options for each host.

Does that work for you both?

[wvandeun]

Starting to look good.
Probably needs some documentation with some examples. (example https://nornir.readthedocs.io/en/latest/plugins/tasks/version_control/gitlab.html)

[floatingstatic]

Updated tests. Will take a crack at updating documentation later today.

[floatingstatic]

So the current docs for the netbox plugin are pretty sparse if dare I say almost non-existent:
https://nornir.readthedocs.io/en/latest/plugins/inventory/netbox.html

Wouldn't it look a bit funny to add documentation for this feature I am adding without also adding documentation for the entire plugin, which frankly I do not really have that much time to invest into at this point. Could we consider updating the documentation for this plugin as a separate PR?

[wvandeun]

LGTM

Would also need you to open a PR with the same changes for NBInventory on https://github.com/wvandeun/nornir_netbox.
If you have the time then it would be good to implement it for NetBoxInventory2 to as well (same repo).

Agree on the documentation. Fixing it is as simple as moving the docstring from the init method to the class def level. And we would need to add a bit of words on how to use the env var, which is completely missing. But sure I can work on that if you don't have the time.

[floatingstatic]

Thanks. I rebased and squashed everything into a single commit. I'm happy to take on adding similar changes to https://github.com/wvandeun/nornir_netbox. As far as documentation, I can take a pass at that as well under a separate PR. Hopefully I can look at both this week. Thank you again for the reviews.