Hi Alvaro, This may take a couple iterations. See inline. From: Alvaro Retana <[email protected]> Date: Monday, June 24, 2019 at 4:52 PM To: Acee Lindem <[email protected]>, "[email protected]" <[email protected]> Cc: "[email protected]" <[email protected]>, Stephane Litkowski <[email protected]>, "[email protected]" <[email protected]> Subject: Re: AD Review for draft-ietf-ospf-yang-21
On June 22, 2019 at 7:03:58 PM, Acee Lindem (acee) ([email protected]<mailto:[email protected]>) wrote: Acee: Hi! Thanks for the review - we will address your comments in the -22 version. See inline. I have a couple of replies. Please take a look at the bottom because it looks like you didn’t get (or missed) part of the review. Thanks! Alvaro. ... 237 9. nsr: Support OSPF Non-Stop Routing (NSR). [minor] Reference? <acee> No IETF reference for OSPF NSR. However, I have a few patents in this area - should the model reference these? __ If they are applicable to this document, then yes, please disclose them. But NSR is not described here (so I wouldn’t think the patents apply), which is why I was asking for a reference. Right now the model says NSR can be enabled/disabled, but it doesn’t explain what is being enabled/disabled. Given that, it is not clear what vendors can claim to support, leading to potential incompatibilities and/or unintended behavior. I would like to either see a description (but this document is probably not the right place for that), a pointer to a description (which doesn’t have to be an IETF document), or to have the support taken out. If the WG agreed to what NSR means, then I would be fine with transcribing some of that here (from an existing mail thread, for example). As you know, Non-Stop routing doesn’t require protocol extensions. I’ll look for a good definition . ... 242 11. admin-control: Support Administrative control of the protocol 243 state. [minor] This seems like a "basic" feature: enabling/disabling the protocol. Why was it decided to make it optional? Are there implementations that don't support enabling/disabling? <acee> I'm happy to remove it. I was not asking you to remove it…I was simply wondering why it is optional. You seem to have removed it completely, but it is still in §2.5. Yes – the YANG tree. I’ve removed it in -23. ... ... 464 2.6. OSPF Area Configuration/Operational State ... 570 | | +--rw hello-interval? uint16 571 | | +--rw dead-interval? uint32 572 | | +--rw retransmit-interval? uint16 [minor] Why aren't rt-types:timer-value-* used for the *-timer/interval definitions? I would have thought this was exactly the reason for the definitions in rfc8294. <acee> I'd like to use these types universally. However, YANG seems to have a restriction that I can't further restrict the range. This really sucks. Will do what I can and make the best trade-off. I see you changed all, except retransmit-interval and restart-interval…. Just pointing it out in case you meant to. These data nodes didn’t have a range of 1…65535 like the rtgwg type and the types don’t support a second range. I may remove these types though since part of the type union for these types is a value that we don’t support (infinity). ... ... 1685 typedef if-state-type { 1686 type enumeration { ... 1712 enum backup { 1713 value "6"; 1714 description 1715 "Interface Backup Designated Router (BDR) state."; 1716 } [nit] s/backup/bdr To match the other instances where a BDR is referenced <acee> Good catch. Fixed. Thanks, Acee What about the rest of the comments?? https://mailarchive.ietf.org/arch/msg/lsr/LKvR2rfkPHJui_GMRerIdDNHkug I don’t know if it’s me, but several people seem to not receive the full length of my comments…but they are complete in the archive…so I’m not sure that the problem is. Please take a look at the archive — I have pasted comments after line 1716 below. I am changing the state to Revised ID Needed. Thanks! Alvaro. ... 2729 container network { 2730 when "derived-from-or-self(../../header/type, " 2731 + "'ospfv3-network-lsa')" { 2732 description 2733 "Only applies to Network LSAs."; 2734 } 2735 description "Network LSA."; [nit] Perhaps network-lsa could be a better name. Fixed in -23. ... 3053 grouping lsa-common { 3054 description 3055 "Common fields for OSPF LSA representation."; 3056 leaf decoded-completed { 3057 type boolean; 3058 description 3059 "The OSPF LSA body is fully decoded."; 3060 } [minor] What is decoded-completed indicating? Does it indicate that no errors where found, IOW, "successful decoding"? Does it mean that the LSA has been decoded (but not an indication of success, or not)? I clarified this. @@ -1955,10 +1955,14 @@ module ietf-ospf { grouping lsa-common { description "Common fields for OSPF LSA representation."; - leaf decoded-completed { + leaf decode-completed { type boolean; description - "The OSPF LSA body is fully decoded."; + "The OSPF LSA body was successfully decoded other than + unknown TLVs. Unknown LSAs types and OSPFv2 unknown + opaque LSA types are not decoded. Additionally, + malformed LSAs are generally not accepted and are + not be in the Link State Database."; Is it a time-based indication; IOW, is there a chance that an LSA has not been processed when retrieving the information...would that result in false?? This wouldn’t be possible in any sensible implementation. Only LSDB only includes the binary form of the data and the API would decode the individual fields on an atomic version of the LSA. ... 3271 grouping instance-fast-reroute-state { 3272 description "IPFRR state data grouping"; [nit] s/IPFRR/IP-FRR That is how the term is used everywhere else in this document. Fixed. ... 3541 leaf hello-interval { 3542 type uint16 { 3543 range "1..65535"; 3544 } 3545 description 3546 "Interval between hello packets (seconds). It must 3547 be the same for all routers on the same network. 3548 Different networks, implementations, and deployments 3549 will use different hello-intervals. A sample value 3550 for a LAN network would be 10 seconds."; 3551 } [minor] Some implementations support having a sub-second hello-interval...but with a timer in seconds that can't be done. This is a good thing. Sub-second hellos were a misguided proprietary feature implemented and then regretted by several prominent vendors. ... 3664 leaf ospfv2-key { 3665 type string; 3666 description 3667 "OSPFv2 authentication key. The 3668 length of the key may be dependent on the 3669 cryptographic algorithm. In cases where it is 3670 not, a key length of at least 32 octets should 3671 be supported to allow for interoperability 3672 with strong keys."; [major] Is there a reference for the part about the "key length of at least 32 octets should be supported to allow for interoperability with strong keys"? No. We should remove this and use what we finally settled on the key-chain (RFC 8177). As a co-author, I wanted to defer to key-chains but there are implementations that allow key specificatons in the protocols (actually most of them support this at least for backward compatibility ... 3719 leaf ospfv3-key { 3720 type string; 3721 description 3722 "OSPFv2 authentication key. The 3723 length of the key may be dependent on the 3724 cryptographic algorithm. In cases where it is 3725 not, a key length of at least 32 octets should 3726 be supported to allow for interoperability 3727 with strong keys."; [major] The same comment from ospfv2-key applies here. Same response applies. [nit] s/OSPFv2 authentication key/OSPFv3 authentication key Fixed in -23 ... 3856 leaf priority { 3857 type uint8 { 3858 range "1..255"; 3859 } 3860 description "Neighbor priority for DR election."; 3861 } [minor] A range is included in this definition, but not in other priority statements before...should a range be included in those other definitions? This will be removed in -23. [minor] Why is 0 not applicable here? It is applicable and indicates the router is not eligible for DR/BDR election. The range will be removed. Refer to appendix C.5 in RFC 2328. All neighbors on NBMA network are to be configured (eligible and non-eligible). ... 3936 leaf dead-timer { 3937 type uint32; 3938 units "seconds"; 3939 config false; 3940 description "This timer tracks the remaining time before 3941 the neighbor is declared dead."; 3942 } [major] For *-timer: Is tracking the remaining time in seconds enough? I would assume that ms would be the right unit. Why seconds? Because sub-second hellos was a bad idea – three words: B-F-D… ... 4390 container preference { 4391 description "Route preference config state."; [minor] Maybe it's just me, Yeah – I think you need to refresh your CCIE certification. but I don't understand what this piece of the model does. It seems like it can be used to configure which type of route is preferred: external over intra-area, for example...but that doens't make too much sense to me. Is that the intended use? Can you please give me an example of when this functionality could be used? I’m going to add a reference to RFC 8349 route-preference and also add to the description that this is “also known as, Administrative Distance”. 4392 choice scope { 4393 description 4394 "Options for expressing preference 4395 as single or multiple values."; 4396 case single-value { 4397 leaf all { 4398 type uint8; 4399 description 4400 "Preference for intra-area, inter-area, and 4401 external routes."; 4402 } 4403 } 4404 case multi-values { 4405 choice granularity { 4406 description 4407 "Options for expressing preference 4408 for intra-area and inter-area routes."; 4409 case detail { 4410 leaf intra-area { 4411 type uint8; 4412 description 4413 "Preference for intra-area routes."; 4414 } 4415 leaf inter-area { 4416 type uint8; 4417 description 4418 "Preference for inter-area routes."; 4419 } 4420 } 4421 case coarse { 4422 leaf internal { 4423 type uint8; 4424 description 4425 "Preference for both intra-area and 4426 inter-area routes."; 4427 } 4428 } 4429 } 4430 leaf external { 4431 type uint8; 4432 description 4433 "Preference for AS external routes."; 4434 } 4435 } 4437 } 4438 } ... 4551 container stub-router { 4552 if-feature stub-router; 4553 description "Set maximum metric configuration"; 4555 choice trigger { 4556 description 4557 "Specific triggers which will enable stub 4558 router state."; 4559 container always { 4560 presence 4561 "Enables unconditional stub router support"; 4562 description 4563 "Unconditional stub router state (advertise 4564 transit links with max metric"; [minor] s//MaxLinkMetric [rfc6897] Fixed in -23 – RFC 6987. ... 5537 4. Security Considerations ... 5552 There are a number of data nodes defined in ietf-ospf.yang module 5553 that are writable/creatable/deletable (i.e., config true, which is 5554 the default). These data nodes may be considered sensitive or 5555 vulnerable in some network environments. Write operations (e.g., 5556 edit-config) to these data nodes without proper protection can have a 5557 negative effect on network operations. For OSPF, the ability to 5558 modify OSPF configuration will allow the entire OSPF domain to be 5559 compromised including peering with unauthorized routers to misroute 5560 traffic or mount a massive Denial-of-Service (DoS) attack. The 5561 security considerations of OSPFv2 [RFC2328] and [RFC5340] apply to 5562 the ietf-ospf.yang module as well. [minor] s/OSPFv2 [RFC2328] and [RFC5340]/OSPFv2 [RFC2328] and OSPFv3 [RFC5340] [major] How do the "security considerations of OSPFv2 [RFC2328] and [RFC5340] apply to the ietf-ospf.yang module"? rfc2328/rfc5340 only talk about authentication of the protocol exchange... Well, whoever would want fewer security considerations… However, I tend to agree and will remove this. I also plan to add the text regarding keys. ... 5626 7.1. Normative References ... 5679 [RFC4750] Joyal, D., Ed., Galecki, P., Ed., Giacalone, S., Ed., 5680 Coltun, R., and F. Baker, "OSPF Version 2 Management 5681 Information Base", RFC 4750, DOI 10.17487/RFC4750, 5682 December 2006, <https://www.rfc-editor.org/info/rfc4750>. ... 5731 [RFC5643] Joyal, D., Ed. and V. Manral, Ed., "Management Information 5732 Base for OSPFv3", RFC 5643, DOI 10.17487/RFC5643, August 5733 2009, <https://www.rfc-editor.org/info/rfc5643>. ... 5745 [RFC5880] Katz, D. and D. Ward, "Bidirectional Forwarding Detection 5746 (BFD)", RFC 5880, DOI 10.17487/RFC5880, June 2010, 5747 <https://www.rfc-editor.org/info/rfc5880>. 5749 [RFC5881] Katz, D. and D. Ward, "Bidirectional Forwarding Detection 5750 (BFD) for IPv4 and IPv6 (Single Hop)", RFC 5881, 5751 DOI 10.17487/RFC5881, June 2010, <https://www.rfc- 5752 editor.org/info/rfc5881<http://editor.org/info/rfc5881>>. [minor] I think these references can be Informative. I agree. ... 5880 7.2. Informative References [major] All the references in this section (except maybe rfc905) point at functionality in the model...as the references above, these should be Normative as well. I agree. These documents are already in the Downref registry: rfc5309, rfc5443, rfc5714, rfc6987. I don’t why a standards track document can’t have normative references to informational RFCs – especially YANG models. The others are not, but I don't expect any issues in the IETF Last Call. If there are, hopefully they will be truncated. Thanks, Acee
_______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
