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

[<[e-mail]>]

Hi Alvaro,

Thanks for your comments. We are working on updating the document accordingly.
Please find some replies inline that may require more discussion.
I have stripped the comments that will be fixed in the next revision.

Brgds,


From: Alvaro Retana [mailto:[e-mail]]
Sent: Tuesday, June 04, 2019 23:34
To: [e-mail]
Cc: Yingzhen Qu; [e-mail]; [e-mail]
Subject: AD Review of draft-ietf-isis-yang-isis-cfg-35

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


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.
[SLI] Using YANG there are multiple ways to do things. People may create new containers, use different namings… and we want to keep the modeling consistency even in the future augmentations.




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.
[SLI] We don’t think that it is a good idea. Sub-second timers were implemented at the time when BFD wasn’t available. BFD provides a better mechanism for fast detection and is supported in the model. BFD is widely deployed compared to sub-second timers which do not scale. If a vendor wants to add the support for sub-second hello, this can be done by augmentation but we don’t want to encourage this usage.

...
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...
[SLI] This is inherited from RFC4444


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"
[SLI] This is inherited from RFC4444

.


...
1039     6.  IS-IS YANG Module
...
1107           Editor:    Stephane Litkowski
1108                 <mailto:[e-mail]<mailto:[e-mail]>>

1110               Derek Yeung
1111                 <mailto:[e-mail]<mailto:[e-mail]>>
1112               Acee Lindem
1113                 <mailto:[e-mail]<mailto:[e-mail]>>
1114               Jeffrey Zhang
1115                 <mailto:[e-mail]<mailto:[e-mail]>>
1116               Ladislav Lhotka
1117                 <mailto:[e-mail]<mailto:[e-mail]>>
1118               Yi Yang
1119                 <mailto:[e-mail]<mailto:[e-mail]>>
1120               Dean Bogdanovic
1121                 <mailto:[e-mail]<mailto:[e-mail]>>
1122               Kiran Agrahara Sreenivasa
1123                 <mailto:[e-mail]<mailto:[e-mail]>>
1124               Yingzhen Qu
1125                 <mailto:[e-mail]<mailto:[e-mail]>>
1126               Jeff Tantsura
1127                 <mailto:[e-mail]<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?
[SLI] NSR is a local well known and deployed mechanism. We may enhance the description if required, however we cannot really provide a reference.

...
1255         feature overload-max-metric {
1256           description
1257             "Support of overload by setting
1258              all links to max metric.";
1259         }

[minor] Reference?
[SLI] Same answer as above.

...
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.
[SLI] There is a copy/paste issue in this section, we will have a look and fix it.

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               }



...
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".
[SLI] We don’t have a reference here, we propose to remove the text about the key length.

...
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...
[SLI] I agree, each implementation and HW as its own limit. 32 is supported by a lot of implementations.We propose to remove the upper bound restriction. Which means, that max-ecmp is at last 1.


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?
[SLI] The reference is under the feature

  feature ietf-spf-delay {
    description
      "Support for IETF SPF delay algorithm.";
    reference "RFC 8405 - SPF Back-off algorithm for link
               state IGPs";
  }


...
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??
[SLI] Agree, again we have mirrored the MIB (RFC4444). I propose to remove it.

...
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.
[SLI] Yes, but this provides an accurate description on the conditions of the flag setting.

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?
[SLI] Agree, we will remove it, especially because the raw-data is already available at the top lsp level.

...
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.
[SLI] will do it (ISO10589)

[minor] This notification is specific to a circuit.  Why aren't the interface and its MTU included in it?
[SLI] We have mirrored from RFC4444. Interface reference is there. MTU is not but not sure that it is mandatory to get if the interface is referenced.

[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.
[SLI] The text is mirrored from RFC4444.

...
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.
[SLI] This is coming from RFC4444.



_________________________________________________________________________________________________________________________

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.