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

[Renato Westphal <[e-mail]>]

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