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

Request Review of draft-ietf-mpls-ldp-yang-06
Requested rev. 06 (document currently at 07)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2019-10-18
Requested 2019-10-03
Requested by Tarek Saad
Authors Kamran Raza, Rajiv Asati, Xufeng Liu, Santosh Esale, Xia Chen, Himanshu Shah
Draft last updated 2019-10-15
Completed reviews Yangdoctors Early review of -01 by Dean Bogdanović (diff)
Yangdoctors Early review of -02 by Jan Lindblad (diff)
Genart Last Call review of -06 by Theresa Enghardt (diff)
Rtgdir Last Call review of -06 by Yingzhen Qu (diff)
Yangdoctors Last Call review of -06 by Jan Lindblad (diff)
Secdir Telechat review of -07 by Shawn Emery
Comments
message from Mehmet.

On 10/1/19, 2:02 PM, "Mehmet Ersue" <mersue@gmail.com> wrote:

    Dear MPLS Chairs,
    
    the draft has been reviewed by a YANG Doctor back in 2017(!).
    
    I would like to suggest to request a new YANG Doctors review at this stage and
    we will ask the same reviewer as in 2017 to review again.
    
    Cheers,
    Mehmet
Assignment Reviewer Jan Lindblad
State Completed
Review review-ietf-mpls-ldp-yang-06-yangdoctors-lc-lindblad-2019-10-15
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/SzjFqAxFxVyrv9DUKXgUsNMzw0U
Reviewed rev. 06 (document currently at 07)
Review result Ready with Issues
Review completed: 2019-10-15

Review
review-ietf-mpls-ldp-yang-06-yangdoctors-lc-lindblad-2019-10-15

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.

><