napalm-automation / napalm-junos

Apache License 2.0
22 stars 42 forks source link

Incorrect outer key returned for get_bgp_neighbors_detail() #139

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

Verbatim copy from https://github.com/napalm-automation/napalm-base/issues/227

According to the documentation both get_bgp_neighbors() and get_bgp_neighbors_detail() should: "Returns a dictionary of dictionaries. The keys for the first dictionary will be the vrf (global if no vrf)." However different values seem to be returned for the outer keys of each calls. (i.e. one is the vrf the other the routing instance?)

get_bgp_neighbors_detail() appears to return a dictionary of routing instances and not a dictionary containing the VRF as returned in get_bgp_neighbors(); it would seem that the get_bgp_neighbors_detail method should also return 'global' vrf as outer key. (only default/global VRF configured)

neighbor_facts = device.get_bgp_neighbors() print("get_bgp_neighbors():")

for vrf in neighbor_facts: print("Outer key values: " + vrf)

detail_facts = device.get_bgp_neighbors_detail() print("get_bgp_neighbors_detail():")

for vrf in detail_facts: print("Outer key values: " + vrf) Results -

Cisco (ASR9010 - 5.3.3):

get_bgp_neighbors():

Outer key values: global

get_bgp_neighbors_detail():

Outer key values: default

JunOS (15.1F5-S4.6)

get_bgp_neighbors():

Outer key values: global

get_bgp_neighbors_detail():

Outer key values: inet6.0

Outer key values: inet.0

Outer key values: inetflow.0

JunOS (12.3R9.4)

get_bgp_neighbors():

Outer key values: global

get_bgp_neighbors_detail():

Outer key values: inet6.0

Outer key values: inet.0

Outer key values: inetflow.0

In examples above outer dict isn't returning the same value for both calls.

jeffgallagher commented 7 years ago

@mirceaulinic - in #139 you mentioned rolling the routing instances into 'default' - for clarity would that be better labeled with an outer key of 'global' such that the outer key will match the output of get_bgp_neighbors()? To be fair - that might also require a change to the IOSXR code as well since it returns 'global' for the get_bgp_neighbors but 'default' for get_neighbors_detail.

mirceaulinic commented 7 years ago

You are correct @jeffgallagher - this should be fixed also in XR. Thanks for pointing it.

dbarrosop commented 7 years ago

rolling the routing instances into 'default'

I think that's very dangerous and might break a lot of code. It will certainly break code, I know several DC network using routing instances as they don't need VRFs (no MPLS so no need of the extra overhead) to isolate L3 networks so this method will all of a sudden work completely different. I think this require more thinking and adding without changing anything.

mirceaulinic commented 7 years ago

I remember discussing the above with @dbarrosop and the cause of this misunderstanding was my wording. For the record, I referred to grouping inet.0, inet6.0 etc into default, leaving unchanged the behaviour for the rest of the VRFs.

dbarrosop commented 7 years ago

Yes, +1 to your last comment and the solution proposed here.

mirceaulinic commented 7 years ago

Closed via https://github.com/napalm-automation/napalm-junos/commit/a995d5ebce084b126a59d8c88ba93fd0b8f4e2ad