Feat(eos_designs): Add support for dot1x_settings.web_authentication.ipv4_standard_acl#7209
Conversation
…ipv4_standard_acl
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an ChangesDot1x captive portal IPv4 ACL support
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Dot1xMixin
participant StructuredConfig
participant StructuredConfigUtils
Dot1xMixin->>StructuredConfig: set captive_portal.access_list_ipv4 = web-acl
Dot1xMixin->>StructuredConfigUtils: _set_ipv4_standard_acl(web-acl)
StructuredConfigUtils->>StructuredConfig: append standard_access_lists entry
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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-7209
# Activate the virtual environment
source test-avd-pr-7209/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@captive-portal-acl#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
python -m ansible.cli.galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,captive-portal-acl --force
cd test-avd-pr-7209
# Run your playbook using `python -m ansible.cli.playbook path/to/playbook.yml ...`You can also test this PR using AVD playground:
|
github-actions
Bot
added
state: CI Updated
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #7209 +/- ##
=======================================
Coverage 92.08% 92.08%
=======================================
Files 775 775
Lines 42329 42331 +2
Branches 10204 10204
=======================================
+ Hits 38978 38982 +4
Misses 1945 1945
+ Partials 1406 1404 -2
🚀 New features to boost your workflow:
|
|
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. |
|
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. |
|
| ipv4_standard_acl: | ||
| type: str | ||
| description: |- | ||
| Standard IPv4 ACL name. | ||
| This ACL must be present in `ipv4_standard_acls` catalog. |
There was a problem hiding this comment.
We reference "standard ACL" here due to the existing eos_cli_config_gen model and output of the EOS CLI helper which states that it expects a standard ACL:
# 4.35.2F
ag-dc1-leaf1a(config-s-s46-dot1x)#captive-portal access-list ipv4 ?
WORD Standard access-list name
ag-dc1-leaf1a(config-s-s46-dot1x)#
In reality this seems like an EOS CLI bug to me.
EOS perfectly accepts references to either standard or extended ACL:
ag-dc1-leaf1a(config-s-s46)#show session-config diffs
--- system:/running-config
+++ session:/s46-session-config
+ip access-list acl-for-test
+ 10 permit ip any 20.20.1.0/24
+ 20 deny tcp any any eq www copy captive-portal
+ 30 deny tcp any any eq https copy captive-portal
+ 40 deny ip any any
ag-dc1-leaf1a(config-s-s46)#dot1x
ag-dc1-leaf1a(config-s-s46-dot1x)#captive-portal access-list ipv4 acl-for-test
ag-dc1-leaf1a(config-s-s46-dot1x)#exit
ag-dc1-leaf1a(config-s-s46)#show session-config diffs
--- system:/running-config
+++ session:/s46-session-config
+dot1x
+ captive-portal access-list ipv4 acl-for-test
+!
+ip access-list acl-for-test
+ 10 permit ip any 20.20.1.0/24
+ 20 deny tcp any any eq www copy captive-portal
+ 30 deny tcp any any eq https copy captive-portal
+ 40 deny ip any any
ag-dc1-leaf1a(config-s-s46)#
but only extended ACL makes sense here.
By default (when no ACL is provided via captive-portal access-list ipv4 <ACL_NAME>) EOS implements implicit TCAM rules which permit DHCP, DNS, etc, and at the same time enforce redirection of all HTTP and HTTPS traffic to the IP address that Portal URL resolves to.
When ACL is explicitly specified by the user, those implicit redirects are not auto-configured and EOS instead applies what user asked (assuming ACL rules will cave copy captive-portal actions forcing portal redirect, like shown in https://www.arista.com/en/support/toi/eos-4-24-2f/14567-802-1x-on-arista-switches#typical-configuration-for-the-acl-for-test-statically-configured).
When we try to apply standard ACL - my understanding is that this will break redirect to the portal and this ACL will simply be applied in the ingress direction on the port (filtering ingress traffic based on the source IP).
There was a problem hiding this comment.
As we are now forcing the ACL referenced by captive-portal to be present in the standard IPv4 ACL catalogue - we may be forcing users to the incorrect configuration, as users now can not reference correct extended IPv4 ACL with correct copy captive-portal actions
There was a problem hiding this comment.
Agreed with Alexey. It looks like a cosmetic bug in the CLI definition. An extended ACL is required for the redirection to work properly. We need to do the following:
1- Update eos_cli_config_gen captive_portal.access_list_ipv4 description
2- Update eos_cli_config_gen ip_access_lists data model to support copy captive-portal
3- Update this PR to use ipv4_acls ACL catalog instead
1 and 2 in the same PR is fine.
6 participants
Change Summary
Add support for
dot1x_settings.web_authentication.ipv4_standard_acl. Presence of this ACL is mandatory inipv4_standard_aclscatalog.Related Issue(s)
Partially Fixes #https://github.com/aristanetworks/avd-internal/issues/623
Component(s) name
arista.avd.eos_designsProposed changes
Add support for
dot1x_settings.web_authentication.ipv4_standard_acl. Presence of this ACL is mandatory inipv4_standard_aclscatalog.How to test
Checklist
User Checklist
Repository Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation