nornir-automation / nornir_napalm

NAPALM's plugins for nornir
Apache License 2.0
63 stars 21 forks source link

Automatic "get_" prefix in napalm_get.py causes issues with NAPALM Getters #83

Open miguletztim opened 1 week ago

miguletztim commented 1 week ago

The line in nornir_napalm/plugins/tasks/napalm_get.py at Line 42:

getter = g if g.startswith("get_") else "get_{}".format(g)

is causing issues when trying to retrieve data using custom getters that do not follow the "get_" prefix convention.

Problem:

Currently, the code forces any custom getter to follow the "get_" prefix format. If the provided getter name does not start with "get_", the code prepends "get_" to the getter name, which can lead to incorrect or failed data retrieval. This behavior is restrictive and does not account for the possibility of users defining or using getters that do not follow this naming convention.

Example Scenario:

If a custom getter named traceroute (without "get_" prefix) is passed to the task, the current logic will transform it to get_traceroute, causing the task to fail if this method does not exist.

Expected Behavior:

The code should not enforcing the "get_" prefix, or provide a clear option to bypass this transformation when needed.

Proposed Solution:

Modify the code to only use the getter name as provided. The logic could be simplified as:

getter = g

This means users must explicitly pass "get_interfaces", "get_bgpneighbors", or any other standard getter prefixed with "get", instead of relying on the function to automatically add it. This would provide clearer control and avoid conflicts.

For reference on standard NAPALM getters, see the Supported NAPALM Getters.

Steps to Reproduce:

  1. Use a custom getter name without a "get_" prefix.
  2. Run the task using nornir_napalm.
  3. Observe the failure or incorrect getter name being used.

Environment:

miguletztim commented 1 week ago

I’ve implemented a fix for this issue, but it appears I lack the permission to push the code as a pull request.

ktbyers commented 1 week ago

Probably the simplest solution is just to make a nornir custom task plugin and use that to call the "getters" that you want to call. You can just use the existing nornir_napalm task-plugin as reference and then modify it to meet your needs.

I am reluctant to change this as this "get_" naming convention is a fairly strong convention in napalm (i.e. napalm-ansible would behave the same way IIRC).

ktbyers commented 6 days ago

Let me know if the proposed workaround is viable for you (or if not, why not).

The proposed fix is not really viable as it breaks existing code. Of course, we could do something like add an additional argument, and default it to the current behavior, but I think the above workaround should work.

dbarrosop commented 6 days ago

A traceroute isn't a getter, it is a command that performs an action. This should be implemented via a custom task the same way we have one for ping. That way the parameters required by it are properly specified and documented.

The reason why the getters where done in the current way was because there are too many and in the way majority of cases there is no need for arguments but that is not true for actions like ping or traceroute