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

Add gratuitous-arp-accepted and unsolicited-na-accepted #1120

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

cagdasalagoz-arista
Copy link

@cagdasalagoz-arista cagdasalagoz-arista commented May 31, 2024

Change Scope

Two new boolean leaf values will be added to paths below,

  • interfaces/interface/subinterfaces/subinterface/ipv4/config/
  • interfaces/interface/subinterfaces/subinterface/ipv6/config/.

A. Yang tree for IPv4

/oc-if:interfaces/oc-if:interface/oc-if:subinterfaces/oc-if:subinterface:
    +--rw ipv4
       +--rw config
       |  +--rw enabled?                     boolean
       |  +--rw mtu?                         uint16
       |  +--rw dhcp-client?                 boolean
       |  +--rw gratuitous-arp-accepted?     boolean       <<<new
       +--ro state
          +--ro enabled?                     boolean
          +--ro mtu?                         uint16
          +--ro dhcp-client?                 boolean
          +--ro gratuitous-arp-accepted?     boolean       <<<new
          +--ro counters

B. Yang tree for IPv6

/oc-if:interfaces/oc-if:interface/oc-if:subinterfaces/oc-if:subinterface:
    +--rw ipv6
       +--rw config
       |  +--rw enabled?                     boolean
       |  +--rw mtu?                         uint32
       |  +--rw dup-addr-detect-transmits?   uint32
       |  +--rw dhcp-client?                 boolean
       |  +--rw unsolicited-na-accepted?     boolean    <<<new
       +--ro state
          +--ro enabled?                     boolean
          +--ro mtu?                         uint32
          +--ro dup-addr-detect-transmits?   uint32
          +--ro dhcp-client?                 boolean
          +--ro unsolicited-na-accepted?     boolean    <<<new
          +--ro counters

As the name suggests these will be used to toggle if the device will accept (and process) or reject the gratuitous arp updates.

This change is backwards compatible

Platform Implementations

A. Implementation for IPv4

Arista

These commands configure interface ethernet 2/1 to accept gratuitous ARP request packets.

switch(config)# interface ethernet 2/1
switch(config-if-Et2/1)# arp gratuitous accept
switch(config-if-Et2/1)#

B. Implementation for IPv6

Arista

switch(config-if-Et1/1)#ipv6 nd na unsolicited accept
   
switch#show ipv6 interface et1/1
. . .
  ND unsolicited NAs are accepted

@cagdasalagoz-arista cagdasalagoz-arista requested a review from a team as a code owner May 31, 2024 11:07
Copy link

google-cla bot commented May 31, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dplore
Copy link
Member

dplore commented Jun 10, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Jun 10, 2024

No major YANG version changes in commit 991105b

@dplore
Copy link
Member

dplore commented Jun 10, 2024

Additional references

for IPv4 gratuitous arp:

Cisco IOS XR - no arp gratuitous ignore

JunOS - set gratuitous-arp-reply

Nokia SR Linux - learn-unsolicited true

For IPv6 accept unsolicited Neighbor Advertisements (related to RFC9131 Section 4.2), see
https://datatracker.ietf.org/doc/html/rfc9131#name-routers-creating-cache-entr

release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
@wenovus
Copy link
Contributor

wenovus commented Jun 25, 2024

/gcbrun

1 similar comment
@wenovus
Copy link
Contributor

wenovus commented Jun 28, 2024

/gcbrun

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

This looks good to me. Small comment on updating the versioning and I think this is ready to go.

release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
@dplore
Copy link
Member

dplore commented Sep 24, 2024

/gcbrun

@dplore dplore self-assigned this Sep 24, 2024
@dplore dplore added the last-call PR that is in final review before merging. label Sep 24, 2024
@dplore
Copy link
Member

dplore commented Sep 24, 2024

Last call for comments. This will merge on Oct 8, 2024

@LimeHat
Copy link

LimeHat commented Oct 8, 2024

Can we please extend this last call? The latest changes escaped my attention, and I have a few concerns about the ipv6 part of this PR. Also, the arista doc reference is still behind the paywall, which makes it harder to follow for the broader public.

  • according to the arista documentation, the config toggle is used to enable rfc9131-based behavior for processing unsolicited NAs (unfortunately, author gave me a different answer in the conversation earlier); which makes the current leaf description inaccurate. (I'll follow up with a more detailed comment)
  • in the current form arista seems to be the only ipv6 reference. Is there a second one?

I have no objection to merging the ipv4 leaf (if the author and the community decides to split this into 2 parts)

@dplore
Copy link
Member

dplore commented Oct 8, 2024

I found this reference in RFC9131 which is relevant to the new leaf /interfaces/interface/subinterfaces/subinterface/ipv6/config/unsolicited-na-accepted

For IPv6 accept unsolicited Neighbor Advertisements (related to RFC9131 Section 4.2), see
https://datatracker.ietf.org/doc/html/rfc9131#name-routers-creating-cache-entr

Seems to make sense to me to add this as well to be up to date with RFC9131?

@dplore
Copy link
Member

dplore commented Oct 8, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 8, 2024

Updated last call to Oct 10, 2024 to give a little time for @cagdasalagoz-arista and @LimeHat to comment.

@cagdasalagoz-arista
Copy link
Author

Correct, the feature is about accepting and processing unsolicited-na requests.
I'll update the unsolicited-na-accepted leaf as :

    leaf unsolicited-na-accepted {
      type boolean;
      default true;
      description
        "When set to true unsolicited neighbor advertisements
        will be accepted and the ARP table will be updated.";
      reference
        "RFC 9131: Routers Creating Cache Entries upon
        Receiving Unsolicited Neighbor Advertisements";
    }

Is this correct? @LimeHat

release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
default true;
description
"When set to true unsolicited neighbor advertisements
will be accepted and the ARP table will be updated.";
Copy link

Choose a reason for hiding this comment

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

Please remove:

  • any references to ARP (it doesn't exist in ipv6).
  • any references to accepted (without specifying the conditions) - this is not what RFC9131 describes.

Unsolicited NA should be accepted with or without 9131 behavior, and will be processed in either case if the entry already exists in cache (this is why I initially asked which behavior you are trying to describe and based on your response asked for the default value).

RFC9131 changes the behavior only for the specific case when the entry doesn't exist yet, quoting the rfc:

This document updates [RFC4861] so that routers create a new Neighbor Cache entry upon receiving an unsolicited Neighbor Advertisement for an address that does not already have a Neighbor Cache entry. These changes do not modify the router behavior specified in [RFC4861] for the scenario when the corresponding Neighbor Cache entry already exists.

The description should be modified accordingly

Choose a reason for hiding this comment

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

description is updated to:

        When set to true neighbors will be learned from unsolicited
        neighbor advertisements.

Copy link
Member

Choose a reason for hiding this comment

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

@cagdasalagoz-arista please the text to use "Neighbor Cache" instead of ARP, since this is IPv6.

Please also update the description so it is clear if you are intending

  1. To enable/disable RFC4861 behavior
    or
  2. To enable/disable RFC9131 behavior

The key difference between the two is whether or not the router already has a Neighbor Cache entry. If there is already an entry, RFC4861 says it can be updated. If there is not one, the cache won't be updated.

RFC9131 says do RFC4861 AND also update the cache is there is no entry.

As written, it is not clear in this yang node which behavior is being configured.

Choose a reason for hiding this comment

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

Hello @dplore ,
I think you accidentally commented on the older version of the commit. The latest version is like this:

    leaf learn-unsolicited {
      type enumeration {
        enum NONE {
          value 0;
        }
        enum GLOBAL {
          value 1;
        }
        enum LINK_LOCAL {
          value 2;
        }
        enum BOTH {
          value 3;
        }
      }
      default "NONE";
      description
        "Sets if neighbors should be learned from unsolicited neighbor
        advertisements for global or link local addresses or both.";
      reference
        "RFC 9131: Routers Creating Cache Entries upon
        Receiving Unsolicited Neighbor Advertisements";
    }

Reference contains RCF9131

release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
release/models/interfaces/openconfig-if-ip.yang Outdated Show resolved Hide resolved
@LimeHat
Copy link

LimeHat commented Oct 9, 2024

Hi Darren @dplore ,

Seems to make sense to me to add this as well to be up to date with RFC9131?

It does make sense, but the initial description of the leaf was not accurate and the first conversation in the comments didn't help to clear it up, unfortunately. :)

Another question is what type of the leaf this should be, boolean or enum (please see my comment above).

@dplore
Copy link
Member

dplore commented Oct 15, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 15, 2024

There's a request for a change. I'm updating the last call date to Oct 22, 2024

@dplore
Copy link
Member

dplore commented Oct 16, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Nov 12, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Nov 12, 2024

Additional implementations:

JunOS ipv6 neighbor discovery guide

Cisco IOS XR - ipv6 nd na glean

@dplore
Copy link
Member

dplore commented Nov 12, 2024

@LimeHat any additional comments?

@dplore dplore requested a review from LimeHat November 12, 2024 05:57
@dplore
Copy link
Member

dplore commented Nov 12, 2024

Reset last call to Nov 15, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call PR that is in final review before merging. non-breaking
Projects
Status: last-call
Development

Successfully merging this pull request may close these issues.

5 participants