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.

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 additional dot1x commands. #4191

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

Conversation

laxmikantchintakindi
Copy link
Contributor

@laxmikantchintakindi laxmikantchintakindi commented Jul 9, 2024

Change Summary

Add support for additional dot1x commands.

Related Issue(s)

Fixes #4118

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Need to support the following CLI commands.

Switch-level:

dot1x
   mac-based-auth radius av-pair user-name delimiter none lowercase

Interface-level:

interface Ethernet<ID>
  dot1x aaa unresponsive phone action traffic allow
  dot1x aaa unresponsive action traffic allow vlan <VID>
  dot1x mac based access-list

How to test

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)

Copy link

github-actions bot commented Jul 9, 2024

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-4191
# Activate the virtual environment
source test-avd-pr-4191/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@additional_dot1x#subdirectory=python-avd" --force
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,additional_dot1x --force
# Optional: Install AVD examples
cd test-avd-pr-4191
ansible-playbook arista.avd.install_examples

@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated labels Jul 9, 2024
@laxmikantchintakindi laxmikantchintakindi force-pushed the additional_dot1x branch 2 times, most recently from 19b4587 to 3ba8917 Compare July 22, 2024 05:53
Copy link
Contributor

@Shivani-gslab Shivani-gslab left a comment

Choose a reason for hiding this comment

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

Please check for coverage improvement for ethernet_interfaces.dot1x in this PR

Copy link

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

Copy link

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

@Shivani-gslab Shivani-gslab marked this pull request as draft August 6, 2024 13:55
@Shivani-gslab Shivani-gslab marked this pull request as ready for review August 6, 2024 14:10
Copy link
Contributor

@Shivani-gslab Shivani-gslab left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -620,7 +633,6 @@ ethernet_interfaces:
pae:
mode: authenticator
authentication_failure:
action: allow
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The test for this already exists at other place in this file and there was a warning in coverage report for it being always true.
That's why changed it to reduce the warnings in report.

same for the below 2 reauth_period and tx_period

@@ -630,9 +642,7 @@ ethernet_interfaces:
timeout:
idle_host: 10
quiet_period: 10
reauth_period: server
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed?

reauth_timeout_ignore: true
tx_period: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed?

Comment on lines 228 to 231
{% if ethernet_interface.dot1x.aaa.unresponsive is arista.avd.defined %}
{% set aaa_config = "dot1x aaa unresponsive" %}
{% if ethernet_interface.dot1x.aaa.unresponsive.phone_action is arista.avd.defined or ethernet_interface.dot1x.aaa.unresponsive.action is arista.avd.defined %}
{% set actions = [{'name': 'phone_action', 'config': aaa_config ~ ' phone action'}, {'name': 'action', 'config': aaa_config ~ ' action'}] %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic make the rest pretty hard to follow. Can we simplify this somehow?
Maybe you could set a tmp variable with action_settings and parse those below - making everything shorter and more readable below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified the template.

{% set action_apply_config = action_apply_config ~ " timeout " ~ ethernet_interface.dot1x.aaa.unresponsive[action.name].cached_results_timeout.time_duration ~ " " ~ ethernet_interface.dot1x.aaa.unresponsive[action.name].cached_results_timeout.time_duration_unit %}
{% endif %}
{% endif %}
{% if ethernet_interface.dot1x.aaa.unresponsive[action.name].traffic_allow is arista.avd.defined(true) or
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this if just contributes to cluttering the template. Please look into simplifying the nesting. It is ok if we have to give the traffic allow part of the string in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -99,6 +99,9 @@ dot1x
radius av-pair framed-mtu {{ dot1x.radius_av_pair.framed_mtu }}
{% endif %}
{% endif %}
{% if dot1x.mac_based_auth_radius.delimiter is arista.avd.defined and dot1x.mac_based_auth_radius.mac_string_letter_case is arista.avd.defined %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI is very different than the variable. I think you should change the model to be av_pair_user_name_delimiter since there could be other options coming later.

Copy link
Contributor

Choose a reason for hiding this comment

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

same for case. Maybe find a better variable name there too. av_pair_user_name_case maybe? (please check wording on EOS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the schema as below:
radius_av_pair_username_format:
delimiter:
mac_string_case:

EOS shows below description for lowercase/uppercase setting.

s1-leaf2(config-dot1x)#mac-based-auth radius av-pair user-name delimiter colon ?
lowercase MAC address string in lowercase
uppercase MAC address string in uppercase

@laxmikantchintakindi laxmikantchintakindi marked this pull request as ready for review August 12, 2024 13:11
Copy link

sonarcloud bot commented Aug 13, 2024

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: conflict PR with conflict state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support additional dot1x commands
5 participants