-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat(eos_designs): Allow reuse of cross-device BGP peer ip #4050
Feat(eos_designs): Allow reuse of cross-device BGP peer ip #4050
Conversation
Review docs on Read the Docs To test this pull request: # Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-4050
# Activate the virtual environment
source test-avd-pr-4050/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@bgp_peer_ip_use_validation#subdirectory=python-avd" --force
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,bgp_peer_ip_use_validation --force
# Optional: Install AVD examples
cd test-avd-pr-4050
ansible-playbook arista.avd.install_examples |
Feedback - it should be "python3" instead of "python" |
I've tested it and it does not warn anymore for when we have repeated IPs across devices. It builds correctly.
That's IP reuse on the same VRF and on the same device, not sure that's a valid config that we should warn or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code now allows duplicates in general and does not warn.
While this does address the problem of duplicates across devices, it is blindly allowing duplicates on the same device, and when that happens, it will create a single entry in the resulting config.
If the duplicates have different data, I believe only the lastly defined will be considered.
IMHO duplicates on the same device and same VRF are an error, likely due to a typo or copy/paste error... @ClausHolbechArista what do you think?
This code should not allow duplicates. Have you tested? Your example above had the same exact info twice. Such duplication is silently ignored. Only one peering will be added to the structured config. Please confirm. |
Yes I did test. Indeed it was an exact duplicate, to simulate a typo on a situation where we have 2 peers that are basically identical except for the IP. Do you mean that if they are not exact duplicates they won't be silently ignored? Still, shouldn't any duplicate be warned about? |
Correct
No, since you may have situations where you define the VRF in multiple tenants, and they get merged, possibly leading to the same peers being repeated (it is a very complicated corner case). There is no reason to error out when there is no conflict. |
ansible_collections/arista/avd/roles/eos_designs/python_modules/network_services/router_bgp.py
Show resolved
Hide resolved
4763dfa
to
26e45f0
Compare
26e45f0
to
1f00d31
Compare
Requested changes has been implemented.
|
Change Summary
Currently if a given BGP peer IP is used on device A and reused on device B, validation fails. However, although not a recommended design, this is a valid config. This PR will relax the BGP peer ip reuse.
Related Issue(s)
Fixes #3914
Component(s) name
arista.avd.eos_designs
Proposed changes
Today, if a given bgp peer IP is used on device A and reused on device B, validation fails.
However, although not a recommended design, this is a valid config.
Currently, validation has to be run set to 'warning' and the user just ignores those messages increasing margin for errors.
Allow IP reuse for BGP peerings across different devices.
How to test
Checklist
User Checklist
Repository Checklist