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

"Kamran Raza (skraza)" <skraza@cisco.com> Thu, 12 October 2017 04:55 UTC

Return-Path: <skraza@cisco.com>
X-Original-To: mpls@ietfa.amsl.com
Delivered-To: mpls@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C69991342DF; Wed, 11 Oct 2017 21:55:07 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.52
X-Spam-Level:
X-Spam-Status: No, score=-14.52 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id C7RpVImVkVqE; Wed, 11 Oct 2017 21:55:05 -0700 (PDT)
Received: from alln-iport-8.cisco.com (alln-iport-8.cisco.com [173.37.142.95]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 93504133211; Wed, 11 Oct 2017 21:55:05 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=11064; q=dns/txt; s=iport; t=1507784105; x=1508993705; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=hDSmBFS6jr1/mCDADptwyDcfluZKPEiFaZgwCykUIHE=; b=NIzTj2v9LVohhd+LRhNqkxf/MTFFZ9lcORVa/bSgGuH3OccRyAmDL03j w9jslGWq1u1rQQiLKAo5t6ZuDBpFUihicchFtAvZwOXr6H6HQRZONdYKV FH6fymSeX8Q/AMO6NT0oIpb6MPoz2sQRnUy7DDHUfomIL/NbFqeIPgvdp Y=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0CbAABc9d5Z/4ENJK1dGQEBAQEBAQEBAQEBBwEBAQEBg1uBUicHg3OKH48ugm+VNoISCoU7AhqERT8YAQIBAQEBAQEBayiFHgYjBA1FEAIBCBoCFBICAgIwFRACBAENBRuKCalBgW06izsBAQEBAQEBAQEBAQEBAQEBAQEBAQEdgQ6CH4FmIYM7K4FxWTWFHDeCRS+CEiAFkUGHHYhfApRnghSJcYcKlTYCERkBgTgBHziBDngVSRIBhQgbgWd2hy6BDoERAQEB
X-IronPort-AV: E=Sophos;i="5.43,364,1503360000"; d="scan'208";a="15365095"
Received: from alln-core-9.cisco.com ([173.36.13.129]) by alln-iport-8.cisco.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 12 Oct 2017 04:54:48 +0000
Received: from XCH-RCD-013.cisco.com (xch-rcd-013.cisco.com [173.37.102.23]) by alln-core-9.cisco.com (8.14.5/8.14.5) with ESMTP id v9C4smWt019202 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Thu, 12 Oct 2017 04:54:48 GMT
Received: from xch-aln-013.cisco.com (173.36.7.23) by XCH-RCD-013.cisco.com (173.37.102.23) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Wed, 11 Oct 2017 23:54:47 -0500
Received: from xch-aln-013.cisco.com ([173.36.7.23]) by XCH-ALN-013.cisco.com ([173.36.7.23]) with mapi id 15.00.1320.000; Wed, 11 Oct 2017 23:54:47 -0500
From: "Kamran Raza (skraza)" <skraza@cisco.com>
To: Jan Lindblad <janl@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "mpls@ietf.org" <mpls@ietf.org>, "draft-ietf-mpls-ldp-yang.all@ietf.org" <draft-ietf-mpls-ldp-yang.all@ietf.org>, "ietf@ietf.org" <ietf@ietf.org>
Thread-Topic: Yangdoctors early review of draft-ietf-mpls-ldp-yang-02
Thread-Index: AQHTN5btsY2kwx9E9ECO5jDkVGClRaLfzXAA
Date: Thu, 12 Oct 2017 04:54:47 +0000
Message-ID: <4D4A49FD-41A7-4D55-BF7D-28B4A76B0A17@cisco.com>
References: <150651994737.24952.4533200012074763035@ietfa.amsl.com>
In-Reply-To: <150651994737.24952.4533200012074763035@ietfa.amsl.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
user-agent: Microsoft-MacOutlook/f.25.0.170815
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.86.253.95]
Content-Type: text/plain; charset="utf-8"
Content-ID: <669BA98DBABA7A43AC7E54CCC0759579@emea.cisco.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/mpls/M7a8SqWe85_cLyHeU77UzDJMNpk>
Subject: Re: [mpls] Yangdoctors early review of draft-ietf-mpls-ldp-yang-02
X-BeenThere: mpls@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
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: Thu, 12 Oct 2017 04:55:08 -0000

Thanks for your detailed review. We will address your comments before posting next rev.

Rgds.

On 2017-09-27, 9:45 AM, "Jan Lindblad" <janl@tail-f.com> wrote:

    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