Hi Mahesh, I'lll post a new version tomorrow including the changes.
> On Jun 29, 2026, at 6:30 PM, Mahesh Jethanandani via Datatracker > <[email protected]> wrote: > > 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. Fixed. > > > 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. Added. > > 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. Fixed. > > > 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. Fixed and added reference to RFC 5307, > > > 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. You are correct in surmising that that these the values for these TE metrics are not configured here. They are either derived (e.g., bandwidth), dynamically measured (e.g., delay), or potentially inherited (e.g., max-reserved-bandwidth). If there are scenarios where configuration is desired, these attributes could be added via augmentation. I'm not going to add descriptions of leaves that are not present. > > > 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. Added > > > 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. Added. > > ---------------------------------------------------------------------- > 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/ Fixed. > > 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. Fixed. Thanks, Acee > > > _______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
