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): Avoid rendering duplicated CoPP ACLs for VRF default #4717

Open
1 task done
alexeygorbunov opened this issue Nov 13, 2024 · 1 comment
Open
1 task done
Labels
type: enhancement New feature or request

Comments

@alexeygorbunov
Copy link
Contributor

alexeygorbunov commented Nov 13, 2024

Enhancement summary

  • The following input data currently renders 3 CoPP ACLs applied to IPv4 and IPv6:
system:
  control_plane:
    ipv4_access_groups:
      - acl_name: "acl4_1"
      - acl_name: "acl4_11"
      - acl_name: "acl4_3"
        vrf: default
    ipv6_access_groups:
      - acl_name: "acl6_1"
      - acl_name: "acl6_11"
      - acl_name: "acl6_3"
        vrf: default
system control-plane
   ip access-group acl4_1 in
   ip access-group acl4_11 in
   ip access-group acl4_3 vrf default in
   ipv6 access-group acl6_1 in
   ipv6 access-group acl6_11 in
   ipv6 access-group acl6_3 vrf default in
  • EOS treats all lines with "missing VRF" and vrf default the same way, with later command (without VRF or with vrf default) overwriting all earlier-entered commands for the same IP version:

Example:

avd-ci-leaf2(config-s-s17)#system control-plane 
avd-ci-leaf2(config-s-s17-system-cp)#ip access-group acl4_1 in
avd-ci-leaf2(config-s-s17-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s17-session-config
+system control-plane
+   ip access-group acl4_1 in
avd-ci-leaf2(config-s-s17-system-cp)#ip access-group acl4_3 vrf default in
avd-ci-leaf2(config-s-s17-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s17-session-config
+system control-plane
+   ip access-group acl4_3 in
avd-ci-leaf2(config-s-s17-system-cp)#ip access-group acl4_1 in
avd-ci-leaf2(config-s-s17-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s17-session-config
+system control-plane
+   ip access-group acl4_1 in
avd-ci-leaf2(config-s-s17-system-cp)#
avd-ci-leaf2(config-s-s17-system-cp)#abort

avd-ci-leaf2(config-s-s18)#system control-plane 
avd-ci-leaf2(config-s-s18-system-cp)#ipv6 access-group acl6_1 in
avd-ci-leaf2(config-s-s18-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s18-session-config
+system control-plane
+   ipv6 access-group acl6_1 in
avd-ci-leaf2(config-s-s18-system-cp)#ipv6 access-group acl6_3 vrf default in
avd-ci-leaf2(config-s-s18-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s18-session-config
+system control-plane
+   ipv6 access-group acl6_3 in
avd-ci-leaf2(config-s-s18-system-cp)#ipv6 access-group acl6_1 in
avd-ci-leaf2(config-s-s18-system-cp)#show session-config diffs
--- system:/running-config
+++ session:/s18-session-config
+system control-plane
+   ipv6 access-group acl6_1 in
avd-ci-leaf2(config-s-s18-system-cp)#

We should only render one CoPP ACL per IP version for default VRF

Which component of AVD is impacted

eos_cli_config_gen

Use case example

N/A

Describe the solution you would like

One of the solutions may be to make vrf attribute of the ipv4_access_groups[] or ipv6_access_groups[] a mandatory requirement. If vrf is not set - J2 should skip this item. If it's set to default - schema is already enforcing that only one item per IPv4 and IPv6 can have this value (effectively enforcing single ACL per IP version in Designed Config)

Example:

{#     control_plane access_groups ipv4 #}
{%     if system.control_plane.ipv4_access_groups is arista.avd.defined %}
{%         set with_vrf_non_default = system.control_plane.ipv4_access_groups | selectattr('vrf', 'arista.avd.defined') | rejectattr('vrf', 'equalto', 'default') | arista.avd.natural_sort | arista.avd.natural_sort('vrf') %}
{%         set with_vrf_default = system.control_plane.ipv4_access_groups | selectattr('vrf', 'arista.avd.defined') | selectattr('vrf', 'equalto', 'default') | arista.avd.natural_sort %}
{%         set sorted_ipv4_access_groups =  with_vrf_default | list + with_vrf_non_default | list %}
{%     endif %}
{%     for acl_set in sorted_ipv4_access_groups | arista.avd.default([]) %}
{%         set cp_ipv4_access_grp = "ip access-group " ~ acl_set.acl_name ~ " vrf " ~ acl_set.vrf ~ " in" %}
   {{ cp_ipv4_access_grp }}
{%     endfor %}
{#     control_plane access_groups ipv6 #}
{%     if system.control_plane.ipv6_access_groups is arista.avd.defined %}
{%         set with_vrf_non_default = system.control_plane.ipv6_access_groups | selectattr('vrf', 'arista.avd.defined') | rejectattr('vrf', 'equalto', 'default') | arista.avd.natural_sort | arista.avd.natural_sort('vrf') %}
{%         set with_vrf_default = system.control_plane.ipv6_access_groups | selectattr('vrf', 'arista.avd.defined') | selectattr('vrf', 'equalto', 'default') | arista.avd.natural_sort %}
{%         set sorted_ipv6_access_groups =  with_vrf_default | list + with_vrf_non_default | list %}
{%     endif %}
{%     for acl_set in sorted_ipv6_access_groups | arista.avd.default([]) %}
{%         set cp_ipv6_access_grp = "ipv6 access-group " ~ acl_set.acl_name ~ " vrf " ~ acl_set.vrf ~ " in" %}
   {{ cp_ipv6_access_grp }}
{%     endfor %}

Describe alternatives you have considered

N/A

Additional context

N/A

Contributing Guide

  • I agree to follow this project's Code of Conduct
@ClausHolbechArista
Copy link
Contributor

This is a general thing in most areas of eos_cli_config_gen. The EOS CLI is a bit inconsistent, but if EOS prints out without vrf default even when entering vrf default we want to accept inputs without the VRF. A way could be to just disallow default as a value in the vrf field, but the simpler way we often turn to is to just not output the vrf default even if set (if vrf != "default" add "vrf "). That indeed means it is possible to create two lines of config if someone gave one line without vrf and one with vrf:default, but it is simple for now. Once we have schema "formats" we could add schema enforcement of valid vrf_name_except_default (a breaking change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants