Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat(eos_validate_state): Added the validation for STUN client configurations #3898

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

Feat(eos_validate_state): Added the validation for STUN client configurations #3898

wants to merge 4 commits into from

Conversation

MaheshGSLAB
Copy link
Contributor

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.

anta.tests.stun:
- VerifyStunClient:
    result_overwrite:
      custom_field: 'source address: 172.18.3.2 source port: 4500'
    stun_clients:
    - source_address: 172.18.3.2
      source_port: 4500
- VerifyStunClient:
    result_overwrite:
      custom_field: 'source address: 100.64.3.2 source port: 4500'
    stun_clients:
    - source_address: 100.64.3.2
      source_port: 4500

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

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@github-actions github-actions bot added role: eos_validate_state state: CI Updated CI scenario have been updated in the PR labels Apr 24, 2024
@github-actions github-actions bot added the state: conflict PR with conflict label Apr 24, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@MaheshGSLAB MaheshGSLAB marked this pull request as ready for review April 25, 2024 07:23
@MaheshGSLAB MaheshGSLAB requested review from a team as code owners April 25, 2024 07:23
@github-actions GitHub Actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/ANTA-Preview.md Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- AvdTestStun: Validate STUN clients source address and port.
- VerifyStunClient: Validate STUN clients source address and port.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/ANTA-Preview.md Outdated
@@ -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.
Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py

class AvdTestStun(AvdTestBase):
"""
AvdTestStun class for STUN tests.
Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py Outdated

# 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__)
Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py Outdated
Comment on lines 38 to 43
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
]
Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py Outdated
Comment on lines 47 to 49
interfaces_data = get(self.structured_config, "ethernet_interfaces")
interface_address = get_item(interfaces_data, "name", source_interface)
ip_address = interface_address.get("ip_address")
Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py
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.
Copy link
Contributor

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?

Copy link
Contributor Author

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

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py Outdated
{
"VerifyStunClient": {
"stun_clients": [{"source_address": source_address, "source_port": source_port}],
"result_overwrite": {"custom_field": f"source address: {source_address} source port: {source_port}"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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}"},

Copy link
Contributor

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.

carl-baillargeon
carl-baillargeon reviewed May 8, 2024
View reviewed changes
ansible_collections/arista/avd/roles/eos_validate_state/python_modules/tests/avdteststun.py Outdated
)

# Return the ANTA tests as a dictionary
return {self.anta_module: anta_tests}
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmuloc gmuloc Awaiting requested review from gmuloc

@carl-baillargeon carl-baillargeon Awaiting requested review from carl-baillargeon

At least 2 approving reviews are required to merge this pull request.

Assignees

@MaheshGSLAB MaheshGSLAB

Labels
role: eos_validate_state state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feat(eos_validate_state): Add validation for STUN client
2 participants
@MaheshGSLAB @carl-baillargeon