[Bug]: Seeing double entries on sync_diff() and order of negation reversed

superloopnetwork commented 1 year ago

Contact Details

wailit@gmail.com

What happened?

It looks like the output on sync_diff() sometimes produces duplicate entries for the parent. At the same time, I noticed the negation sequence is reversed. Let me demonstrate... Code below to reproduce:

from ciscoconfparse import CiscoConfParse
config = [
    "vlan 1",
    "vlan 2",
    " name management",
    ]
p = CiscoConfParse(config=config)
required_lines = [
    "vlan 2",
    " name finance",
    "vlan 3",
    " name management",
    "vlan 4",
    " name IT",
    ]

diffs = p.sync_diff(required_lines,'', '.+')  
diffs

Result:

>>> diffs
['no vlan 1', 'vlan 2', ' name finance', 'vlan 2', ' no name management', 'vlan 3', 'vlan 3', ' name management', 'vlan 4', 'vlan 4', ' name IT']

Noticed the duplicate vlan3 and vlan4 entries? In addition, the negation sequence should be reversed... Look at vlan 2; it corrects the name to 'finance' and then negates 'management'. In the actual IOS/NXOS, the name would be overwritten first with 'finance', and then when it goes to negate 'management', it wouldn't exist (hence, throwing an error in the IOS). If the order of operation was reversed, (negate management, then re-create finance), it would then work correctly. Is this a setting that I'm missing?

CiscoConfParse Version

1.7.15

What Operating System are you using?

Linux - Debian, Ubuntu, CentOS, RHEL or others

What Python version(s) have this problem?

Python 3.8

Show us how to reproduce the problem. Please tell us if the problem is specific to certain inputs or situations.

from ciscoconfparse import CiscoConfParse
config = [
    "vlan 1",
    "vlan 2",
    " name management",
    ]
p = CiscoConfParse(config=config)
required_lines = [
    "vlan 2",
    " name finance",
    "vlan 3",
    " name management",
    "vlan 4",
    " name IT",
    ]

diffs = p.sync_diff(required_lines,'', '.+')  
diffs

Python tracebacks

No traceback, just questionable result/output.

['no vlan 1', 'vlan 2', ' name fianance', 'vlan 2', ' no name management', 'vlan 3', 'vlan 3', ' name management', 'vlan 4', 'vlan 4', ' name IT']

Relevant log output

N/A

Code of Conduct

[X] I agree to follow this project's Code of Conduct

superloopnetwork commented 1 year ago

Interesting... Upgrading to the latest version of ciscoconfparse (1.7.24) produces this result:

>>> from ciscoconfparse import CiscoConfParse
>>> config = [
...     "vlan 1",
...     "vlan 2",
...     " name management",
...     ]
>>> p = CiscoConfParse(config=config)
>>> required_lines = [
...     "vlan 2",
...     " name fianance",
...     "vlan 3",
...     " name management",
...     "vlan 4",
...     " name IT",
...     ]
>>> 
>>> diffs = p.sync_diff(required_lines,'', '.+')  
>>> diffs
['no vlan 1', 'vlan 2', ' name finance', ' no name management', 'vlan 3', 'vlan 3', ' name management', 'vlan 4', 'vlan 4', ' name IT']

The double no longer shows on vlan 2 and the negation is still placed after the overwrite of 'name finance'. We still see a double on vlan 3 and vlan 4 parent..

Trying other configs:

>>> config = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/5
...  switchport access vlan 30
...  switchport mode access
...  switchport nonegotiate
... !
... interface GigabitEthernet1/0/6
...  switchport access vlan 30
...  switchport mode access
... !""".splitlines()
>>> 
>>> required_lines = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/6
...  switchport access vlan 30
...  switchport mode trunk
... !""".splitlines()
>>> p = CiscoConfParse(config=config)
>>> diffs = p.sync_diff(required_lines,'', '.+')  
>>> diffs
['no interface GigabitEthernet1/0/5', ' no switchport access vlan 30', ' no switchport mode access', ' no switchport nonegotiate', 'interface GigabitEthernet1/0/6', ' switchport mode trunk', ' no switchport mode access']

The negation is again in reverse order for interface GigabitEthernet1/0/6 but no duplicates on the parent... It seems like sometimes I'm able to reproduce the duplicates depending on the configs and sometime not. I can't figure out the 'pattern' or why it's doing that.

Here is one that again, has the duplicate of the parent of GigabitEthernet1/0/6:

>>> from ciscoconfparse import CiscoConfParse
>>> config = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/5
...  switchport access vlan 30
...  switchport mode access
...  switchport nonegotiate
... !""".splitlines()
>>> 
>>> required_lines = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/6
...  switchport access vlan 30
...  switchport mode trunk
... !""".splitlines()
>>> p = CiscoConfParse(config=config)
>>> diffs = p.sync_diff(required_lines,'', '.+')  
>>> diffs
['no interface GigabitEthernet1/0/5', ' no switchport access vlan 30', ' no switchport mode access', ' no switchport nonegotiate', 'interface GigabitEthernet1/0/6', 'interface GigabitEthernet1/0/6', ' switchport mode trunk', 'interface GigabitEthernet1/0/6', ' switchport access vlan 30']

And in this example, it seems like it doesn't know that if we negate the parent, all configs below are gone. Hence, no need to negate the child one by one...

>>> from ciscoconfparse import CiscoConfParse
>>> config = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/5
...  switchport access vlan 30
...  switchport mode access
...  switchport nonegotiate
...  channel-protocol lacp
...  channel-group 6 mode active
...  spanning-tree bpduguard enable
... !""".splitlines()
>>> 
>>> required_lines = """interface GigabitEthernet1/0/4
...  switchport access vlan 40
...  switchport mode access
...  spanning-tree portfast
... !
... interface GigabitEthernet1/0/6
...  switchport access vlan 30
...  switchport mode trunk
... !""".splitlines()
>>> p = CiscoConfParse(config=config)
>>> diffs = p.sync_diff(required_lines,'', '.+')  
>>> diffs
['no interface GigabitEthernet1/0/5', ' no switchport access vlan 30', ' no switchport mode access', ' no switchport nonegotiate', ' no channel-protocol lacp', ' no channel-group 6 mode active', ' no spanning-tree bpduguard enable', 'interface GigabitEthernet1/0/6', 'interface GigabitEthernet1/0/6', ' switchport mode trunk', 'interface GigabitEthernet1/0/6', ' switchport access vlan 30']

The above examples may be a very complicated problem to solve due to the variations of cases...

mpenning commented 1 year ago

Thank you for taking time to document and file this issue... I don't know how long it will take to fix this, but I will concede that excellent IOS diffs are perhaps the thorniest problem I have tried to solve. I have a bit of hope that Google's diff_match_patch library will make things easier than other options I've tried.

That said, it's probably going to be a while before I work on this TBH.

superloopnetwork commented 11 months ago

In other words, there will no diff feature/support going forward in 2.0, correct?

On Mon, Oct 30, 2023, 7:18 PM Mike Pennington @.***> wrote:

FYI, I want to sunset all diff support in 1.9.x; by the time we get to 2.0, complex / ill-conceived features diffs this should be removed.

— Reply to this email directly, view it on GitHub https://github.com/mpenning/ciscoconfparse/issues/280#issuecomment-1786193754, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRLKQLTC3K25DPRMXJOQT3YCAYWBAVCNFSM6AAAAAAZASIRH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGE4TGNZVGQ . You are receiving this because you authored the thread.Message ID: @.***>

mpenning commented 11 months ago

In other words, there will no diff feature/support going forward in 2.0,

Or someone else fixes this... Internally, Cisco has a working diff implementation that comes with IOS. So, diffs are doable, but I am apparently not doing it right and the existing CiscoConfParse HDiff() code is kindof ugly.

If nobody fixes it by the time I get to 2.0, I'll remove the feature... and someone can still implement diffs as a separate package. If so, these (and perhaps HDiff()) could give someone ideas...

netdevops/hier_config (this library looks like a pretty good, there are missing dependencies: such as vrf and ip address)
robphoenix/diffios

superloopnetwork commented 11 months ago

No worries, Mike. I greatly appreciate all your time and effort that you have put into this library. Can't express how thankful I am for your work - Thank you, Sir.

On Mon, Oct 30, 2023 at 8:25 PM Mike Pennington @.***> wrote:

In other words, there will no diff feature/support going forward in 2.0,

Or someone else fixes this... Internally, Cisco has a working diff implementation that comes with IOS. So, diffs are doable, but I am apparently not doing it right and the existing CiscoConfParse HDiff() code https://github.com/mpenning/ciscoconfparse/blob/aefd5481a3113c4a31791191c2c3e91e9ff1f4db/ciscoconfparse/ciscoconfparse.py#L3619 is kindof ugly.

If nobody fixes it by the time I get to 2.0, I'll remove the feature... and someone can still implement diffs as a separate package. If so, this (and perhaps HDiff()) could give someone ideas...

https://github.com/robphoenix/diffios

— Reply to this email directly, view it on GitHub https://github.com/mpenning/ciscoconfparse/issues/280#issuecomment-1786248073, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRLKQM6HU76FYJ65JGNKCTYCBAONAVCNFSM6AAAAAAZASIRH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGI2DQMBXGM . You are receiving this because you authored the thread.Message ID: @.***>

mpenning commented 10 months ago

This should be fixed in version 1.9.40. @superloopnetwork please confirm.

Example usage of the new (and fixed) Diff() object...

from ciscoconfparse.ciscoconfparse import Diff

old_config = [
    "vlan 1",
    "vlan 2",
    " name management",
]
new_config = [
    "vlan 2",
    " name finance",
    "vlan 3",
    " name management",
    "vlan 4",
    " name IT",
]

diff = Diff(old_config=old_config, new_config=new_config)
for line in diff.diff():
    print(line)

This prints:

no vlan 1
vlan 2
  name finance
vlan 3
  name management
vlan 4
  name IT

superloopnetwork commented 10 months ago

Hey @mpenning. The output is 100% correct to remediate. I notice the diff object is now different than the old - It looks like there is more intelligence built in... The old diff would produce 'vlan 2' followed by the negation of 'name management' and then going into 'vlan 2' again to apply 'name finance'. Something like this if I'm not mistaken:

vlan 2
  no name management
vlan 2
  name finance

Is the new diff object intelligent to know that an overwrite of 'name management' is enough to make the correct remediation? In other words, going back to your example that you once had (http://pennington.net/tutorial/ciscoconfparse/ccp_tutorial.html#slide14).

>>> print '\n'.join(parse.sync_diff(REQUIRED_CONFIG, ''))
interface GigabitEthernet0/1
 no ip address 10.0.0.1 255.255.255.0
interface GigabitEthernet0/1
 ip address 172.16.1.1 255.255.255.0
 no ip proxy-arp

Notice that it has to jump back into the 'Interface GigabitEthernet0/1' config to apply the new ip address.... Does the new diff object address this scenario?

Another thought that came to mind; I was playing around with diffios library (the one you mentioned above) and noticed that anything that is past 1 level deep in config will cause issues as it's not able to pick up the sub-parent. If we refer to the below example:

switch(config)# router bgp 64496
switch(config-router)# address-family ipv4 unicast
switch(config-router-af)# network 192.0.2.0

It wouldn't be able to pick up 'address-family ipv4 unicast' as part of the list if there was a diff for 'network 192.0.2.0'. The remediation would not include 'address-family ipv4 unicast' but would include 'network 192.0.2.0'. So the remediation look something like this for diffios:

['router bgp 64494', 'network 192.0.2.0']

Skipping the 'address-family ipv4 unicast' sub-parent config (not sure if i'm making any sense here...?).

Not a big deal as I don't have much configs in Cisco that are more than 1 level deep but I'm curious to know if your diff object that you created is able to overcome N levels deep?

As a side question (since I cannot find your contact email to ask), Is there a way for HDiff() output to be a more focused diff? For example, I have a huge config to diff (1000 lines). However, my actual diff is only 2 lines (+ and a -)... The output is the entire 1000 lines config plus the diffs. Any way to reduce the number of lines in the output so it's just focused on the actual diffs? (again, not a big deal if it can't but I couldn't find any flags in your documentation that would turn such feature on...)

Thanks for your help!

mpenning commented 8 months ago

FYI I'm going to close this out. I believe all your needs are met with the ciscoconfparse2 diff API. You can find the documentation for it here.

mpenning / ciscoconfparse