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

[John Scudder <[e-mail]>]

Hi Authors, WG,

Here’s the remainder of the review, that I promised when I sent the partial review of version 11. I’ve done the rest of my comments vs. version 13. Thanks for the quick work on that, the changes looked good. I may have some more followup later, I’m not sure.

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

--- draft-ietf-idr-bgp-ls-sr-policy-13.txt	2025-02-14 20:36:46
+++ draft-ietf-idr-bgp-ls-sr-policy-13-jgs-comments.txt	2025-02-14 21:48:42
@@ -1159,6 +1159,11 @@
 
    *  Length: variable, dependent on the size of the Extended Admin
       Group.  MUST be a multiple of 4 octets.
++--
+jgs: This is a pretty picky detail, but here and elsewhere that you
+specify "multiple of", I'm assuming it's really "nonzero multiple of",
+is that right?
++--
 
    *  Exclude-Any-Size: one octet to indicate the size of Exclude-Any
       EAG bitmask size in multiples of 4 octets. (e.g.  value 0
@@ -1312,6 +1317,14 @@
       requested as specified in the form of flags.  The following flags
       are defined and the other bits MUST be cleared by the originator
       and MUST be ignored by a receiver.
++--
+jgs: You say "level of disjointness" as though there were an ordering
+over these things, but as far as I can tell these are really orthogonal
+quantities with no ordering. For instance, two paths might be disjoint
+from an SRLG perspective but not a node perspective... or they might be
+disjoint from a node perspective, but not an SRLG perspective. I have
+more comments about this later.
++--
 
           0 1 2 3 4 5 6 7
          +-+-+-+-+-+-+-+-+
@@ -1332,15 +1345,25 @@
          and indicates that the link disjointness is not requested when
          clear.
 
-      -  F-Flag: Indicates that the computation may fallback to a lower
+      -  F-Flag: Indicates that the computation may fall back to a lower
          level of disjointness amongst the ones requested when all
          cannot be achieved when set and indicates that fallback to a
          lower level of disjointness is not allowed when clear.
++--
+jgs: This is the part that got me worried about the "level of" thing. Falling
+back to a lower level implies levels are defined. If you do want to impute
+an ordering over these things, please state or reference it explicitly. 
+(Athough as I mentioned previously, I don't think there is a natural 
+ordering.)
 
+Please consider and revise as needed wherever levels of disjointness are
+discussed.
++--
 
 
 
 
+
 Previdi, et al.          Expires 18 August 2025                [Page 24]
 
 Internet-Draft    Advertising SR Policies using BGP-LS     February 2025
@@ -1406,7 +1429,27 @@
       identifier for a set of disjoint paths.  A PCEP Association Object
       [RFC8697] (including its optional TLVs) MAY also be advertised to
       convey the disjoint group identifier.
++--
+jgs: I'm a little lost at sea here.
 
+First: The diagram shows this as "Disjoint Group Identifier" but here
+you call it "Disjointness Group Identifier". Please pick one.
+
+Second: The diagram says "(variable)" for size but here you say it's
+four octets. Which is it? I'm guessing 4-octet is right, in which case
+please fix the diagram.
+
+Third: Is the Disjoint(ness) Group Identifier meant to map to the
+identifier carried in the PCEP Association Object? Because the
+Association ID field in that object is two bytes, but you've defined
+your field as four bytes. Or, is it either/or, you either use the
+Association Object or this field? But in that case, you'd need a
+distinguished not-set value, or a flag to indicate the present field is
+to be disregarded. So it's always going to have a value (even if it's
+zero). Are considerations needed for a disagreement between the present
+field and the Association Object?
++--
+
 5.6.5.  SR Bidirectional Group Constraint Sub-TLV
 
    The SR Bidirectional Group Constraint sub-TLV is an optional sub-TLV
@@ -1487,7 +1530,36 @@
       supported BGP message size by the implementation.  Refer section
       5.3 of [RFC9552] for discussion on implications of encoding large
       sets of information into BGP-LS.
++--
+jgs: First and easiest, the syntax of
+
+                                                                The PCEP
+      Association Object MUST NOT be encoded and this sub-TLV skipped
+      along with an error log, if the object size is such that the
+      update for a single SR Policy Candidate Path NLRI would exceed the
+      supported BGP message size by the implementation.  
+      
+is hard to follow because it puts the condition last. I suggest a rewrite 
+along the lines of,
 
+NEW:
+      If the PCEP object size is such that the update for a single SR
+      Policy Candidate Path NLRI would exceed the supported BGP message
+      size by the implementation, then the PCEP Association Object MUST
+      NOT be encoded and this sub-TLV skipped along with an error log. 
+      
+But second, why does this, and only this, sub-TLV warrant a special 
+warning? Isn't this a general problem for BGP messages, especially with
+variable-sized content? For that matter, this isn't even the only place
+you mention the PCEP Association Object -- does this same consideration
+apply to the other use, and if not why not? 
+
+Finally, all the concerns about Disjoint Group Identifier apply here, 
+viz "variable" vs "4-octet", 4-octet vs. the 2-octet encoding of the
+PCEP Association Object identifier, and whether disagreement between the
+two is a concern.
++--
+
 5.6.6.  SR Metric Constraint Sub-TLV
 
    The SR Metric Constraint sub-TLV is an optional sub-TLV of the SR
@@ -1535,9 +1607,14 @@
    *  Length: 12 octets
 
    *  Metric Type: 1-octet field which identifies the type of the metric
-      being used.  The Table 1 below lists the metric types introduced
+      being used.  Table 1 below lists the metric types introduced
       by this document along with reference for each.  The reference is
       to IS-IS (equivalent also exist for OSPF) specifications where
++--
+jgs: This doesn't appear to be anywhere close to specific enough. Please
+provide a reference for OSPF as well. For that matter, is there any 
+reason not to cover both OSPFv2 and OSPFv3?
++--
       those metric types are defined for a link while in the SR Policy
       context those relate to the candidate path or the segment list.
       The metric type code points that may be used in this sub-TLV are
@@ -1575,6 +1652,14 @@
       | Point |    Metric Type     |         Reference               |
       +-------+--------------------+---------------------------------+
       |  0    | IGP                | [RFC5305] Section 3             |
++--
+jgs: Even in the IS-IS context (never mind my OSPF related complaints)
+this doesn't appear specific enough. Looking at RFC 5305 Section 3, I
+see at least two different things called "metric", the "default metric"
+and the "TE Default metric". This feels like a place where the 
+implementor is being invited to use their creativity, which is not 
+something our specs should do.
++--
       |  1    | Min Unidirectional | [RFC8570] Section 4.2           |
       |       | Delay              |                                 |
       |  2    | TE                 | [RFC5305] Section 3.7           |
@@ -1592,6 +1677,10 @@
       +-------+--------------------+---------------------------------+
 
                    Table 1 BGP-LS SR Policy Metric Types
++--
+jgs: I think all the above have to be normative references since they 
+provide the necessary details about the respective values.
++--
 
    *  Flags: 1-octet field that indicates the validity of the metric
       fields and their semantics.  The following bit positions are
@@ -1641,7 +1730,25 @@
       metric to accommodate for other factors such as bandwidth
       availability, minimal SID stack depth, and maximizing of ECMP for
       the SR path computed.
++--
+jgs: This might be a place where I am running afoul of lack of domain
+knowledge, but I would have assumed we desire to minimize some metrics
+(e.g. latency) but maximize others (say, bandwidth). Above you're 
+implying we want only to maximize metrics (you talk about "minimum 
+metric"). Does this make sense in some context that pertains to this
+spec?
 
+Related, "loosens the criteria... up to the specified metric" and so on
+seems insufficiently precise. That is to say, is the effective metric...
+the specified metric plus the margin? Minus? Plus-or-minus?
+
+Come to think of it, I don't know what the "computed path metric" is, 
+which means I don't know what I'm supposed to apply the percentage to.
+I guess maybe I don't need to, precisely, it just means the effective 
+metric is equal to the specified metric times (1 + percentage value/100)
+(or maybe it's +/-).
++--
+
    *  Metric Bound: 4-octet value which indicates the maximum metric
       that is allowed when the B-flag is set.  If the computed path
       metric crosses the specified bound value then the path is
@@ -1650,6 +1757,11 @@
    The absolute metric margin and the metric bound values are encoded as
    specified for each metric type.  The percentage metric margin is
    encoded as an unsigned integer percentage value.
++--
+jgs: Some of the metrics you're referencing aren't 4-byte values (the
+IS-IS ones for sure). Probably one would just do the "obvious" thing and
+zero-fill the MSBs, but I would prefer you say so. 
++--
 
 5.7.  SR Segment List TLV
 
@@ -1803,6 +1915,10 @@
    SHOULD NOT include any SR Segment sub-TLVs.
 
 5.8.  SR Segment Sub-TLV
++--
+jgs: Given this is a sub-TLV of the SR Segment List TLV, shouldn't it
+be Section 5.7.1?
++--
 
    The SR Segment sub-TLV describes a single segment in a SID-List.  One
    or more instances of this sub-TLV in an ordered manner constitute a
@@ -1872,12 +1988,12 @@
          dynamically resolved value by headend when clear
 
       -  V-Flag: Indicates the SID has passed verification or did not
-         require verification when set.  When V-Flage is clear, it
+         require verification when set.  When V-Flag is clear, it
          indicates the SID has failed verification.
 
       -  R-Flag: Indicates the SID has been resolved or did not require
          resolution (e.g. because it is not the first SID) when set.
-         When R-Flage is clear, it indicates the SID has failed
+         When R-Flag is clear, it indicates the SID has failed
          resolution.
 
       -  A-Flag: Indicates that the Algorithm indicated in the Segment
@@ -1920,7 +2036,7 @@
    Section 4 of [RFC9256] defines multiple types of segments and their
    description.  This section defines the encoding of the Segment
    Descriptors for each of those Segment types to be used in the Segment
-   sub-TLV describes previously in Section 5.8.
+   sub-TLV described previously in Section 5.8.
 
    The following types are currently defined and their mapping to the
    respective segment types defined in [RFC9256]:
@@ -1948,9 +2064,35 @@
   +------+-------------------------------------------------------------+
 
                     Table 2  SR Segment Types
++--
+jgs: It makes me sad that we're defining a new registry that appears to
+need to be kept in sync with 
+https://www.iana.org/assignments/segment-routing/segment-routing.xhtml#segment-types
 
+Is there a good reason this can't/shouldn't be a new column in the
+existing Segment Types registry? Beyond the obvious desire to avoid 
+duplication, unifying the registries might also offer a helpful hint to
+people defining new Segment Types that they should also not forget to 
+define a new Segment Descriptor.
++--
+
 5.8.1.1.  Type 1: SR-MPLS Label
++--
+jgs: In this and the following subsections you aren't using the "(Type
+A)" labeling you've used in Table 2. If I had to choose one or the
+other, I choose the descriptive name, every time. But, is it common in
+the implementor community to refer to "Type A" segments as such? Because
+if so, it would seem kind to title this subsections as you've done in
+Table 2, e.g. "Type 1: SR-MPLS Label (Type A)".
 
+If you think this will add confusion, I don't insist. It seems like the
+UX error was created in RFC 9256 though, when it chose to associate a
+meaningless label that has no wire-encoding, with the descriptive name.
+That error having been made, we should at least strive not to make
+things worse. (I regret not having complained about this when evaluating
+RFC 9256 but so it goes.)
++--
+
    The Segment is SR-MPLS type and is specified simply as the label.
    The format of its Segment Descriptor is as follows:
 
@@ -2052,7 +2194,32 @@
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 
          Figure 23  Type 4 Segment Descriptor
++--
+jgs: in this diagram and many of the following ones, you've used a fixed
+layout that depicts the (in this case) IPv6 Node Global Address as being
+4 octets. Thank you for providing the size in parentheses, but please
+also adopt a diagram convention to show the box size is not literal. 
+In earlier diagrams, you've used "//" to end the box in such cases, and
+that works. Alternately (and better, IMO) in cases where the box is of
+fixed size (like here) just depict it accurately, as in:
+
+    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
+   +-+-+-+-+-+-+-+-+
+   |   Algorithm   |
+   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+   |                                                               |
+   |          IPv6 Node Global Address (16 octets)                 |
+   |                                                               |
+   |                                                               |
+   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+
+After all, we don't pay by the byte, so why not provide the most 
+accurate depiction you can? 
 
+I'm not going to flag diagrams individually; please review them all.
++--
+
    Where:
 
    *  Algorithm: 1-octet value that indicates the algorithm used for
@@ -2119,6 +2286,9 @@
 
    *  IPv4 Local Address: 4-octet value which carries the local IPv4
       address associated with the node
++--
+jgs: shouldn't that be "associated with the interface"?
++--
 
 
 
@@ -2132,6 +2302,10 @@
 
    *  IPv4 Remote Address: 4-octet value which carries the remote IPv4
       address associated with the node's neighbor.  This is optional and
++--
+jgs: shouldn't that be "associated with the interface on the node's
+neighbor"?
++--
       MAY be set to 0 when not used (e.g. when identifying point-to-
       point links).
 
@@ -2245,7 +2419,7 @@
    *  IPv6 Node Global Address: 16-octet value which carries the IPv6
       global address associated with the node
 
-5.8.1.10.  Type 10: SRv6 END.X SID as an Anterface ID
+5.8.1.10.  Type 10: SRv6 END.X SID as an Interface ID
 
    The Segment is SRv6 END.X SID type and is specified as a pair of IPv6
    global address and interface ID for local and remote nodes.  The
@@ -2317,6 +2491,12 @@
       address associated with the node's neighbor
 
 5.9.  SR Segment List Metric Sub-TLV
++--
+jgs: I'm not going to retype them here but since this structure is
+quite similar to the one described in 5.6.6, I imagine that my 
+comments there about margin and bound, if they have any validity at
+all, might also apply here. Please take it into consideration.
++--
 
    The SR Segment List Metric sub-TLV reports the computed metric of the
    specific SID-List.  It is used to report the type of metric and its
@@ -2377,7 +2557,7 @@
    *  Length: 16 octets
 
    *  Metric Type: 1-octet field which identifies the type of metric.
-      The Table 1 in Section 5.6.6 lists the metric types introduced by
+      Table 1 in Section 5.6.6 lists the metric types introduced by
       this document.  The metric type code points that may be used in
       this sub-TLV are also listed in Section 8.6 of this document.
       Note that the metric type in this field is not taken from the "IGP
@@ -2621,7 +2801,7 @@
    registry group.
 
    The following table lists the status of TLV code points that have
-   been allocated by IANA and others that are pending allocation:
+   been allocated by IANA: