A YANG Module for IS-IS Reverse Metric
draft-ietf-lsr-yang-isis-reverse-metric-06
Yes
John Scudder
No Objection
Erik Kline
Francesca Palombini
Murray Kucherawy
Zaheduzzaman Sarker
Éric Vyncke
(Martin Vigoureux)
Note: This ballot was opened for revision 04 and is now closed.
John Scudder
Yes
Erik Kline
No Objection
Francesca Palombini
No Objection
Murray Kucherawy
No Objection
Roman Danyliw
No Objection
Comment
(2021-11-28 for -04)
Not sent
I'll echo the polish Ben Kaduk suggested on structuring the security considerations of the sensitive read and write nodes in Section 4.
Zaheduzzaman Sarker
No Objection
Éric Vyncke
No Objection
Alvaro Retana Former IESG member
No Objection
No Objection
(2021-11-22 for -04)
Sent
whole-lan is only applicable to multi-access interfaces. I was expecting something similar to how "priority" is defined (in the main module), but I can't find that here. Am I missing something?
Benjamin Kaduk Former IESG member
No Objection
No Objection
(2021-11-27 for -04)
Sent
Thanks for the simple and easy-to-read document! My comments are pretty minor. Section 2.2 grouping reverse-metric-if-config-data { description "IS-IS reverse metric config data."; container reverse-metric { description "IS-IS reverse metric data."; uses reverse-metric-data; leaf exclude-te-metric { type boolean; default false; description "If true and there is a TE metric defined for this interface then do not send the TE metric sub-TLV in the reverse metric TLV."; reference "RFC8500, Section 3.5"; } } } grouping tlv16-reverse-metric { description "IS-IS reverse metric TLV data."; container reverse-metric { description "IS-IS reverse metric TLV data."; uses reverse-metric-data; leaf te-metric { type uint32; description "The TE metric value from the sub-TLV if present."; reference "RFC8500, Section 3.5"; } } } Please confirm that Section 3.5 of RFC 8500 is the right reference for both of these; I didn't really see much there that lined up well with these descriptions. container reverse-metric { description "Announce a reverse metric to neighbors."; uses reverse-metric-if-config-data; container level-1 { description "Announce a reverse metric to level-1 neighbors."; uses reverse-metric-if-config-data; } container level-2 { description "Announce a reverse metric to level-2 neighbors."; uses reverse-metric-if-config-data; } } Are the interactions between the toplevel "reverse-metric-if-config-data" and the per-level uses of that grouping adequately specified by the core draft-ietf-isis-yang-isis-cfg such that we don't need to repeat them here? (I think it probably is.) Section 4 These are the subtrees and data nodes and their sensitivity/ vulnerability: I think the intent of the security considerations template is that we specifically talk about what bad things would happen if some unauthorized entity was writing values to the ("config true") nodes, or reading values from the ("config false") nodes. The current text here seems to just be listing the nodes without saying what happens if they're misconfigured or the contents are leaked to an unauthorized party. (On first glance, it seems like the security considerations of RFC 8500 basically cover everything that we would need to say. It's short, so we could repeat the content, or we could say that these YANG nodes correspond directly to the RFC 8500 functionality and the considerations of the functionality are described in RFC 8500.) Section 5 The core NETCONF/RESTCONF RFCs may not need to be classified as normative (we only reference them from the security considerations boilerplate), but there's not much harm in leaving them here. Appendix A Thank you for including the examples!
Lars Eggert Former IESG member
No Objection
No Objection
(2021-11-29 for -04)
Sent
Thanks to Vijay Gurbani for their General Area Review Team (Gen-ART) review (https://mailarchive.ietf.org/arch/msg/gen-art/vTI3euK_FgI2nI-8482WikVehC8). ------------------------------------------------------------------------------- All comments below are about very minor potential issues that you may choose to address in some way - or ignore - as you see fit. Some were flagged by automated tools (via https://github.com/larseggert/ietf-reviewtool), so there will likely be some false positives. There is no need to let me know what you did with these suggestions. Section 2.2. , paragraph 11, nit: > eudo-node LSP for this interface. Otherwise it will only increment the metric > ^^^^^^^^^ A comma may be missing after the conjunctive/linking adverb "Otherwise". Section 2.2. , paragraph 15, nit: > ic announcements from neighbors. By default reverse metric handling is disab > ^^^^^^^^^^ Did you mean: "By default,"? These URLs point to tools.ietf.org, which is being deprecated: * https://tools.ietf.org/html/rfcXXXX * https://tools.ietf.org/html/draft-ietf-isis-yang-isis-cfg-42 * https://tools.ietf.org/wg/lsr/
Martin Vigoureux Former IESG member
No Objection
No Objection
(for -04)
Not sent
Robert Wilton Former IESG member
No Objection
No Objection
(2021-11-29 for -04)
Sent
Hi Chris, Thanks for this YANG module. Nit: - Copyright statement in the YANG module should presumably be updated to 2021, to match the revision entry. A few comments on the YANG model: - For the interface config, reverse-metric turns up twice in the path. You could perhaps drop it from the grouping so that it only appears once? - Would it be helpful to make the top level reverse-metric container have presence? This might make more sense if constraints are ever added to validate that a metric is specified at the top level, or under at least one of the levels. - Am I right in assuming that that the level-1/level-2 config is effectively hierarchical and would override the config under the reverse-metric grouping? E.g., is it allowed to specify a metric at the top level, and the whole-lan flag only under the level-1? If so, would it be helpful to document this hierarchical behaviour in the description for the fields? - There is a default assigned to exclude-te-metric, but no default assigned to whole-lan and allow-unreachable. If the config is not hierarchical, then should these all have defaults? If the config is hierarchical then perhaps they should not have any defaults, and the use statement for the top level reverse-metric grouping should refine them with default values? Assuming that the descriptions make their hierarchical nature clear? Security Considerations: Would it also be helpful to include a reference back to the security considerations of the base ISIS YANG module, since the concerns that apply to metrics there would seem to mostly also apply here. References: - Probably need to add XML and JSON references or otherwise change the requests to edit-config or RESTCONF requests. - XML reference can be as per RFC 8342, JSON should probably be to RFC 7951. A.1. Example Enable XML Suggest retitling to: Enablement Example Using XML YANG Instance Data" A.2. Example Use XML Suggest retitling to: "Usage Example using XML YANG Instance Data" A.3. Example JSON Suggest retitling to: "Usage Example using JSON YANG Instance Data" Thanks, Rob