Subject: [Idr] Rtgdir early review of draft-ietf-idr-bgpls-srv6-ext-07

From: Adrian Farrel via Datatracker <[email-masked]>
Date: Sat, 29 May 2021 14:20:00 -0700

Reviewer: Adrian Farrel
Review result: Has Issues

I have been selected to do a routing directorate "early" review of this
draft. https://datatracker.ietf.org/doc/draft-ietf-idr-bgpls-srv6-ext

The routing directorate will, on request from the working group chair,
perform an "early" review of a draft before it is submitted for
publication to the IESG. The early review can be performed at any time
during the draft's lifetime as a working group document. In this case,
it appears that the review request was triggered by the request to the
AD for publication. The purpose of this early review is to ensure as
wide of a review as possible.

For more information about the Routing Directorate, please see 
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Document: draft-ietf-idr-bgpls-srv6-ext-07.txt
Reviewer: Adrian Farrel
Review Date: 29-May-2021
Intended Status: Standards Track

== Summary ==

Some minor issues and lot of nits.

== Comments ==

I am one of the Designated Experts for the BGP-LS registries and I had
previously considered the draft when an Early Allocation request was 
made. That request was approved.

The document is clear enough and I believe I could implement it if I
had to. The approach described in this document is consistent with the
body of BGP-LS work, but I think it is important to note that the 
mechanism set out here differs slightly from the mechanism defined to
do similar function with SR-MPLS. This distinction is clearly made in
Appendix A. However, the WG chairs and AD will want to be sure that the
WG recognise this difference and are OK with having two slightly 
different mechanisms running side by side.

There are some Minor Issues that I think should be addressed before
IETF Last Call in case they create any points that community review
might object to.

There are also quite a lot of nits in the document. I have tried to 
capture them below, but the large number means I may have missed some.  
I recommend fixing these before IETF Last Call so that other reviewers
has a cleaner document to look at.

== Minor Issues ==

You will need to reduce the number of front-page authors to five or
fewer, or you will need to provide the document shepherd with a credible
reason to diverge from this rule.

---

Section 1

   On the similar lines,
   introducing the SRv6 related information in BGP-LS allows consumer
   applications that require topological visibility to also receive the
   SRv6 SIDs from nodes across a domain or even across Autonomous
   Systems (AS), as required.

I caution you to be *very* careful with the word "domain". The IESG has
recently been concerned by the definition contained in 8402. In that
document, the "SR domain" (also just "domain") is the collection of all
interconnected SR-capable nodes. Here (and in other parts of the
document) I think you are using "domain" in a more restricted sense. I
don't know how you fix that, but I believe you should do something.
Perhaps you can call out the terminology issues here by noting the 8402
definition and specifically defining what you mean by your local
definition of "domain".

---

Section 2

   The SRv6 SIDs associated with the node are advertised using the BGP-
   LS SRv6 SID NLRI introduced in this document.  This enables the BGP-
   LS encoding to scale to cover a potentially large set of SRv6 SIDs
   instantiated on a node with the granularity of individual SIDs and
   without affecting the size and scalability of the BGP-LS updates.

The claims of scalability are not expanded here or anywhere else in the
document. Scalability of BGP-LS is important, so I'd prefer some 
explanation. But if that isn't available, it might be better to leave
out these mentions.

---

4.1, 4.2, and 7.2

You should consider asking IANA to create a registry for the flags
fields. This would make it easier to keep track of future assignments.

You should also state clearly that the three flags fields are aligned so
that an addition to one is always also made to the other (if this is the
case).

---

4.1 and 4.2

      Sub-TLVs : Used to advertise sub-TLVs that provide additional
      attributes for the specific SRv6 SID.

You need to say where these sub-TLVs are defined. If there are none 
defined yet, you should say so and explain why some might be defined in
the future. 

You should also say where the sub-TLV type values are tracked.

Compare with 5.1

---

5.1

You should say where the sub-TLVs are tracked, and you should consider a
registry for the flags.

---

7.1

   The SRv6 Endpoint Behavior TLV is a mandatory TLV that MUST be
   included in the BGP-LS Attribute associated with the BGP-LS SRv6 SID
   NLRI.  

What if it is missing?

---

8.

   The total of the locator block, locator node, function and argument
   lengths MUST be less than or equal to 128.

What if they add up to more than 128?

---

Well done for including Appendix A. I assume that the WG discussed the
relative merits of having a slightly different approach for SR-MPLS and
SRv6, and contrasted that with the benefits of a consistent approach. I
personally think that making a difference is unfortunate, but if the WG
was happy with this decision then I guess it's OK.

Without launching into a sales pitch, it might be nice if Appendix A was
to very briefly explain why the difference.

== Nits ==

You have:
- "SR for [the] MPLS data-plane" (Abstract)
- SR/MPLS (Section 4.1)
- "SR with [the] MPLS data-plane" (Section 7.2)
- "SR-MPLS" (Sections 10 and 11, Appendix A)

8402 has "SR-MPLS"

---

Abstract

s/Segment Routing (SR) over IPv6 (SRv6)/Segment Routing over IPv6 (SRv6)/

s/by the various/by various/

OLD
   BGP Link-state (BGP-LS) address-family solution for SRv6 is similar
   to BGP-LS for SR for MPLS data-plane.  This draft defines extensions
   to the BGP-LS to advertise SRv6 Segments along with their behaviors
   and other attributes via BGP.
NEW
   This document defines extensions to BGP Link-state (BGP-LS) to
   advertise SRv6 segments along with their behaviors and other
   attributes via BGP.  The BGP-LS address-family solution for SRv6
   described in this document is similar to BGP-LS for SR for the MPLS
   data-plane defined in a separate document.
END

---

Section 1

s/A SRv6 Segment/An SRv6 segment/

OLD
   The IS-IS [I-D.ietf-lsr-isis-srv6-extensions] and OSPFv3
   [I-D.ietf-lsr-ospfv3-srv6-extensions] link-state routing protocols
   have been extended to advertise some of these SRv6 SIDs and
   SRv6-related information.
NEW
   The IS-IS and OSPFv3 link-state routing protocols have been extended
   to advertise some of these SRv6 SIDs and SRv6-related information
   [I-D.ietf-lsr-isis-srv6-extensions],
   [I-D.ietf-lsr-ospfv3-srv6-extensions].
END

s/Certain other/Other/

s/On the similar lines/On similar lines/

s/(e.g./(e.g.,/

---

Section 2

   o  SRv6 SID of the IGP Adjacency SID or the BGP EPE Peer Adjacency
      SID [RFC8402] is advertised via SRv6 End.X SID TLV introduced in
      this document (Section 4.1)

   o  SRv6 SID of the IGP Adjacency SID to a non-Designated Router (DR)
      or non-Designated Intermediate-System (DIS) [RFC8402] is
      advertised via SRv6 LAN End.X SID TLV introduced in this document
      (Section 4.2)

1. s/SRv6 SID/The SRv6 SID/ twice
2. It is confusing that the first bullet talks about advertising the
   IGP Adjacency with no qualification, but the second bullet talks
   about the IGP Adjacnecy to a non-DR or non-DIS. That appears to make
   the first bullet a superset of the second bullet. Sections 4.1 and
   4.2 make this a lot clearer: perhaps you could bring some of that 
   clarity to these bullets.

---

Section 2

   o  SRv6 Locator is advertised via SRv6 Locator TLV introduced in this
      document (Section 5.1)

s/SRv6/The SRv6/
s/SRv6/the SRv6/

---

2.

   The SRv6 SIDs associated with the node are advertised using the BGP-
   LS SRv6 SID NLRI introduced in this document.

Please include a forward pointer to Section 6.

---

2.

   o  The endpoint behavior of the SRv6 SID is advertised via SRv6
      Endpoint Behavior TLV (Section 7.1)

s/via SRv6/via the SRv6/

---

2.

   o  The BGP EPE Peer Node and Peer Set context for a Peer Node and
      Peer Set SID [RFC8402] respectively is advertised via SRv6 BGP EPE
      Peer Node SID TLV (Section 7.2)

s/via SRv6/via the SRv6/

The text is hard to read. Maybe...

   o  The BGP EPE Peer Node context for a Peer Node, and the Peer Set 
      context for a Peer Set SID [RFC8402] are advertised via the SRv6 
      BGP EPE Peer Node SID TLV (Section 7.2).

---

2.

s/(e.g./(e.g.,/

---

2.

   When the BGP-LS router is advertising topology information that it
   sources from the underlying link-state routing protocol (as described
   in [RFC7752]), then it maps the corresponding SRv6 information from
   the SRv6 extensions for IS-IS [I-D.ietf-lsr-isis-srv6-extensions] and
   OSPFv3 [I-D.ietf-lsr-ospfv3-srv6-extensions] protocols to their BGP-
   LS TLVs/sub-TLVs for all SRv6 capable nodes in that routing protocol
   domain.  When the BGP-LS router is advertising topology information
   from the BGP routing protocol (e.g. for EPE as described in
   [I-D.ietf-idr-bgpls-segment-routing-epe]), then it advertises the
   SRv6 information from the local node alone.

I'd suggest moving this paragraph to be the second in the section as it
explains what is going on.

---

3.1 and 

   This TLV maps to
   the SRv6 Capabilities sub-TLV and the SRv6 Capabilities TLV of the
   IS-IS and OSPFv3 protocol SRv6 extensions respectively.
   :
   :
   o  Flags: 2 octet field.  The following flags are defined:

     0                   1
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    | |O|       Reserved            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                Figure 2: SRv6 Capability TLV Flags Format

      *  O-flag: If set, then router is capable of supporting SRH O-bit
         Flags, as specified in [I-D.ietf-6man-spring-srv6-oam].

      *  The rest of the bits are reserved for future use and MUST be
         set to 0 and ignored on receipt.

Would be good to include references for the two capabilities TLVs.

It would also help to not include the bit definitions here. I think I
am correct that if a new bit is defined for inclusion in the ISIS SRv6
Capabilities sub-TLV flags field, you expect to pick it up here
automatically.

It looks like the mapping is in the wrong direction. I.e., this TLV maps
from, not to, the IGP TLVs.

I think you would say... 

   This TLV maps from
   the SRv6 Capabilities sub-TLV of the IS-IS SRv6 extensions [ref] and
   the SRv6 Capabilities TLV of the OSPFv3 SRv6 extensions [ref].
   :
   :
   o  Flags: 2 octet field.  The contents of this field are copied from
      the IS-IS or OSPF SRv6 Capabilities TLVs and have the same
      meaning.

For what it's worth, it's a pitty that there is no registry for tracking
the flags, but that's a problem with draft-ietf-lsr-isis-srv6-extensions
and not with this document.

---

4.1

s/End.X with USP and/End.X with USP, and/   (twice)
s/This TLV is also used by BGP/This TLV is also used by BGP-LS/

---

4.1 Figures 3 and 5

Convetion is that if you show all of the bytes, you close the start and 
end of the lines with a vertical pipe "|". Or you can leave out bytes 
and close the line ends with a tilda "~" or double slash "//" (see
Figure 9).

---

4.1, 4.2, 5.1, and 6.1

Can you say what the Length field is. "The length of the value part of
the TLV in bytes including any sub-TLVs that may be present." ??

---

4.1, 4.2, 5.1, and 7.1

      Algorithm: 1 octet field.  Algorithm associated with the SID.
      Algorithm values are defined in the IANA IGP Algorithm Type
      registry.

It would be good to add a reference to RFC 8665  or to
https://www.iana.org/assignments/igp-parameters/igp-parameters.xhtml.

---

4.2

s/normally an IGP node only/an IGP node normally only/
s/to announce SRv6 SID/to announce the SRv6 SID/
s/(i.e./(i.e.,/
s/End.X with USP and/End.X with USP, and/   (twice)

---

4.2

   The IS-IS and OSPFv3 SRv6 LAN End.X SID TLVs have the following
   format:

Do you mean the BGP-LS TLVs?

---

5.1

OLD
   As specified in [RFC8986], an SRv6 SID comprises of Locator, Function
   and Argument parts.
NEW
   As specified in [RFC8986], an SRv6 SID comprises Locator, Function,
   and Argument parts.
END

---

6. Figure 9

There is a broken format line in the middle of the figure.

---

6.

The figure has
    |                        Identifier                             |
    |                        (64 bits)                              |
and the text has
   o  Identifier: 8 octet value as defined in [RFC7752].

Be consistent?

---

6.

   o  Protocol-ID: 1 octet field that specifies the protocol component
      through which BGP-LS learns the SRv6 SIDs of the node.  The
      Protocol-ID registry was created by [RFC7752] and then extended by
      other BGP-LS extensions.

- s/Protocol-ID registry/BGP-LS Protocol-ID registry/
- Probably, "...and then additional assignments were made for other
  BGP-LS extensions."

---

7.1

s/are bound with a SID/are bound to a SID,/

---

7.2

s/The similar/Similar/

---

7.2
   is an optional TLV for use in the BGP-LS Attribute for an SRv6 SID
Is that "OPIONAL"?

But then...
   This TLV MUST be
   included along with SRv6 SIDs that are associated with the BGP Peer
   Node or Peer Set functionality.
... is confusing.

I suspect that the second sentence should be...
   If this TLV is present, it MUST be accompanied by the SRv6 SIDs that
   are associated with the BGP Peer Node or Peer Set functionality.

---

7.2

s/For a SRv6/For an SRv6/

---

8.

s/SRv6 SID Structure TLV is used/The SRv6 SID Structure TLV is used/

---

8.

   optional TLV

Is that "OPTIONAL"?

---

9.

OLD
   This document requests assigning code-points from the IANA "Border
   Gateway Protocol - Link State (BGP-LS) Parameters" registry as
   described in the sub-sections below.
NEW
   IANA has made early allocations of code points from the "Border
   Gateway Protocol - Link State (BGP-LS) Parameters" registry as
   described in the sub-sections below.  IANA is requested to updated
   the references to indicate this document when it is published as an
   RFC.
END

---

9.2

It would make sense to list the codepoints in numeric order. I.e., lift
518 to the top.

---

10.

s/(e.g./(e.g.,/

---

11.

s/(e.g./(e.g.,/

---

Appendix A

s/session(s)/session/
s/advertised as attribute/advertised as attributes/