Mohamed Boucadair has entered the following ballot position for draft-ietf-lsr-isis-flex-algo-yang-08: Discuss
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/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- Hi Yingzhen, Acee, and Madhavi, Thank you for the effort put into this specification. Please find some comments and suggestions. These are easy-to-fix. Let me know if any clarification is needed. # Lack or narrative text RFC9907 says: The narrative sections MUST include an overview section that describes the scope and field of application of the data model(s) defined by the specification and that specifies the relationship (if any) of these data models to other standards, particularly to standards containing other YANG data models. The narrative part SHOULD include one or more sections to briefly describe the structure of the data models defined in the specification. At least an overview is needed (MUST part above). I remember that we had the same comment for RFC9903. Having something similar to the text you added in “Design of the YANG Module for OSPF MPLS Segment Routing” of RFC9903 would be great. Thanks. # te-metric CURRENT: leaf te-metric { type uint32; description "TE default metric."; reference "RFC 5305: IS-IS Extensions for Traffic Engineering, Section 3.7"; Why not using this type from draft-ietf-teas-rfc8776-update typedef te-metric { type uint32; description "Traffic Engineering (TE) metric."; reference "RFC 3630: Traffic Engineering (TE) Extensions to OSPF Version 2, Section 2.5.5 RFC 5305: IS-IS Extensions for Traffic Engineering, Section 3.7"; } # admin-group CURRENT: leaf admin-group { type uint32; description "Administrative Group / Resource Class/Color."; reference "RFC 5305: IS-IS Extensions for Traffic Engineering, Section 3.1"; } This definition deviates from the one in draft-ietf-teas-rfc8776-update: typedef admin-group { type yang:hex-string { /* 01:02:03:04 */ length "1..11"; } description "Administrative group / resource class / color representation in 'hex-string' type. The most significant byte in the hex-string is the farthest to the left in the byte sequence. Leading zero bytes in the configured value may be omitted for brevity."; reference "RFC 3630: Traffic Engineering (TE) Extensions to OSPF Version 2 RFC 5305: IS-IS Extensions for Traffic Engineering RFC 7308: Extended Administrative Groups in MPLS Traffic Engineering (MPLS-TE)"; } Unless there is a valid reason, I recommend to reuse the type defined in 8776. # Priority Unicity CURRENT: list unreserved-bandwidth { leaf priority { type uint8 { range "0 .. 7"; } description "Priority from 0 to 7."; } leaf unreserved-bandwidth { type rt-types:bandwidth-ieee-float32; description "Unreserved bandwidth."; } description "List of unreserved bandwidths for different priorities."; } Do we allow to repeat the same priority? How to ensure unicity? At least for ro, this should be flagged as an anomaly. A discussion in an OPS Considerations Section would be helpful here. ## Idem for CURRENT: list fad-tlv { leaf algo-number { type uint8; description "Flex-algo number - A value not in the inclusive range of 128-255 is an invalid FAD TLV specification."; } leaf metric-type { type identityref { base iana-metric-type:metric-type; } description "Type of metric to be used during the calculation."; } leaf calc-type { type identityref { base iana-algo-types:algo-type; } description "IGP algorithm types, value from 0 to 127 as defined under 'Interior Gateway Protocol (IGP) Parameter' by IANA."; } leaf priority { type uint8; description "Priority of the advertisement."; } # Deviate from RFC 9479 CURRENT: leaf l-flag { type boolean; description "Legacy Flag. When set, all of the applications specified in the bit mask MUST use the legacy advertisements."; } RFC9479: When the SABM Length or UDABM Length is non-zero and the L-flag is NOT set, all applications specified in the bit mask MUST use the link attribute advertisements in the sub-TLV. ## The description does not mirror exactly the behavior in RFC 9479. ## An easy fix is to remove the normative language and update the description to focus on the leaf itself (management) not the on-wire spec. # Length fields: Intended use CURRENT: leaf sabm-length { type uint8; description "Standard Application Identifier Bit Mask Length in octets."; } leaf udabm-length { type uint8; description "User Defined Application Identifier Bit Mask Length in octets."; } … leaf length-of-sub-tlvs { type uint8; description "Length of sub-tlvs."; } ## I understand this is needed for the protocol, but why do we need these for managing an implem? ## If these are maintained (which I don’t see why), then you need to update the module to have a constraint for sabm per: RFC 9479: This field is omitted if SABM Length is 0. # Provision for future augmentations, but without guidance CURRENT: container udabm { description "User Defined Application Identifier Bit Mask. This container is to be augmented by user defined applications."; } The intended use/guidance should be defined in the main document. Please add some text to set the expectations for authors for future augmentations. # Abnormal configuration CURRENT: | +--ro ipv4-interface-addr | | +--ro ipv4-int-addr? inet:ipv4-address | +--ro ipv4-neighbor-addr | | +--ro ipv4-neighbor-addr? inet:ipv4-address | +--ro ipv6-interface-addr | | +--ro ipv6-int-addr? inet:ipv6-address | +--ro ipv6-neighbor-addr | +--ro ipv6-neighbor-addr? inet:ipv6-address The module/document does not include any discussion about checks to detect abnormal behaviors with regards to address family mismatch. Can we please gave some text? # srlg CURRENT: leaf-list srlg { type uint32; description "SRLG value of the link."; } Why no reusing this type from draft-ietf-teas-rfc8776-update? typedef srlg { type uint32; description "Shared Risk Link Group (SRLG) type."; reference "RFC 4203: OSPF Extensions in Support of Generalized Multi-Protocol Label Switching (GMPLS) RFC 5307: IS-IS Extensions in Support of Generalized Multi-Protocol Label Switching (GMPLS)"; } # Update to draft-ietf-teas-rfc8776-update CURRENT: import ietf-te-types { prefix te-types; reference "RFC8776: Common YANG Data Types for Traffic Engineering."; } # missing normative import CURRENT: import ietf-te { prefix te; reference "RFC XXXX: A YANG Data Model for Traffic Engineering Tunnels, Label Switched Paths, and Interfaces"; } RFC9907: For every "import" or "include" statement that appears in a module contained in the specification that identifies a module in a separate document, a corresponding normative reference to that document MUST appear in the Normative References section. # Consistency with RFC 9350 CURRENT: identity m-bit { base fad-flags; description "M bit, when set, the flex-algo specific prefix and ASBR metric MUST be used for inter-area and external prefix calculation."; } ## RFC 9350 has M-flag. If we refer to the same object, then please update to be consistent with 9350. ## This is an identity, not a Boolean. I don’t understand what is meant by set here. ## (almost) Duplicate RFC 9350 behavior, but more importantly lack of management object description M-flag: when set, the Flex-Algorithm-specific prefix metric MUST be used for inter-area and external prefix calculation. This flag is not applicable to prefixes advertised as SRv6 locators. Note also the exception part in the RFC 9350. Please update the description to focus on the intended use of the management object. # extended-admin-group CURRENT: leaf-list extended-admin-groups { type uint64; description "Extended administrative group as defined in RFC 7308."; } Any reason why we can’t use extended-admin-group from draft-ietf-teas-rfc8776-update? As I’m there, please s/ extended-admin-groups/ extended-admin-group # Valid Examples RFC9907 says: Examples that are meant to illustrate a valid data instance MUST be validated (Section 3.10). The JSON Example in Appendix B is not valid (at least modules prefixes are needed). Please run yangson or yanglint for all the examples. ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- # Title OLD: YANG Data Model for IS-IS Application-Specific Link Attributes and Flexible Algorithm NEW: A YANG Data Model for IS-IS Application-Specific Link Attributes and Flexible Algorithm Please update the modules accordingly. # Module, model, etc. Per https://datatracker.ietf.org/doc/html/rfc9907#section-2.5, Note that the term "YANG model" is sometimes used as an abbreviation of "YANG data model". However, that term should be avoided in favor of "YANG data model". Likewise, "YANG data module" has no meaning and must be avoided. OLD: This document defines two YANG [RFC7950] data modules. NEW: This document defines a YANG [RFC7950] data model, which is composed to two YANG modules. OLD: Algorithm in JSON format using the YANG model NEW: Algorithm in JSON format using the YANG module # Tree diagrams RFC9907: If YANG tree diagrams are used, then an informative reference to the YANG tree diagrams specification MUST be included in the document. Please update 1.1 with the following: NEW: The meanings of the symbols in the YANG tree diagrams are defined in [RFC8340]. # RFC9911 CURRENT: import ietf-inet-types { prefix inet; reference "RFC 6991: Common YANG Data Types"; } Please update to 9911. # IANA-Maintained Modules CURRENT: import iana-igp-link-attr-apps { prefix iana-link-attr-apps; reference "RFC XXXX: YANG Data Model for OSPF Application-Specific Link Attributes and Flexible Algorithm"; } … import iana-igp-algo-types { prefix iana-algo-types; reference "RFC XXXX: YANG Data Model for OSPF Application-Specific Link Attributes and Flexible Algorithm"; } import iana-igp-metric-types { prefix iana-metric-type; reference "RFC XXXX: YANG Data Model for OSPF Application-Specific Link Attributes and Flexible Algorithm"; } The import applies, not only for the initial version. Please consider updating the reference to follow this part from 9907: RFC9907: If an IANA-maintained YANG module is imported by another module, a normative reference with the IANA URL from which to retrieve the IANA-maintained YANG module SHOULD be included. Although not encouraged, referencing the RFC that defines the initial version of the IANA module is acceptable in specific cases (e.g., the imported version is specifically the initial version, the RFC includes useful description about the usage of the module). # RFC XXXX is ambiguous as it refers to distinct docs CURRENT: reference "RFC XXXX: YANG Data Model for OSPF Application-Specific Link Attributes and Flexible Algorithm"; "RFC XXXX: A YANG Data Model for Traffic Engineering Tunnels, Label Switched Paths, and Interfaces"; # Only NMDA exceptions are needed to be called out CURRENT: This YANG model conforms to the Network Management Datastore Architecture (NMDA) as described in RFC 8342. Consider delete that statement. # Follow IETF Template OLD: This version of this YANG module is part of RFC XXXX; see the RFC itself for full legal notices. NEW: All revisions of IETF and IANA published modules can be found at the YANG Parameters registry group (https://www.iana.org/assignments/yang-parameters). This version of this YANG module is part of RFC XXXX; see the RFC itself for full legal notices. # Consider adding a reference statement to rfc9479 for application-identifier-bit-mask grouping # l-flag: can be set to true or false OLD: leaf l-flag { type boolean; description "Legacy Flag. When set, … } NEW: leaf l-flag { type boolean; description "Legacy Flag. When set to true, … } # Missing units statements CURRENT: leaf sabm-length { type uint8; description "Standard Application Identifier Bit Mask Length in octets."; } leaf udabm-length { type uint8; description "User Defined Application Identifier Bit Mask Length in octets."; } Consider adding a units statement rather than having this in the description. # Lists and leaf-lists CURRENT: leaf-list sabm-bits { type identityref { base iana-link-attr-apps:igp-link-attr-app; } description "SABM bits list. This list will contain identities for the bits which are set in the SABM bits."; } RFC 9907 says: List identifiers SHOULD be singular with the surrounding container name plural. Similarly, "leaf-list" identifiers SHOULD be singular. NEW: leaf-list sabm-bit { type identityref { base iana-link-attr-apps:igp-link-attr-app; } description "SABM bits list. This list will contain identities for the bits which are set to 1 in the SABM bits."; } ## (also fixed s/set/set to 1) ## There are several such constructs that need to be fixed. I’m not listing all these, but please check. # Not only configuration, but also for retrieval CURRENT: /* Configuration */ Please update to “Management” or something similar. ## Idem OLD: This augments IS-IS protocol configuration NEW: This augments IS-IS protocol management ## Idem OLD: This augments IS-IS interface configuration NEW: This augments IS-IS interface management # m-bit CURRENT: identity m-bit { base fad-flags; description "M bit, when set, the flex-algo specific prefix and ASBR metric MUST be used for inter-area and external prefix calculation."; } Same comments as l-flag # Embedded references in the description CURRENT: description "Extended administrative group as defined in RFC 7308."; } Please use a reference statement for this (and similar) # Applies when enabled to true OLD: leaf advertise-definition { type boolean; default "true"; description "Enable to advertise the flex-algo definition."; } NEW/ leaf advertise-definition { type boolean; default "true"; description "When set to true, enables to advertise the flex-algo definition."; } # Specification? CURRENT: "Flex-algo identifier which is not supported by the IS-IS instance. A value not in the inclusive range of 128-255 is indicative of an invalid FAD algorithm specification."; Which specification? # Update Security considerations to cover the notifications # Two modules OLD: This document registers a URI in the IETF XML registry NEW: This document registers two URIs in the IETF XML registry OLD: This document registers the YANG module NEW: This document registers the YANG modules # This dependency can be avoided as the normative source of the IANA-maintained module is the IANA registry itself. CURRENT: 7.1. Normative References [I-D.ietf-lsr-ospf-flex-algo-yang] Qu, Y. and A. Lindem, "YANG Data Model for OSPF Application-Specific Link Attributes and Flexible Algorithm", Work in Progress, Internet-Draft, draft-ietf- lsr-ospf-flex-algo-yang-07, 30 March 2026, <https://datatracker.ietf.org/doc/html/draft-ietf-lsr- ospf-flex-algo-yang-07>. # Not Normative: Please move the following to Informative CURRENT: 7.1. Normative References [RFC4252] Ylonen, T. and C. Lonvick, Ed., "The Secure Shell (SSH) Authentication Protocol", RFC 4252, DOI 10.17487/RFC4252, January 2006, <https://www.rfc-editor.org/info/rfc4252>. [RFC6241] Enns, R., Ed., Bjorklund, M., Ed., Schoenwaelder, J., Ed., and A. Bierman, Ed., "Network Configuration Protocol (NETCONF)", RFC 6241, DOI 10.17487/RFC6241, June 2011, <https://www.rfc-editor.org/info/rfc6241>. [RFC8040] Bierman, A., Bjorklund, M., and K. Watsen, "RESTCONF Protocol", RFC 8040, DOI 10.17487/RFC8040, January 2017, <https://www.rfc-editor.org/info/rfc8040>. [RFC8446] Rescorla, E., "The Transport Layer Security (TLS) Protocol Version 1.3", RFC 8446, DOI 10.17487/RFC8446, August 2018, <https://www.rfc-editor.org/info/rfc8446>. [RFC9000] Iyengar, J., Ed. and M. Thomson, Ed., "QUIC: A UDP-Based Multiplexed and Secure Transport", RFC 9000, DOI 10.17487/RFC9000, May 2021, <https://www.rfc-editor.org/info/rfc9000>. Hope this helps. Cheers, Med _______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
