Re: [mpls] Review of draft-ietf-mpls-ldp-yang-09

["Rajiv Asati (rajiva)" <[e-mail]>]

Hi Renato,

Thanks for the feedback to the WG document. This really helps us to build a solid spec.

1. You are right about adjacency configuration (RW) and adjacency state (RO). The spec well covers all of those.

The flag you referred to is defined as ReadOnly (RO) for operational state, exactly as you pointed out, to capture the neighbor being active or passive. Not for config. Hello-adjacencies below is a container, btw, that includes both tx and rx adjacencies.

          |  |     |  |  +--ro hello-adjacencies
          |  |     |  |  |  +--ro hello-adjacency* [adjacent-address]

          |        |  |     +--ro flag*               identityref
< >

         container hello-adjacencies {
               …
             uses ldp:adjacency-state-attributes;

Config (RW) for targeted neighbor covers what you suggested well -
          |  +--rw targeted
          |     +--rw hello-holdtime?     uint16
          |     +--rw hello-interval?     uint16
          |     +--rw hello-accept
          |     |  +--rw enabled?                 boolean
          |     |  +--rw ldp-ext:neighbor-list?   neighbor-list-ref




2.  Targeted neighbor is not bound to an interface (per RFC5036), so that interface leaf you referred to is actually not part of targeted discovery container, rather basic discovery container (which indeed allows for one or more interfaces per neighbor).

         container hello-adjacencies {
           config false;
           description
             "Containing a list of Hello adjacencies.";
                                . . . . . .
             leaf interface {
               type if:interface-ref;
               description "Interface for this adjacency.";



3. You are right that a neighbor can be discovered via both basic and extended discovery. What you point out is well covered in the tree in two distinct places – one for basic and the other for extended/targeted. Pls see section 6.1 and figure 5.

         +--rw discovery
            +--rw interfaces
            |  +--rw interface* [interface]
            |     +--rw address-families
            |        +--rw ipv4
            |           +--ro hello-adjacencies
            |              +--ro hello-adjacencies* [adjacent-address]
            |                 +--ro adjacent-address
            |                    . . . .
            |                    . . . .
            +--rw targeted
               +--rw address-families
                  +--rw ipv4
                     +--ro hello-adjacencies
                        +--ro hello-adjacencies*
                        |                  [local-address adjacent-address]
                        +--ro local-address
                           +--ro adjacent-address
                              . . . .
                              . . . .




Also, page#52 clarifies the following for local-address -

                   leaf local-address {
                     type inet:ipv4-address;
                     description
                       "The local address used as the source address to
                        send targeted Hello messages.
                        If the value is not specified, the
                        transport-address is used as the source
                        address.";


4. Good point about a good “deployment” practice for different protocols to leverage the same router-id. However, LDP protocol (RFC5036) specifies LSR-ID and allows it to be different, if/as needed (independent of that, one can choose different LSR ID for different VRFs on a node).

Thankfully, the team agreed to keep the default along the line of what you pointed.


5. Well, think of IPv6-only node, which unfortunately needs to have IPv4 for OOB management access. That would result in LDP IPv4 AFI being enabled by default, and hence, would need to be explicitly disabled via this container.

I will let my co-authors add further clarification.

6. Yes, it does apply.
And  that configuration will allow for the targeted adjacency to be created for 1.1.1.1, but peering relationship will be denied.  If the intent is to not let the targeted adjacency be created, then policy could be created to deny 1.1.1.1.

Wrt custom hello/hold timers per targeted neighbors, AFAIK, RFC5036 doesn’t specify that, nor do any of the implementations.
Only custom hello/hold timers global config.


7. AFAIK, this is because of RFC5036 that specifies 5s to be the lowest hello interval.  😊

https://datatracker.ietf.org/doc/html/rfc5036#section-3.5.2


8. Thanks for reviewing so diligently and pointing these typos out.
Thankfully, our amazing RFC Editor team has fixed all of them (I just checked their version) except interface’s counters (within the statistics container), which I believe is correct (since it is about the node, not about the peer).

--
Cheers,
Rajiv Asati
CTO, CX APJC

“Focus on what we can do with what we have, not the other way around.”


From: Renato Westphal <[e-mail]>
Date: Thursday, January 20, 2022 at 10:41 AM
To: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Review of draft-ietf-mpls-ldp-yang-09
Resent-From: <[e-mail]>
Resent-To: Himanshu Shah <[e-mail]>, <[e-mail]>, Rajiv Asati <[e-mail]>, <[e-mail]>, <[e-mail]>, John Scudder <[e-mail]>, <[e-mail]>, <[e-mail]>, "[e-mail]" <[e-mail]>, Alvaro Retana <[e-mail]>, <[e-mail]>, Loa Andersson <[e-mail]>, 'Mach Chen' <[e-mail]>, DEBORAH BRUNGARD <[e-mail]>
Resent-Date: Thursday, January 20, 2022 at 10:41 AM

Hi all,

I've implemented the ietf-mpls-ldp YANG module recently and I thought it would be a good idea to provide some feedback to the module authors.

Overall I think the current module hierarchy is really good, it captures exactly what one should expect from an LDP implementation. I couldn't think of anything that is missing. I also liked the split between a base module and an extended one.

I only have a few points/questions to make:

1) I find the "adjacency-flag-base" identity (and the ones derived from it) to be confusing:

  identity adjacency-flag-base {
    description "Base type for adjacency flags.";
  }

  identity adjacency-flag-active {
    base adjacency-flag-base;
    description
      "This adjacency is configured and actively created.";
  }

  identity adjacency-flag-passive {
    base adjacency-flag-base;
    description
      "This adjacency is not configured and passively accepted.";
  }

The way I see it, no adjacency can be explicitly configured -- they are all created dynamically upon receipt of hello packets.

Targeted neighbors, on the other hand, can be either configured explicitly or created on demand (upon receipt of hello packets with the 'R' bit set -- "Request Send Targeted Hellos"). So wouldn't it be better to move the active/passive attribute from adjacencies to targeted neighbors?

2) I think the following state leaf is problematic for targeted adjacencies:

  leaf interface {
    type if:interface-ref;
    description "Interface for this adjacency.";
   }

The reason being that it's theoretically possible for one adjacency to receive hello packets from the same targeted neighbor on multiple different interfaces.

3) In some parts of the module tree, there are adjacency lists indexed like this:

  +--ro ldp:hello-adjacency* [local-address adjacent-address]
     +--ro ldp:local-address       inet:ipv4-address
     +--ro ldp:adjacent-address    inet:ipv4-address

One problem is that it's perfectly possible to have two different adjacencies with the same local and adjacent addresses, one being a link adjacency and the other an extended adjacency. Adding an "adjacency-type" key would probably be necessary to guarantee disambiguation in such cases.

Also, it'd be good to clarify whether the local-address leaf refers to the hello's source address or to the advertised transport address (when it's present).

4) I wonder if the following leaf is necessary:

  leaf lsr-id {
    type rt-types:router-id;
    description
      "Specify the value to act as the LDP LSR ID.
       If this attribute is not specified, LDP uses the router
       ID as determined by the system.";
  }

Shouldn't the router-id leaf from the ietf-routing module be enough for this purpose?

5) I'd like to know what's the rationale behind the following presence container:

  container address-families {
    description
      "Per-vrf per-af params.";
    container ipv4 {
      presence
        "Present if IPv4 is enabled.";
      [...]
    }
  }

Since not having that container explicitly enabled in the configuration won't prevent an LDPv4 neighborship to be formed, wouldn't a regular non-presence container be enough here? The "presence" statement doesn't seem to add any meaning to the existence of the container.

Also, the "per-vrf" comment in the description seems unnecessary.

6) I have one question about configuration precedence. Here's an example configuration:

      "control-plane-protocol": [
        {
          "type": "ietf-mpls-ldp:mpls-ldp",
          "name": "main",
          "ietf-mpls-ldp:mpls-ldp": {
            "global": {
              [snip]
            },
            "discovery": {
              "interfaces": {
                [snip]
              },
              "targeted": {
                "hello-accept": {
                  "enabled": true
                },
                "address-families": {
                  "ipv4": {
                    "target": [
                      {
                        "adjacent-address": "1.1.1.1",
                        "enabled": false
                      }
                    ]
                  }
                }
              }
            }
          }
        }
      ]

In that configuration, the "1.1.1.1" targeted neighbor is explicitly configured and disabled. Also, the global "hello-accept" setting is enabled.

When a hello packet from "1.1.1.1" is received, should a dynamic targeted neighbor be created for it? I think the answer boils down to whether the targeted neighbor configuration applies to dynamic targeted neighbors as well.

Also, having the ability to configure custom hello timers for targeted neighbors could be useful for some operators.

7) I think some timer ranges are too restrictive. Example:

  leaf hello-holdtime {
    type uint16 {
      range 15..3600;
    }
    [snip]
  }
  leaf hello-interval {
    type uint16 {
      range 5..1200;
    }
    [snip]
  }

While a hello holdtime of 15 seconds is a sane default, I don't think we should prohibit the operator from configuring smaller values.

The OSPF and IS-IS modules, for instance, use rt-types:timer-value-seconds16 for leafs like these.

8) A couple of trivial typo fixes:
  * Page 30: s/On or more flags/One or more flags/
  * Page 30: s/extended discoveris/extended discoveries/
  * Page 42: s/since the the state/since the state/
  * Page 42: s/interface's counters/peer's counters/
  * Page 54: s/targettted adjacency/targeted adjacency/

Regards,
--
Renato Westphal