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_designs): Add support for l3_port_channel_interfaces for WAN #4752

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

Conversation

ashenoy-arista
Copy link

@ashenoy-arista ashenoy-arista commented Nov 21, 2024

Change Summary

Revised schema to support L3 Port-Channel interfaces and supporting changes to eos_designs
Primary use-case is to allow such interfaces as wan-facing interfaces.

Related Issue(s)

Fixes #4695

Component(s) name

arista.avd.eos_designs

Proposed changes

How to test

This change introduces revised schema within eos_designs with node key l3_port_channels
to represent L3 port-channel interfaces that may be used as wan-facing interfaces.
Includes logic to support the newly added schema and unit tests to validate handling of newly added schema attributes.

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)

@ashenoy-arista ashenoy-arista requested review from a team as code owners November 21, 2024 17:10
Copy link

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

@ashenoy-arista ashenoy-arista marked this pull request as draft November 21, 2024 17:10
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role type: code quality CI and development toolset state: CI Updated CI scenario have been updated in the PR labels Nov 21, 2024
@github-actions github-actions bot removed role: eos_cli_config_gen issue related to eos_cli_config_gen role type: code quality CI and development toolset state: CI Updated CI scenario have been updated in the PR labels Nov 21, 2024
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 2 times, most recently from a8fa6b9 to 1d61e17 Compare November 27, 2024 20:49
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Dec 3, 2024
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 2 times, most recently from cfae876 to fccd4c8 Compare December 3, 2024 04:05
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 2 times, most recently from b8175d8 to ffc3188 Compare December 5, 2024 06:50
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_designs): Add revised schema to support L3 Port-Channel interfaces Feat(eos_designs): Add suport for l3_port_channel_interfaces for WAN Dec 6, 2024
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 3 times, most recently from c689203 to 41612ab Compare December 12, 2024 07:39
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 3 times, most recently from bc465c9 to 4722769 Compare December 16, 2024 22:54
@github-actions github-actions bot added the state: conflict PR with conflict label Dec 16, 2024
Copy link

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

@github-actions github-actions bot removed the state: conflict PR with conflict label Dec 16, 2024
Copy link

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

@github-actions github-actions bot added the state: conflict PR with conflict label Dec 17, 2024
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.

@github-actions github-actions bot removed the state: conflict PR with conflict label Dec 17, 2024
@github-actions github-actions bot added the state: conflict PR with conflict label Dec 20, 2024
Copy link

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

@github-actions github-actions bot removed the state: conflict PR with conflict label Dec 24, 2024
Copy link

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

@ashenoy-arista ashenoy-arista changed the title Feat(eos_designs): Add suport for l3_port_channel_interfaces for WAN Feat(eos_designs): Add support for l3_port_channel_interfaces for WAN Jan 8, 2025
@ashenoy-arista ashenoy-arista force-pushed the samplePRBranch branch 2 times, most recently from 38cffe1 to 9b5741b Compare January 9, 2025 21:54
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

partial review - need to continue but sending initial comments

@@ -430,7 +431,9 @@ interface Dps1
| --------- | ----------- | ------------- | ---------- | ----| ---- | -------- | ------ | ------- |
| Ethernet1.42 | RED-TEST | - | 10.42.3.1/24 | RED | - | False | - | - |
| Ethernet1.666 | BLUE-TEST | - | 10.66.3.1/24 | BLUE | - | False | - | - |
| Ethernet4 | REGION2-INTERNET-CORP_inet-site3-wan1_inet-cloud_Ethernet8 | - | dhcp | default | - | False | ACL-INTERNET-IN_Ethernet4 | - |
| Ethernet4 | REGION2-INTERNET-CORP_inet-site3-wan1_inet-cloud | 4 | *dhcp | **default | **- | *False | *ACL-INTERNET-IN_Port-Channel4 | **- |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the ACL be there? I think it should not be inherited?

Copy link
Author

Choose a reason for hiding this comment

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

You might be correct here.
The generated config itself does not have any ACL being applied to Ethernet4 interface.
ACL-INTERNET-IN_Port-Channel4 is being applied for Port-Channel4 interface. So that seems fine.

Issue is with documentation table being generated.
This documentation related snippet appears to be being generated via logic in python-avd/pyavd/_eos_cli_config_gen/j2templates/documentation/ethernet-interfaces.j2 lines 430-440
In the case when ethernet intf is a member of a port-channel, we seem to be setting ip_address, vrf, mtu, shutdown, acl_in, acl_out for ethernet_interface based on values derived from corresponding Port-Channel<> interface.
Would it be correct to inherit values for such attributes?

- name: Port-Channel4
mode: active
member_interfaces:
- name: Ethernet4
Copy link
Contributor

Choose a reason for hiding this comment

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

let's set peer_interface here for the example

Comment on lines +60 to +61
raw_eos_cli: |
dhcp server ipv4
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 move this back to structured_config? If not lets open an issue to add this in eos_cli_config_gen

Copy link
Contributor

Choose a reason for hiding this comment

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

confirmed it is not there - lets open an issue to support this

Copy link
Author

Choose a reason for hiding this comment

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

Issue has been opened for eos_cli_config_gen to support dhcp_server_ipv4 attribute when populated for Port-Channel interface within structured config
#4875

Once we have the fix, we could replace raw_eos_cli with block below
structured_config: dhcp_server_ipv4: true

@@ -276,14 +273,6 @@ metadata:
tags:
- name: Type
value: lan
- interface: Ethernet4
Copy link
Contributor

Choose a reason for hiding this comment

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

just confirming we should not have a tag here for Port-Channel?

(From our discussion I think thats correct but jsut want to make sure)

Copy link
Author

Choose a reason for hiding this comment

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

Based on our discussion, we are NOT generating any interface tag for L3 Port-channel and its member ethernet interfaces. Once we have added support in CVaaS side to handle tags/etc associated with Port-Channel interface, we would then revisit this to generate interface tags.

@@ -24,4 +24,4 @@ wan_path_groups:

expected_error_message: >-
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add a similar test for port-channel to make sure we catch this as well (from a security purposes)

Comment on lines +128 to +130
# TODO: Unable to add validation for 'mode' setting for Port-Channel sub-interface.
# Since we have default value specified in schema for this,
# we end up finding default value even when no explicit value is specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ClausHolbechArista not sure what we can do here

Copy link
Contributor

Choose a reason for hiding this comment

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

if you call it with ._get("mode") you will only get the value if it was manually given.

# Since we have default value specified in schema for this,
# we end up finding default value even when no explicit value is specified.
if l3_port_channel.member_interfaces:
msg = f"Port-Channel sub-interface '{l3_port_channel}' has 'member_interfaces' set.This is not a valid setting."
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
msg = f"Port-Channel sub-interface '{l3_port_channel}' has 'member_interfaces' set.This is not a valid setting."
# TODO: Add better context with source
msg = f"Port-Channel sub-interface '{l3_port_channel}' has 'member_interfaces' set.This is not a valid setting."

please add a negative unit test for this

Comment on lines +139 to +142
for parent_port_channel in subif_parent_port_channel_names:
if parent_port_channel not in regular_l3_port_channel_names:
msg = "At least one L3 Port-Channel subinterface does not have parent Port-Channel interface specified."
raise AristaAvdInvalidInputsError(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should not try to be more specifc here by listing all of the ones with issues using set difference:

  • refactor error message
Suggested change
for parent_port_channel in subif_parent_port_channel_names:
if parent_port_channel not in regular_l3_port_channel_names:
msg = "At least one L3 Port-Channel subinterface does not have parent Port-Channel interface specified."
raise AristaAvdInvalidInputsError(msg)
if (missing_parent_port_channel := subif_parent_port_channel_names.difference(regular_l3_port_channel_names)):
msg = "At least one L3 Port-Channel subinterface does not have parent Port-Channel interface specified."
raise AristaAvdInvalidInputsError(msg)

Comment on lines +146 to +147
# Note: structured config for individual member ethernet ports of each port-channel
# would be generated by logic within EthernetInterfacesMixin class.
Copy link
Contributor

Choose a reason for hiding this comment

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

no need

Suggested change
# Note: structured config for individual member ethernet ports of each port-channel
# would be generated by logic within EthernetInterfacesMixin class.

Comment on lines +134 to +136
else:
# This is a regular Port-Channel (not sub-interface)
regular_l3_port_channel_names.add(interface_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

switch if and else and use

if "."  not in interface_name:
  regular_l3_port_channel_names.add(interface_name)
  continue
<continue with code for subif>

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

Successfully merging this pull request may close these issues.

Feat(eos_designs): Add support for Port-Channels for WAN interfaces
6 participants