[Lsr] AD Review of draft-ietf-isis-yang-isis-cfg-35
Alvaro Retana <aretana.ietf@gmail.com> Tue, 04 June 2019 21:34 UTC
Return-Path: <aretana.ietf@gmail.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 11F61120397; Tue, 4 Jun 2019 14:34:34 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.997
X-Spam-Level:
X-Spam-Status: No, score=-1.997 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8AKAZgkkthV4; Tue, 4 Jun 2019 14:34:29 -0700 (PDT)
Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D6F1B1200A4; Tue, 4 Jun 2019 14:34:25 -0700 (PDT)
Received: by mail-ed1-x52e.google.com with SMTP id a8so2582539edx.3; Tue, 04 Jun 2019 14:34:25 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:mime-version:date:message-id:subject:to:cc; bh=ZWkKg9Tb72zQQhv+nCioVe83cgSKz+WHGcq7XQSVGK0=; b=tNPx1iGG31mZIQpDjx2Zv8Rq+rlx8yOSB5zKSctHBuO6u142JwgpR0nO9G71AFuOzK Y/U4+clYBP6PcMj71Le1dfMUBQpWuOxpGklADhmN2fgD7zrwP1ls1QLzzLsbYdxuntyf iInzh6lHvTYAV6k5e+ttTEwPZcRzWMu3RKkvVHjQQxwK0GMAt24rZPii2mtvkYGK18p3 rLEPLxjW6ov75p9gwjSbhxZ/ClY+APRj3wITJv2OKpHtgsWUZ0fIUwz3JTVxmJSyk0XZ rbki0o5gKUOrN+3xHfajhQ7xknkU/4lLrmG0W02Em5sirmnGEKaz48MVBsHrL4BMisp5 r4gg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:mime-version:date:message-id:subject:to:cc; bh=ZWkKg9Tb72zQQhv+nCioVe83cgSKz+WHGcq7XQSVGK0=; b=Y7GdH/BQIRA3qb+dwcrslCUTeC3r/Aapsd4e5WBQXDNBodF0vj04qOBKXZZIKTx8gv pKpxjKl6gk+zMXgUPLVO0ozJVGyT//qJ7jgHQ/h0n9DeqaWCm8GEpZcdtNzrdkVXTsLR KovH0cqWPUMtL/xzl/6s5Fss64Mtl0Y0Ckbzg8xttIFeax/WdnH+2UFbtP1Q3rudRk3p FyD21aBNvM+Bn+bJfTaJpUgVnOcEla5OdbfJledWgeSe131pKA83ADRRSg8hvGjhDlTs 6KPxUk+3ySJXzbPVl5i798dZ67K7aARaJcGOJ1ZPeWOQWt23dMh+2qhsx4AHfMeouPT5 FMSg==
X-Gm-Message-State: APjAAAVlQsy8mB19TP6aoBATN9Hm2cyzWMnRpcvoMSffW71apQl6TwMn eyiUF7MPx78A7wE+MH9LfBJaDwcz+uE1Z8/6Ds9fQQ==
X-Google-Smtp-Source: APXvYqyWvK85bwv+dPKad5/PQ8HBlmNJyy3A6ISgX7vsPDHzg4sl6PmHxJi8GXC+VeCG8ilvdKETClxevAvCxLYCmgE=
X-Received: by 2002:a17:906:2ada:: with SMTP id m26mr4029896eje.265.1559684063784; Tue, 04 Jun 2019 14:34:23 -0700 (PDT)
Received: from 1058052472880 named unknown by gmailapi.google.com with HTTPREST; Tue, 4 Jun 2019 14:34:22 -0700
From: Alvaro Retana <aretana.ietf@gmail.com>
MIME-Version: 1.0
Date: Tue, 04 Jun 2019 14:34:22 -0700
Message-ID: <CAMMESsxQGGj_PmmjeRqBfTgU=Z2=Eu8Yn9FXLHgEm1PorTaUqA@mail.gmail.com>
To: draft-ietf-isis-yang-isis-cfg@ietf.org
Cc: Yingzhen Qu <yingzhen.ietf@gmail.com>, lsr-chairs@ietf.org, lsr@ietf.org
Content-Type: multipart/alternative; boundary="000000000000957309058a863e6e"
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/eQLV1_xUYsP3zo2kDujRfGSWs6s>
Subject: [Lsr] AD Review of draft-ietf-isis-yang-isis-cfg-35
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 04 Jun 2019 21:34:34 -0000
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:stephane.litkowski@orange.com> 1110 Derek Yeung 1111 <mailto:derek@arrcus.com> 1112 Acee Lindem 1113 <mailto:acee@cisco.com> 1114 Jeffrey Zhang 1115 <mailto:zzhang@juniper.net> 1116 Ladislav Lhotka 1117 <mailto:llhotka@nic.cz> 1118 Yi Yang 1119 <mailto:yiya@cisco.com> 1120 Dean Bogdanovic 1121 <mailto:deanb@juniper.net> 1122 Kiran Agrahara Sreenivasa 1123 <mailto:kkoushik@brocade.com> 1124 Yingzhen Qu 1125 <mailto:yiqu@cisco.com> 1126 Jeff Tantsura 1127 <mailto:jefftant.ietf@gmail.com> [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] AD Review of draft-ietf-isis-yang-isis-cfg-… Alvaro Retana
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… stephane.litkowski
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Alvaro Retana
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Acee Lindem (acee)
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Alvaro Retana
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Alvaro Retana
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Acee Lindem (acee)
- Re: [Lsr] AD Review of draft-ietf-isis-yang-isis-… Alvaro Retana