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, [email protected] 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:[email protected]] > Sent: Wednesday, January 02, 2019 12:17 > To: LITKOWSKI Stephane OBS/OINIS; Ebben Aries; [email protected] > Cc: [email protected]; [email protected] > 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" <[email protected]> > 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: <[email protected]> > > > > 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:[email protected]] > > Sent: Tuesday, November 27, 2018 12:11 > > To: Ebben Aries; [email protected] > > Cc: [email protected]; [email protected]; 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" <[email protected]> > > Sent: Tuesday, October 23, 2018 12:45 AM > > > > > Reviewer: Ebben Aries > > > Review result: On the Right Track > > > > > > 1 module in this draft: > > > - [email protected] > > > > > > 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 [email protected]: > > > - 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 > > [email protected] > > 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 > [email protected] > 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/> _______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
