Adding inventory plugin for netbox #180
Conversation
|
@bdlamprecht this is what I wanted to do with "requests-mock". Right now we just need to mock the response of netbox by putting the contents we expect in a file: https://github.com/nornir-automation/nornir/pull/180/files#diff-62b8d7e6b6ab84dc213beb67c0081642 And then another json blob with the expected inventory: https://github.com/nornir-automation/nornir/pull/180/files#diff-1a9b65979e19e3ccdaf998204841d7cd I apologize for making you implement initially the containerized test, I thought it was going to be easier :( |
| }, | ||
| "2-Distribution": { | ||
| "name": "2-Distribution", | ||
| "nornir_host": { |
There was a problem hiding this comment.
@bdlamprecht This doesn't seem correct, I guess you wanted to put here just the address, not the object itself, is that correct?
There was a problem hiding this comment.
I don't quite follow your question here.
There was a problem hiding this comment.
Ah, yes, I see what you're referring to.
This line...
nornir/nornir/plugins/inventory/netbox.py
Line 40 in 8b56f97
Should actually look like this:
temp["nornir_host"] = d["primary_ip"]["address"]
I saw that before, but must have missed it when committing the changes. 😊
There was a problem hiding this comment.
Cool, will update :)
nornir/plugins/inventory/netbox.py
Outdated
| # Copy temporary dict to outer dict | ||
| devices[d["name"]] = temp | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
What is the reason this could happen?
There was a problem hiding this comment.
This is for when the nb_token is incorrect, it basically returns a dict such as {results:"Invalid token"}. Did know how else to do this.
There was a problem hiding this comment.
Do you get a status_code error? Something like a 403? Because then Response.raise_for_status might be a better alternative. Can you check and tell me which status code it returns?
nornir/plugins/inventory/netbox.py
Outdated
| # Attempt to add 'platform' based of value in 'slug' | ||
| try: | ||
| temp["nornir_nos"] = d["platform"]["slug"] | ||
| except TypeError: |
There was a problem hiding this comment.
What is the reason this could happen?
There was a problem hiding this comment.
Similar to the above try / except clause above for nornir_host, this one is for when a platform hasn't been assigned and use_slugs is True. Without a platform, there is no attribute for slug.
When use_slugs is False and no platform is defined, it automatically returns a None value.
There was a problem hiding this comment.
Ok, so this should be temp["nornir_nos"] = d.get("platform", {}).get("slug") instead of the try...except... block.
nornir/plugins/inventory/netbox.py
Outdated
| # Add value for IP address | ||
| try: | ||
| temp["nornir_host"] = d["primary_ip"] | ||
| except TypeError: |
There was a problem hiding this comment.
What is the reason this could happen?
There was a problem hiding this comment.
A device can have many interfaces, each with many ip_addresses.
One interface / ip_address combination needs to be selected as the primary_ip for each device.
This is try/except clause is for when that hasn't happened, then it will return an object with a None value for nornir_host.
There was a problem hiding this comment.
Ok, will replace with temp["nornir_host"] = d.get("primary_ip", None)
Pull Request Test Coverage Report for Build 835
💛 - Coveralls |
|
@bdlamprecht I added a few comments inline for the PR. |
|
I thought it would be helpful to provide the output from the import for all of the required objects in the proper order (i.e. It may or may not be what you wanted, but better safe than sorry. 😄
Non- |
nornir/plugins/inventory/netbox.py
Outdated
| # Assign temporary dict to outer dict | ||
| devices[d["name"]] = temp | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
@bdlamprecht when this happens, what status code does nsot return? I am wondering if we can just do raise_for_status.
There was a problem hiding this comment.
I pretty sure you meant NetBox instead of nsot here (:smile:) :
what status code does nsot return?
I don't think it returns a status code.
When I add the following code after passing an invalid nb_token:
except KeyError:
# DEBUG
print(nb_devices)
I get the following dict back:
{'detail': 'Invalid token'}
Hope that answers your question.
There was a problem hiding this comment.
It should return either a 401 or 403, otherwise, that's clearly a bug on Netbox and not very nice from an API perspective.
I think it's better if we test though. Would you mind changing:
nb_devices = requests.get(
"{}/api/dcim/devices/?limit=0".format(nb_url), headers=headers
).json()
with:
r = requests.get("{}/api/dcim/devices/?limit=0".format(nb_url), headers=headers)
r.raise_for_status()
nb_devices = r.json()
And test it with both a wrong and a correct token?
There was a problem hiding this comment.
With that change, it does appear to return a 403 with an invalid token:
HTTPError: 403 Client Error: Forbidden for url: http://localhost:8080/api/dcim/devices/?limit=0
Is that all you needed me to check on?
There was a problem hiding this comment.
Thanks! That's what I was hoping for. I will commit a couple of more changes and I think we will be ready to go here.
There was a problem hiding this comment.
Awesome! 💯
I'm happy to test it out once you get it all cleaned up the way that you'd like.
Cheers
|
@dbarrosop Hoping to get this inventory plugin merged soon so I can continue my testing to eventually migrate away from Ansible sooner rather than later.
Keep me updated, and again, thanks for |
|
@bdlamprecht this one should be ready, please test it works as expected. |
|
Yeah, it appears to work fine, both with and without invalid tokens (works and raises a I attempted to look into how you're using Thanks again! |
|
One last thought, as I mentioned, the I'm wondering if it would be possible to code up a method to only query against certain to this (respective to the order mentioned above): Or do you think the current implementation is good and it is up to the user to then filter on what devices they are specifically concerned with? |
|
sorry for barging in, |
|
That’s easy to implement, however, I’d like you to check first why it takes 5s for only 624 devices. Can you run |
|
@dbarrosop Yeah, that is a good idea about measuring the time. Honestly, I may have exaggerated ( 😄 ) the amount of time by saying ~5 seconds, but in all actuality is most likely ~2 seconds. I'll do that on Monday when I'm working again and let you know. Cheers! |
|
Should we merge this one? We can send a different PR later on to filter hosts by site or any other attribute later on if needed. |
|
LGTM! |
|
Yeah, I say merge it and then it can be enhanced later. |
No description provided.