Segment Routing over IPv6 (SRv6) Network Programming
draft-ietf-spring-srv6-network-programming-28

I think my main comments were addressed between -20 and -24; thanks.

Thanks for responding to the SECDIR review (and thank you to Brian Weis for doing it)

Thanks for addressing my DISCUSS and COMMENTs.

===[ remaining COMMENTs
** Section 4.1.  Pseudo-code without formal definition might be open to interpretation.  My nits (which likely apply to other sections) are that:
-- it might not be clear that S03, S06 and S10 are effectively returns/exits from this “behavior”, and if they are run, S12-S15 should never be reached (otherwise Segments Left or Hop Limit = -1)

-- (for consistency) the IF … ELSEIF … END construct is used in Section 4.8, but not here.

Thank you for the work put into this document.

Thank you also for having edited the document based on the INT-DIR review by Brian Haberman (thank you Brian for the detailed review!) and replying to Brian's review.
https://datatracker.ietf.org/doc/review-ietf-spring-srv6-network-programming-18-intdir-telechat-haberman-2020-09-14/

Please find below a couple of non-blocking COMMENT points and nits.

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==
-- Section 3 --
There is some text copied from section 4.3 from RFC 8754. But, even if it seems obvious that the behaviors specified in this document _ONLY_ apply when "A FIB entry that represents a locally instantiated SRv6 SID", I suggest to spell it out to avoid any ambiguity.

-- Section 3.2 --
I am a little ambivalent with Brian's point about the location of the given examples. On one hand, those real case examples of SID allocation are really helpful to understand the impact of allocation; on the other hand, it is usual to move examples in appendix. Curious to see if other IESG members have other point of view.

In the last short example (this one could stay in the current location), should the value of F and A be stated? They are obvious from the used SID value of course, but, let's be complete ?
    
-- Section 3.3 --
Please follow RFC 5952 and write all IPv6 in lowercase.

-- Section 4.2 and 4.3 --
As "End.X" and "End.T" behaviors are variants of the "End" behavior, does the section 4.1.1 (Upper-Layer Header) also apply ?

-- Section 4.9 --
For consistency with previous behavior descriptions of End.DT[46], I suggest to mention that 143 has been reserved by IANA.

== NITS ==
-- Section 2 --
In the bullet list, there are a couple of missing '.' at the end of the list items.

-- Section 3 --
"longest-prefix-match lookup on the packets destination address" "packets" should probably be singular.

In "IPv6 subnet allocated for SRv6 SIDs by the operator", should it rather be "prefix" than "subnet" ?

-- Section 3.2 --
Strongly suggest to be consistent with the use of "IANA Endpoint Behavior codepoint" as sometimes it is shortened into "IANA Behavior" (also unsure whether the "IANA" qualification is really useful here)

-- Section 4.2 --
"one for the bundle(LAG)" Link Aggregation has not been expanded before and I am not sure whether the 'LAG' adds anything to the paragraph.

-- Section 4.16 --
Should PSP, USP, USD be expanded in the short introduction of this section ?

-- Section 8 --

The usual location of the security considerations is after all specification section, so, it should be after the 'control plane' section.

[Thanks for addressing my DISCUSS points.]

Throughout the document:
It should be “an SID”, “an FIB”, “an RIR”, and some others, not “a”, because one reads these as “ess-eye-dee” and “eff-eye-bee”, not as the expansions thereof.

— Section 3.1 —

   An SRv6 endpoint behavior MAY require additional information

I think this is a statement of fact, so “may” (or “might”), not “MAY”.

— Section 3.2 —
Nit: The plural of “SID” is “SIDs”, without an apostrophe.

   The provider historically deployed IPv6 and assigned
   infrastructure address from a portion of the fc00::/7 prefix

Nit: “addresses”.

   In another example, a large mobile and fixed line service provider

Nit: hyphenate “fixed-line”.

   IPv6 address consumption in both these examples is minimum,

Nit: “minimal” (also, put a comma before “respectively”).

   A remote node uses the IANA behavior codepoint to map the received
   SID (B:N:FUNCT) to a behavior.

I don’t know what “IANA behavior codepoint” means.  As the rest of the sentence makes it clear that this is mapping to “a behavior”, maybe it’s better to say “registered codepoint”, or some such?  Or, as you say a couple of paragraphs down, “Endpoint Behavior codepoint”?  In any case, please be consistent about “IANA behavior”, “IANA Behavior”, and “IANA Endpoint Behavior”.  My preference would be to avoid saying “IANA” in these, but use your judgment on what’s most understandable and clear.

   o  Assign an SRv6 Locator 2001:db8:bbbb:3::/64 to the Router 3 in
      their SR Domain

What is “the Router 3” (and router 4)?  There’s no introduction nor diagram that says.  Also, please be consistent in capitalization of these.

— Section 3.3 —

   routing protocol specific aspects that are outside the scope of this

Nit: “routing-protocol-specific” needs to be hyphenated.  As that’s awkward, you might want to reword to avoid it, “are specific to the routing protocol and are outside the scope....”

— Section 4 —

   The pseudocode describing these behaviors detail local processing

Nit: “details”, singular, to match “pseudocode”.

— Section 4.1 —

   The End behavior operates on the same FIB table (i.e.  VRF, L3 relay
   id) associated to the packet.

Is “i.e.” really what you want here?  How does “VRF, L3 relay” equate to “FIB table”?

— Section 4.16.1.3 —

   segments left to 0.  The SDN controller knows that no-other node

Nit: “no other” should not be hyphenated.  For that matter, the word “other” isn’t needed anyway, because you have “after” in there.

   -as part of the decapsulation process the egress PE is required to
    terminates less bytes from the packet.

I can’t figure this out.  It looks like it should be “required to terminate”, but I don’t know what it means to “terminate less bytes”.  Can you reword this?

    to the lookup engine in the forwarding ASIC.

“ASIC” needs to be expanded.  As this is the only place you use it, I suggest just using the expansion and not bothering with the abbreviation.

— Section 7 —

   This document introduces SRv6 Endpoint and SR Policy Headend
   behaviors for implementation on SRv6 capable nodes in the network.
   As such, this document does not introduce any new security
   considerations.

I’m not convinced of this.  It seems that misuse (such as injection or alteration) of some of these Behaviors could, for example, result in packets being forwarded to nodes they were not intended to go to.  Is it not important to discuss issues such as that: how these Behaviors might be attacked?  Is that really fully covered in 8754 and 8402?

— Section 8.1 —

   The presence of SIDs in the IGP do not imply any routing semantics

Nit: “does not”, to match the singular subject “the presence”.

   an IPv6 address is solely governed by the, non-SID-related, IGP

Nit: remove both commas.

   not governed neither influenced in any way by a SID advertisement

Nit: make it “neither governed nor influenced”

   build TI-LFA [I-D.ietf-rtgwg-segment-routing-ti-lfa] based FRR
   solutions

Nit: there needs to be a hyphen in there, but the citation gets in the way.  Make it, “build FRR solutions based on TI-LFA [I-D.ietf-rtgwg-segment-routing-ti-lfa]”.

— Section 8.4 —

   For example, a BGP-LS advertisement of H.Encaps behavior would
   describe the capability of node N to perform a H.Encaps behavior,
   specifically it would describe how many SIDs could be pushed by N
   without significant performance degradation.

This is a comma splice.  Split the sentence before “specifically” (and put a comma after “specifically”).

Please define RIR on first use.

Hi,

Thank for this document.  Not surprisingly given the large number of contributors working on this document I only have a few minor comments:


    2.  Terminology

       NH: Next-header field of the IPv6 header [RFC8200].  NH=SRH means
       that the next-header of the IPv6 header is Routing Header for
       IPv6(43) with the Type field set to 4.
   
Suggest expanding "4" to SRH(4)


    10.1.  Ethernet Next Header Type

       This document requests IANA to allocate, in the "Protocol Numbers"
       registry (https://www.iana.org/assignments/protocol-numbers/protocol-
       numbers.xhtml), a new value for "Ethernet" with the following
       definition: The value 143 in the Next Header field of an IPv6 header
       or any extension header indicates that the payload is an Ethernet
       [IEEE.802.3_2012].

Should this point to the latest published version of 802.3 (2018), or does it specifically need to reference an older version?

   
    3.1.  SID Format

       This document defines an SRv6 SID as consisting of LOC:FUNCT:ARG,
       where a locator (LOC) is encoded in the L most significant bits of
       the SID, followed by F bits of function (FUNCT) and A bits of
       arguments (ARG).  L, the locator length, is flexible, and an operator
       is free to use the locator length of their choice.  F and A may be
       any value as long as L+F+A <= 128.  When L+F+A is less than 128 then
       the remainder of the SID MUST be zero.

This was reasonably clear to me, but since the rest of the description refers to bits, then I wonder whether the last sentence would be more precise as "the remaining bits of the SID must be set to zero"?


    4.1.  End: Endpoint
       S09.   If ((Last Entry > max_LE) or (Segments Left > Last Entry+1)) {

It wasn't entirely apparent to me where 'Last Entry' comes from, although that became more clear when I saw its usage later.  Possibly it would be helpful to list "Segments Left" and "Last Entry" in the terminology as taken from RFC 8754?


    4.1.  End: Endpoint
       S12.   Decrement Hop Limit by 1
       
Would it be more clear to expand this to say "Decrement IPv6 Hop Limit by 1", since it is referred to as the IPv6 Hop Limit previously.  If this is changed, then there are a couple of other places in the document that should be checked as well.

   
    4.2.  End.X: Layer-3 Cross-Connect

       Note that if N has an outgoing interface bundle I to a neighbor Q
       made of 10 member links, N may allocate up to 11 End.X local SIDs:
       one for the bundle(LAG) itself and then up to one for each Layer-2
       member link.

This behaviour seems slightly unusual to me.  I presume that the intent here is for the SR program to control which member link the traffic travels over rather than the normal behaviour of relying on the Bundle hashing.  Possibly adding a bit more text to clarify the expected behaviour might be helpful.


    4.4.  End.DX6: Decapsulation and IPv6 Cross-Connect

       S05.  If the End.DX6 SID is bound to an array of L3 adjacencies, then
       one entry of the array is selected based on the hash of the packet's
       header Section 7.

The end of this sentence doesn't scan for me, probably it should be something end something like: ", see Section 7"


    6.  Counters

       A node supporting this document SHOULD implement a combined traffic
       counter (packets and bytes) per local SID entry, for traffic that
       matched that SID and was processed correctly.  The retrieval of these
       counters via MIB, NETCONF/YANG or any other means is outside the
       scope of this document.
   
From my reading, a combined counter implies a single counter that is incremented by both the number of packets processed and number of bytes processed.  Normally in the management models you would normally expect this to be modeled as a pair of counters, and I presume that is what is desired here?  If so I would suggest clarifying this text to indicate that it is a pair of counters (one for packets, one for bytes) that are required here.


    Section 4, various places.

At various points the Upper-Layer Header type is compared to a number (e.g. 4, 41, 143).  It might be a bit more readable if the associated names were also included in the description.  E.g., 4(IPv4), 41(IPv6), etc.


    4.10.  End.DX2V: Decapsulation and VLAN L2 Table Lookup

Is this lookup just on the outermost VLAN Id, or all VLAN tags in the exposed Ethernet header?  Or is this implementation specific?

Regards,
Rob