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

<stephane.litkowski@orange.com> Mon, 07 January 2019 09:22 UTC

Return-Path: <stephane.litkowski@orange.com>
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 20569130E8D; Mon, 7 Jan 2019 01:22:06 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.6
X-Spam-Level:
X-Spam-Status: No, score=-2.6 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001] 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 HdQk44elmz67; Mon, 7 Jan 2019 01:22:03 -0800 (PST)
Received: from orange.com (mta134.mail.business.static.orange.com [80.12.70.34]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 89117124BAA; Mon, 7 Jan 2019 01:22:03 -0800 (PST)
Received: from opfednr01.francetelecom.fr (unknown [xx.xx.xx.65]) by opfednr22.francetelecom.fr (ESMTP service) with ESMTP id 43Y8zZ0WBvz10RD; Mon, 7 Jan 2019 10:22:02 +0100 (CET)
Received: from Exchangemail-eme2.itn.ftgroup (unknown [xx.xx.31.59]) by opfednr01.francetelecom.fr (ESMTP service) with ESMTP id 43Y8zY6gDtzDq78; Mon, 7 Jan 2019 10:22:01 +0100 (CET)
Received: from OPEXCLILMA4.corporate.adroot.infra.ftgroup ([fe80::65de:2f08:41e6:ebbe]) by OPEXCLILM43.corporate.adroot.infra.ftgroup ([fe80::ec23:902:c31f:731c%19]) with mapi id 14.03.0415.000; Mon, 7 Jan 2019 10:22:01 +0100
From: stephane.litkowski@orange.com
To: tom petch <ietfc@btconnect.com>, Ebben Aries <exa@juniper.net>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "draft-ietf-isis-yang-isis-cfg.all@ietf.org" <draft-ietf-isis-yang-isis-cfg.all@ietf.org>, "lsr@ietf.org" <lsr@ietf.org>
Thread-Topic: [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29
Thread-Index: AQHUooyeYurHcWiMDkiXmGEu17qhUKWjif7w
Date: Mon, 07 Jan 2019 09:22:00 +0000
Message-ID: <7264_1546852922_5C331A39_7264_253_1_9E32478DFA9976438E7A22F69B08FF924B78A3A5@OPEXCLILMA4.corporate.adroot.infra.ftgroup>
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>
In-Reply-To: <009101d4a28c$905b2300$4001a8c0@gateway.2wire.net>
Accept-Language: fr-FR, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.168.234.4]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/LXQOPHarvDKdXdLSwYaT64y8hzo>
Subject: Re: [Lsr] 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 09:22:06 -0000

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
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
[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
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.