Hi All, Any feedbacks about the discuss item raised by Med?
G/ -----Original Message----- From: Mohamed Boucadair via Datatracker <nore...@ietf.org> Sent: Tuesday, March 25, 2025 3:19 PM To: The IESG <i...@ietf.org> Cc: draft-ietf-isis-sr-y...@ietf.org; lsr-cha...@ietf.org; lsr@ietf.org; cho...@chopps.org Subject: Mohamed Boucadair's Discuss on draft-ietf-isis-sr-yang-25: (with DISCUSS and COMMENT) CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information. Mohamed Boucadair has entered the following ballot position for draft-ietf-isis-sr-yang-25: 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-isis-sr-yang/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- Hi Stéphane, Yingzhen, Pushpasis, Helen, and Jeff, Thank you for the effort put into this well-written document. I appreciate your perseverance to push this specification for almost 10 years. I have a DISCUSS point and a set of comments that I'd like we address. Main comments are tagged with (*). # DISCUSS CURRENT: leaf length { type uint8; description "Length of the SID value. YANG model specification is necessary since it dictates the semantics of the SID."; } leaf sid { type uint32; description "Segment Identifier (SID) - A 20 bit label or 32 bit SID. If the length is set to 3, then the 20 rightmost bits represent an MPLS label. If the length is set to 4, then the value is a 32-bit index."; } This construct mirrors how the object is encoded on the wire, but when it comes to management a type is better instead fo inferring its from the length. I suggest to use an explicit type here instead of an implicit type. ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- # Abstract OLD: This document defines a YANG data module that can be used to configured and manage IS-IS Segment Routing for MPLS data plane. NEW: This document defines a YANG data module that can be used to manage IS-IS Segment Routing for MPLS data plane. Delete "configure" as this is covered by "manage". Remember the FCAPS ;-) # Section 2 Clarify this is a device model + Cite where the base model is defined when first cited + avoid restricting the model to configuration. We claim anyway that the module is NMDA compliant. OLD: This document defines a YANG model for IS-IS Segment Routing for the MPLS data plane. It is an augmentation of the IS-IS base model. The IS-IS SR MPLS YANG module requires support for the base segment routing module [RFC9020], which defines the global segment routing configuration independent of any specific routing protocol configuration, and support of IS-IS base model [RFC9130] which defines basic IS-IS configuration and state. NEW: This document defines a device YANG model for IS-IS Segment Routing for the MPLS data plane. It is an augmentation of the IS-IS base model [RFC9130]. The IS-IS SR MPLS YANG module requires support for the base segment routing module [RFC9020], which defines the global SR management structure independent of any specific routing protocol, and support of IS-IS base model [RFC9130] which defines basic IS-IS configuration and state. # Section 3 (*) I would split the long tree into small snippets and help readers go through to understand the rationale for each augment. # Section 4 (*) I have several comments about the module: * Please run "pyang -f yang --yang-canonical" as the current order does not follow a canonical order * Not sure we can claim that the module covers extension that is "which is common across all of the vendors". I would simply delete that mention. * The boilerplate is not needed * It is unfortunate that the naming in the module reasons about TLV objects * There are missing reference statements * Need to clarify the mapping with the flags as defined in the base spec (e.g., af-flag/F-flag, sf-flag/S-Flag) * There are several layers without clear explanation (e.g., sr-capability/sr-capability may question why we need two layers, do we need the tree levels prefix-sid-sub-tlvs/prefix-sid-sub-tlvs/prefix-sid-sub-tlv?) * No need to repeat the parent prefix * Use of plural for lists and leaf-list, while singular is recommended in the guidelines. * There are no reference to justify the default values * Weird to have "Configuration" while we claim the module is NMDA compliant. A version that fixes many of these can be found at: https://github.com/boucadair/IETF-Drafts-Reviews/blob/master/YANG/ietf-isis-sr-mpls.yang. Please double check/complete. Thanks. A diff to track the changes can be seen here: https://github.com/boucadair/IETF-Drafts-Reviews/commit/395b6ece9e7939c33af7d150a71bfd3948bb5ba1 # Section 5 ## Please update this section to follow the template defined in 8407bis (*) ## I don't find "node-msd-tlv" defined in the model CURRENT: Some of the readable data nodes in the modules may be considered sensitive or vulnerable in some network environments. It is thus important to control read access (e.g., via get, get-config, or notification) to these data nodes. /isis:router-capabilities/sr-capability /isis:router-capabilities/sr-algorithms /isis:router-capabilities/local-blocks /isis:router-capabilities/srms-preference /isis:router-capabilities/node-msd-tlv ## nit OLD: Unauthorized access to any data node of these subtrees can disclose the operational state information of IS-IS protocol on this device. NEW: Unauthorized access to any data node of these subtrees can disclose the operational state information of IS-IS protocol on a device. # Section 7 Consider updating as follows OLD: The IANA is requested to assign one new URI from the IETF XML registry ([RFC3688]). Authors are suggesting the following URI: URI: urn:ietf:params:xml:ns:yang:ietf-isis-sr-mpls Registrant Contact: The IESG. XML: N/A, the requested URI is an XML namespace This document also requests one new YANG module name in the YANG Module Names registry ([RFC6020]) with the following suggestion : name: ietf-isis-sr-mpls namespace: urn:ietf:params:xml:ns:yang:ietf-isis-sr-mpls prefix: isis-sr-mpls reference: RFC XXXX NEW: The IANA is requested to assign a new URI from the IETF XML registry [RFC3688]: URI: urn:ietf:params:xml:ns:yang:ietf-isis-sr-mpls Registrant Contact: The IESG. XML: N/A, the requested URI is an XML namespace This document also requests a new YANG module name in the YANG Module Names registry ([RFC6020]): name: ietf-isis-sr-mpls namespace: urn:ietf:params:xml:ns:yang:ietf-isis-sr-mpls prefix: isis-sr-mpls maintained by IANA? N reference: RFC XXXX # References (*) RFC8446, RFC8340, RFC8040, RFC6242, and RFC6241 are listed as Normative, while they shouldn't. Please move those to be listed as Informative. # Appendix A (*) The JSON example is broken. Please update and validate using yanglint or yangson, for example. OLD: { "routing": { "router-id": "1.1.1.1", "control-plane-protocols": { "control-plane-protocol": { "type": "isis:isis", "name": "isis", "isis": { "system-id": "1111.2222.3333", "interfaces": { "interface": { "name": "", "segment-routing": { "adjacency-sid": { "adj-sids": { "value": 38888 } } } } }, "segment-routing": { "enabled": true }, "protocol-srgb": { "srgb": { "lower-bound": 4000, "upper-bound": 5000 } } } } } } } NEW: { "ietf-routing:routing": { "router-id": "1.1.1.1", "control-plane-protocols": { "control-plane-protocol": { "type": "ietf-isis:isis", "name": "isis", "ietf-isis": { "system-id": "1111.2222.3333", "interfaces": { "interface": { "name": "", "ietf-isis-sr-mpls:segment-routing": { "adjacency-sid": { "adj-sids": { "value": 38888 } } } } }, "ietf-isis-sr-mpls:segment-routing": { "enabled": true }, "ietf-isis-sr-mpls:protocol-srgb": { "srgb": { "lower-bound": 4000, "upper-bound": 5000 } } } } } } } Cheers, Med _______________________________________________ Lsr mailing list -- lsr@ietf.org To unsubscribe send an email to lsr-le...@ietf.org
