napalm-automation / napalm-base

Apache License 2.0
33 stars 48 forks source link

An option to make napalm extendable #306

Closed itdependsnetworks closed 6 years ago

itdependsnetworks commented 6 years ago

I briefly spoke to @ktbyers about this, and I do not expect this to go in as described here, just wanted to use it as the basis of discussion.

Napalm's ability to work with multiple vendors is amazing, but I run into situations where I simply know my ask does not fit into a vendor neutral model or deals with proprietary one-off protocols. This is made more difficult when using ansible for instance, as I only have access to the defined getters... without modifying and managing that code.

I was looking for a way to extend napalm in a somewhat predictable matter. Here is what I came up with. You would need to have a customnapalm library and Custom class to import anywhere you wanted to use the additional features.

respective driver update:

(napalm-dir) ken@kcelenza-ubuntu:/github/sample/1/napalm-ios$ git diff
diff --git a/napalm_ios/__init__.py b/napalm_ios/__init__.py
index a322d7f..2aba9d5 100644
--- a/napalm_ios/__init__.py
+++ b/napalm_ios/__init__.py
@@ -14,7 +14,10 @@

 """napalm_ios package."""
 import pkg_resources
-from napalm_ios.ios import IOSDriver
+try:
+    from custom_napalm.ios import CustomIOSDriver as IOSDriver
+except:
+    from napalm_ios.ios import IOSDriver

 try:
     __version__ = pkg_resources.get_distribution('napalm-ios').version
diff --git a/napalm_ios/ios.py b/napalm_ios/ios.py
index 2447e73..c027f70 100644
--- a/napalm_ios/ios.py
+++ b/napalm_ios/ios.py
@@ -25,7 +25,10 @@ import telnetlib
 import copy

 from netmiko import ConnectHandler, FileTransfer, InLineTransfer
-from napalm_base.base import NetworkDriver
+try:
+    from custom_napalm.base import CustomNetworkDriver as NetworkDriver
+except ImportError:
+    from napalm_base.base import NetworkDriver
 from napalm_base.exceptions import ReplaceConfigException, MergeConfigException, \
             ConnectionClosedException, CommandErrorException

(napalm-dir) ken@kcelenza-ubuntu:/github/sample/1/napalm-ios$

example extension

(napalm-dir) ken@kcelenza-ubuntu:~/.virtualenvs/napalm-dir/local/lib/python2.7/site-packages/custom_napalm$ ls
base.py  __init__.py  ios.py
(napalm-dir) ken@kcelenza-ubuntu:~/.virtualenvs/napalm-dir/local/lib/python2.7/site-packages/custom_napalm$ cat base.py
from napalm_base.base import NetworkDriver
class CustomNetworkDriver(NetworkDriver):

    def get_eigrp(self):
        raise NotImplementedError

    def get_trustsec(self):
        raise NotImplementedError

(napalm-dir) ken@kcelenza-ubuntu:~/.virtualenvs/napalm-dir/local/lib/python2.7/site-packages/custom_napalm$ cat ios.py

from napalm_ios.ios import IOSDriver
class CustomIOSDriver(IOSDriver):
    """NAPALM Cisco IOS Handler."""

    def get_eigrp(self):
        ntp_servers = {}
        command = 'show run | include ntp server'
        output = self._send_command(command)

        for line in output.splitlines():
            split_line = line.split()
            if "vrf" == split_line[2]:
                ntp_servers[split_line[4]] = {}
            else:
                ntp_servers[split_line[2]] = {}

        return ntp_servers

(napalm-dir) ken@kcelenza-ubuntu:~/.virtualenvs/napalm-dir/local/lib/python2.7/site-packages/custom_napalm$
coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.1%) to 61.141% when pulling 6ca770ee41beae8cb82270c826c4e9b0a4787f9a on itdependsnetworks:napalm_extendable into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

itdependsnetworks commented 6 years ago

An example of this working.

>>> from napalm import get_network_driver
>>>
>>> ios_device='10.1.100.1'
>>> ios_user='ntc'
>>> ios_password='password'
>>> driver = get_network_driver('ios')
>>> device = driver(ios_device, ios_user, ios_password)
>>> device.open()
>>> device.get_eigrp()
{u'128.138.140.44': {}}
>>> device.get_trustsec()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ken/.virtualenvs/napalm-dir/local/lib/python2.7/site-packages/custom_napalm/base.py", line 8, in get_trustsec
    raise NotImplementedError
NotImplementedError

Note: I just copied ntp code over to have the method return something

ktbyers commented 6 years ago

I think the key aspect of this is to make it work with things that are layered on top of NAPAlM (like Ansible and potentially Salt).

What @itdependsnetworks did can probably be simplified. I might try to mock something up over the weekend.

ktbyers commented 6 years ago

@itdependsnetworks Okay, I think you can just do this:

$ cat napalm_ios_custom.py
from napalm_ios.ios import IOSDriver

class CustomIOSDriver(IOSDriver):
    """NAPALM Cisco IOS Handler."""

    def get_eigrp(self):
        ntp_servers = {}
        command = 'show run | include ntp server'
        output = self._send_command(command)

        for line in output.splitlines():
            split_line = line.split()
            if "vrf" == split_line[2]:
                ntp_servers[split_line[4]] = {}
            else:
                ntp_servers[split_line[2]] = {}

        return ntp_servers
$ cat test_x.py 
from napalm_base import get_network_driver
from getpass import getpass

ios_device='cisco1.twb-tech.com'
ios_user='pyclass'
ios_password=getpass()
driver = get_network_driver('ios_custom')
device = driver(ios_device, ios_user, ios_password)
device.open()

print
print "-" * 80
print "EIGRP"
print device.get_eigrp()
print
print "-" * 80
print "ARP Table"
print device.get_arp_table()
print
print "-" * 80
print "Method that doesn't exist"
print device.get_trustsec()
$ python test_x.py 
Password: 

--------------------------------------------------------------------------------
EIGRP
{u'130.126.24.24': {}, u'152.2.21.1': {}}

--------------------------------------------------------------------------------
ARP Table
[{u'interface': u'FastEthernet4', u'ip': u'10.220.88.1', u'mac': u'00:62:EC:29:70:FE', u'age': 8.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.20', u'mac': u'C8:9C:1D:EA:0E:B6', u'age': 0.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.21', u'mac': u'1C:6A:7A:AF:57:6C', u'age': 113.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.29', u'mac': u'52:54:AB:BE:5B:7B', u'age': 138.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.30', u'mac': u'52:54:AB:71:E1:19', u'age': 49.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.32', u'mac': u'52:54:AB:C7:26:AA', u'age': 140.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.33', u'mac': u'52:54:AB:3A:8D:26', u'age': 11.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.37', u'mac': u'00:01:00:FF:00:01', u'age': 156.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.38', u'mac': u'00:02:00:FF:00:01', u'age': 83.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.39', u'mac': u'64:64:9B:E8:08:C8', u'age': 4.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.40', u'mac': u'00:1C:C4:BF:82:6A', u'age': 16.0}, {u'interface': u'FastEthernet4', u'ip': u'10.220.88.41', u'mac': u'00:1B:78:73:56:34', u'age': 13.0}]

--------------------------------------------------------------------------------
Method that doesn't exist
Traceback (most recent call last):
  File "test_x.py", line 22, in <module>
    print device.get_trustsec()
AttributeError: 'CustomIOSDriver' object has no attribute 'get_trustsec'
ktbyers commented 6 years ago

We would then in napalm-ansible just need to make this not be an enumerated choices (i.e. switch it to just be a string:

dev_os=dict(type='str', required=False, choices=os_choices),

I don't know if this applies at all in Salt or StackStorm2 (i.e. what would be need to augment NAPALM in those contexts and what pattern would work).

ktbyers commented 6 years ago

The trick is that what you load here:

driver = get_network_driver('ios_custom')

Most match a Python module named napalm_ios_custom.py that is somewhere on your Python sys.path.

dbarrosop commented 6 years ago

My suggestion would be to just implement the method in napalm-base. The EIGRP example is a straightforward one and I don't think having a get_eigrp method would be terrible. For more vendor specific stuff, for example, qfabric, we could have get_junos_qfabric and for things that are a mixed combination of similar functionality with very different behaviors like chassis we could have a get_chassis and then have some common stuff and then a vendor dictionary inside were we put things that are unique to the platform.

ktbyers commented 6 years ago

@dbarrosop I think the broader issue is that end-users need a way to augment our methods and still have it work with napalm-ansible (without modifying napalm-ansible). I think we should eliminate the choices argument or at least give a way to turn it off (from an Ansible playbook).

dbarrosop commented 6 years ago

Well, what’s the problem then? If someone wants to subclass a driver they are free to do so. We can change the ansible modules to not have hardcoded drivers and just rely on ger_network_drivers to find the correct one. I don’t think we have to change anything else for that.

dbarrosop commented 6 years ago

Ok, reread your last comment and I just realized I said exactly the same as you... sorry, I am very tired today :P

ktbyers commented 6 years ago

No worries, I will make this change in napalm-ansible.

itdependsnetworks commented 6 years ago

This certainly makes sense from napalm perspective, and I don't expect this to be implemented, but I figured I would give it the "old college try." EIGRP is just an example, I have dozens of these that I have to figure out on short notice based on customer requirements. My current solution is to use other solutions, e.g. ntc-templates, custom filters, etc.... However I would love the ability to use the same napalm methodology for these cases.

dbarrosop commented 6 years ago

I think the right thing to do is to remove the restrictions in the ansible modules. That way you can have your own subdrivers inheriting from the main drivers and upstream changes at your own pace without being blocked by us. Would that work? Am I even managing to get my point across?

itdependsnetworks commented 6 years ago

Everything you said makes sense.

It's difficult when managing multiple environments/customers. They will not be able to upgrade without having to carry code over, which most likely they would not understand.

dbarrosop commented 6 years ago

A way to do that is by having your own repo as entry point. So instead of installing napalm they install ntc-napalm which basically has napalm as requirement and include your own subclasses

itdependsnetworks commented 6 years ago

Understood, though that comes with it's own set of complexity.

Thanks for your consideration :)

dbarrosop commented 6 years ago

I would suggest revisiting this after the reunification as get_network_drivers will certainly change then so we might be able to do something there.

itdependsnetworks commented 6 years ago

I'll just submit the same idea with a new marketing spin and hope to sneak it in :)

ktbyers commented 6 years ago

I had some further discussions with @itdependsnetworks on this as I wasn't seeing how we weren't meeting his requirements.

He wants something like the following:

Look for custom_napalm_ios.py first and if that exists use that.
If that doesn't exist fall back to napalm_ios.py.

Basically try to use the custom module first per platform and then fallback to the standard module if the custom module doesn't exist.

dbarrosop commented 6 years ago

Yeah, I realized about it when checking his proposed code, that's why I prefer to revisit the idea after the reunification, get_network_drivers will change dramatically by then so I'd rather look at this then. What I don't like is that we are conflating business logic with module discovery, the whole purpose of get_network_drivers being dynamic was for people to implement their own drivers but then I was expecting people to call custom modules with custom names like ntc_ios or my_driver_icant_publish_because_legal rather than just ios and rely on some customer order. As mentioned, let's revisit after the reunification.

@itdependsnetworks add some buzzwords like "intent based" or "software defined class discovery" and we will be all in ;)