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

From: Renato Westphal <[email-masked]>
Date: Thu, 20 Jan 2022 12:40:48 -0300

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

---

From: Erik Auerswald <[email-masked]>
Date: Thu, 20 Jan 2022 17:24:30 +0100

Hello Renato,

> Shouldn't the router-id leaf from the ietf-routing module be enough for

Many currently available routers allow to configure per-protocol and
(if applicable) per-process router-id values.  I would not like if
implementing YANG based management would lose this capability.

Best regards,
Erik Auerswald
-- 
Dipl.-Inform. Erik Auerswald
Gesellschaft für Fundamental Generic Networking mbH
Geschäftsführung: Volker Bauer, Jörg Mayer
Gerichtsstand: Amtsgericht Kaiserslautern - HRB: 3630

---

From: "Rajiv Asati (rajiva)" <[email-masked]>
Date: Thu, 20 Jan 2022 23:09:21 +0000

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 <[email-masked]>
Date: Thursday, January 20, 2022 at 10:41 AM
To: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Review of draft-ietf-mpls-ldp-yang-09
Resent-From: <[email-masked]>
Resent-To: Himanshu Shah <[email-masked]>, <[email-masked]>, Rajiv Asati <[email-masked]>, <[email-masked]>, <[email-masked]>, John Scudder <[email-masked]>, <[email-masked]>, <[email-masked]>, "[email-masked]" <[email-masked]>, Alvaro Retana <[email-masked]>, <[email-masked]>, Loa Andersson <[email-masked]>, 'Mach Chen' <[email-masked]>, DEBORAH BRUNGARD <[email-masked]>
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.

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

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?

  leaf interface {
    type if:interface-ref;

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:

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).

  leaf lsr-id {
    type rt-types:router-id;
    description

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:

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.

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

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.

  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.

Regards,
--
Renato Westphal

---

From: Renato Westphal <[email-masked]>
Date: Mon, 24 Jan 2022 18:27:11 -0300

> implementing YANG based management would lose this capability.

Indeed. It seems however that most IGP models lack a router-id leaf. The
OSPF module is an exception, where there's an explicit-router-id leaf
guarded by a feature of the same name. It would be necessary to check
previous discussions to understand the reason behind this.

My suggestion to remove the lsr-id field was mostly in the vein of
maintaining the LDP module consistent with the IGP modules (even though LDP
isn't exactly an IGP).

Regards,
-- 
Renato Westphal

---

From: Renato Westphal <[email-masked]>
Date: Mon, 24 Jan 2022 18:59:11 -0300

Thank you for taking the time to answer my questions Rajiv. Please see my
comments inline.

Em qui., 20 de jan. de 2022 às 20:09, Rajiv Asati (rajiva) <[email-masked]>
escreveu:

> 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.
>
>
>

Ack. If I understood correctly, an active adjacency means that hello
messages are both being sent and received, whereas a passive adjacency is
only receiving but not sending hellos (can only happen for targeted
adjacencies).

While a find the adjacency-flag descriptions a bit confusing, the
distinction between active and passive adjacencies makes sense.

> 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).
>
>
>

That leaf lies indirectly under the peer list:
/ietf-routing:routing/control-plane-protocols/control-plane-protocol/ietf-mpls-ldp:mpls-ldp/peers/peer/address-families/ipv4/hello-adjacencies/hello-adjacency/interface

That means it's present for both basic and extended adjacencies as far as I
understand it.

> 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
>

The problem is that there's also a hello-adjacency list within the peer
list. There interface and targeted adjacencies are displayed together, and
their keys might collide and overlap one another.

>                         transport-address is used as the source

That's the description of the local-address configuration leaf for targeted
neighbors. In my previous comment, I was referring to the local-address
state leaf from lines 1167 and 1276.

Upon further thought, I think those leafs descriptions are actually fine.
It should only make sense to display the IP source address as the transport
address would be the same for all adjacencies.

> 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).
>
>
>
Ack, thanks for the clarification.

The fact that RFC5036 doesn't mandate the LSR-ID to use the Router-ID (for
the first four bytes) is a good argument to preserve the lsr-id leaf.
Regarding your second point, I think one should be able to use YANG schema
mount to set different ietf-routing's Router-IDs for different VRFs. If it
wasn't for that, all IGP models would need to have a router-id leaf as well
(which they don't).

> 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 think that makes sense for the ipv4 presence container within
"global/address-families". For LDP neighbors, I can see no use for an
"ipv4" presence container (instead of a regular container).

> 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.
>

Ack. Thanks for the clarification!

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

Ack.

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

I couldn't find in that RFC section any reference to a minimum allowed
hello interval. Could you precise which excerpt you're using as a reference?

For what it's worth, I've seen numerous vendors supporting smaller hello
interval and holdtime values.

Also, maybe it would be a good idea to support infinite hold-time values
(0xffff) as they are specified by RFC 5036. It doesn't seem like they are
widely implemented though.

> 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).
>

I was referring to the statistics container within the peer-state-derived
grouping [1]. There I think "interface’s counters" was a copy and paste
error.

I'm happy to know the other typos were already fixed :)

[1] Full path to be precise:
/ietf-routing:routing/control-plane-protocols/control-plane-protocol/ietf-mpls-ldp:mpls-ldp/peers/peer/statistics

Regards,
-- 
Renato Westphal