Last Call Review of draft-ietf-mpls-ldp-yang-06
review-ietf-mpls-ldp-yang-06-yangdoctors-lc-lindblad-2019-10-15-00

Modules: ietf-mpls-ldp.yang, ietf-mpls-ldp-extended.yang
Review result: Ready with Issues
Reviewer: Jan Lindblad

This is my YANG doctor LC review of draft-ietf-mpls-ldp-yang-06. I find the
module ready with issues, even if there are a couple rather fundamental things
to work out. It is evident that the authors have spent considerable time to
make the module clean and tidy, which is why I can say it's more or less ready
even when there are fundamentals to work out. In my judgement it may be
possible to work out all remaining issues rather quickly.

#1) Fitting mpls-ldp into the ietf-routing framework

The modules at hand, ietf-mpls-ldp.yang and ietf-mpls-ldp-extended.yang, are
augmenting into the ietf-routing module (RFC 8022). I find this highly
appropriate. RFC 8022 has a section on how control plane protocols are meant to
fit into the ietf-routing framework:

5.3.2.  Defining New Control-Plane Protocols

   It is expected that future YANG modules will create data models for
   additional control-plane protocol types.  Such a new module has to
   define the protocol-specific configuration and state data, and it has
   to integrate it into the core routing framework in the following way:

   o  A new identity MUST be defined for the control-plane protocol, and
      its base identity MUST be set to "rt:control-plane-protocol" or to
      an identity derived from "rt:control-plane-protocol".
...
   o  Configuration parameters and/or state data for the new protocol
      can be defined by augmenting the "control-plane-protocol" data
      node under both "/routing" and "/routing-state".

The current draft is doing neither of the above two points. IMHO, it should do
both. Or if there is strong reason not to do that, incorporate a statement from
the ietf-routing authors and a clear explanation of why the mpls-ldp related
modules would be exempted. At present, this deviation from the RFC 8022
architecture is never mentioned in the draft.

Even if this issue falls under fundamental issues, the way I look at it, it
would be quite easy to fix. Adjusting the two draft modules to comply with the
points above requires changes to two dozen augment statement paths and a
handful of leafref paths. Adding the identity and corresponding when derived
from statement are a couple of lines.

In principle, this change opens up for multiple instances of mpls-ldp. This
might be useful if a system is used with two entirely separate networks (such
as two separate customers), where each one might be running an mpls-ldp
instance. If this is not desired, a must statement could be introduced to limit
the number of instances to one.

#2) Global level vs. local (e.g. interface or peer) level

The module has several configurations that have a global and local level, such
as configurations per peer or per interface. Presumably, these local level
configurations are meant to override the global default when set and let the
global default prevail when nothing specific is specified at the local level.

The way this is being currently implemented in the modules, however, makes it
impossible not to set a value at the local level, so the local setting will
always prevail. The global configuration would have no effect at all (except in
those cases where a local level configuration is marked with an if-feature and
the server would not announce that feature).

For example:

On the global level, there is a graceful-restart/enable leaf:

      container global {
...
        container graceful-restart {
          leaf enable {
            type boolean;
            default false;
          }

Further down, we have another graceful-restart/enable leaf, used per peer:

  grouping graceful-restart-attributes-per-peer {
...
    container graceful-restart {
      leaf enable {
        type boolean;
        default false;
      }

I would assume this latter enable leaf should take precedence over the global
graceful-restart when set on the peer. But if not set on the peer, the global
graceful-restart value should be used. Since the peer graceful-restart/enable
leaf has a default value, however, it will never happen that it is not set, and
will thus render the global value completely irrelevant for all peers.

The fix is simple: Do not have a default statement on the local leafs. Explain
in the description statement what happens when the element has no value (the
globally configured value will be used). Make sure the global leaf has a
default (they all do already).

The same applies to a rather long list of leafs in the module. I don't dare
listing them here, because I would surely miss a few. My point is that the
global/local relationships needs to be identified, local defaults removed. It
would also make sense to mention how this is supposed to work in the
descriptions.

#3) Unlinked multikey leafrefs

The grouping ldp-peer-ref has two leafrefs, one for each key in the list of ldp
peers. Two leafrefs are exactly what you need to reference an entry in a list
with two keys, so that's good. It is not enough to simply have two unlinked
leafrefs, however. That would still allow them to have values that are valid
separately, but not point to any entry in the list combined. If you have a copy
of "Network Programmability with YANG", this is explained in detail on pp.
451-454.

To link the leafrefs, you'd need to update the path expression in the
label-space-id leaf as follows:

  grouping ldp-peer-ref {
    description
      "An absolute reference to an LDP peer, by the LDP ID, which
       consists of the LSR ID and the Label Space ID.";

    leaf lsr-id {
      type leafref {
        path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
          + "ldp:peers/ldp:peer/ldp:lsr-id";
      }
      description
        "The LSR ID of the peer, as a portion of the peer LDP ID.";
    }
    leaf label-space-id {
      type leafref {
        path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
          + "ldp:peers/ldp:peer[lsr-id =
          current()/../lsr-id]/ldp:label-space-id";
      }

The paths above would also have to be adjusted as part of fixing issue #1
above. If this fixing results in allowing multiple mpls-ldp instances, the
grouping above needs to be extended with one additional, linked leafref that
points out which mpls-ldp instance is being targeted. Or if that is always the
same instance as the reference is coming from, a reformulation of the paths
above could ensure no mpls-ldp instance name would be required. In that case,
the paths should be relative and never venture outside the mpls-ldp container.
I would be happy to explain this further.

#4) Weakly defined types for neighbor-list-ref, prefix-list-ref, peer-list-ref

These types are not leafrefs, but strings without any YANG substatements to
define the format. The only thing the description does is to claim that the
entities they refer to are outside the scope of this document. For an
operator/programmer encountering this type, that isn't very helpful, and is not
going to be interoperable.

  typedef neighbor-list-ref {
    type string;
    description
      "A type for a reference to a neighbor address list.
       The string value is the name identifier for uniquely
       identifying the referenced address list, which contains a list
       of addresses that a routing policy can applied. The definition
       of such an address list is outside the scope of this
       document.";
  }

I'm not sure if this is fixable by sharpening the YANG module, but maybe more
could be done to guide a confused reader. What would the user do to find out
the format of this type and valid values? Add to the description.

#5) Interfaces or mpls, who's on top?

In the mpls-ldp modules, there are two interface lists and two leafrefs
pointing to interfaces. This makes me wonder if some of the YANG structure
would belong more naturally as an augment to ietf-interfaces instead? I mean,
it doesn't sound fun to define the same interfaces in a lot of different
places, just because a protocol needs some additional information. This is just
a casual observation from an mpls-innocent guy, so you can disregard if you
feel it's irrelevant.

#6) MD5 key

There is a leaf md5-key of type string. Is this leaf sensitive from a security
point of view? A plaintext string would not be ideal if that is the case.
Choose a crypto type instead. Also, the format of this string is not defined.
Hex without spaces? Explain the format in the description.

><