[Lsr] AD Review of draft-ietf-isis-yang-isis-cfg-35

[Alvaro Retana <[e-mail]>]

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:[e-mail]>

1110           Derek Yeung
1111             <mailto:[e-mail]>
1112           Acee Lindem
1113             <mailto:[e-mail]>
1114           Jeffrey Zhang
1115             <mailto:[e-mail]>
1116           Ladislav Lhotka
1117             <mailto:[e-mail]>
1118           Yi Yang
1119             <mailto:[e-mail]>
1120           Dean Bogdanovic
1121             <mailto:[e-mail]>
1122           Kiran Agrahara Sreenivasa
1123             <mailto:[e-mail]>
1124           Yingzhen Qu
1125             <mailto:[e-mail]>
1126           Jeff Tantsura
1127             <mailto:[e-mail]>

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