Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get_Environment from napalm should not need any decoding #8063

Merged
merged 2 commits into from
Dec 21, 2021

Conversation

rizlas
Copy link
Contributor

@rizlas rizlas commented Dec 14, 2021

Fixes: #7246

As stated already in the issue, napalm method get_environment is currently failing. Dictionary coming from napalm driver are already sanitized and therefore param decode_keys should be set to false before calling decode_dict.

https://github.com/napalm-automation/napalm/blob/develop/napalm/base/base.py#L465
https://github.com/napalm-automation/napalm/blob/develop/napalm/ios/ios.py#L2197

This was tested with ios driver.

@jeremystretch
Copy link
Member

jeremystretch commented Dec 20, 2021

decode_dict() was introduced as part of the fix for #7041. I'm not comfortable altering its execution without understanding the ramifications of doing so. @thatmattlove can you comment?

@thatmattlove
Copy link
Contributor

I'm fairly certain that while this may fix data from IOS devices, it will break data from Aruba CX devices, thus regressing #7041.

I can see a few options forward:

  1. Add some logic to decode_dict() determine if keys need to be decoded in the first place (i.e., checking for escape characters)
  2. Add some logic to napalm() to adjust the decode_keys keyword argument as needed, which would require us to maintain a mapping of device types that need their responses decoded (to my knowledge, it's only the Aruba CX platform).
  3. Add some logic to napalm() to only add the decode_keys keyword argument to get_interfaces()
  4. Submit a PR to napalm-automation-community/napalm-aruba-cx that implements our decode_dict function for get_interfaces, however I'm hesitant to do this since I'm not overly familiar with napalm, nor Aruba CX.

4 is probably the best, 3 is probably the easiest :)

@jeremystretch
Copy link
Member

IMO this looks like a bug in the Arbua NAPALM driver. The below output was cited under #7041:

"Port": {
    "1%2F4%2F1": {
        "interfaces": [
            "1%2F4%2F1"
        ],
        "name": "1/4/1"
    },
    "1%2F4%2F2": {
        "interfaces": [
            "1%2F4%2F2"
        ],
        "name": "1/4/2"
    },

The interface names are URL-escaped for some reason, which I would not expect. I noticed that the slash in some IP addresses gets escaped as well.

We really shouldn't need to do any processing of the NAPALM response data; decode_dict() should not be needed. Unless I'm missing something?

@thatmattlove
Copy link
Contributor

I don't disagree. #7041 was originally opened because of [object Object] being displayed as the config, and this should have been fixed by the changes made here. During testing I went a step further and fixed the key encoding issue. I recall not being able to determine if the issue was with Aruba CX's response, NAPALM core, or the NAPALM driver.

If we don't care about "breaking" Aruba CX NAPALM interface checks, then I have no issue removing decode_dict() from the codebase.

netbox/dcim/api/views.py Outdated Show resolved Hide resolved
Test without decode_dict function

Co-authored-by: Jeremy Stretch <jstretch@ns1.com>
@jeremystretch jeremystretch merged commit 7569544 into netbox-community:develop Dec 21, 2021
@schwos
Copy link

schwos commented Dec 27, 2021

To report the same error experienced here was impacting EOS Napalm driver as well within Netbox; with the modification to line 504 in view.py of the api resolved the error..

@schwos
Copy link

schwos commented Dec 27, 2021

I can also confirm that this did not effect the following drivers and the upgrades made to increase their functionality within the napalm tool and more specifically the get_facts and get_environment calls within Napalm;
napalm_ftos, or
napalm_dell10os

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get Environment Failure
4 participants