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

Fix(eos_designs)!: VARPv6 config is not generated even when "ipv6_enable: true" is specified #4208

Merged
merged 8 commits into from
Aug 6, 2024

Conversation

bjmeuer
Copy link
Contributor

@bjmeuer bjmeuer commented Jul 12, 2024

Change Summary

VARPv6 is now configured not only when "ipv6_address" is given, it also is configured if "ipv6_enable:true" is given

Related Issue(s)

Fixes #4203

Component(s) name

arista.avd.eos_designs

Proposed changes

before for VARPv6 this was checked:
if vlan_interface_config["ipv6_address"] is not None:

after the change it is:
if vlan_interface_config["ipv6_address"] is not None or vlan_interface_config["ipv6_enable"]:

How to test

tenants:
  - name: All-Switches
    mac_vrf_vni_base: 10000

    vrfs:
      - name: HOSTING
        vrf_id: 1
        mlag_ibgp_peering_vlan: 4093
        ...
        svis:
          - id: 10
            name: hosting
            tags: ['fabric']
            description: hosting SVI
            enabled: true
            ip_address_virtual: 1.1.1.1/24
            ipv6_enable: true
            ipv6_virtual_router_addresses:
              - fe80::1

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)

@bjmeuer bjmeuer requested review from a team as code owners July 12, 2024 07:59
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Jul 12, 2024
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-4208
# Activate the virtual environment
source test-avd-pr-4208/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/bjmeuer/avd.git@varpv6_ipv6enable#subdirectory=python-avd" --force
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/bjmeuer/avd.git#/ansible_collections/arista/avd/,varpv6_ipv6enable --force
# Optional: Install AVD examples
cd test-avd-pr-4208
ansible-playbook arista.avd.install_examples

@bjmeuer bjmeuer changed the title FIX(eos_designs): VARPv6 config is not generated even when "ipv6_enable: true" is specified Fix(eos_designs): VARPv6 config is not generated even when "ipv6_enable: true" is specified Jul 12, 2024
@bjmeuer bjmeuer requested a review from gmuloc July 15, 2024 06:27
gmuloc
gmuloc previously requested changes Jul 17, 2024
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.

So I think we would need to adjust the schema description for the key under SVI:

                # IPv6 VARP addresses.
                # Requires an IPv6 address to be configured on the SVI.
                # If ipv6_address_virtuals is also set, ipv6_virtual_router_addresses will take precedence
                # _if_ there is an ipv6_address configured for the node.
                ipv6_virtual_router_addresses:

The second line needs to be changed. Also the fourth line is not correct anymore. Can you please adjust the documentation with what you feel makes sense?

Point of attention

Right now I feel like this Fix is a breaking change to the current behavior for at least one case, the existing behavior is that:

  • If ipv6_enable is true BUT no ipv6 address is set, then ipv6_address_virtuals (Anycast IPv6 addresses) are configured, as ipv6_virtual_router_address is ignored if present.
  • With your change, ipv6_virtual_router_address is not ignored anymore in this situation (which is what your fix is about) and so we would not be configuring the Anycast IPv6 addresses anymore.

From a behavior point of view it can break existing deployment where people would have both ipv6_address_virtuals and ipv6_virtual_router_address set (probably a bad input choice as one is useless today), no IPv6 address defined, and ipv6_enable: true. I know this is probably a corner case but as we are following semver this is something we need to consider.

@ClausHolbechArista for review when he is back.

@gmuloc
Copy link
Contributor

gmuloc commented Jul 17, 2024

The following inputs would give a different outcome before/ after the PR:

Change of behavior for ipv6_enable: true

          - id: 666
            enabled: true
            name: VARPv6_vs_AnycastV6
            ipv6_enable: true
            ipv6_virtual_addresses:
              - 2001:db8::cafe
            ipv6_virtual_router_addresses:
              - fe80::1

Bug fix where AVD is wrongly checking for ip_router_virtual_address instead of ipv6_router_virtual_addresses

With the config below, because AVD is wrongly checking for IPv4 VARP to decide or not to configure IPv6 Anycast vurtal addresses, this will generate IPv6 Anycast config before the PR but not after.

          - id: 42
            enabled: true
            name: AnycastGW_checking_v4VARP_Bug
            # the next two keys will generate the IPv4 VARP
            ip_virtual_router_addresses:
              - 10.10.10.10
            nodes:
              - node: dummy
                 ip_address: 19.168.250.5/24
            # Then because of the bug the next key will generate the Anycast IPv6 virtual IPs
            ipv6_virtual_addresses:
              - 2001:db8::cafe

@ClausHolbechArista
Copy link
Contributor

We will move this to 5.0 to avoid any concerns of breaking changes. We need to add some info in the release notes to tell about the potential breakage although it is a very small risk.
Moving the PR to draft until 4.10 has been released.

@ClausHolbechArista ClausHolbechArista marked this pull request as draft July 22, 2024 09:59
@ClausHolbechArista ClausHolbechArista added this to the v5.0.0 milestone Jul 22, 2024
Copy link

github-actions bot commented Aug 6, 2024

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

@github-actions github-actions bot added the state: conflict PR with conflict label Aug 6, 2024
Copy link

github-actions bot commented Aug 6, 2024

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

@github-actions github-actions bot added the type: documentation Improvements or additions to documentation label Aug 6, 2024
@ClausHolbechArista ClausHolbechArista marked this pull request as ready for review August 6, 2024 09:14
@ClausHolbechArista ClausHolbechArista requested review from a team August 6, 2024 09:14
Copy link

sonarcloud bot commented Aug 6, 2024

@ClausHolbechArista ClausHolbechArista changed the title Fix(eos_designs): VARPv6 config is not generated even when "ipv6_enable: true" is specified Fix!(eos_designs): VARPv6 config is not generated even when "ipv6_enable: true" is specified Aug 6, 2024
Co-authored-by: laxmikantchintakindi <159624484+laxmikantchintakindi@users.noreply.github.com>
@ClausHolbechArista ClausHolbechArista changed the title Fix!(eos_designs): VARPv6 config is not generated even when "ipv6_enable: true" is specified Fix(eos_designs)!: VARPv6 config is not generated even when "ipv6_enable: true" is specified Aug 6, 2024
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

Great test cases with good descriptions. Thank you for fixing these issues.

Copy link

sonarcloud bot commented Aug 6, 2024

@ClausHolbechArista ClausHolbechArista merged commit 67b2445 into aristanetworks:devel Aug 6, 2024
40 checks passed
jrecchia1029 pushed a commit to jrecchia1029/ansible-avd that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change rn: Fix(eos_designs)! state: CI Updated CI scenario have been updated in the PR type: documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eos_designs: VARPv6 config is not generated even when "ipv6_enable: true" is specified
4 participants