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

Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de> Mon, 07 January 2019 10:15 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 5058E130DE3; Mon, 7 Jan 2019 02:15:33 -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 pRPX3bmreGfn; Mon, 7 Jan 2019 02:15:30 -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 B5D4A12D4F1; Mon, 7 Jan 2019 02:15:29 -0800 (PST)
Received: from localhost (demetrius5.irc-it.jacobs-university.de [10.70.0.222]) by atlas5.jacobs-university.de (Postfix) with ESMTP id 92FD9D70; Mon, 7 Jan 2019 11:15:27 +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 jHMtkDfqwFOc; Mon, 7 Jan 2019 11:15:27 +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; Mon, 7 Jan 2019 11:15:27 +0100 (CET)
Received: from localhost (demetrius3.jacobs-university.de [212.201.44.48]) by hermes.jacobs-university.de (Postfix) with ESMTP id 76A2320046; Mon, 7 Jan 2019 11:15:27 +0100 (CET)
X-Virus-Scanned: amavisd-new at jacobs-university.de
Received: from hermes.jacobs-university.de ([212.201.44.23]) by localhost (demetrius3.jacobs-university.de [212.201.44.32]) (amavisd-new, port 10024) with ESMTP id w6MafPfVCnjz; Mon, 7 Jan 2019 11:15:26 +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 5AEBB20045; Mon, 7 Jan 2019 11:15:26 +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; Mon, 7 Jan 2019 11:15:24 +0100
Received: by anna.localdomain (Postfix, from userid 501) id 4D0A7300572A08; Mon, 7 Jan 2019 11:15:23 +0100 (CET)
Date: Mon, 7 Jan 2019 11:15:23 +0100
From: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>
To: <stephane.litkowski@orange.com>
CC: 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: <20190107101523.as7u7y64trth5cro@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: <154025553381.13801.5009678921928527816@ietfa.amsl.com> <03ff01d48641$8f8d8600$4001a8c0@gateway.2wire.net> <19850_1543334259_5BFD6973_19850_302_6_9E32478DFA9976438E7A22F69B08FF924B7768B5@OPEXCLILMA4.corporate.adroot.infra.ftgroup> <009101d4a135$a59c2780$4001a8c0@gateway.2wire.net> <009101d4a28c$905b2300$4001a8c0@gateway.2wire.net> <10843_1546855502_5C33244E_10843_420_6_9E32478DFA9976438E7A22F69B08FF924B78A451@OPEXCLILMA4.corporate.adroot.infra.ftgroup>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <10843_1546855502_5C33244E_10843_420_6_9E32478DFA9976438E7A22F69B08FF924B78A451@OPEXCLILMA4.corporate.adroot.infra.ftgroup>
User-Agent: NeoMutt/20180716
X-ClientProxiedBy: SXCHMB04.jacobs.jacobs-university.de (10.70.0.156) To sxchmb03.jacobs.jacobs-university.de (10.70.0.155)
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/zQKjjfsr4D6RWMJZAuuMOvgqw90>
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: Mon, 07 Jan 2019 10:15:33 -0000

Defining and standardizing arbitrary limits is, well, arbitrary. What
we often want, I think, is 'implementations should at least handle a
length of x characters' but we have no way to say that in YANG today
(other than description statements). Also note that # characters != #
bytes with UTF-8/Unicode character sets.

/js

On Mon, Jan 07, 2019 at 10:05:00AM +0000, stephane.litkowski@orange.com wrote:
> Hi Tom,
> 
> Regarding the length enforcement for operational state leaves that are using string, do we need to always do it as a best practice ? There are plenty of strings with no enforcement today like the "non-best-reason" that you have pointed.
> 
> Brgds,
> 
> 
> -----Original Message-----
> From: tom petch [mailto:ietfc@btconnect.com] 
> Sent: Wednesday, January 02, 2019 12:17
> To: LITKOWSKI Stephane OBS/OINIS; Ebben Aries; yang-doctors@ietf.org
> Cc: draft-ietf-isis-yang-isis-cfg.all@ietf.org; lsr@ietf.org
> Subject: Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29
> 
> 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
> 
> RFC5029 is in the YANG module but not the references of the I-D
> 
> PSNPs, CSNPs
> 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.
> 
>         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.
> 
> You have a mixture of
> System-id system-id System id System ID System Id system id system ID
> suggest consistency; system-id wfm
> 
> You have a mixture of
> lsp-id LSPID LSP ID
> here, perhaps lsp-id for the names and LSP ID in the text
> 
>       case password {        leaf key {           type string;
> perhaps better with a minimum length
> 
>         leaf i-e {          type boolean;
> what is true and what false?  here I am reluctant even to guess
> 
> /"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).
> 
> 
>  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.
> 
>       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 {
> 
>   rpc clear-adjacency {
>           "Name of the IS-IS protocol instance whose IS-IS
>            information is being queried.
> queried or cleared?
> 
> Tom Petch
> 
> 
> ----- Original Message -----
> From: "tom petch" <ietfc@btconnect.com>;
> Sent: Monday, December 31, 2018 6:21 PM
> Subject: Re: [Lsr] Yangdoctors last call review of
> draft-ietf-isis-yang-isis-cfg-24
> 
> 
> > 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
> > To: Ebben Aries; yang-doctors@ietf.org
> > Cc: draft-ietf-isis-yang-isis-cfg.all@ietf.org; lsr@ietf.org; Tarek
> Saad
> > (tsaad)
> > 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
> >
> >
> ________________________________________________________________________
> > _________________________________________________
> >
> > 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.
> >
> > _______________________________________________
> > Lsr mailing list
> > Lsr@ietf.org
> > https://www.ietf.org/mailman/listinfo/lsr
> 
> 
> _________________________________________________________________________________________________________________________
> 
> 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.
> 
> _______________________________________________
> 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/>