-
Notifications
You must be signed in to change notification settings - Fork 184
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_validate_state): Added the validation for STUN client configurations #3898
base: devel
Are you sure you want to change the base?
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
ansible_collections/arista/avd/roles/eos_validate_state/ANTA-Preview.md
Outdated
Show resolved
Hide resolved
@@ -151,6 +151,9 @@ title: Ansible Collection Role eos_validate_state - Preview Integration with ANT | |||
- (New) AvdTestAPIHttpsSSL (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- VerifyAPIHttpsSSL: Validate eAPI HTTPS SSL profile status. | |||
|
|||
- (New) AvdTestStun (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- AvdTestStun: Validate STUN clients source address and port. |
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 add more details about the test here? Since it's related to WAN devices, we should mention it somehow.
|
||
class AvdTestStun(AvdTestBase): | ||
""" | ||
AvdTestStun class for STUN tests. |
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 know this follows the other test classes, but I think we should give more details here, especially since it's related to WAN devices.
|
||
# Check if there are any path groups with STUN configuration | ||
if (path_groups := get(self.structured_config, "router_path_selection.path_groups")) is None: | ||
LOGGER.info("No local interface found with STUN configuration. %s is skipped.", self.__class__.__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.
We are checking for path_groups but the log says local interface? Shouldn't we loop over the path_groups for local interfaces instead? If not the log message is misleading.
stun_interfaces = [ | ||
local_interfaces.get("name") | ||
for path_group in path_groups | ||
for local_interfaces in path_group.get("local_interfaces") | ||
if get(local_interfaces, "stun.server_profiles") is not None | ||
] |
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.
Might be helpful to take a look at the mixin classes under plugin_utils/eos_validate_state_utils
. There are validations methods you can use to make sure the data is present and logs accordingly.
interfaces_data = get(self.structured_config, "ethernet_interfaces") | ||
interface_address = get_item(interfaces_data, "name", source_interface) | ||
ip_address = interface_address.get("ip_address") |
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.
Same thing here. I think you could leverage the get_interface_ip
method of the mixin class.
LOGGER.info("No IP address found for interface %s. Skipping this interface.", source_interface) | ||
continue | ||
source_address = ip_address.split("/")[0] | ||
source_port = 4500 # TODO: Keeping source port as default 4500. We might need to change it later. |
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.
Is there a source_port key in the structured config?
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 not right now hence keeping it 4500 which we will change once it is supported in structured config
{ | ||
"VerifyStunClient": { | ||
"stun_clients": [{"source_address": source_address, "source_port": source_port}], | ||
"result_overwrite": {"custom_field": f"source address: {source_address} source port: {source_port}"}, |
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.
"result_overwrite": {"custom_field": f"source address: {source_address} source port: {source_port}"}, | |
"result_overwrite": {"custom_field": f"Source IPv4 Address: {source_address} Source Port: {source_port}"}, |
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.
Could this be IPv6? If so you can adjust my suggestion.
) | ||
|
||
# Return the ANTA tests as a dictionary | ||
return {self.anta_module: anta_tests} |
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.
Could anta_tests
be an empty list? If so we need to return None
instead like the other tests.
@@ -151,6 +151,9 @@ title: Ansible Collection Role eos_validate_state - Preview Integration with ANT | |||
- (New) AvdTestAPIHttpsSSL (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- VerifyAPIHttpsSSL: Validate eAPI HTTPS SSL profile status. | |||
|
|||
- (New) AvdTestStun (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- VerifyStunClient: Validate the STUN client is configured with the specified IPv4 source address and port. Validate the public IP and port if provided. For now AVD use source port as static 4500 and do not validate public IP and port. Please add the test in custom anta catalogs to validate public ip and port. |
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.
- VerifyStunClient: Validate the STUN client is configured with the specified IPv4 source address and port. Validate the public IP and port if provided. For now AVD use source port as static 4500 and do not validate public IP and port. Please add the test in custom anta catalogs to validate public ip and port. | |
- VerifyStunClient: Validate the STUN client is configured with the specified IPv4 source address and port. Validate the public IP and port if provided. For now AVD sets the source port to 4500 and does not validate public IP and port. Please add the test in a custom ANTA catalog to validate public IP and port. |
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.
Remove the source port 4500 for now. Add stuff about the fact that we do STUN check only for WAN case for now (because we look for path-groups)
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py
Show resolved
Hide resolved
LOGGER.info("No local interface found with STUN configuration. %s is skipped.", self.__class__.__name__) | ||
return None | ||
|
||
# Generate the ANTA tests for each interface with STUN configuration |
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.
# Generate the ANTA tests for each interface with STUN configuration | |
# Generate the ANTA tests for each identified local interface. |
for source_interface in stun_interfaces: | ||
ip_address = self.get_interface_ip("ethernet_interfaces", source_interface) | ||
source_address = ip_address.split("/")[0] | ||
source_port = 4500 # TODO: Keeping source port as default 4500. We might need to change it later. |
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.
Let's remove this - there is no way to configure source port in EOS today it seems (I know ANTA test can do it but for now we can't :) )
class AvdTestStun(AvdTestBase): | ||
""" | ||
AvdTestStun class for STUN tests. | ||
Validates the configuration of the STUN client, focusing specifically on the IPv4 source address and port, |
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.
Is it really the configuration that is validated or the state?
Maybe something like (and copy/adjust in doc)
Validates the configuration of the STUN client, focusing specifically on the IPv4 source address and port, | |
Validates the presence of a STUN client translation for a given source IPv4 address and port for WAN scenarios. The list of expected translations for each device is built searching through router_path_selection.path_groups.local_interfaces |
@@ -151,6 +151,9 @@ title: Ansible Collection Role eos_validate_state - Preview Integration with ANT | |||
- (New) AvdTestAPIHttpsSSL (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- VerifyAPIHttpsSSL: Validate eAPI HTTPS SSL profile status. | |||
|
|||
- (New) AvdTestStun (No Ansible tags, use the new `skipped_tests` variable instead) | |||
- VerifyStunClient: Validate the STUN client is configured with the specified IPv4 source address and port exclusively for WAN scenarios. Additionally, validate the public IP and port if provided. Please add the test in a custom ANTA catalog to validate public IP and port. |
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.
1 lets remove the part where we suggest custom ANTA catatlog for public IP and port. AVD does not offer this capability to be derived from the structured config
- update using the suggestion I made in the test docstring. What the test is really checking is that there is an entry in the STUN client translations for the given local interfaces in each path-group.
if self.validate_data(data=local_interfaces, data_path=f"router_path_selection.path_groups.[{group_idx}]", required_keys="stun.server_profiles") | ||
] |
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 log is missing which index from local_interfaces is missing:
<dc1-leaf1a> Key 'router_path_selection.path_groups.[0].local_interfaces' is missing. AvdTestStun is skipped.
<dc1-leaf1a> Key 'router_path_selection.path_groups.[1].stun.server_profiles' is missing. AvdTestStun is skipped.
<dc1-leaf1a> No local interface found with STUN configuration. AvdTestStun is skipped.
It should be:
<dc1-leaf1a> Key 'router_path_selection.path_groups.[0].local_interfaces' is missing. AvdTestStun is skipped.
<dc1-leaf1a> Key 'router_path_selection.path_groups.[1].local_interfaces[0].stun.server_profiles' is missing. AvdTestStun is skipped.
<dc1-leaf1a> No local interface found with STUN configuration. AvdTestStun is skipped.
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
ip_address = self.get_interface_ip("ethernet_interfaces", source_interface) | ||
source_address = ip_address.split("/")[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.
get_interface_ip
could return None
so you need to guard this.
ip_address = self.get_interface_ip("ethernet_interfaces", source_interface) | |
source_address = ip_address.split("/")[0] | |
if (ip_address := self.get_interface_ip("ethernet_interfaces", source_interface)) is None: | |
continue | |
source_address = str(ip_interface(ip_address).ip) |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Quality Gate passedIssues Measures |
Change Summary
Added the validation for STUN client configuration.
Related Issue(s)
Fixes #3889
Component(s) name
arista.avd.eos_validate_state
Proposed changes
We have the ANTA testcase ready for validate the STUN client configuration hence added the support in AVD. Right now it will collect the all the local interfaces from path group where stun server profiles are attached. Source port is 4500(default) for now.
How to test
Run the molecule or test it on lab with eos_validate_state in playbook.
Note: Need ANTA==0.14.0 to test this.
Checklist
User Checklist
Repository Checklist