Hi Andrew,
Thanks a lot for your detailed review and comments.
The authors have clarified and amended the text in response to your comments,
including both Part 1 and Part 2, and these modifications were included as part
of the recently submitted:
https://www.ietf.org/archive/id/draft-ietf-spring-srv6-srh-compression-09.txt
Please see the authors’ replies in-line with [Authors] below. Any further
comments or suggestion, please let us know.
Best regards,
Weiqiang Cheng
Part 1
======
> In section 3 it states:
>
> “In an SRv6 domain, the SIDs are allocated from a particular IPv6 prefix:
> The Locator-Block. All SRv6 SIDs instantiated from the same Locator-Block
> share the same most significant bits”
>
> This is problematic – since my reading of the first half of this would imply
> that there is a single prefix. The second half implies there could be
> multiple such prefix’s – so clarity is needed.
[Authors] We rephrased this sentence to make it clearer:
```
In an SR domain, all SRv6 SIDs instantiated from the same Locator-
Block share the same most significant bits.
```
> In section 3 paragraph 4 it states:
>
> “An SR source node constructs and compresses the SID-List depending on the
> capabilities of each SR segment endpoint node that the packet should
> traverse, as well as it’s own compression abilities”
>
> For this to be implementable I would argue that it is necessary to specify
> how the constructor (be it controller or otherwise) is aware of the
> capabilities of each node the packet should be transverse. Effectively – how
> does a node know what the nodes beneath it are capable of. A lack of
> knowledge of this could lead to serious operational problems. I also
> question why the source node’s own compression abilities play into this –
> since the source node should merely be constructing a SID list. If indeed
> the node’s own compression abilities factor into this, we return to the
> problem of multiple solutions.
[Authors] The "capabilities" of an SR segment endpoint node are the SIDs
instantiated on this node. A node supporting C-SID compression instantiates
C-SID flavored SIDs, while a node that does not support the compression
instantiates SIDs without these flavors. The SIDs and their respective flavors
are advertised by the control plane protocols (see section 8). The
"capabilities" of the SR source node refers to the flexibility offered in
section 6 to implement a different compression method than the one described in
this document (with some restrictions), which may result in a slightly
different compressed segment list.
> In section 3 paragraph 7 it states:
>
> “The compressed Segment List encoding supports any Locator-Block allocation.
> While other options are supported and may provide higher efficiency, each
> routing domain can be allocated a /48 prefix from a global IPv6 block (see
> Section 6.2)”
>
> I would like to understand how this aligns to draft-ietf-6man-sids or if
> there is any plan to align the text.
[Authors] We added a reference to draft-ietf-6man-sids in section 3:
```
For example, each routing domain within the SR domain can be
allocated a /48 Locator-Block from a global IPv6 block available to
the operator, or from a prefix allocated to SRv6 SIDs as discussed
in Section 5 of [I-D.ietf-6man-sids].
```
> In Section 4.1 – it states:
>
> “When processing a SID with the NEXT-C-SID flavor, while the Argument value
> is non-zero, the SR segment endpoint node constructs the next SID by copying
> the SID Argument value immediately after the Locator-Block, thus overwriting
> the previous SID Locator-Node and Function, and filling the least significant
> bits of the argument with zeros. When the Argument value is 0, the SR segment
> endpoint node instead copies the next 128-bit Segment List entry from the SRH
> to the Destination Address field of the IPv6 header. In both cases, the SR
> segment endpoint node then forwards the packet to the new destination. The
> C-SID sequence ends with the last C-SID container.”
>
> Since this text explicitly states that when the argument value is zero, the
> following entry must be copied into the DA, this implies that an SRH is
> mandatory. If this is the case, it should be explicitly stated. If not,
> then descriptive text needs to be inserted to explain how this works in the
> absence of an SRH. A document should be able to stand without ambiguity
> without a reading of the pseudocode. This text also fails to explain what
> happens in the event of an SRH being present but exhausted.
> The view on a mandatory SRH is again confirmed with the pseudocode segments –
> the document states that the pseudocode in 4.1.1 should be inserted between
> S01 and S02 of the code specified in Section 4.1 of RFC8986 – when you do
> that you come to the following:
[Authors] We clarified in section 4.1 (and 4.2) that such text is only a
high-level, generic description of the packet processing by the SR segment
endpoint node and that the normative behaviors are described in the subsections
of 4.1.1-7 and 4.2.1-8. These behaviors also support the case where no SRH is
present in the packet.
> ```
> When an SRH is processed {
> If (DA.Argument != 0) {
> If (IPv6 Hop Limit <= 1) {
> Send an ICMP Time Exceeded message to the Source Address,
> Code 0 (Hop limit exceeded in transit),
> interrupt packet processing and discard the packet.
> }
> Copy the value of DA.Argument into the bits [LBL..(LBL+AL-1)]
> of the Destination Address.
> Set the bits [(LBL+AL)..127] of the Destination Address to
> zero.
> Decrement Hop Limit by 1.
> Submit the packet to the egress IPv6 FIB lookup for
> transmission to the next destination.
> }
> If (Segments Left == 0) {
> Stop processing the SRH, and proceed to process the next
> header in the packet, whose type is identified by
> the Next Header field in the routing header.
> }
> If (IPv6 Hop Limit <= 1) {
> Send an ICMP Time Exceeded message to the Source Address
> with Code 0 (Hop limit exceeded in transit),
> interrupt packet processing, and discard the packet.
> }
> max_LE = (Hdr Ext Len / 2) - 1
> If ((Last Entry > max_LE) or (Segments Left > Last Entry+1)) {
> Send an ICMP Parameter Problem to the Source Address
> with Code 0 (Erroneous header field encountered)
> and Pointer set to the Segments Left field,
> interrupt packet processing, and discard the packet.
> }
> Decrement IPv6 Hop Limit by 1
> Decrement Segments Left by 1
> Update IPv6 DA with Segment List[Segments Left]
> Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination
> }
> ```
>
> As a further note to the above pseudocode – there is an assumption made that
> the reader would know that submitting a packet to the egress IPv6 FIB is a
> terminating action. However, this is not unambiguous, and I think should be
> made clear.
[Authors] This phrasing is inherited from RFC 8754 and RFC 8986. Since the
action taken on the packet is the same as the one in the pseudocodes of RFC
8754 and RFC 8986, we should probably stick to the same phrasing.
> In section 4.1.2 last paragraph – it refers to pseudocode insertion between
> lines S01 and S02 of the pseudocode specified in section 4.2 of RFC8986 – I
> am presuming this is an incorrect reference, because there is no pseudocode
> in section 4.2 of RFC8986
[Authors] We clarified that S01 and S02 indeed refer to section 4.1 (not 4.2)
of RFC 8986.
```
The resulting pseudocode is inserted between lines S01 and S02 of
the SRH processing in Section 4.1 of [RFC8986] after applying the
modification described in Section 4.2 of [RFC8986].
```
> In section 4.1.3 last paragraph – same problem as the above – it refers to
> pseudocode insertion between lines S01 and S02 of pseudocode specified in
> section 4.3 of RFC8986 – section 4.3 of RFC8986 does not contain pseudocode.
[Authors] We clarified this with a similar modification as in section 4.1.2.
```
The resulting pseudocode is inserted between lines S01 and S02 of
the SRH processing in Section 4.1 of [RFC8986] after applying the
modification described in Section 4.3 of [RFC8986].
```
> In section 4.1.5 it prefers to the process in 4.14 of RFC8986 – this again
> requires pseudocode modification – except my understanding is the pseudocode
> that requires modification is in 4.13 of RFC8986 – this level of abstraction
> makes this incredibly difficult to read.
[Authors] The clarification in section 4.1.4 should make this a little easier
to follow.
> In section 4.1.7 it states – “The pseudocodes defined in Section 4.1.1,
> Sections 4.1.2 and Section 4.1.3 of this document are inserted at the
> beginning of the modified upper-layer header processing defined in Section
> 4.16.3 of RFC8986 for End, End.X and End.T respectively.” This is a little
> ambiguous. I am presuming that this should say “inserted directly after S01
> of the pseudocode specified in Section 4.16.3 of RFC8986. I make this
> assumption since the pseudocode specified in the sections listed (4.1.1,
> 4.1.2 and 4.1.3) is terminating in nature, and as such if you inserted this
> before S01 (at the beginning) the remaining pseudocode would not be processed.
[Authors] We separated the pseudocode in the previous sections from the
upper-layer header processing. The packet processing remains the same, but this
simplifies the text as there is no longer any modification of the USD flavor.
```
USD: The USP flavor defined in Section 4.16.3 of [RFC8986] is
unchanged when combined with the NEXT-C-SID flavor.
```
Also note that the pseudocode N01-09 in sections 4.1.1-3 is terminating only if
DA.Argument is non-zero.
> In section 4.2 the text states that the first SID is copied into the DA field
> of the IPv6 header, either at the SR source node, or at the previous SR
> segment endpoint node. This text is a direct contradiction of statements
> that this is a single solution since the source node will only be inserting a
> SID list into an SRH – and hence only acting at a control plane layer. While
> theoretically this can be done in the control plane when assembling the
> encapsulated packet, the text itself does not state that and as written, the
> head end node would need to have knowledge of REPLACE behavior at more than a
> control plane layer.
[Authors] The statement that "[the] first SID is copied into the Destination
Address field of the IPv6 header either at the SR source node [...] or at the
previous SR segment endpoint node" referred to "whoever puts that SID in the
IPv6 destination address".
- An SR source node writes the first SID of the segment list in the IPv6
destination address when creating the packet (see section 4.1 of RFC 8754).
- An SR segment endpoint node copies the next SID of the segment list in the
IPv6 destination as part of its segment endpoint processing (see sections 4.3.1
of RFC 8754 and 4 of RFC 8986).
In any case, we removed this sentence in revision -09 to avoid any further
confusion.
Part 2
======
> In Section 4.2 it states:
>
> “The C-SID sequence ends with a last C-SID in the last C-SID container that
> does not have the REPLACE-C-SID flavor, or with the special C-SID value 0, or
> where reaching the end of the segment list, whichever comes first.”
>
> This implies that a container can have parts of it that are not valid C-SID’s
> and are not set to zero. I found this confusing, and I’d like to understand
> the process for differentiating between a valid C-SID in the container and “a
> part of the C-SID container that does not contain the REPLACE-C-SID flavor.”
> I can fully understand a case where you had:
>
> Second c-sid|first c-sid|0|0 (since that would terminate on the zero) – the
> text however implies that you can have:
> Second c-sid|first c-sid|xxxxx – where xxxxxx could be variable length and
> not be a c-sid at all.
>
> Further to this – it implies that in the above the xxxxx may not be the end
> of the c-sid list – since the text states three different conditions –
>
> 1. When the last C-SID in the last C-SID container does not have a
> REPLACE-C-SID flavour (OR)
> 2. The C-SID has special value zero (OR)
> 3. When reaching the end of the segment list
>
> This implies – that (a) could happen before (c) – so this needs clarity.
[Authors] We clarified that the last C-SID in a C-SID sequence does not need
to have the REPLACE-C-SID flavor.
```
The last C-SID in the C-SID sequence is not required to have the
REPLACE-C-SID flavor. It can be bound to any behavior and flavor(s),
including the NEXT-C-SID flavor, as long as it meets the conditions
defined in Section 6.
```
> As a note – and I realize that this is also syntax used in RFC8986 – the
> pseudo-code at points says effectively If(Segments Left > Last Entry) or
> equivalent. Now – logically what that means is “Segments Left > Last Entry
> Position” – but – what it says is – “Segments Left > Whatever is in the last
> entry” – and to a programmer who is implementing based entirely on
> pseudo-code without deep understanding of the document (or modifying it) this
> could potentially create confusion. I may consider submitting an erratum on
> this against other documents where this happens. (Basically, there is a big
> difference between a comparison between the value of last entry and the
> position of the last entry in code terms – and this is further complicated
> when in section 4.1 there is a comparison between a position and a set value
> on S09)
[Authors] The term "Last Entry" refers to the field of the SRH (see section 2
of RFC 8754). This Last Entry field contains the index of the last element in
the Segment List, so the pseudocode correctly compares two indexes.
> As another point about the pseudocode detailed in section 4.2.1 on close
> examination:
>
> After replacing S02 with the given pseudocode – and replacing S09 – S16 we
> end up with this:
>
> ```
> When an SRH is processed {
> If (Segments Left == 0 and (DA.Arg.Index == 0 or
> Segment List[0][DA.Arg.Index-1] == 0)) {
> Stop processing the SRH, and proceed to process the next
> header in the packet, whose type is identified by
> the Next Header field in the routing header.
> }
> If (IPv6 Hop Limit <= 1) {
> Send an ICMP Time Exceeded message to the Source Address
> with Code 0 (Hop limit exceeded in transit),
> interrupt packet processing, and discard the packet.
> }
> max_LE = (Hdr Ext Len / 2) - 1
> If (DA.Arg.Index != 0) {
> If ((Last Entry > max_LE) or (Segments Left > Last Entry)) {
> Send an ICMP Parameter Problem to the Source Address,
> Code 0 (Erroneous header field encountered),
> Pointer set to the Segments Left field,
> interrupt packet processing and discard the packet.
> }
> Decrement DA.Arg.Index by 1.
> If (Segment List[Segments Left][DA.Arg.Index] == 0) {
> Decrement Segments Left by 1.
> Decrement IPv6 Hop Limit by 1.
> Update IPv6 DA with Segment List[Segments Left]
> Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination.
> }
> } Else {
> If((Last Entry > max_LE) or (Segments Left > Last Entry+1)){
> Send an ICMP Parameter Problem to the Source Address,
> Code 0 (Erroneous header field encountered),
> Pointer set to the Segments Left field,
> interrupt packet processing and discard the packet.
> }
> Decrement Segments Left by 1.
> Set DA.Arg.Index to (128/LNFL - 1).
> }
> Decrement IPv6 Hop Limit by 1.
> Write Segment List[Segments Left][DA.Arg.Index] into the bits
> [LBL..LBL+LNFL-1] of the Destination Address of the IPv6
> header.
> Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination.
> }
> ```
>
> if(Segments Left == 0 AND (DA.Arg.Index == 0 or Segment
> List[0][DA.Arg.Index-1] == 0)) – this is an AND condition – this states that
> the condition is valid when Segments Left is zero in certain cases.
>
> This becomes a problem further down where we get to this:
>
> If (DA.Arg.Index != 0) {
> If ((Last Entry > max_LE) or (Segments Left > Last Entry)) {
> Send an ICMP Parameter Problem to the Source Address,
> Code 0 (Erroneous header field encountered),
> Pointer set to the Segments Left field,
> interrupt packet processing and discard the packet.
> }
> Decrement DA.Arg.Index by 1.
> If (Segment List[Segments Left][DA.Arg.Index] == 0) {
> Decrement Segments Left by 1.
> Decrement IPv6 Hop Limit by 1.
> Update IPv6 DA with Segment List[Segments Left]
> Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination.
> }
>
> Since Segments Left can be zero in the containing IF stanza – you could end
> up decrementing segments left while it is zero – which would be -1 (or in
> standard coding terms if we assume that segments left is unsigned – this
> would wrap setting segments left back to 255 – and causing a significant
> logic error with what you are updating the IPv6 DA with) – This comment
> applies to anywhere this pseudocode is used and modified (as it is in the
> subsequent sections)
[Authors] The last clause in the IF statement on line S02 (`Segment
List[Segments Left][DA.Arg.Index-1] == 0`) is meant specifically to handle this
scenario. In the case that Segment Left is 0 and the next C-SID (`Segment
List[Segments Left][DA.Arg.Index-1]`) is 0, the SR endpoint node immediately
stops processing the SRH.
> In Section 4.2.2 it states that the pseudo-code in section 4.2.1 is modified
> by replacing S18 and S29 with “S01. Submit the packet to IPv6 module for
> transmission to the new destination via a member of J”
>
> Firstly – stating it in this manner would throw out the ordering (S18 is
> still S18 and not S01 etc) and secondly – while J is defined in RFC8986 it
> took some searching to find it – I believe for clarity that J needs to be
> clearly defined in this document to make this more readable.
[Authors] We clarified the text and pseudocode in section 4.2.2 as follows,
and added an explicit reference to RFC 8986 for the definition of variable J.
```
The pseudocode in Section 4.2.1 of this document is modified by
replacing line R10 and R21 as shown below.
R10. Submit the packet to the IPv6 module for transmission to the
new destination via a member of J.
R21. Submit the packet to the IPv6 module for transmission to the
new destination via a member of J.
Note: the variable J is defined in Section 4.2 of [RFC8986].
```
However we would prefer for consistency to avoid redefining terms and variables
that are defined in other RFCs.
> In Section 4.2.3 the comments about ordering and the text remain the same as
> the above comment, in addition, T is defined in section 4.3 of RFC8986 – but
> that definition should be carried over into this document for readability.
[Authors] We clarified this with a similar modification as in section 4.2.2.
```
The pseudocode in Section 4.2.1 of this document is modified by
replacing line R10 and R21 as shown below.
R10.1. Set the packet's associated FIB table to T.
R10.2. Submit the packet to the egress IPv6 FIB lookup for
transmission to the new destination.
R21.1. Set the packet's associated FIB table to T.
R21.2. Submit the packet to the egress IPv6 FIB lookup for
transmission to the new destination.
Note: the variable T is defined in Section 4.3 of [RFC8986].
```
> Same comment about ordering applies to section 4.2.4
[Authors] We clarified this with a similar modification as in section 4.2.2 and
4.2.3.
```
The pseudocode in Section 4.2.1 of this document is modified by
replacing line R10 and R21 as shown below.
R10.1. Push a new IPv6 header with its own SRH containing B.
R10.2. Set the outer IPv6 SA to A.
R10.3. Set the outer IPv6 DA to the first SID of B.
R10.4. Set the outer Payload Length, Traffic Class, Flow Label,
Hop Limit, and Next Header fields.
R10.5. Submit the packet to the egress IPv6 FIB lookup for
transmission to the next destination.
R21.1. Push a new IPv6 header with its own SRH containing B.
R21.2. Set the outer IPv6 SA to A.
R21.3. Set the outer IPv6 DA to the first SID of B.
R21.4. Set the outer Payload Length, Traffic Class, Flow Label,
Hop Limit, and Next Header fields.
R21.5. Submit the packet to the egress IPv6 FIB lookup for
transmission to the next destination.
Note: the variables A and B, as well as the values of the Payload
Length, Traffic Class, Flow Label, Hop Limit, and Next Header are
defined in Section 4.13 of [RFC8986].
```
> In section 4.2.7 it states
>
> “As per section 6.4, when allocating a C-SID value from a Local Identifiers
> Block (LIB), an extra prefix of Locator_block:FunctionID::/LBL+FL is required
> on the Segment Endpoint node to support LIB matching in forwarding. The new
> behaviors with REPLACE-C-SID flavor explicitly require the node to do so in
> SID initiation.”
>
> I failed to understand the latter part of this – “explicitly require the node
> to do so” – do WHAT exactly – this failed to get through my parser
[Authors] We removed this text, which was redundant with section 6.4 (5.4 in
revision -09).
> In Section 4.2.8 it states that S28 should be modified with the pseudocode
> listed as S28.1. This is ambiguous – since S28 could then read:
>
> S28.0 Write Segment List[Segments Left][DA.Arg.Index] into the bits
> [LBL..LBL+LNFL-1] of the Destination Address of the IPv6
> header.
> S28.1. If (Segments Left == 0 and (DA.Arg.Index == 0 or
> Segment List[0][DA.Arg.Index-1] == 0)) {
> S29. Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination.
> S30. }
> S31. }
>
> OR S28.0 and S28.1 could be swapped reading:
>
> S28.1 If (Segments Left == 0 and (DA.Arg.Index == 0 or
> Segment List[0][DA.Arg.Index-1] == 0)) {
>
> S28.2 Write Segment List[Segments Left][DA.Arg.Index] into the bits
> [LBL..LBL+LNFL-1] of the Destination Address of the IPv6
> header.
> S29. Submit the packet to the egress IPv6 FIB lookup for
> transmission to the new destination.
> S30. }
> S31. }
[Authors] The text in section 4.2.8 says that the "instructions defined in
Section 4.16.1.2 of [RFC8986] are **inserted after** line S17 and S28" (R09 and
R20 in revision -09), and later refers to modifying the first line of the
**inserted instructions after** S28 (R20 in revision -09). This seems quite
clear to me.
> In Section 5.2 it states:
>
> “If N1 and N2 are two different physical nodes of the SRv6 domain and I is
> the local C-SID value, then N1 and N2 may bind two different behaviors to I”
>
> While N1 and N2 here presumably are “Node1 and Node2” – this needs more text
> to clarify this is the actual meaning – basically either needs to be written
> out or defined directly above the text.
[Authors] We clarified the text in section 5.2 as follows:
```
Let N1 and N2 be two different physical nodes of the SR domain and I
a local C-SID value, N1 may allocate value I to SID S1 and N2 may
allocate the same value I to SID S2.
```
> In section 6.2 it states:
>
> “The RECOMMENDED Locator-Block sizes for the NEXT-C-SID flavor are 16, 32, or
> 48 bits. The smaller the block length, the higher the compression
> efficiency.”
>
> I presume that “smaller the block length” is an actual literal reference to
> 16 < 32 < 48 – however – this could probably be re-worded in terms of
> shorter/longer – since a /16 is a way larger block than a /32 etc. I also
> strongly question the recommendation of a /16 locator block if this refers to
> global space – since this is a *vast* amount of space
[Authors] We reworked this text and removed the reference to a /16 block. We
also moved these recommendations to sections 4.1 and 4.2 (following Adrian's
suggestion).
_______________________________________________
spring mailing list
spring@ietf.org
https://www.ietf.org/mailman/listinfo/spring
