IS-IS Extensions to Support Segment Routing over the IPv6 Data Plane
draft-ietf-lsr-isis-srv6-extensions-19

Thanks for the follow-up and improved text; much appreciated.

Thanks for the work on this document, and for addressing my DISCUSS points.

Francesca

I support Erik’s DISCUSS especially in light of Warren’s ABSTAIN. 

Below are some questions and comments. I'd appreciate a reply, thanks.

1. Abstract 

   The Segment Routing (SR) allows for a flexible definition of end-to-

You either have too many, or not enough words here. Either “segment routing“ (remove “the“), or something like “the segment routing architecture“.

   called "segments".  Segment routing architecture can be implemented

And here you do need “the“ in front of “segment”. 

   over an MPLS data plane as well as an IPv6 data plane.  This document

“An” -> “the” (two places)

   over an IPv6 data plane.

“An” -> “the”

   This documents updates RFC 7370 by modifying an existing registry.

“Documents” -> “document”

Also, this doesn’t seem to me like an update to RFC 7370. It’s normal for an RFC to update an IANA registry, without saying it updates a previous RFC that established that registry. I think the “updates” just confuses matters and clutters things up, and should be removed. 

2. Section 1

   This documents updates [RFC7370] by modifying an existing registry
   (Section 11.1.2).

“Documents” -> “document”

See also comment #1 regarding update.

3. Section 4.3

   A non-zero SRH Max H.encaps MSD indicates that the headend can insert
   an SRH up to the advertised value.

“Up to the advertised value” doesn’t make sense. I guess you mean “can insert an SRH with up to the advertised number of SIDs”?

4. Section 4.4

   The Maximum End D MSD Type specifies the maximum number of SIDs
   present in an SRH when performing decapsulation.  

That makes sense. 

              These includes, but
   not limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with
   USD as defined in [RFC8986].

That doesn’t. How can a number include all those specific things? A number’s just an integer, right? Maybe you are providing some helpful context about the types of SIDs that are permitted to be present? If so, I expect this is specified elsewhere already, and this sentence is not helping; I would suggest removing it. If it does need to stay, it needs a rewrite for grammar and clarity. (Maybe you want something like “As specified in [RFC 8986] the permitted SID types include, but are not limited to, <list>.”)

      If the advertised value is zero or no value is advertised
      then the router cannot apply any behavior that results in
      decapsulation and forwarding of the inner packet if the
      other IPv6 header contains an SRH.

What “other” IP header? Do you mean the outer header? The inner header? Something else?

5. Section 5

   In cases where a locator advertisement is received in both a Prefix
   Reachability TLV and an SRv6 Locator TLV - (e.g. prefix, prefix-
   length, MTID all being equal and Algorithm being 0 in Locator TLV),

Since “e.g.” means “for example” you’re saying the thing in the parentheses is only one example of a locator advertisement received in both? What’s another example? (Maybe the algo being 1 in both cases?)

But in any case the text above appears redundant with the text that immediately follows. Can’t the text above be deleted?

   In case where the same prefix, with the same prefix-length, MTID, and
   algorithm is received in both a Prefix Reachability TLV and an SRv6
   Locator TLV, the Prefix Reachability advertisement MUST be preferred
   when installing entries in the forwarding plane.  This is to prevent
   inconsistent forwarding entries between SRv6 capable and SRv6
   incapable routers.

“In case” -> “in the case” or “in cases”

6. Section 7.2

   Supported behavior values together with parent TLVs in which they
   area advertised are specified in Section 10 of this document.  Please

“Area” -> “are”

      Length: variable.

      Flags: 1 octet.  No flags are currently defined.

When you write “length: variable“, I think you mean “length: a 1 octet field, whose value is variable“, don’t you? Just like you write “flags: 1 octet“? I get that the value placed into the length field is variable, but the way you’ve written it, it says that the length of the length field is itself variable, which would make no sense.

This is not the only place in the document you specify a length field this way, but I guess you should fix all of them. I’m only noting it here.

   The SRv6 End SID MUST be a subnet of the associated Locator.  SRv6
   End SIDs which are not a subnet of the associated locator MUST be
   ignored.

Is a host route (which is what a SID is) technically a “subnet”? Can something which is not a net, be a subnet? It reads funny that way. If you change it, possibly “MUST be covered by the associated Locator“? (Although then for clarity you might need to define “covered“, which might not be any better.) (Same comment applies to Section 8 which also mentions “subnet”.)

7. Section 8.1

         B-Flag: Backup flag.  If set, the SID is eligible for
         protection (e.g., using IPFRR) as described in [RFC8355].

Please expand IPFRR on first use. And since you say “e.g.” meaning “for example”, do you contemplate some other kind of protection which is not IPFRR? If not, I think you could delete the parenthesis without loss of clarity. 

8. Section 9

   SRv6 SID Structure Sub-Sub-TLV is used to advertise the as defined in
   [RFC8986].  It has the following format:

You’re missing something between “advertise the” and “as defined”. 

      Length: 4 octets.

Similar to my earlier comment, I think what you mean is something like “Length: a 1 octet field, whose value is 4”. 

   associated with it.  It's usage is outside of the scope of this
   document.

“It’s” -> “its”

9. Section 11

   and sub-sub-TLVs as well updating the ISIS TLV registry and defining

“As well as”

   a new registry.

Doesn’t it define several new registries?

10. Section 11.2

Shouldn’t there be a new subsection for the registry creation?

[EDIT]: I'm updating my position from Abstain to NoObjection. I wish that there was a sub-position of "Erk, I really don't like this, but my unhappiness isn't actually with your document, but rather something you reference/rely on" :-)
My unhappiness is actually with RFC8986, this just carries that structure.


I have stolen this text from Ben, because it's well written and reflects my position:
"The relationship of SRv6 SIDs to the IPv6 addressing
architecture (RFC 4291) was quite controversial during the processing of
what since became RFC 8986.  I was able to reconcile the two at the time
by virtue of noting that the substructure of the SID can be considered
to be logically local to the node advertising the SID and therefore not
an observable part of the protocol.  In that sense, the wire-visible use
and processing of SIDs remains that of normal IPv6 addresses.  However,
introducing a sub-sub-TLV to specifically carry the structure of the SID
causes that reasoning to no longer apply, and seemingly re-opens the
question of whether the SID substructure is consistent with the IPv6
addressing architecture.  In the absence of some compelling use case, I
cannot support publishing a mechanism that triggers this controversy."

My view is somewhat stronger - a number of people agreed to  ABSTAINED on RFC 8986 instead of DISCUSSing because it was agreed that it did not try and redefine what an IPv6 address is. This document (section 9) tries to implement structure.

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated), and some nits.

Thank you to Christian Hopps for his shepherd's write-up (including the WG consensus). 

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

-- Section 2 --
Any reason why the bits of the 'Flags' field are not reserved (except for the O-bit) ?  or be in a to-be-created IANA registry? I would expect the default value for sending them (usually 0) and what to do on the receiving side(s) when the value is not 0 (usually ignore them). E.g., see the section 7.1

-- Section 3 --
Isn't it obvious that the "Segment Routing Algorithm" is also supported by SRv6 routers ? Consider removing this section ?
If you prefer to keep it, then please use the wording "Segment Routing Algorithm" exactly as in the IANA registry.

-- Section 7.1 (also 7.2 and possibly others) --
The header 'Length' should not be defined as 'variable' as it is probably a fixed length of 1 octet. Specifying how it is measured and in which units (octets, 32-bit word, ...) is probably welcome.

-- Section 7.2 --
What is the default value of the "Flags" field?

== NITS ==

-- title-
Should the plural form be used for 'extension' ?

-- Section 1 --
s\topology/algorithm specific\topology/algorithm-specific\ ?

-- Section 13 --
While there is a rather long contributors list, there is no acknowledgement section. The latter is of course optional but usual. Should the doc shepherd or last call reviewers be acknowledged ?

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools, so there will likely be some false positives. There is no need
to let me know what you did with these suggestions.

"Abstract", paragraph 2, nit:
-    The Segment Routing (SR) allows for a flexible definition of end-to-
-   ----
+    Segment Routing (SR) allows for a flexible definition of end-to-

"Abstract", paragraph 3, nit:
-    This documents updates RFC 7370 by modifying an existing registry.
-                 -
+    This document updates RFC 7370 by modifying an existing registry.

Section 4.1, paragraph 3, nit:
>  SRH Max Segments Left Type: 41 If no value is advertised the supported va
>                                 ^^
"If" at the beginning of a sentence usually requires a 2nd clause. Maybe a
comma, question or exclamation mark is missing, or the sentence is incomplete
and should be joined with the following sentence.

Section 4.4, paragraph 2, nit:
>  when performing decapsulation. These includes, but not limited to, End.DX6,
>                                 ^^^^^^^^^^^^^^
The verb 'includes' is singular. Did you mean: "This includes" or "These
include"?

Section 6, paragraph 11, nit:
>  of the embedded "X-bit" in any advertisement of the same prefix in TLVs 236
>                                 ^^^^^^^^^^^^^^^^
The usual collocation for "advertisement" is "for", not "of". Did you mean
"advertisement for"?

Section 7.1, paragraph 16, nit:
> T be ignored if the Loc-Size is outside of this range. Locator: 1-16 octe
>                                 ^^^^^^^^^^
This phrase is redundant. Consider using "outside".

Section 7.2, paragraph 2, nit:
> ogether with parent TLVs in which they area advertised are specified in Secti
>                                   ^^^^^^^^^^^^
Do not use a noun immediately after the pronoun 'they'. Use a verb or an
adverb, or possibly some other part of speech.

Section 8.1, paragraph 19, nit:
> e required in order to advertise all of the SRv6 SIDs associated with that n
>                                  ^^^^^^^^^^
Consider using "all the".

Section 8.2, paragraph 2, nit:
>  SID is associated. Given that a large number of neighbors may exist on a giv
>                                ^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, or simply use "many" or "numerous"

Section 8.2, paragraph 2, nit:
> bors may exist on a given LAN a large number of SRv6 LAN END.X SID sub- TLVs
>                               ^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, or simply use "many" or "numerous"

Section 8.2, paragraph 2, nit:
> e required in order to advertise all of the SRv6 SIDs associated with that n
>                                  ^^^^^^^^^^^^^
Consider using "all the".

Section 9, paragraph 16, nit:
> SID Structure Sub- Sub-TLV MUST be lower or equal to 128 bits. If the sum of
>                                    ^^^^^
Did you mean "less than"?

Section 9, paragraph 17, nit:
> ure of the SID associated with it. It's usage is outside of the scope of this
>                                    ^^^^
Did you mean "Its" (possessive pronoun) instead of 'It's' (short for 'it is')?

Section 9, paragraph 17, nit:
> associated with it. It's usage is outside of the scope of this document. 10.
>                                   ^^^^^^^^^^
This phrase is redundant. Consider using "outside".

Section 11.1, paragraph 1, nit:
> t makes the following registrations in the the IS-IS TLV Codepoints registry.
>                                        ^^^^^^^
Maybe you need to remove one determiner so that only "the" or "the" is left.

Section 11.8, paragraph 2, nit:
> ts in the Flags field of the ISIS SRv6 SRv6 Locator TLV specified in this doc
>                                   ^^^^^^^^^
Possible typo: you repeated a word

Section 12, paragraph 3, nit:
> hat document apply too. The advertisement of an incorrect MSD value may have
>                             ^^^^^^^^^^^^^^^^
The usual collocation for "advertisement" is "for", not "of". Did you mean
"advertisement for"?

Document references draft-ietf-6man-spring-srv6-oam-08, but
draft-ietf-6man-spring-srv6-oam-10 is the latest available revision.

Document references draft-ietf-lsr-flex-algo-13, but
draft-ietf-lsr-flex-algo-15 is the latest available revision.

(5) The paragraph that begins “ In cases where a locator advertisement...” appears to be mangled.

Hi,

Thanks for this document.

In various places, this document references lists of numerical algorithm numbers, or TLV Ids without any associated human names.  I would have found this document to be a bit more readable if the names of the algorithms and TLVs were also used alongside their numerical ids.

Thanks,
Rob