Re: [Lsr] [yang-doctors] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29

Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de> Wed, 09 January 2019 17:14 UTC

Return-Path: <j.schoenwaelder@jacobs-university.de>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D27AD130F9C; Wed, 9 Jan 2019 09:14:28 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001] autolearn=ham autolearn_force=no
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 lOqo6_r2CeVC; Wed, 9 Jan 2019 09:14:22 -0800 (PST)
Received: from atlas5.jacobs-university.de (atlas5.jacobs-university.de [212.201.44.20]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B483F130EF2; Wed, 9 Jan 2019 09:14:21 -0800 (PST)
Received: from localhost (demetrius5.irc-it.jacobs-university.de [10.70.0.222]) by atlas5.jacobs-university.de (Postfix) with ESMTP id 4F48C10DB; Wed, 9 Jan 2019 18:14:20 +0100 (CET)
X-Virus-Scanned: amavisd-new at jacobs-university.de
Received: from atlas5.jacobs-university.de ([10.70.0.217]) by localhost (demetrius5.jacobs-university.de [10.70.0.222]) (amavisd-new, port 10032) with ESMTP id EGfGpj97lK1E; Wed, 9 Jan 2019 18:14:20 +0100 (CET)
Received: from hermes.jacobs-university.de (hermes.jacobs-university.de [212.201.44.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "hermes.jacobs-university.de", Issuer "Jacobs University CA - G01" (verified OK)) by atlas5.jacobs-university.de (Postfix) with ESMTPS; Wed, 9 Jan 2019 18:14:20 +0100 (CET)
Received: from localhost (demetrius2.jacobs-university.de [212.201.44.47]) by hermes.jacobs-university.de (Postfix) with ESMTP id 31A5A20047; Wed, 9 Jan 2019 18:14:20 +0100 (CET)
X-Virus-Scanned: amavisd-new at jacobs-university.de
Received: from hermes.jacobs-university.de ([212.201.44.23]) by localhost (demetrius2.jacobs-university.de [212.201.44.32]) (amavisd-new, port 10024) with ESMTP id pLxfgLQ41Ndu; Wed, 9 Jan 2019 18:14:18 +0100 (CET)
Received: from exchange.jacobs-university.de (SXCHMB01.jacobs.jacobs-university.de [10.70.0.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "exchange.jacobs-university.de", Issuer "DFN-Verein Global Issuing CA" (verified OK)) by hermes.jacobs-university.de (Postfix) with ESMTPS id 89B8320045; Wed, 9 Jan 2019 18:14:18 +0100 (CET)
Received: from anna.localdomain (10.50.218.117) by sxchmb03.jacobs.jacobs-university.de (10.70.0.155) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1591.10; Wed, 9 Jan 2019 18:14:18 +0100
Received: by anna.localdomain (Postfix, from userid 501) id B57153005777D1; Wed, 9 Jan 2019 18:14:17 +0100 (CET)
Date: Wed, 9 Jan 2019 18:14:17 +0100
From: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>
To: <stephane.litkowski@orange.com>, tom petch <ietfc@btconnect.com>, Ebben Aries <exa@juniper.net>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "draft-ietf-isis-yang-isis-cfg.all@ietf.org" <draft-ietf-isis-yang-isis-cfg.all@ietf.org>, "lsr@ietf.org" <lsr@ietf.org>
Message-ID: <20190109171417.7htajkttb4zd32k3@anna.jacobs.jacobs-university.de>
Reply-To: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>
Mail-Followup-To: stephane.litkowski@orange.com, tom petch <ietfc@btconnect.com>, Ebben Aries <exa@juniper.net>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "draft-ietf-isis-yang-isis-cfg.all@ietf.org" <draft-ietf-isis-yang-isis-cfg.all@ietf.org>, "lsr@ietf.org" <lsr@ietf.org>
References: <009101d4a28c$905b2300$4001a8c0@gateway.2wire.net> <7264_1546852922_5C331A39_7264_253_1_9E32478DFA9976438E7A22F69B08FF924B78A3A5@OPEXCLILMA4.corporate.adroot.infra.ftgroup> <009901d4a779$cc96bfe0$4001a8c0@gateway.2wire.net> <1676_1547025526_5C35BC76_1676_161_1_9E32478DFA9976438E7A22F69B08FF924B78C4EC@OPEXCLILMA4.corporate.adroot.infra.ftgroup> <01ab01d4a80f$aa07ca00$4001a8c0@gateway.2wire.net> <20190109120930.k7ok2i7wa3h35x4t@anna.jacobs.jacobs-university.de> <21953_1547040425_5C35F6A9_21953_272_1_9E32478DFA9976438E7A22F69B08FF924B78C61B@OPEXCLILMA4.corporate.adroot.infra.ftgroup> <20190109142657.2eqybfdnmqfmr3re@anna.jacobs.jacobs-university.de> <27064_1547045741_5C360B6D_27064_256_1_9E32478DFA9976438E7A22F69B08FF924B78C73A@OPEXCLILMA4.corporate.adroot.infra.ftgroup> <20190109151859.qspww2savq2ktxiz@anna.jacobs.jacobs-university.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20190109151859.qspww2savq2ktxiz@anna.jacobs.jacobs-university.de>
User-Agent: NeoMutt/20180716
X-ClientProxiedBy: SXCHMB01.jacobs.jacobs-university.de (10.70.0.120) To sxchmb03.jacobs.jacobs-university.de (10.70.0.155)
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/DbAkUX-187O5CtgMJRDoEB-h4zI>
Subject: Re: [Lsr] [yang-doctors] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 09 Jan 2019 17:14:30 -0000

What is the relevance / lifetime of the error strings? When do error
leafs come into existence? How long do they stay? Can I distinguish
between a recent or an old error? I am just asking questions you
should have thought about.

Searching further for strings in draft-ietf-isis-yang-isis-cfg-29.txt,
I see this:

      leaf authentication-type {
        type string;
        description
          "Authentication type to be used with IS-IS node.";
      }
      leaf authentication-key {
        type string;
        description
          "Authentication keyto be used. For security reasons,
           the authentication key MUST NOT be presented in
           plaintext format. It is recommended to use an MD5
           hash to present the authentication-key.";
      }
      description
        "IS-IS node authentication information container -
         IS-IS reference is TLV 10.";

Every implementor can make up his own identifier format and choose a
format for the keys arbitrarily? Some do MD5 some SHA-1? Do we not
have proper identifiers and formats defined already in some other YANG
modules? You should look at RFC 7317, which did establish an IANA
registry that you may find useful, perhaps also here (appears actually
multiple times):

      case password {
        leaf key {
          type string;
          description
            "This leaf specifies the authentication key.";
        }
        leaf crypto-algorithm {
          type identityref {
            base key-chain:crypto-algorithm;
          }
          description
            "Cryptographic algorithm associated with key.";
        }

What are these hostname strings?

        leaf hostname {
          type string;
          description
      	    "Hostname associated with the system ID.";
        }

Are these names following DNS conventions, similar to lets say this
taken from RFC 7317:

      leaf hostname {
        type inet:domain-name;
       description
         "The name of the host.  This name can be a single domain
          label or the fully qualified domain name of the host.";
      }

/js

On Wed, Jan 09, 2019 at 04:18:59PM +0100, Juergen Schoenwaelder wrote:
> What is the value of your error leafs if there is no error to report
> yet? Do you really want to rule out empty strings? Anyway, the semantics
> are up to you to figure out.
> 
> /js
> 
> On Wed, Jan 09, 2019 at 02:55:40PM +0000, stephane.litkowski@orange.com wrote:
> > Ok, so if we are silent about "must support at least", I think the only thing to do is to have 'type string "1.."' to avoid empty string as I don't see any requirement to have a minimum of x characters.
> > 
> > 
> > -----Original Message-----
> > From: Juergen Schoenwaelder [mailto:j.schoenwaelder@jacobs-university.de] 
> > Sent: Wednesday, January 09, 2019 15:27
> > To: LITKOWSKI Stephane OBS/OINIS
> > Cc: tom petch; Ebben Aries; yang-doctors@ietf.org; draft-ietf-isis-yang-isis-cfg.all@ietf.org; lsr@ietf.org
> > Subject: Re: [yang-doctors] [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29
> > 
> > Implementations must be robust and be able to deal with rogue input, a
> > length restriction in the data model does not help solving that problem.
> > 
> > Perhaps an example helps for the other question:
> > 
> > leaf example {
> >   type string "3..";
> >   description
> >     "implementation must support a length of at least 42 characters"
> > }
> > 
> > Valid input can be any string which has at least three characters and
> > implementations are expected to handle at least strings up to 42
> > characters. An other alternative is to be silent about any
> > expectations. A third option is to let server implementors document
> > via deviations any length restrictions they may impose in their
> > implementation.
> > 
> > My preference is actually to be silent about "must support at least"
> > constrained unless there is a clear technical reason for a specific
> > number.
> > 
> > /js
> > 
> > On Wed, Jan 09, 2019 at 01:27:05PM +0000, stephane.litkowski@orange.com wrote:
> > > Hi Juergen,
> > > 
> > > There is something that I'm missing (sorry for that). How defining a minimum length helps to deal with rogue input ? I see a rogue input more being a too long string. Too short can happen if a specific leaf really requires a minimum length string, but I don't think that we have such a case.
> > > In addition, when your talk about minimum length in your email below, do I need to understand it as  "minimum length to be supported by the implementation" ? (you said before: "'implementations should at least handle a length of x characters'"). This means that the user can input a string of any size between 0 and this minimum length to be supported. Or is it really a minimum length so the user cannot input something shorter than this value ?
> > > 
> > > Brgds,
> > > 
> > > 
> > > -----Original Message-----
> > > From: Juergen Schoenwaelder [mailto:j.schoenwaelder@jacobs-university.de] 
> > > Sent: Wednesday, January 09, 2019 13:10
> > > To: tom petch
> > > Cc: LITKOWSKI Stephane OBS/OINIS; Ebben Aries; yang-doctors@ietf.org; draft-ietf-isis-yang-isis-cfg.all@ietf.org; lsr@ietf.org
> > > Subject: Re: [yang-doctors] [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29
> > > 
> > > Hi,
> > > 
> > > just to clarify: My position is that implementations have to be able
> > > to deal with rogue input no matter whether there is a length
> > > restriction defined in a data model or not. If people argue for length
> > > restrictions, then this should be done for interoperability reasons.
> > > And for this you may want to define a minimum length that must be
> > > supported instead of a maximum length that defines a hard upper
> > > boundary (and the only place to define such a minimum length and
> > > implementations must support so far is in the description statement).
> > > 
> > > /js
> > > 
> > > On Wed, Jan 09, 2019 at 11:37:45AM +0000, tom petch wrote:
> > > > Stephane
> > > > 
> > > > Thanks for persisting.
> > > > 
> > > > On string lengths, I take your point about no user input to Operational
> > > > State; there, my concern is more about giving guidance to implementors
> > > > as to what they should cater for - as I said, this has been a topic of
> > > > lively discussion in other WG.  Even before that, whenever I see a
> > > > string, I think is there an implicit length restriction and if not,
> > > > should there be an explicit one which, as Juergen suggested, could be in
> > > > the description clause.  My experience is that those working with
> > > > networks think about size, about length; those coming from applications
> > > > tend to think 'What is a few terabytes between friends?' and are unaware
> > > > that sizes that may be commonplace in servers and associated storage can
> > > > create difficulties over a network.  Whatever, I leave this one up to
> > > > you.
> > > > 
> > > > On references, I would like a change; you say this information is in the
> > > > base ISO spec.  Well, yes, to me that means that it should be a
> > > > Normative Reference.  I could not understand the description of e.g.
> > > > 'i/e' and needed to look it up but seemingly cannot do so with the
> > > > listed references of the I-D.  Note that RFC such as RFC5305 and RFC6119
> > > > do reference International Standard 10589 and I think that this one
> > > > should too, perhaps in s.2.7 and s.5.
> > > > 
> > > > Tom Petch
> > > > 
> > > > ----- Original Message -----
> > > > From: <stephane.litkowski@orange.com>;
> > > > Sent: Wednesday, January 09, 2019 9:18 AM
> > > > 
> > > > Hi Tom,
> > > > 
> > > > Please find inline answers.
> > > > 
> > > > 
> > > > -----Original Message-----
> > > > From: tom petch [mailto:ietfc@btconnect.com]
> > > > Sent: Tuesday, January 08, 2019 18:45
> > > > 
> > > > Ok.  Top-posting the ones that are not 'ok':
> > > > 
> > > > I said that I thought that LSP did not need expanding on first use and
> > > > then checked the RFC Editor list to find that it is not one they regard
> > > > as well-known and that LSR protocols use it differently to others, so I
> > > > suggest expanding LSP on first use.
> > > > 
> > > > [SLI] Already done for the next version.
> > > > 
> > > > On lengths of text messages, perhaps I am too sensitive to buffer
> > > > overrun attacks and the like and so want a maximum on many things (and
> > > > then people attach a friendly, 20Mbyte photo to their e-mail at
> > > > Christmas and
> > > > wonder why their ESP rejects the message so I do not congratulate them
> > > > on the latest addition to the family:-).  The IDR WG had a lively
> > > > discussion about maximum message lengths in 2017 and that has also
> > > > stayed in my mind.  I have seen the comments on this by Juergen and
> > > > Lada; perhaps as Juergen intimates, something in the Description would
> > > > help; and while the server may not be rogue, it may not have a perfect
> > > > implementation.
> > > > 
> > > > [SLI] What I need to understand from your comment on string length
> > > > enforcement is if it applies to operational state or just config states
> > > > ? I do not see any issue of not enforcing the operational state as there
> > > > is no input from the user there and so no attack vector, this is purely
> > > > internal to the implementation. For config statements, it makes sense as
> > > > there is an input from the user that can be anything.
> > > > 
> > > > 
> > > > On the length of password, I saw a Security AD wanting clarification on
> > > > this not too long ago so you may see this comment again from one such .
> > > > Likewise, MD5 tends to be a red flag although I see it appears in bgp
> > > > yang.
> > > > 
> > > > I like the sort of detail in ippm-twamp-yang, on algorithms, entropy and
> > > > such like (although I have not seen a review by Security AD/directorate
> > > > of that).
> > > > 
> > > > But I am left confused as to exactly what the cited object is doing.
> > > > Yes, TLV10 provides authentication for any PDU, but what are the fields
> > > > in the YANG module doing?  Is
> > > >        leaf authentication-type {
> > > > the first octet of TLV10?  Is
> > > >       leaf authentication-key {
> > > > the rest of TLV10?  And where is this 'presented' as the YANG module
> > > > says?  Are you thinking of a YANG client presenting the field to a user
> > > > at a terminal, one router presenting it to another, or what?
> > > > 
> > > > I am using RFC5310 as my source for TLV10 and wondering why that is not
> > > > a Normative Reference for this I-D
> > > > 
> > > > [SLI] TLV 10 is defined in the base ISO spec of IS-IS. RFC5310 just adds
> > > > the crypto auth as new types.
> > > > The authentication-type is the first byte of the TLV (called
> > > > Authentication Type as well).
> > > > The authentication-key cannot be mapped directly to the
> > > > authentication-value field. This is the case for ClearText password
> > > > authtype but not crypto which adds a keyID in front of the
> > > > authentication data.
> > > > 
> > > > 
> > > > On the I/E bit, the question is, which standard?  I have 30 Normative
> > > > references to choose from.   I found up/down in RFC5305, but only by
> > > > accident, and I have not found i/e yet so a reference would be good.
> > > > 
> > > > [SLI] I/E bit is part of the base ISO spec of IS-IS and relevant for
> > > > "legacy advertisements" of prefix and links. By the way, after checking,
> > > > the position of the i-e leaf is currently wrong and needs to be within
> > > > the metric (default, delay...). I will fix this.
> > > > 
> > > > 
> > > > Tom Petch
> > > > 
> > > > ----- Original Message -----
> > > > From: stephane.litkowski@orange.com
> > > > Sent: Monday, January 07, 2019 9:22 AM
> > > > 
> > > > Hi Tom,
> > > > 
> > > > Thanks for your comments.
> > > > I wish you an happy new year !
> > > > 
> > > > Please find inline comments.
> > > > 
> > > > Brgds,
> > > > 
> > > > -----Original Message-----
> > > > From: tom petch [mailto:ietfc@btconnect.com]
> > > > Sent: Wednesday, January 02, 2019 12:17
> > > > 
> > > > Here are the rest of my comments on -29 with a slight tweak to the
> > > > subject line.  I would regard most of these (but not the first two) as
> > > > non-discussable, ie I won't complain if you disagree:-)
> > > > 
> > > > RFC1195 is in the YANG module but not the references of the I-D
> > > > [SLI] Will fix it
> > > > 
> > > > RFC5029 is in the YANG module but not the references of the I-D
> > > > [SLI] Will fix it
> > > > 
> > > > PSNPs, CSNPs
> > > > [SLI] Will fix it
> > > > 
> > > > expand on first use - LSP I think ok
> > > > 
> > > >         leaf best {           type boolean;
> > > > what is true and what false?  I can guess from the English semantics of
> > > > the name but would rather not guess.
> > > > [SLI] Will fix it
> > > > To replace the current description which is : ""Indicates if the
> > > > alternate is the preferred."", do you prefer: "Set to true when the
> > > > alternate is preferred, set to false otherwise" ?
> > > > 
> > > > 
> > > >         leaf non-best-reason {          type string;
> > > > suggest a maximum length, perhaps 127 or 255 ( unless you expect
> > > > screenshots or packet traces to be attached).  As it stands, you could
> > > > validly receive
> > > > a length of 18446744073709551615.
> > > > [SLI] Agree, will fix it
> > > > 
> > > > You have a mixture of
> > > > System-id system-id System id System ID System Id system id system ID
> > > > suggest consistency; system-id wfm
> > > > [SLI] Will fix it
> > > > 
> > > > You have a mixture of
> > > > lsp-id LSPID LSP ID
> > > > here, perhaps lsp-id for the names and LSP ID in the text
> > > > [SLI] Will fix it
> > > > 
> > > >       case password {        leaf key {           type string;
> > > > perhaps better with a minimum length
> > > > [SLI] I agree that it could make sense but is it really something that
> > > > we should impose ?
> > > > 
> > > > 
> > > >         leaf i-e {          type boolean;
> > > > what is true and what false?  here I am reluctant even to guess
> > > > [SLI] This is coming from the standard, is it really worth repeating it
> > > > ? Same for up/down bit.
> > > > 
> > > > 
> > > > /"Authentication keyto/  "Authentication key to/
> > > > 
> > > > "     the authentication key MUST NOT be presented in"
> > > > RFC2119 language means that RFC2119 boilerplate should be in the YANG
> > > > module (but without the [..] ie the reference must be plain text not an
> > > > anchor).
> > > > 
> > > > [SLI] You are right, it is missing.
> > > > 
> > > > 
> > > >  It is recommended to use an MD5
> > > >            hash to present the authentication-key.";
> > > > Mmm I think that this may be a red flag to security AD or directorate as
> > > > being too vague as well as MD5 too weak; and I think this should be
> > > > explicitly called out in Security Considerations.
> > > > 
> > > > [SLI] I agree that there is a point to discuss here. The fact is that we
> > > > must not retrieve passwords in clear text. Maybe it is something with a
> > > > wider scope than IS-IS. How do the other models deal with passwords
> > > > retrieved through "get" or "get-config" ?
> > > > 
> > > > 
> > > >       list level-db {        key level;        leaf level {
> > > > A common convention is for a list of leaf thing to be named things i.e.
> > > >       list levels {         key level;        leaf level {
> > > > 
> > > > [SLI] ack
> > > > 
> > > >   rpc clear-adjacency {
> > > >           "Name of the IS-IS protocol instance whose IS-IS
> > > >            information is being queried.
> > > > queried or cleared?
> > > > [SLI] "cleared"
> > > > 
> > > > Tom Petch
> > > > 
> > > > 
> > > > ----- Original Message -----
> > > > From: "tom petch" <ietfc@btconnect.com>;
> > > > Sent: Monday, December 31, 2018 6:21 PM
> > > > 
> > > > 
> > > > > Stephane
> > > > >
> > > > > A new and different comment.
> > > > >
> > > > >   grouping neighbor-gmpls-extensions {
> > > > >
> > > > > has a text reference to RFC5307 which does not appear in the
> > > > references
> > > > > for the I-D.  However, before adding it, I would like to know why it
> > > > is
> > > > > a good reference for switching capabilities (which is part of this
> > > > > grouping).  I think that the reference for switching capabilities
> > > > should
> > > > > be RFC7074 (which this I-D does not currently reference and should
> > > > IMO).
> > > > >
> > > > > And that begs the  question, why is switching-capability an
> > > > unrestricted
> > > > > uint8 when only 12 values are valid and three are deprecated?
> > > > >
> > > > > So why not use
> > > > >
> > > > > draft-ietf-teas-yang-te-types?
> > > > >
> > > > > I have a number of additional comments on cfg-29 but this is the one
> > > > > that may take some discussion.
> > > > >
> > > > > Tom Petch
> > > > >
> > > > >
> > > > > ----- Original Message -----
> > > > > From: <stephane.litkowski@orange.com>;
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > Thanks for your comments. I will fix them asap.
> > > > > Regarding:
> > > > > " Line length is within the RFC limit but the effect is to spread many
> > > > > of the description clauses over multiple lines with indentation of 56
> > > > > characters, not user friendly e.g.
> > > > >                                         description
> > > > >                                                 "List of max LSP
> > > > > bandwidths for different
> > > > >                                                  priorities.";
> > > > > "
> > > > > What's your suggestion on this one ?
> > > > >
> > > > > Brgds,
> > > > >
> > > > > -----Original Message-----
> > > > > From: tom petch [mailto:ietfc@btconnect.com]
> > > > > Sent: Tuesday, November 27, 2018 12:11
> > > > > Subject: Re: [Lsr] Yangdoctors last call review of
> > > > > draft-ietf-isis-yang-isis-cfg-24
> > > > >
> > > > > Some quirks in-25
> > > > >
> > > > > I see lots of YANG reference statements - good - but no mention of
> > > > them
> > > > > in the I-D references - not so good.  My list is
> > > > >
> > > > > 5130
> > > > > 5305
> > > > > 5306
> > > > > 5880
> > > > > 5881
> > > > > 6119
> > > > > 6232
> > > > > 7794
> > > > > 7810
> > > > > 7917
> > > > > 8405
> > > > >
> > > > > Also perhaps
> > > > > OLD
> > > > >     reference "RFC XXXX - YANG Data Model for Bidirectional
> > > > >                Forwarding Detection (BFD).Please replace YYYY with
> > > > >                                                          published RFC
> > > > > number for draft-ietf-bfd-yang.";
> > > > >
> > > > > NEW
> > > > >     reference "RFC YYYY - YANG Data Model for Bidirectional
> > > > >                Forwarding Detection (BFD).
> > > > >
> > > > > -- Note to RFC Editor Please replace YYYY with published RFC
> > > > > number for draft-ietf-bfd-yang.";
> > > > >
> > > > > OLD
> > > > >       reference "draft-ietf-bfd-yang-xx.txt:
> > > > >                  YANG Data Model for Bidirectional Forwarding
> > > > >                  Detection (BFD)";
> > > > > NEW
> > > > >     reference "RFC YYYY - YANG Data Model for Bidirectional
> > > > >                Forwarding Detection (BFD).
> > > > >
> > > > > -- Note to RFC Editor Please replace YYYY with published RFC
> > > > > number for draft-ietf-bfd-yang.";
> > > > >
> > > > >
> > > > > Line length is within the RFC limit but the effect is to spread many
> > > > of
> > > > > the description clauses over multiple lines with indentation of 56
> > > > > characters, not user friendly
> > > > > e.g.
> > > > >                                         description
> > > > >                                                 "List of max LSP
> > > > > bandwidths for different
> > > > >                                                  priorities.";
> > > > >
> > > > >
> > > > > Acknowledgements is TBD. I note that the editor list of the YANG
> > > > module
> > > > > is somewhat longer than the editor list of the I-D.
> > > > >
> > > > > I note that the augmentation of interfaces seems to have no
> > > > conditional
> > > > > and so will augment all interfaces. I think that this is a generic
> > > > issue
> > > > > but do not see it being addressed anywhere.
> > > > >
> > > > > In a similar vein, you are defining MPLS objects and I am unclear
> > > > > whether or not those should be conditional, or part of the MPLS YANG
> > > > > modules or both (copying Tarek for this)
> > > > >
> > > > > Tom Petch
> > > > >
> > > > > ----- Original Message -----
> > > > > From: "Ebben Aries" <exa@juniper.net>;
> > > > > Sent: Tuesday, October 23, 2018 12:45 AM
> > > > >
> > > > > > Reviewer: Ebben Aries
> > > > > > Review result: On the Right Track
> > > > > >
> > > > > > 1 module in this draft:
> > > > > > - ietf-isis@2018-08-09.yang
> > > > > >
> > > > > > No YANG compiler errors or warnings (from pyang 1.7.5 and yanglint
> > > > > 0.16.54)
> > > > > >
> > > > > > "ietf-isis@2018-08-09" module is compatible with the NMDA
> > > > > architecture.
> > > > > >
> > > > > > Module ietf-isis@2018-08-09.yang:
> > > > > > - Both the description and the draft name reference that this module
> > > > > is
> > > > > >   specific to configuration but contains operational state nodes in
> > > > > addition
> > > > > >   to RPCs and notifications.  Any wording suggesting this is only
> > > > > >   configuration should be changed
> > > > > > - Module description must contain most recent copyright notice per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.1
> > > > > > - Module description reads "common across all of the vendor
> > > > > implementations".
> > > > > >   I don't think this needs to be called out as such as that is the
> > > > > overall
> > > > > >   intention of *all* IETF models
> > > > > > - This module contains '22' features (and the respective OSPF module
> > > > > currently
> > > > > >   contains '26').  While it is understood the purpose of these
> > > > > features in the
> > > > > >   module, take precaution as to complexity for clients if they need
> > > > to
> > > > > >   understand >= quantity of features per module in use on a
> > > > > >   network-element.  We are going to end up w/ feature explosion to
> > > > > convey
> > > > > >   *all* possible features of each network-element leading to
> > > > > divergence back
> > > > > >   towards native models at the end of the day.  A large amount of
> > > > > these
> > > > > >   feature names could be defined within a more global namespace
> > > > (e.g.
> > > > > nsr) but
> > > > > >   this gives us a granular yet cumbersome approach (e.g. feature
> > > > > isis:nsr,
> > > > > >   ospf:nsr, etc..)
> > > > > > - RPC 'clear-adjacency' does not have any input leaf that covers
> > > > > clearing a
> > > > > >   specific neighbor/adjacency (See comments below as well regarding
> > > > > RPC
> > > > > >   alignment w/ the OSPF model)
> > > > > > - RPC 'clear-adjacency' has an input node of 'interface' however
> > > > this
> > > > > is just
> > > > > >   a string type.  Is there any reason this is not a
> > > > > leafref/if:interface-ref
> > > > > >   (much like in the OSPF model)
> > > > > > - Child nodes within a container or list SHOULD NOT replicate the
> > > > > parent
> > > > > >   identifier per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.3.
> > > > > 1.
> > > > > >
> > > > > >   A case in point is the list /afs/af that has a leaf of 'af'
> > > > > >       <afs>
> > > > > >         <af>
> > > > > >           <af>ipv4</af>
> > > > > >           <enable>true</enable>
> > > > > >         </af>
> > > > > >       </afs>
> > > > > >
> > > > > >   Not only is this replication, but we should likely not abbreviate
> > > > > 'afs' if
> > > > > >   we are using the expanded 'address-family' in other IETF models
> > > > such
> > > > > as
> > > > > >   ietf-i2rs-rib
> > > > > >
> > > > > >
> > > > > > General comments on the draft + nits:
> > > > > > - Since YANG tree diagrams are used, please include an informative
> > > > > reference
> > > > > >   per
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.4
> > > > > > - Section 1.1 does not need to exist since this would be covered by
> > > > > the
> > > > > >   reference mentioned above
> > > > > > - Reference to NMDA compliance should be contained within Section 1
> > > > > (vs.
> > > > > >   Section 2) per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.5
> > > > > > - Section 2: It seems reference should be given to the location of
> > > > > where the
> > > > > >   ietf-routing module is defined (As well as reference to NMDA RFC
> > > > in
> > > > > the
> > > > > >   above reference)
> > > > > > - Section 2.1: "Additional modules may be created this to
> > > > support..."
> > > > > needs
> > > > > >   slight rewording adjustment
> > > > > > - Section 3: The RPC operations are named 'clear-adjacency' and
> > > > > >   'clear-database' rather w/ reliance off namespacing for
> > > > uniqueness.
> > > > > This
> > > > > >   section refers to 'clear-isis-database' and 'clear-isis-adjacency'
> > > > > > - Section 4: Notification name mismatch in this section from actual
> > > > > naming
> > > > > >   within the module (e.g. 'adjacency-change' should rather be
> > > > > >   'adjacency-state-change')
> > > > > > - Section 7: Security Considerations will need updating to be
> > > > > patterned after
> > > > > >   the latest version of the template at
> > > > > >   https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.7
> > > > > > - Section 12: All modules imported within this module MUST be
> > > > > referenced
> > > > > >   within this section per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.9.
> > > > > >   There are quite a few missing from this section right now
> > > > > > - Appendix A: Some of the XML elements are off in alignment
> > > > > > - Appendix A: Examples must be validated.  The example given has the
> > > > > following
> > > > > >   issues:
> > > > > >   - /routing[name='SLI'] and /routing/description are invalid data
> > > > > nodes and
> > > > > >     do not exist.  I'm not sure why they are in the XML example here
> > > > > >   - The example is meant to reference configuration however
> > > > > >     /routing/interfaces is a r/o container
> > > > > >   - The control-plane-protocol 'type' needs to be qualified - e.g.
> > > > > >     <type
> > > > > xmlns:isis="urn:ietf:params:xml:ns:yang:ietf-isis">isis:isis</type>
> > > > > >   - The area-address does not validate against the pattern regex and
> > > > > must end
> > > > > >     with a '.'  e.g.
> > > > > >     <area-address>49.0001.0000.</area-address>
> > > > > >   - metric-type/value is set to 'wide' which is invalid.  This
> > > > should
> > > > > rather
> > > > > >     be 'wide-only'
> > > > > >   - isis/afs/af/af is set to 'ipv4-unicast' which is invalid.  This
> > > > > should
> > > > > >     rather be 'ipv4' per iana-routing-types
> > > > > >   - /interfaces/interface/type must be populated and is invalid.
> > > > This
> > > > > should
> > > > > >     rather be qualified as such:
> > > > > >     <type
> > > > >
> > > > xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:softwar
> > > > > eLoopback</type>
> > > > > >     <type
> > > > >
> > > > xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:etherne
> > > > > tCsmacd</type>
> > > > > >   - /interfaces/interface/link-up-down-trap-enable must have a value
> > > > > >     associated as such:
> > > > > >     <link-up-down-trap-enable>enabled</link-up-down-trap-enable>
> > > > > >   - NP container 'priority' has a must statement checking if an
> > > > > interface-type
> > > > > >     is set to 'broadcast' however if you take the XML example from
> > > > > this
> > > > > >     section, it will fail to validate even if <priority> is not
> > > > > defined
> > > > > >     underneath an interface-type of 'point-to-point'.  It seems to
> > > > me
> > > > > that
> > > > > >     this logic may need to be readjusted or not exist at all
> > > > (priority
> > > > > can
> > > > > >     still be set on implementations on loopback interfaces - which
> > > > > would
> > > > > >     default to 'broadcast' in the example here).  Could you not
> > > > solve
> > > > > this
> > > > > >     with use of 'when' vs. 'must' as such:
> > > > > >
> > > > > >           when '../interface-type = "broadcast"' {
> > > > > >               description "Priority can only be set for broadcast
> > > > > interfaces.";
> > > > > >           }
> > > > > >
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.18
> > > > > ..2.
> > > > > >
> > > > > >   - /interfaces/interface/ipv4/mtu must contain a valid value (and
> > > > > likely not
> > > > > >     need to be defined for Loopback0)
> > > > > >   - 'isis/mpls-te/ipv4-router-id' is invalid and should rather be
> > > > > >     'isis/mpls/te-rid/ipv4-router-id'
> > > > > >   - 'isis/afs/af/enabled' is invalid and should rather be
> > > > > 'isis/afs/af/enable'
> > > > > >   - Examples should use IPv6 addresses where appropriate per
> > > > > >
> > > > >
> > > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.12
> > > > >
> > > > >
> > > > ________________________________________________________________________
> > > > > _________________________________________________
> > > > 
> > > > _______________________________________________
> > > > yang-doctors mailing list
> > > > yang-doctors@ietf.org
> > > > https://www.ietf.org/mailman/listinfo/yang-doctors
> > > 
> > > -- 
> > > Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> > > Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> > > Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>
> > > 
> > > _________________________________________________________________________________________________________________________
> > > 
> > > Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> > > pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> > > a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> > > Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
> > > 
> > > This message and its attachments may contain confidential or privileged information that may be protected by law;
> > > they should not be distributed, used or copied without authorisation.
> > > If you have received this email in error, please notify the sender and delete this message and its attachments.
> > > As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> > > Thank you.
> > > 
> > 
> > -- 
> > Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> > Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> > Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>
> > 
> > _________________________________________________________________________________________________________________________
> > 
> > Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> > pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> > a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> > Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
> > 
> > This message and its attachments may contain confidential or privileged information that may be protected by law;
> > they should not be distributed, used or copied without authorisation.
> > If you have received this email in error, please notify the sender and delete this message and its attachments.
> > As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> > Thank you.
> > 
> 
> -- 
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>