[Idr] AD review of draft-ietf-idr-bgp-ls-sr-policy-11

[John Scudder <[e-mail]>]

Hi Authors, WG,

Due to time constraints, I’m sending this review now even though I haven’t completed the line-by-line part of my review, so you should expect a followup. I hope that’s OK for everyone. I’m already comfortable that the document is of sufficient quality to be sent for IETF Last Call, so I’m requesting that now.

I’ve supplied my questions and comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive questions and comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached the iddiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply.

Thanks,

—John

P.S.: I note that the current version is 12. This review is against verson 11 but there’s nothing in the version 12 changeset that conflicts.

--- draft-ietf-idr-bgp-ls-sr-policy-11.txt	2025-02-10 13:39:33
+++ draft-ietf-idr-bgp-ls-sr-policy-11-jgs-comments.txt	2025-02-13 14:22:15
@@ -128,6 +128,17 @@
    multiple SID-Lists are active then traffic is load balanced over
    them.  This document covers the advertisement of state information at
    the individual SR Policy CP level.
++--
+jgs: The 84 occurrences of "CP" made me sad. The use of this initialism
+impeded the flow of the document for me. I don't see any benefit of using
+it vs. writing out "Candidate Path". Yes, it would be a big changeset but
+an easy change (it's just search and replace).
+
+I don't insist that you change it, and in particular if the Segment 
+Routing document set has already institutionalized this piece of (ahem
+unnecessary) jargon, then the ship has sailed. But please consider whether
+it would be OK to make the change.
++--
 
    SR Policies are generally instantiated at the head-end and are based
    on either local configuration or controller-based programming of the
@@ -245,6 +256,9 @@
 
    The encodings specified in this document (specifically in Section 4
    and Section 4) make use of flags that convey various types of
++--
+jgs: "Section 4 and Section 4"
++--
    information of the SR Policy.  The document uses the term "set" to
    indicate that the value of a flag bit is 1 and the term "clear" when
    the value is 0.
@@ -329,6 +343,12 @@
       advertisement of SR Policies.
 
    *  "Identifier" is an 8 octet value as defined in [RFC9552].
++--
+jgs: Please provide the section reference within RFC 9552. Also, In 
+RFC 9552, it's called "Instance-ID" I believe -- please use terminology
+consistent with the authoritative reference unless a variation is 
+needed.
++--
 
 
 
@@ -343,6 +363,12 @@
 
    *  The SR Policy Candidate Path Descriptor TLV is specified in
       Section 4.
++--
+jgs: I found the Local Node Descriptor rules inconsistent/confusing and
+had an offline discussion with Ketan about it. I think we have a good 
+understanding and Ketan is going to refactor this section to clarify
+what rules apply to what roles. Noting here to keep the WG in the loop.
++--
 
    The Local Node Descriptor TLV MUST include the following Node
    Descriptor TLVs only when the headend node is the BGP-LS Producer:
@@ -353,7 +379,16 @@
    *  Autonomous System Number (TLV 512) [RFC9552], which contains the
       ASN or AS Confederation Identifier (ASN) [RFC5065], if
       confederations are used, of the headend node of the SR Policy.
++--
+jgs: in addition to the more major issue mentioned previously, there is an
+editorial/readability issue above. I suggest using parentheses, as in,
 
+NEW:
+   *  Autonomous System Number (TLV 512) [RFC9552], which contains the
+      ASN (or AS Confederation Identifier (ASN) [RFC5065], if
+      confederations are used) of the headend node of the SR Policy.
++--
+
    The Local Node Descriptor TLV MUST include at least one of the
    following Node Descriptor TLVs:
 
@@ -378,6 +413,9 @@
       or a 6-octet ISO System-ID is to be done based on the length of
       that sub-TLV since the Protocol-ID in the NLRI shall always be
       "Segment Routing".
++--
+jgs: Another minor one. Above, the "shall" might as well be "SHALL".
++--
 
    When a Path Computation Element (PCE) node is functioning as the BGP-
    LS Producer on behalf of one or more headends, it MAY include its own
@@ -386,6 +424,12 @@
    MUST NOT include its identifiers in the Node Descriptor TLV in the
    NLRI as the Node Descriptor TLV MUST only carry the identifiers of
    the SR Policy headend.
++--
+jgs: As a reminder, I found the introduction of the PCE operating "on
+behalf of" the headend to be jarring, arriving as it did without context.
+Please provide context, even if it's only a forward reference (although
+right now I only see Section 6 paragraph 1, which is also pretty light).
++--
 
 
 
@@ -401,6 +445,11 @@
    used with the Protocol-ID set to Segment Routing to advertise the SR
    Policy Candidate Path NLRI Type.  It is a mandatory TLV for SR Policy
    Candidate Path NLRI type.  The TLV has the following format:
++--
+jgs: I think the sentence "It is used with the Protocol-ID set to Segment 
+Routing to advertise the SR Policy Candidate Path NLRI Type" is redundant
+and can be deleted.
++--
 
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
@@ -565,7 +614,15 @@
       -  D-Flag: Indicates the dataplane for the BSIDs and if they are
          16 octet SRv6 SID when set and are 4 octet SR/MPLS label value
          when clear.
++--
+jgs: Editorial. Perhaps,
 
+NEW:
+      -  D-Flag: Indicates the dataplane for the BSIDs and if they are
+         16 octet SRv6 SID (when set) or are 4 octet SR/MPLS label value
+         (when clear).
++--
+
       -  B-Flag: Indicates the allocation of the value in the BSID field
          when set and indicates that BSID is not allocated when clear.
 
@@ -627,6 +684,16 @@
    TLV for advertisement of the SRv6 Binding SID is supported only for
    backward compatibility with implementations that followed early
    versions of this specification.
++--
+jgs: This seems like a poster child for the use of the word 
+"deprecated", as in,
+
+                                      The use of the SR Binding SID sub-
+   TLV for advertisement of the SRv6 Binding SID has been deprecated,
+   and is documented here only for
+   backward compatibility with implementations that followed early
+   versions of this specification.
++--
 
 5.2.  SRv6 Binding SID TLV
 
@@ -761,6 +828,10 @@
       described in section 5 of [RFC9256] unless stated otherwise for
       individual flags.  Other bits MUST be cleared by the originator
       and MUST be ignored by a receiver.
++--
+jgs: The semantics are not, in fact, described in Section 5 of RFC 9256,
+as far as I can tell. Please check.
++--
 
           0                   1
           0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5
@@ -794,6 +865,11 @@
       -  E-Flag: Indicates that the CP has been evaluated for validity
          (e.g. headend may evaluate CPs based on their preferences) when
          set and has not been evaluated for validity when clear.
++--
+jgs: I found the above to be pretty weird. I take it on faith that it 
+makes sense in context. I wouldn't mind a primer on what's going on here,
+though.
++--
 
       -  V-Flag: Indicates the CP has at least one valid SID-List when
          set and indicates no valid SID-List is available or evaluated
@@ -933,6 +1009,16 @@
    of this TLV is advertised for a given CP.  If multiple instances are
    present, then the first one is considered valid and the rest are
    ignored.
++--
+jgs: I wasn't able to make sense of 
+
+   The constraints may also be applied to an explicit path
+   where the computation entity is expected to validate that the path
+   satisfies the specified constraints and if not the path is to be
+   invalidated
+
+Please rewrite or help me understand.
++--
 
    The TLV has the following format:
 
@@ -1033,6 +1119,9 @@
          traverses hop-by-hop over the links corresponding to those
          adjacency SIDs when set and indicates that the CP is not using
          only hop-by-hop adjacency SIDs when clear.
++--
+jgs: Not using? Or not restricted to using? I think the latter.
++--
 
    *  RESERVED1: 2 octets.  MUST be set to 0 by the originator and MUST
       be ignored by a receiver.