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

[Ladislav Lhotka <[e-mail]>]

Juergen Schoenwaelder <[e-mail]> writes:

> 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 != #

IMO this is not a big issue here because the leaf in question is state
data. We have to assume that the client doesn't connect to a rogue
server, and that legitimate servers behave reasonably.

Lada

> bytes with UTF-8/Unicode character sets.
>
> /js
>
> On Mon, Jan 07, 2019 at 10:05:00AM +0000, [e-mail] 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:[e-mail]] 
>> Sent: Wednesday, January 02, 2019 12:17
>> To: LITKOWSKI Stephane OBS/OINIS; Ebben Aries; [e-mail]
>> Cc: [e-mail]; [e-mail]
>> 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" <[e-mail]>
>> 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: <[e-mail]>
>> >
>> > 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:[e-mail]]
>> > Sent: Tuesday, November 27, 2018 12:11
>> > To: Ebben Aries; [e-mail]
>> > Cc: [e-mail]; [e-mail]; 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" <[e-mail]>
>> > Sent: Tuesday, October 23, 2018 12:45 AM
>> >
>> > > Reviewer: Ebben Aries
>> > > Review result: On the Right Track
>> > >
>> > > 1 module in this draft:
>> > > - [e-mail]
>> > >
>> > > 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 [e-mail]:
>> > > - 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
>> > [e-mail]
>> > 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
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/yang-doctors
>
> -- 
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   [phone]         <https://www.jacobs-university.de/>

-- 
Ladislav Lhotka
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67