Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

interfaces_ip primary IP check #206

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

interfaces_ip primary IP check #206

wants to merge 3 commits into from

Conversation

itdependsnetworks
Copy link

For IPv4 addresses, look to add check for primary IP. This method should be backwards compatible with current implementation.

For IPv4 addresses, look to add check for primary IP. This method should be backwards compatible with current implementation.
'primary_ip': True
},
u'10.66.50.1': {
'prefix_length': 24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the primary key here as well, please

mirceaulinic
mirceaulinic approved these changes Feb 12, 2017
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the field in the test models & both old and new testing framework.

@mirceaulinic
Copy link
Member

mirceaulinic commented Feb 12, 2017

As this is a API change, we'll need to apply the changes for the following drivers:

  • eos
  • ios
  • iosxr
  • junos
  • nxos
  • ros
  • vyos

So it might take a while to introduce this.
@itdependsnetworks if you can give a helping hand with this, it would be much appreciated.

@dbarrosop
Copy link
Member

Could you create a label and tag all the issues/PRs related to this with it so it's easier to track with zenhub?

Thanks!

@mirceaulinic
Copy link
Member

Yeah.. lemme install it first and play :)

@mirceaulinic
Copy link
Member

Hi @itdependsnetworks - would you be able to provide an implementation for one or more drivers? Here you can see the issues linked to the parent: #207. Thanks!

@itdependsnetworks
Copy link
Author

itdependsnetworks commented Mar 8, 2017

Apologies for not getting back. Yea, Should be able to on ios, nxos, junos, and eos. Give me a week or two.

@narJH27
Copy link
Contributor

narJH27 commented Apr 7, 2017

@itdependsnetworks @mirceaulinic Any update on this enhancement wrt updating the individual drivers?

@itdependsnetworks
Copy link
Author

Been crazy busy, will try to get to this on the weekend... But I have said that before :(

@narJH27
Copy link
Contributor

narJH27 commented Apr 7, 2017

@itdependsnetworks Sounds good! In case you are unable to get some of it done by early next week, let me know and maybe I can help out.

@narJH27 narJH27 mentioned this pull request Apr 11, 2017
@narJH27
Copy link
Contributor

narJH27 commented Apr 11, 2017

@mirceaulinic @itdependsnetworks I have implemented the changes for napalm_base, eos, and iosxr. Pending merge to base, for the other PR's to pass the checks.

},
u'2001:DB8:1::1': {
'prefix_length': 64
'prefix_length': 64,
'primary_ip': True
Copy link
Member

@dbarrosop dbarrosop Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no concept of primary or secondary ip on IPv6 so I'd hardcode this to True for simplicity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so even when a router needs to source an IP, how does it choose which one?

Does it make sense not to include on IPv6 at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a whole RFC for that (surprise!): https://tools.ietf.org/html/rfc6724
Actually, IPv4 doesn't have the concept at all either. AFAICT only Cisco has the "secondary" keyword. Juniper certainly doesn't.

Does it make sense not to include on IPv6 at all?

You are probably right. You will have to slightly adapt the test so it uses two different models; one for IPv4 and one for IPv6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants