Early Review of draft-ietf-mpls-ldp-yang-02
review-ietf-mpls-ldp-yang-02-yangdoctors-early-lindblad-2017-09-27-00

Request Review of draft-ietf-mpls-ldp-yang-01
Requested rev. 01 (document currently at 07)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2017-04-07
Requested 2017-03-13
Requested by Loa Andersson
Authors Kamran Raza, Rajiv Asati, Xufeng Liu, Santosh Esale, Xia Chen, Himanshu Shah
Draft last updated 2017-09-27
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)
Assignment Reviewer Jan Lindblad
State Completed
Review review-ietf-mpls-ldp-yang-02-yangdoctors-early-lindblad-2017-09-27
Reviewed rev. 02 (document currently at 07)
Review result Almost Ready
Review completed: 2017-09-27

Review
review-ietf-mpls-ldp-yang-02-yangdoctors-early-lindblad-2017-09-27

I had a read through of the draft RFC and looked at the YANG model in particular. Generally speaking, I think the YANG looks good. I don't know much about MPLS, however, so I can't judge the usefulness of the model. I'm bringing up a few points for discussion below.

1) IETF, OpenConfig and NMDA

Early on in the draft, there is this section:

   For the configuration and state data, this model follows the similar
   approach described in [I-D.openconfig-netmod-opstate] to represent
   the configuration (intended state) and operational (applied and
   derived) state.  This means that for every configuration (rw) item,
   there is an associated (ro) item under "state" container to represent
   the applied state.  Furthermore, protocol derived state is also kept
   under "state" tree corresponding to the protocol area (discovery,
   peer etc.).  [Ed note: This document will be (re-)aligned with
   [I-D.openconfig-netmod-opstate] once that specification is adopted as
   a WG document].

This alignment is sorely needed. I don't want to open up a new chapter in the IETF vs. OpenConfig style modeling debate, but I will simply note that the current model does not fit nicely into the surrounding MPLS, routing and interface models. Since consistency is a primary factor for ease of use, the model isn't very easy to use in its current form.

2) Lack of default/mandatory/description

As has become a standing item in my reviews, there are a number of leafs that have no default, or mandatory statement, and no leaf name or description making it obvious to the user what would happen if this leaf is not set. This is particularly problematic with the "leaf enable {type boolean;} leafs. I get "enabled false" and "enabled true" even without any description. But what if enabled isn't specified? At the minimum level, the description should describe this case. Better if there was a default making this clear. Or if a default really isn't appropriate, so make it mandatory.

Here are a list of leafs/line numbers where I think this is a problem. There are many others that have no default/mandatory/description, but where I suspect people who understand LDP might have an opinion about reasonable behavior if not set.

ietf-mpls-ldp:
  251  leaf hello-holdtime {
  261  leaf hello-interval {
  335  leaf enable {
  346  leaf hello-holdtime {
  357  leaf hello-interval {
  380  leaf enable {
  385  leaf reconnect-time {
  395  leaf recovery-time {
  406  leaf forwarding-holdtime {
  424  leaf enable {
  429  leaf reconnect-time {
  439  leaf recovery-time {
  462  leaf lsr-id {
  533  leaf session-ka-holdtime {
  544  leaf session-ka-interval {
  777  leaf enable {
  892  leaf enable {
1011  leaf enable {
1131  leaf enable {
1261  leaf lsr-id {
1331  leaf lsr-id {

ietf-mpls-ldp-extended:
  183  leaf transport-address {
  193  leaf transport-address {
  204  leaf key-chain {
  218  leaf enable {
  228  leaf enable {
  238  leaf enable {
  249  leaf igp-synchronization-delay {
  300  leaf ldp-disable {
  325  leaf helper-enable {
  336  leaf transport-address {
  354  leaf transport-address {
  375  leaf igp-synchronization-delay {
  473  leaf admin-down {
  500  leaf enable {
  505  leaf peer-list {
  537  leaf enable {
  620  leaf enable {
  723  leaf enable {
  728  leaf local-address {
  781  leaf interface {

3) Odd naming convention for keys

Many list keys are named by the same (or similar) name as the list. Repetitions of the same symbol makes it harder for users/programmers to get their code right, and even discuss the code. A common practice when there is no obvious identifier to use with the key is to use "name" or "id" for the key.

    list peer {
      key "peer advertisement-type";

The peer is keyed by "peer"? and advertisement-type. Looking at the "peer", it's a leafref to an lsr-id. So maybe that's a good name?

    list peer {
      key "lsr-id advertisement-type";

ietf-mpls-ldp:
  296  list peer { key "peer advertisement-type"; --> "lsr-id advertisement-type"?
  932  list address { key "address"; --> ipv4?
  944  list fec-label { key "fec"; --> prefix?
  979  list interface { key "interface"; --> name?

ietf-mpls-ldp-extended:
  279  list interface { key "interface"; --> name?
  288  list address-family { key "afi"; --> name?
  577  list address { key "address"; --> ipv6?
  589  list fec-label { key "fec"; --> prefix?

4) Password handling

The session auth password is commented out in the current model for some reason. Password handling is somewhat of a wart in an inconvenient place in YANG today, causing a whole array of interop issues of varying severity. One of the solutions working best with the current YANG spec (could perhaps be fixed in future YANG versions) is to not include passwords in the configuration, but only have an operational element with the encrypted value or last changed timestamp and and rpc/action to set the password.

/*
    leaf session-authentication-md5-password {
      type string {
        length "1..80";
      }
      description
        "Assigns an encrypted MD5 password to an LDP
         peer";
    } // md5-password
*/

There may be other possibilities as well at modeling this in a friendlier way.

5) Neighbor/prefix/peer list references

The ietf-mpls-ldp-extended module defined three reference types as strings. Are these free form strings, or is there any guidance that can be provided to users? Where would I go to find out what values are possible? I don't suppose any of these could be a leafref.

  typedef neighbor-list-ref {
    type string;
    description
      "A type for a reference to a neighbor list.";
  }

  typedef prefix-list-ref {
    type string;
    description
      "A type for a reference to a prefix list.";
  }

  typedef peer-list-ref {
    type string;
    description
      "A type for a reference to a peer list.";

6) Some short/pointless descriptions

    leaf prefix {
      type inet:ip-prefix;
      description
        "FEC.";
    }

    leaf lsr-id {
      type yang:dotted-quad;
      description "Router ID.";
    }

Descriptions like these aren't helpful. Explain how to arrive at a good value, and what it means if the value is absent.

I stumbled over a copy-paste description at line 235. Presumably hello-dropped should have a different description.

      leaf hello-received {
        type yang:counter64;
        description
          "The number of hello messages received.";
      }
      leaf hello-dropped {
        type yang:counter64;
        description
          "The number of hello messages received.";
      }

7) Captialized enum

The YANG convention is that all enums, all symbols actually, are in all lower case.

ietf-mpls-ldp:
  915  enum Ordered {

ietf-mpls-ldp-extended:
  560  enum Ordered {

That's it, thank you.
/jan