Mahesh Jethanandani has entered the following ballot position for draft-ietf-lsr-isis-flex-algo-yang-15: No Objection
When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.) Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ for more information about how to handle DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-lsr-isis-flex-algo-yang/ ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- 1. Section 2, ietf-isis-link-attr, fa-in-all-ag-sub-tlv grouping: 979 > description 980 > "The flex-algo include-all admin group sub-tlv. Note that 981 > the presence of more than one Include-Any Extended Admin 982 > Group sub-TLV will result in an invalid FAD TLV."; The description for the fa-in-all-ag-sub-tlv container says "Include-Any" where it should say "Include-All". This appears to be a copy-paste from the fa-in-any-ag-sub-tlv grouping. In my opinion, this should be corrected because an implementor reading this description will be misled about which constraint applies. 2. Section 4, Security Considerations — missing writable container: 1302 > There are a number of data nodes defined in this YANG module that are 1303 > writable/creatable/deletable (i.e., "config true", which is the 1304 > default). All writable data nodes are likely to be reasonably 1305 > sensitive or vulnerable in some network environments. ... 1311 > /isis:isis/isis-link-attr/link-attr-op-mode - Modification to the 1312 > protocol operation mode may result in traffic disruption. 1313 > 1314 > /isis:isis/isis-flex-algo/flex-algo - Modification to flex-algo 1315 > configuration could be used to mount a DoS attack. The Security Considerations list two sensitive writable subtrees but omit the interface-level ASLA configuration: 639 > container isis-asla { 640 > list interface-asla { 641 > key "link-attr-app"; The `isis-asla/interface-asla` list is writable (config true) and controls per-application TE metric, admin-group, and SRLG values on each IS-IS interface. Unauthorized modification of these attributes can redirect traffic flows for specific applications or create routing inconsistencies in multi-area topologies. RFC 9907 requires that particularly sensitive writable nodes be explicitly listed by name. In my view, this subtree should be added to the writable nodes list in the Security Considerations. 3. Section 3, ietf-isis-flex-algo, admin-groups — leaf-list of full bitmasks vs. single bitmask in the LSDB: 1127 > leaf-list exclude-admin-groups { 1128 > type te-types:extended-admin-group; 1129 > description 1130 > "Exclude rule used during the flex-algo 1131 > path computation."; The LSDB operational state models each admin group sub-TLV as a single leaf: 773 > +--ro extended-admin-group? 774 > te-types:extended-admin-group RFC 9350 Section 6.1 states: "The IS-IS FAEAG sub-TLV MUST NOT appear more than once in a single IS-IS FAD sub-TLV." The same constraint applies to the Include-Any sub-TLV (Section 6.2) and the Include-All sub-TLV (Section 6.3). Each sub-TLV carries a single Extended Admin Group value (a variable-length bitmask per RFC 7308). The LSDB model is therefore correct in using a single optional leaf for each. The config model uses `leaf-list exclude-admin-groups` (and similarly for include-any and include-all), where `te-types:extended-admin-group` is a variable-length hex-string representing the full bitmask. It is not defined how multiple leaf-list entries are combined into a single sub-TLV that can appear on the wire. In my opinion, the config model should either use a single leaf (consistent with the LSDB representation and the protocol constraint) or include a clear description of how multiple leaf-list entries are merged when building the wire-format sub-TLV. 4. Section 2, ietf-isis-link-attr, link-local-id / link-remote-id union type: 504 > leaf link-local-id { 505 > type union { 506 > type inet:ipv4-address; 507 > type uint32; 508 > } 509 > description 510 > "Local identifier of the link. 511 > It could be an IPv4 address or a local identifier."; 513 > leaf link-remote-id { 514 > type union { 515 > type inet:ipv4-address; 516 > type uint32; 517 > } These leaves model the Link Local/Remote Identifiers sub-TLV. RFC 5307 Section 1.1 defines this sub-TLV as carrying a pair of 32-bit integer link identifiers — not IPv4 addresses. IPv4 interface and neighbor addresses are separate sub-TLVs, already modeled in the ipv4-interface-addr and ipv4-neighbor-addr containers present in the same link-id-sub-tlvs container. Including `inet:ipv4-address` in the union conflates the Link Local/Remote Identifiers sub-TLV with those separate IPv4 address sub-TLVs. The type of link-local-id and link-remote-id should be `uint32`, matching the protocol definition. If there are implementations that reuse an IPv4 address as the link identifier value, that practice should be explicitly documented rather than absorbed silently into the type union. 5. Section 2, ietf-isis-link-attr, interface-asla config completeness: 639 > container isis-asla { 640 > list interface-asla { 641 > key "link-attr-app"; 651 > leaf te-metric { ... } 658 > leaf admin-group { ... } 665 > container srlgs { ... } The `interface-asla` list allows configuring te-metric, admin-group, and SRLG per application. RFC 9479 defines the ASLA sub-TLV as a general mechanism for advertising any link attribute (including TE bandwidth metrics, delay, and loss) on a per-application basis. The LSDB operational state for the asla-sub-tlv (lines 130-178) exposes max-bandwidth, max-reservable-bandwidth, unreserved-bandwidths, and various performance metrics (unidirectional-link-delay, delay-variation, loss, etc.) — but none of these are configurable through the interface-asla list. If per-application bandwidth and performance metric values are intentionally auto-derived from the link layer (rather than configured), in my view, the document should explicitly say so. Without this explanation, it is unclear whether the absence of bandwidth and delay configuration is a deliberate design decision or an incomplete model. 6. Section 4, Security Considerations — RFC 9907 template, RPC/action clause: This is more of an NIT. The Security Considerations section addresses writable and readable data nodes but does not include an explicit statement about RPC or action operations. RFC 9907 Section 3.7 requires that "Operations (i.e., YANG 'rpc' statements) that are potentially harmful to system behavior or that raise significant privacy concerns MUST be explicitly listed by name." Neither module defines any RPCs or actions. I would suggest adding a sentence stating that no RPC or action operations are defined in these modules. 7. Section 2, ietf-isis-link-attr — missing RFC 8294 in normative references: 286 > import ietf-routing-types { 287 > prefix rt-types; 288 > reference 289 > "RFC 8294: Common YANG Data Types for the 290 > Routing Area"; The ietf-isis-link-attr module imports ietf-routing-types (RFC 8294) and uses rt-types:bandwidth-ieee-float32 in multiple leaves. RFC 8294 does not appear in the normative references in Section 7.1. Please add. ---------------------------------------------------------------------- NIT ---------------------------------------------------------------------- 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, ietf-isis-link-attr, unreserved-bandwidth list, priority leaf description: 395 > description 396 > "Priority from 0 to 7. There should on be a single 397 > unreserved-bandwidth per priority."; s/There should on be a single/There should only be a single/ Section 3, ietf-isis-flex-algo, import of ietf-te-types, reference string: Section 3, ietf-isis-flex-algo, contact field: 875 > Author: Acee Lindem 876 > <mailto:[email protected]> 877 > Author: Madhavi Joshi 878 > <[email protected]> The mailto: URI scheme prefix is present for Qu and Lindem in this module but absent for Joshi. The same inconsistency appears in the ietf-isis-link-attr module contact field. For consistency, please add the mailto: prefix uniformly across all authors in both modules. _______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
