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
_______________________________________________
Lsr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/lsr
