-
Notifications
You must be signed in to change notification settings - Fork 223
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_cli_config_gen): Add support for ip name server groups #4763
base: devel
Are you sure you want to change the base?
Feat(eos_cli_config_gen): Add support for ip name server groups #4763
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-4763
# Activate the virtual environment
source test-avd-pr-4763/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@feat/eos_cli/ip-name-server-group#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,feat/eos_cli/ip-name-server-group --force
# Optional: Install AVD examples
cd test-avd-pr-4763
ansible-playbook arista.avd.install_examples |
13bff48
to
52d9f5a
Compare
@@ -5,7 +5,7 @@ | |||
# Line above is used by RedHat's YAML Schema vscode extension | |||
# Use Ctrl + Space to get suggestions for every field. Autocomplete will pop up after typing 2 letters. | |||
type: dict | |||
keys: | |||
$defs: |
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.
add default: default
on the vrf to document what you are doing in the templates
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.
Removed this defs and added all the keys in ip_name_server_groups
schema with default vrf default
.
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.
I guess the reason for this (==removing the defs) was because we are not currently injecting default
for ip name-servers
?
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.
Yes.
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.
ok thanks
seems that default: default
would make sense for the other one as well:
site3-leaf1(config-if-Et4)#ip name-server 1.1.1.1
site3-leaf1(config)#show run se name-ser
ip name-server vrf MGMT 10.14.0.1
ip name-server vrf MGMT 192.168.17.1
ip name-server vrf default 1.1.1.1
but that would be a breaking change in terms of what we produce as the current template does not inject VRF default. so we can revisit this later.
Can you please open an issue to track this?
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.
3e14cbf
to
ae1ef81
Compare
python-avd/pyavd/_eos_cli_config_gen/j2templates/eos/ip-name-server-groups.j2
Outdated
Show resolved
Hide resolved
...s/arista/avd/molecule/eos_cli_config_gen/inventory/host_vars/host1/ip_name_server_groups.yml
Outdated
Show resolved
Hide resolved
fd9d66f
to
37daa62
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
3fa7b5e
to
69ae148
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
69ae148
to
f555179
Compare
python-avd/pyavd/_eos_cli_config_gen/j2templates/eos/ip-name-server-groups.j2
Show resolved
Hide resolved
ansible_collections/arista/avd/molecule/eos_cli_config_gen/intended/configs/host1.cfg
Outdated
Show resolved
Hide resolved
Use of this source code is governed by the Apache License 2.0 | ||
that can be found in the LICENSE file. | ||
#} | ||
{% for name_server_group in ip_name_server_groups | arista.avd.natural_sort("name") %} |
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.
please add a documentation templates for ip name-server groups
listing the servers inside
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.
Added.
29f42a2
to
cabcd45
Compare
name_servers: | ||
- ip_address: 8.8.8.8 | ||
vrf: vrf1 | ||
priority: 0 |
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.
please remove the test for priority 0
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.
Done.
keys: | ||
ip_address: | ||
type: str | ||
required: true |
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.
since we have ip_address as primary key we may not need required as true
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.
also we can have the same ip address in different vrf.
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.
Removed primary_key
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.
I don't think we are allowed to keep required yet, please check once.
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.
Also, you can keep ip_address
as primary key and use allow_duplicate in order to use same ip address.
@@ -0,0 +1,33 @@ | |||
--- |
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.
rename the file to ip-name-server-groups
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.
done.
priority: 3 | ||
|
||
- name: mynameserver0 | ||
ip_domain_list: domain-list1 |
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.
you can improve the coverage by not defining ip_domain_list and dns_domain here. Add one more name server when just define dns_domain and do not define name_servers.
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.
Added.
| IP Address | VRF | Priority | | ||
| ----------- | --- | -------- | |
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.
| IP Address | VRF | Priority | | |
| ----------- | --- | -------- | | |
| IP Address | VRF | Priority | | |
| ---------- | --- | -------- | |
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.
Fixed
f773c16
to
706180d
Compare
706180d
to
36830c4
Compare
keys: | ||
ip_address: | ||
type: str | ||
required: true |
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.
I don't think we are allowed to keep required yet, please check once.
keys: | ||
ip_address: | ||
type: str | ||
required: true |
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.
Also, you can keep ip_address
as primary key and use allow_duplicate in order to use same ip address.
{% do all_server_groups.append(server) %} | ||
{% endfor %} | ||
{% for server in all_server_groups | arista.avd.natural_sort("ip_address") | arista.avd.natural_sort("vrf") | arista.avd.natural_sort("priority") %} | ||
{% if server.priority_defined %} |
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.
No need of priority_defined
you can just check if priority != 0
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.
Fixed.
dns domain arista.avd.com | ||
ip domain-list domain-list1 |
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.
Can we give different values for dns domain
and ip domain-list
across list.
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.
done.
@@ -416,6 +416,27 @@ monitor server radius | |||
probe threshold failure 100 | |||
probe method access-request username arista password 7 141600021F102B | |||
! | |||
ip name-server group mynameserver0 |
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.
Have we verified config on cvp?
for more information, see https://pre-commit.ci
6209a56
to
172a66d
Compare
Change Summary
Add support for ip name server groups
Related Issue(s)
Fixes #4698
Component(s) name
arista.avd.eos_cli_config_gen
Proposed changes
There is no input variable for applying the ip name-server group to monitor connectivity via eos_cli_config_gen. Below contains the use cause example.
ip name-server group {name_server_group}
name-server vrf {name_server_vrf} {IP}
monitor connectivity
vrf {name_server_vrf}
interface set {set_name} {loopback_name}
description {description}
!
host {host_name}
url {url}
name-server group {name_server_group}
How to test
Checklist
User Checklist
Repository Checklist