[mpls] Yangdoctors early review of draft-ietf-mpls-ldp-yang-02

Jan Lindblad <janl@tail-f.com> Wed, 27 September 2017 13:45 UTC

Return-Path: <janl@tail-f.com>
X-Original-To: mpls@ietf.org
Delivered-To: mpls@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 67F67133338; Wed, 27 Sep 2017 06:45:47 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Jan Lindblad <janl@tail-f.com>
To: yang-doctors@ietf.org
Cc: mpls@ietf.org, draft-ietf-mpls-ldp-yang.all@ietf.org, ietf@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.62.1
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <150651994737.24952.4533200012074763035@ietfa.amsl.com>
Date: Wed, 27 Sep 2017 06:45:47 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/mpls/-oPUMsT9_Wmkkr1acKieeRuT5Rs>
Subject: [mpls] Yangdoctors early review of draft-ietf-mpls-ldp-yang-02
X-BeenThere: mpls@ietf.org
X-Mailman-Version: 2.1.22
List-Id: Multi-Protocol Label Switching WG <mpls.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mpls>, <mailto:mpls-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mpls/>
List-Post: <mailto:mpls@ietf.org>
List-Help: <mailto:mpls-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mpls>, <mailto:mpls-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 27 Sep 2017 13:45:47 -0000

Reviewer: Jan Lindblad
Review result: Almost Ready

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