Dear authors:
Thank you for the work on this document!!
I just (finally!) finished reading this model. I have several comments
in-line below, including some that I consider major -- I would like those
to be addressed before moving the document forward.
Thanks!
Alvaro.
[Line numbers come from id-nits.]
....
88 1. Introduction
90 This document defines a YANG ([RFC7950]) data model for IS-IS routing
91 protocol.
[nit] s/([RFC7950])/[RFC7950]
....
103 2. Design of the Data Model
105 The IS-IS YANG module augments the "control-plane-protocol" list in
106 ietf-routing module (defined in [RFC8349]) with specific IS-IS
107 parameters.
[nit] s/(defined in [RFC8349])/[RFC8349]
....
417 2.2. Multi-topology Parameters
....
427 Some specific parameters could be defined on a per topology basis
428 both at global level and at interface level: for example, an
429 interface metric can be defined per topology.
[nit] s/specific parameters could be defined/specific parameters can be
defined
....
436 2.3. Per-Level Parameters
....
442 o a top-level container: corresponds to level-1-2, so the
443 configuration applies to both levels.
445 o a level-1 container: corresponds to level-1 specific parameters.
447 o a level-2 container: corresponds to level-2 specific parameters.
....
468 An implementation SHOULD prefer a level specific parameter over a
469 level-all parameter. As example, if the priority is 100 for the
470 level-1, 200 for the level-2 and 250 for the top-level configuration,
471 the implementation should use 100 for the level-1 and 200 for the
472 level-2.
[nit] In the description above, maybe use top-level, or even level-1-2,
instead of level-all. That term is used later in the text, but it would be
clearer to understand that background here.
[major] "An implementation SHOULD prefer a level specific parameter over a
level-all parameter." When would the level-specific parameter not be
preferred? IOW, why is that not a MUST?
474 Some parameters like "overload bit" and "route preference" are not
475 modeled to support a per level configuration. If an implementation
476 supports per level configuration for such parameter, this
477 implementation SHOULD augment the current model by adding both
478 level-1 and level-2 containers and SHOULD reuse existing
479 configuration groupings.
[major] "...SHOULD augment the current model by adding both level-1 and
level-2 containers" What other way would that be done? I think that
Normative language is not needed in this case.
....
506 If an implementation does not support per level configuration for a
507 parameter modeled with per level configuration, the implementation
508 SHOULD advertise a deviation to announce the non-support of the
509 level-1 and level-2 containers.
511 Finally, if an implementation supports per level configuration but
512 does not support the level-1-2 configuration, it SHOULD also
513 advertise a deviation.
[major] "SHOULD advertise a deviation" According to rfc7950: "Deviations
MUST never be part of a published standard"; I realize that this document
doesn't include one, but it Normatively recommends their use.
s/SHOULD/should
515 2.4. Per-Interface Parameters
....
523 Each interface has some interface-specific parameters that may have a
524 different per level value as described in previous section. An
525 interface-specific parameter always overrides an IS-IS global
526 parameter.
[major] In ยง2.3 (Per-Level Parameters) the preference of level-specific
parameters is Normatively mandated. Why isn't that done here? For
clarity, I think it should.
528 Some parameters like hello-padding are defined as containers to allow
529 easy extension by vendor specific modules.
531 +--rw interfaces
532 +--rw interface* [name]
533 +--rw name if:interface-ref
534 +--rw level-type? level
535 +--rw lsp-pacing-interval?
536 | rt-types:timer-value-milliseconds
537 +--rw lsp-retransmit-interval?
538 | rt-types:timer-value-seconds16
539 +--rw passive? boolean
540 +--rw csnp-interval?
541 | rt-types:timer-value-seconds16
542 +--rw hello-padding
543 | +--rw enable? boolean
544 +--rw mesh-group-enable? mesh-group-state
545 +--rw mesh-group? uint8
546 +--rw interface-type? interface-type
547 +--rw enable? boolean {admin-control}?
[nit] It might be clearer if the "enable" (admin-control) statement is
placed closer to the top of this branch...so that it's clear that it is
controlling the interface state.
548 +--rw tag* uint32 {prefix-tag}?
549 +--rw tag64* uint64 {prefix-tag64}?
550 +--rw node-flag? boolean {node-flag}?
551 +--rw hello-authentication
552 | +--rw (authentication-type)?
553 | | +--:(key-chain) {key-chain}?
554 | | | +--rw key-chain?
555 | | | key-chain:key-chain-ref
556 | | +--:(password)
557 | | +--rw key? string
558 | | +--rw crypto-algorithm? identityref
559 | +--rw level-1
560 | | +--rw (authentication-type)?
561 | | +--:(key-chain) {key-chain}?
562 | | | +--rw key-chain?
563 | | | key-chain:key-chain-ref
564 | | +--:(password)
565 | | +--rw key? string
566 | | +--rw crypto-algorithm? identityref
567 | +--rw level-2
568 | +--rw (authentication-type)?
569 | +--:(key-chain) {key-chain}?
570 | | +--rw key-chain?
571 | | key-chain:key-chain-ref
572 | +--:(password)
573 | +--rw key? string
574 | +--rw crypto-algorithm? identityref
575 +--rw hello-interval
576 | +--rw value? rt-types:timer-value-seconds16
577 | +--rw level-1
578 | | +--rw value? rt-types:timer-value-seconds16
579 | +--rw level-2
580 | +--rw value? rt-types:timer-value-seconds16
[minor] Some implementations support having a sub-second
hello-interval...but with a timer in seconds that can't be done.
....
860 2.5. Authentication Parameters
862 The module enables authentication configuration through the IETF key-
863 chain module ([RFC8177]). The IS-IS module imports the "ietf-key-
864 chain" module and reuses some groupings to allow global and per
865 interface configuration of authentication. If a global
866 authentication is configured, an implementation SHOULD authenticate
867 PSNPs (Partial Sequence Number Packets), CSNPs (Complete Sequence
868 Number Packets) and LSPs (Link State Packets) with the authentication
869 parameters supplied. The authentication of HELLO PDUs (Protocol Data
870 Units) can be activated on a per interface basis.
[nit] s/([RFC8177])/[RFC8177]
....
894 2.8. IP FRR
896 This YANG module supports LFA (Loop Free Alternates) ([RFC5286]) and
897 remote LFA ([RFC7490]) as IP FRR techniques. The "fast-reroute"
898 container may be augmented by other models to support other IPFRR
899 flavors (MRT, TILFA ...).
[nit] s/([RFC5286])...([RFC7490])/[RFC5286]...[RFC7490]
[minor] "IPFRR" is used above and in other places, but "IP-FRR" is used in
several places as well. Please be consistent.
....
909 The "candidate-disabled" allows to mark an interface to not be used
910 as a backup.
[major] "candidate-disabled" doesn't exist...but "candidate-enable" does...
....
951 4. Notifications
....
957 lsp-too-large: raised when the system tries to propagate a too
958 large PDU.
[nit] s/a too large PDU/a PDU that is too large
....
978 sequence-number-skipped: This notification is sent when the system
979 receives a PDU with its own system ID and different contents. The
980 system has to reissue the LSP with a higher sequence number.
[nit] That's the last thing I would have guessed that this action would
have been called... Maybe it's just me...
982 authentication-type-failure: This notification is sent when the
983 system receives a PDU with the wrong authentication type field.
985 authentication-failure: This notification is sent when the system
986 receives a PDU with the wrong authentication information.
[minor] Why do we need both of these? Given that they both provide the
same information (including the raw PDU), and that
authentication-type-failure is a specific case of receiving "a PDU with the
wrong authentication information".
....
1039 6. IS-IS YANG Module
....
1107 Editor: Stephane Litkowski
1108 <mailto:[email protected]>
1110 Derek Yeung
1111 <mailto:[email protected]>
1112 Acee Lindem
1113 <mailto:[email protected]>
1114 Jeffrey Zhang
1115 <mailto:[email protected]>
1116 Ladislav Lhotka
1117 <mailto:[email protected]>
1118 Yi Yang
1119 <mailto:[email protected]>
1120 Dean Bogdanovic
1121 <mailto:[email protected]>
1122 Kiran Agrahara Sreenivasa
1123 <mailto:[email protected]>
1124 Yingzhen Qu
1125 <mailto:[email protected]>
1126 Jeff Tantsura
1127 <mailto:[email protected]>
[major] The list above is out of date, and it doesn't match the author list
on the first page of this document.
....
1239 feature nsr {
1240 description
1241 "Non-Stop-Routing (NSR) support.";
1242 }
[minor] Reference?
....
1255 feature overload-max-metric {
1256 description
1257 "Support of overload by setting
1258 all links to max metric.";
1259 }
[minor] Reference?
....
1880 enum l1-up-internal {
1881 description "Level 1 internal route
1882 and not leaked to a lower level";
1883 }
[minor] "Level 1 internal route and not leaked to a lower level" When
would an L1 route ever be leaked to a lower level?? The same comment
applies below.
1884 enum l2-up-external {
1885 description "Level 2 external route
1886 and not leaked to a lower level";
1887 }
1888 enum l1-up-external {
1889 description "Level 1 external route
1890 and not leaked to a lower level";
1891 }
1892 enum l2-down-internal {
1893 description "Level 2 internal route
1894 and leaked to a lower level";
1895 }
1896 enum l1-down-internal {
1897 description "Level 1 internal route
1898 and leaked to a lower level";
1899 }
1900 enum l2-down-external {
1901 description "Level 2 external route
1902 and leaked to a lower level";
1903 }
1904 enum l1-down-external {
1905 description "Level 1 external route
1906 and leaked to a lower level";
1907 }
[minor] What level is lower than L1? The same comment applies to the
l1-down-internal description.
....
2233 grouping hello-authentication-cfg {
2234 choice authentication-type {
2235 case key-chain {
2236 if-feature key-chain;
2237 leaf key-chain {
2238 type key-chain:key-chain-ref;
2239 description "Reference to a key-chain.";
2240 }
2241 }
2242 case password {
2243 leaf key {
2244 type string;
2245 description "Authentication key specification - The
2246 length of the key may be dependent on the
2247 cryptographic algorithm. In cases where
2248 it is not, a key length of at least 32 octets
2249 should be supported to allow for
2250 interoperability with strong keys.";
2251 }
[major] I'm hoping that the "Authentication key specification" is specified
somewhere else so that a reference can be included here -- specially the
part about the "key length of at least 32 octets should be supported to
allow for interoperability with strong keys".
....
2510 if-feature max-ecmp;
2511 type uint16 {
2512 range "1..32";
2513 }
2514 description
2515 "Maximum number of Equal-Cost Multi-Path (ECMP) paths.";
2516 }
[nit] Why just 32? Is that an architectural limit somehow? It seems like
a good opportunity to extend the number now and avoid future need to
change...
2517 container ietf-spf-delay {
2518 if-feature ietf-spf-delay;
2519 uses ietf-spf-delay;
2520 description "IETF SPF delay algorithm configuration.";
2521 }
[minor] Reference?
....
2654 container hello-padding {
2655 leaf enable {
2656 type boolean;
2657 default "true";
[minor] I just noticed that in some cases (like here) quotes are used for
the default ("true"), but in other cases no quotes are used. I'm not
completely sure about the YANG syntax for booleans (rfc7950 wasn't clear to
me), but I think that at least the document should be consistent.
....
3022 grouping packet-counters {
....
3100 container unknown {
3101 leaf in {
3102 type uint32;
3103 description "Received unknown PDUs.";
3104 }
3105 leaf out {
3106 type uint32;
3107 description "Sent unknown PDUs.";
3108 }
[major] How can an implementation send unknown (to it!) PDUs??
....
3995 grouping tlv242-router-capabilities {
3996 container router-capabilities {
3997 list router-capability {
3998 leaf flags {
3999 type bits {
4000 bit flooding {
4001 position 0;
4002 description
4003 "If the S bit is set, the IS-IS Router CAPABILITY
4004 TLV MUST be flooded across the entire routing
4005 domain. If the S bit is clear, the TLV MUST NOT
4006 be leaked between levels. This bit MUST NOT
4007 be altered during the TLV leaking.";
4008 }
[major] This is a description of the behavior (copied from rfc7981!), not a
description of the field.
4009 bit down {
4010 position 1;
4011 description
4012 "When the IS-IS Router CAPABILITY TLV is leaked
4013 from level-2 to level-1, the D bit MUST be set.
4014 Otherwise, this bit MUST be clear. IS-IS Router
4015 capability TLVs with the D bit set MUST NOT be
4016 leaked from level-1 to level-2 in to prevent
4017 TLV looping.";
4018 }
[major] Same comment as above...
....
4036 leaf binary {
4037 type binary;
4038 description
4039 "Binary encoding of the IS-IS node capabilities";
4040 }
[major] I may be missing this completely... How are the capabilities
encoded? Does this information come from the sub-TLVs in TLV 242?
Somewhere else?
....
4540 notification lsp-too-large {
4541 uses notification-instance-hdr;
4542 uses notification-interface-hdr;
4544 leaf pdu-size {
4545 type uint32;
4546 description "Size of the LSP PDU";
4547 }
4548 leaf lsp-id {
4549 type lsp-id;
4550 description "LSP ID";
4552 }
4553 description
4554 "This notification is sent when we attempt to propagate
4555 an LSP that is larger than the dataLinkBlockSize for the
4556 circuit. The notification generation must be throttled
4557 with at least 5 seconds between successive
4558 notifications.";
4559 }
[minor] A reference to dataLinkBlockSize would be very nice.
[minor] This notification is specific to a circuit. Why aren't the
interface and its MTU included in it?
[major] "must be throttled" Why is this text not Normative? It seems to
me that throttling is a good practice...in fact, it may be a good idea to
specify it for all notifications. There are 12 total instances of the same
text.
....
4845 notification lsp-received {
....
4865 }
4866 description
4867 "This notification is sent when an LSP is received.
4868 The notification generation must be throttled with at
4869 least 5 seconds between successive notifications.";
4870 }
[minor] In the case of lsp-received, 5 seconds is a very long
time...specially since there's the potential to receive many LSPs within a
short period of time. I'm not advocating against throttling, but I am
curious how an event with multiple LSPs in a short time (<< 5 sec) would be
handled.
....
4897 7. Security Considerations
....
4941 Some of the RPC operations in this YANG module may be considered
4942 sensitive or vulnerable in some network environments. It is thus
4943 important to control access to these operations. The OSPF YANG
4944 module support the "clear-adjacency" and "clear-database" RPCs. If
4945 access to either of these is compromised, they can result in
4946 temporary network outages be employed to mount DoS attacks.
[minor] s/OSPF/IS-IS ;-)
....
4954 9. IANA Considerations
4956 The IANA is requested to assign two new URIs from the IETF XML
4957 registry ([RFC3688]). Authors are suggesting the following URI:
[nit] s/([RFC3688])/[RFC3688]
....
4963 This document also requests one new YANG module name in the YANG
4964 Module Names registry ([RFC6020]) with the following suggestion:
[nit] s/([RFC6020])/[RFC6020]
....
4973 10.1. Normative References
....
5083 [RFC7810] Previdi, S., Ed., Giacalone, S., Ward, D., Drake, J., and
5084 Q. Wu, "IS-IS Traffic Engineering (TE) Metric Extensions",
5085 RFC 7810, DOI 10.17487/RFC7810, May 2016,
5086 <https://www.rfc-editor.org/info/rfc7810>.
[major] This RFC has been Obsoleted by rfc8570. Please update the
reference.
....
5144 10.2. Informative References
5146 [RFC5443] Jork, M., Atlas, A., and L. Fang, "LDP IGP
5147 Synchronization", RFC 5443, DOI 10.17487/RFC5443, March
5148 2009, <https://www.rfc-editor.org/info/rfc5443>.
[major] As all the other references to IS-IS functionality, this should
also be Normative. Note that this reference is already in the Downref
registry; IOW, there are no issues with it being an Informational RFC.
_______________________________________________
Lsr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/lsr
