Re: [spring] [EXTERNAL] A review of draft-ietf-spring-srv6-srh-compression-08

+1

From: spring [mailto:spring-bounces@ietf.org] On Behalf Of Alexander Vainshtein
Sent: Thursday, September 21, 2023 7:31 PM
To: adrian@olddog.co.uk
Cc: spring-chairs@ietf.org; spring@ietf.org
Subject: Re: [spring] [EXTERNAL] A review of draft-ietf-spring-srv6-srh-compression-08

Adrian,
A really amazing piece of a review!

Regards,
Sasha

Get Outlook for Android<https://aka.ms/AAb9ysg>

________________________________
From: spring <spring-bounces@ietf.org<mailto:spring-bounces@ietf.org>> on behalf of Adrian Farrel <adrian@olddog.co.uk<mailto:adrian@olddog.co.uk>>
Sent: Thursday, September 21, 2023 4:36:39 PM
To: 'SPRING WG List' <spring@ietf.org<mailto:spring@ietf.org>>
Cc: spring-chairs@ietf.org<mailto:spring-chairs@ietf.org> <spring-chairs@ietf.org<mailto:spring-chairs@ietf.org>>
Subject: [EXTERNAL] [spring] A review of draft-ietf-spring-srv6-srh-compression-08

Hi,

This review is in answer to Joel's request on the mailing list. I come to
this document without a lot of history on SRH compression (although
I had some chats with Cheng Li, which helped me to not embarrass
myself with some of my more stupid questions) and I have deliberately
not read any of the threads on the list. If my comments raise questions
that have already been discussed and resolved, then I offer no
apologies because the document needs clarity.

Ideally, a constructive review would provide suggested resolutions with
proposed new text. Some of my recommendations, however, would constitute
significant rewriting (which I haven't signed up for). I'm not sure whether
the authors will be enthusiastic to make such wholesale changes, but I offer
up the ideas in any case.

The big question was whether I believed I could create an interoperable
implementation from this document. I think I mainly could. That is, for
the core features and avoiding the corner cases, I think it would
probably work out of the box. For example, using 32 bit LNFs and running
in a domain where all nodes were capable of just one of the C-SIDs, and
so on. I am less convinced that the more tricky corner cases would work
without some careful interop testing and bug fixing, so out-of-the-box
deployments might be unwise. I am also pretty sure that diagnostics on a
broken system would be hard, but I think that any problems would be
consistent (all traffic on an SR path is not delivered to the right
place) which makes diagnostics a bit easier.

I did find a lot of minor points (shown below). This is very probably
caused by the authors being "too close to the problem" such that they
know what they meant even if the reader is not so sure. Hopefully, some
polish will sort this out.

The more major points don't seem (to me) to be show-stoppers, but they
do need to be resolved.

The volume of points I have raised probably means that I have missed
quite a lot as well.

There's a lot of work here, and I am not sure that I am signed up to
review fixes or to debate the ins and outs of each point. Similarly, I
didn't commit to a further review of the cleaned-up document to see
whether I can find additional issues. I would, however, be happy to
clarify anything I have said.

Cheers,
Adrian

===

0. Please get into the habit of running idnits before posting a new
revision.

== Missing Reference: 'RFC8200' is mentioned on line 996, but not
defined

1. Please expand "SRH" in the document name.

2. Please expand "SR" in the Abstract.

3. The Abstract and Introduction say "a compressed SRv6 Segment-List
encoding", but it seems that this document describes two such
encodings. Perhaps, "...which enable compression of the Segment List
in the Segment Routing Header (SRH)."

4. It would be nice if the Abstract gave a clue as to why the Segment
List needs to be compressed. For example, "to enable more entries in
the Segment List without causing excessive growth in the size of the
packet header."

5. Please decide whether "Segment List" or "Segment-List" or "segment
list" and fix it in the whole document.

6. In the Introduction. s/segment identifier/Segment Identifier/

7. Decide between "Segment Routing Header" and "Segment Routing header"
and fix the whole document.

8. The motivation for this draft is indicated in the Introduction with
a reference to draft-srcompdt-spring-compression-requirement. A
few things:
a. This draft was replaced by draft-ietf-spring-compression-
requirement two years ago.
b. draft-ietf-spring-compression-requirement has expired and perhaps
the WG intends it to fade away now that this draft is close to
completion.
c. If you don't want to set out more details of the motivation in
this document (which would be fine) then I think the reference to
this draft needs to be normative - without reading it, I cannot
know why this work exists and whether I need to implement it. But,
considering #b, you might find it easier to add a few lines to
summarise the motivation and drop the reference.

9. Section 2 has...

| This document leverages the terms defined in [RFC8402], [RFC8754],
| and [RFC8986]. The reader is assumed to be familiar with this
| terminology.
|
| This document introduces the following new terms:
|
| * Locator-Block: The SRv6 SID block (IPv6 prefix allocated for SRv6
| SIDs by the operator) of an SRv6 SID Locator, as defined in
| Section 3.1 of [RFC8986].
|
| * Locator-Node: The identifier of the parent node instantiating a
| SID in an SRv6 SID Locator, as defined in Section 3.1 of
| [RFC8986].

Well, this document doesn't introduce those terms as they are defined
in RFC 8986. And anyway, the initial paragraph assumes the reader is
familiar with Locator-Block and Locator-Node.

10. It is unfortunate that you chose "Compressed-SID" and not
"Compressible-SID", or if you actually meant "Compressed-SID" then
the definition in Section 2 should say not that the SID supports
compressed encoding, but is a compressed encoding. But it can't be
used as a term both for a SID that could be compressed and (as in
the definition of C-SID container) an SID that is compressed.

11. Section 2 has:

| * C-SID sequence: A group of one or more consecutive segment list
| entries carrying the common Locator-Block and a series of C-SID
| containers.

But surely, according to the definition of C-SID container, a group
of one segment list would be a single C-SID container, not a series.

12. Please choose "Compressed Segment List encoding" or "compressed
Segment List encoding" and fix the document accordingly.

13. Section 2 has:

| A compressed Segment List encoding may also contain
| any number of uncompressed SID sequences.

Yeah, and zero is any number. But I think you say this more
gracefully.

14. Section 3

| In an SRv6 domain, the SIDs are allocated from a particular IPv6
| prefix: the Locator-Block.

This appears to say that all SIDs in an SRv6 domain come from the
same Locator-Block. That isn't true according to the definition of SR
domain in 8402. So maybe you should write,

| In an SRv6 domain, each SID is allocated from a particular IPv6
| prefix: the Locator-Block.

15. Section 3

| When the combined length of the SRv6 SID Locator, Function, and
| Argument is smaller than 128 bits, the trailing bits are set to zero.

Please say the trailing bits of what.

16. Section 3

| When a sequence of consecutive SIDs in a Segment List shares a common
| Locator-Block, a compressed Segment-List encoding can optimize the
| packet header length by avoiding the repetition of the Locator-Block
| and trailing bits with each individual SID.

I think you are saying two separate things in this paragraph, but the
text is not very clear:
- a compressed Segment-List encoding can be used
- the act of compressing the Segment-List might reduce the packet
header length

17. Section 3

| The compressed Segment List encoding is fully compliant with the
| specifications in [RFC8402], [RFC8754], and [RFC8986].

Well, 8754 unwisely includes a copy of the definition of the SRH from
RFC 8200 saying...

| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| | |
| | Segment List[0] (128-bit IPv6 address) |
| | |
| | |
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

...and...

| Segment List[0..n]: 128-bit IPv6 addresses representing the nth
| segment in the Segment List. The Segment List is encoded starting
| from the last segment of the SR Policy. That is, the first
| element of the Segment List (Segment List[0]) contains the last
| segment of the SR Policy, the second element contains the
| penultimate segment of the SR Policy, and so on.

Of course, the 128 bit SRH SID list entry has long since stopped
being an IPv6 address, per se. And this draft places C-SID containers
into the SRH such that a "Segment List entry" of 128 bits may now be
all or just part of a C-SID container.

What is more, you make changes to the SRH processing defined in RFC
8986.

This just means that "fully compliant" is a stretch.

Perhaps you simply don't need this sentence? Or perhaps you mean to
say that it doesn't change the processing of uncompressed SID
sequences.

18. Section 3

| It is expected that compressed encoding flavors be available on
| devices with limited packet manipulation capabilities, such as legacy
| ASICs.

An interesting statement. Who expects this? What would happen if it
wasn't true? Why bother saying it?

19. Section 3

| 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).

a. There are two compressed encodings defined in this document, so
"The compressed Segment List encoding..." seems wrong.
b. What is the second sentence actually saying?

20. Section 4

| This section defines several options to achieve compressed Segment
| List encoding in the form of two new flavors

Several options? Or two options?

21. Section 4

| two new flavors for the End, End.X,
| End.T, End.B6.Encaps, End.B6.Encaps.Red, and End.BM behaviors of
| [RFC8986]

Unclear from this why NEXT-C-SID does not support End.DX and End.DT
while REPLACE-C-SID does.

22. Section 4

| These flavors could also be combined with behaviors
| defined in other documents.

I mean, yes, but, what?

23. Section 4

| The compressed encoding can be achieved by leveraging any of these SR
| segment endpoint flavors. The NEXT-C-SID flavor and the REPLACE-
| C-SID flavor expose the same high-level behavior in their use of the
| SID argument to determine the next segment to be processed, but they
| have different low-level characteristics that can make one more or
| less efficient than the other for a particular SRv6 deployment.

a. This is the first mention of NEXT-C-SID and REPLACE-C-SID and
there is no way to understand this paragraph without reading the
rest of the document. I think you need some introductory text to
talk about the fact that the document:
- has two flavors
- the names of the two flavors
- some high-level distinguishing factors of the flavors
Then it would make sense to include this paragraph.

b. The second half of the paragraph appears to say that one or other
flavor may be a better choice depending on the nature of the SRv6
deployment. But I don't find any help in deciding which one I
should deploy in my network. Is that text that is missing (perhaps
from Section 12), or is it discussed in another document (in which
case, please give a reference). Of course, if implementations only
support one flavor, then my deployment choice is highly limited,
and if some of my implementations support only one flavor while
others support only the other flavor, then my deployment choice is
confused!

24. Section 4

| It is RECOMMENDED, for ease of operation, that a single compressed
| encoding flavor be used in a given SRv6 domain. However, in a multi-
| domain deployment, different flavors can be used in different
| domains.

a. s/can/MAY/

b. What is an "SRv6 domain" in this context? Is it the same as an "SR
domain" from RFC 8402? If so, given the ambiguity of the
definition in 8402 (an SR domain contains all of the nodes
participating in the source-based routing model and most commonly
includes all of the protocol instances in a network, but can be
split into multiple SR domains) it would be really helpful to add
some clarity to this paragraph.

In fact, your second sentence says the same as the first sentence!

Of course, if you mean a different type of domain, then please
clarify.

c. "Ease of operation" hides quite a lot! Perhaps you would add a
note to say that both flavors can coexist within the same SR
domain.

25. Section 4

| * Variable LBL is the Locator-Block length of the SID.
|
| * Variable LNFL is the sum of the Locator-Node and the Function
| lengths of the SID. It is also referred to as C-SID length.
|
| * Variable AL is the Argument length of the SID.

It would be helpful to set these variables in the context of RFC
8986. I think that 8986 calls LBL as L, LNFL as L+F, and AL as A.

26. Section 4.1

| An SR segment endpoint node instantiating a SID with the NEXT-C-SID
| flavor MUST accept any argument value for that SID.

It is clear that the endpoint knows the flavor of the SIDs it has
initiated, but it is not clear how the flavor of the SID is known
by anyone else in order that they can compress it or use an
argument value.

27. The Figure 1 caption is odd?

| +------------------------------------------------------------------+
| | Locator-Block |Loc-Node| Argument |
| | |Function| |
| +------------------------------------------------------------------+
| <-------- LBL ---------> < LNFL > <------------- AL ------------->
|
| Figure 1: Example of a NEXT-C-SID flavored SID structure using a
| 48-bit Locator-Block, 16-bit combined Locator-Node and Function,
| and 64-bit Argument

I don't see anything in the figure that is:
- an example
- a indication that the LBL, LNFL, and AL are 48, 16, and 48

28. Section 4.1

| A C-SID sequence using NEXT-C-SID comprises one or more C-SID
| containers, each carrying the common Locator-Block followed by a
| series of C-SIDs.

I think that causes confusion. By saying "the common" you imply that
each container carries the same LB. Perhaps you could s/the common/a/

29. In general, Section 4.1 is hard to parse. It just takes a lot of
effort to learn:

- A C-SID container is exactly 128 bits long (i.e., only one entry
on the SID stack)
- The maximum number of C-SIDs carried in a C-SID container is
(128-LBL)/LNFL
- The "current" SID is constructed as LB+LNF (well, perhaps we know
that, but you could still say so)
- The LNF of the "next" SID is the first lefthand LNFL bits of the
Argument

It would be really nice if the text made these points rather than
leaving the reader to have to work them out.

Indeed, it seems probable that some of the text in 4.1 is incorrect
although the description of a sequence of SIDs may be different from
their encoding in the SRH (or maybe not!). It is notable, I think,
that the SIDs in the SRH appear (left to right) as last to first
while the C-SIDs in the container appear (left to right) as first to
last. It would really help the reader to have this said clearly.
This reader assumed that the C-SID container sequence follows the
same order as the regular SIDs in the SRH.

Oh, then I got to Section 6.1 which seems very, very, very important!
Should you move 6.1 up into 4.1 to sort out loads of other issues
that I've raised (e.g. parts of #37 and #38)?

| 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.

a. "When processing a SID with the NEXT-C-SID flavor"
You have not indicated how a processor would know it is doing
this. I think it knows because it originated the SID (i.e.,
advertised it or let the control know it could use it).

b. "by copying the SID Argument value immediately after the Locator-
Block"
I think you are talking about the destination not the source as
well as the source, so you are missing an important "to" and
probably a useful "entire", although that is an implementation
detail, not a functional thing - that is, the required function
is:
- copy the first LNFL bits of the Argument into the LNF
- "shuffle left" the C-SIDs in the Argument so that the first
is replaced with the second, and so on, and the last is replaced
with zeros
It is probably OK to use the "copy the Arguments" approach so long
as it is made clear as:
"by copying the entire Argument value from the SID into the SID at
the point immediately after the Locator-Block"
c. "and filling the least significant bits of the argument with
zeros"
Well, how many bits shall I fill with zeros? The text is ambiguous
and the overwrite doesn't automatically cause the correct number of
bits to be filled with zeros.
d. The text is coy about the fact that the C-SID shuffle is taking
place in the DA field

30. 4.1 ends with

| The C-SID sequence ends with the last C-SID container.

But isn't it the case that each C-SID container is independent? That
is, each C-SID container looks like Figure 1 and starts with an LB. In
that case, this sentence is meaningless.

31. In 4.1.1, etc., I really wish you hadn't numbered your new lines of
code using numbers that already exist in the code block into which
you are making an insertion. The complete code block in once case
will now read:
S01.
S01.
S02.
S03.
S04.
S05.
S06.
S07.
S08.
S09.
S02.
S03.
:
:
That is frankly ugly.
And in the other case
S01.
S02.
S03.
S04.
S05.
S06.
S07.
S08.
S09.
S01.
S02.
S03.
:
:
which is just messy.
Neither case matches what is in Appendix B.1

32. 4.1.2

| The resulting pseudocode is inserted between lines S01 and S02 of the
| SRH processing in Section 4.2 of [RFC8986], and a second time before
| line S01 of the upper-layer header processing in Section 4.1.1 of
| [RFC8986], or prior to processing any extension header other than
| Hop-by-Hop or Destination Option.

I cannot parse your "or prior". You have "A, and B, or C." I also
cannot tell how I would choose to pick the "or" alternative.

33. 4.1.3

| The resulting pseudocode is inserted between lines S01 and S02 of the
| SRH processing in Section 4.3 of [RFC8986], and a second time before
| line S01 of the upper-layer header processing in Section 4.1.1 of
| [RFC8986], or prior to processing any extension header other than
| Hop-by-Hop or Destination Option.

4.3 of 8986 does not have a line S01 or S02. One might deduce the
meaning related to 4.3's implied use of code from 4.1 (8986 doesn't
make an explicit reference), but it would be helpful to make it
clear.

Same problem with "or prior" as in 4.1.2

34. 4.1.4 and 4.1.5 took a lot of work to understand that the DA field
with the half-processed C-SID container will be in place and ready to
continue when the encapsulation caused by the B6-Encaps SID is
stripped. Perhaps I am just not familiar enough with these SIDs, but
it wouldn't hurt to add words to explain the situation at the tunnel
end.

35. 4.1.7

| USD: The USD flavor is unchanged when combined with the NEXT-C-SID
| flavor. The pseudocodes defined in Section 4.1.1, Section 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.

Doesn't the latter part of this paragraph actually say that the USD
flavor processing *is* changed from 8986?

s/pseudocodes/pseudocode/

36. Figure 2 has the same caption issue as Figure 1

37. 4.2 uses an example as a definition. Thus...

| Each container is a 128-bit segment list entry holding a series of
| C-SIDs without the Locator-Block, as shown in Figure 3.

But Figure 3 is "only" an example, and is not definitive. I think the
example is fine, but definitive would look like...

+------------------------------------------------------------------+
| nth C-SID | ......... | 2nd C-SID | 1st C-SID | zero |
| (posn 0) | | (posn n-2) | (posn n-1) | pad |
+------------------------------------------------------------------+
<--- LNFL ---> <--- LNFL ---> <--- LNFL --->

This seems important because it will not always be the case that
LNFL divides into 128 perfectly, and we need to show which end carries
the padding. I see from the pseudocode in 4.2.1 that there is an
expectation that this series of C-SIDs can be read as an array which
would require it to be left justified with any padding on the right
hand end.
I do also see in 4.2.1...

| S25. Set DA.Arg.Index to (128/LNFL - 1).

... which implies that it is required that LNFL *does* divide into 128
perfectly.

Clearly some clarification is required.

38. I am pretty sure that I can read 4.2 to say that multiple C-SID
containers per Figure 3 can be present following an initial
LB-carrying C-SID container so that the whole might look like...

+------------------------------------------------------------------+
| Locator-Block | Locator-Node | Argument |
| | + Function | |
+------------------------------------------------------------------+
| nth C-SID | ......... | 2nd C-SID | 1st C-SID | zero |
+------------------------------------------------------------------+
| mth C-SID | ..... | n+2nd C-SID | n+1st C-SID | zero |
+------------------------------------------------------------------+

Although I am not clear how the wrapping happens when the 128 bits is
not cleanly filled with C-SIDs.

It would be really helpful to say all this clearly.

39. There is a nice feature buried in 4.2 that says that a C-SID in the
C-SID container can be of not REPLACE-C-SID flavor without it being an
error and with processing working correctly. I can see how this would
be helpful. I wish that was called out more clearly.

I think the same could be the case for NEXT-C-SID (that when you copy
across the AL you end up with a SID (LB+LNF) that is not a NEXT-C-SID
flavor. Presumably this is also allowed and also brings the C-SID
container processing to a stop.

It seems that there is a requirement in both cases that a C-SID of
one flavor must not appear in a C-SID container of the other flavor
even as a "last SID" as described in 4.2. That seems an important
thing to say.

40. Note that, per #39, there is a gnarly error condition. If a not
REPLACE-C-SID LNF finds its way into a C-SID sequence and it is not
at the end of the sequence, that is OK because the rest of the
sequence will be skipped, except...

If the not-REPLACE-C-SID is in one C-SID container and there follows
another (continuation) C-SID container, then stuff will go badly
wrong! The packet will either be misrouted or dropped at the next
node. Dropping is not so bad, misrouting is embarrassing and to be
avoided. In both cases, network diagnostics are needed.

In general, it may be argued that this is an encoding error. Whoever,
built the SID list made a mess. But there is the case where the node
that initiated the SID has made a mistake, and there may be a transit
case where a SID is moving between flavors.

41. Can I just say that in 4.2.2, "Replace S18 and S29 with S01" is
not as clear as it could be.

4.2.3 is even a little unclearer as it replaces two lines with two
lines. But it intends to replace each of the instances of one line
with a pair of lines.

And so on in 4.2.4

42. 4.2.4 S04 could usefully say to what these fields are set.

43. There is an obvious imbalance between NEXT-C-SID and REPLACE-C-SID
wrt the End.DX and End.DT SIDs. If those types of SID must not be
given NEXT-C-SID flavor, then something needs to be said.

44. 4.2.7 has

| 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 don't think 6.4 uses a MUST so there is no requirement.

45. Could you begin Sections 5.1 and 5.2 with full sentences?

46. 5.1 has

| A Global C-SID typically identifies a shortest path to a node in the
| SRv6 domain. An IP route is advertised by the parent node to each of
| its global C-SIDs, under the associated Locator-Block. The parent
| node executes a variant of the End behavior.
|
| A node can have multiple global C-SIDs under the same Locator-Block
| (e.g., one per IGP flexible algorithm). Multiple nodes may share the
| same global C-SID (anycast).

a. "typically" leaves the reader wanting to know about other possible
cases.

b. If a Global C-SID identifies a path, how come a route is advertised
to a global C-SID? What does that mean?

c. Please decide "Global C-SID" or "global C-SID"

d. "a variant of the End behavior" Which variant?

e. "IGP flexible algorithm" needs a citation.

f. Should "(anycast)" be "(for example, anycast)"?

47. 5.2 has

| A local C-SID may identify a cross-connect to a direct neighbor over
| a specific interface or a VPN context.

a. "may identify" makes the reader wonder how to decide what it
identifies and what other choices exist.

b. The term "a cross-connect" is peculiar when talking about packet
routers. Is this just "a connection"?

48. Section 6.1 looks like it is really important and should be moved
to Section 4.1.

There are two uses of RECOMMENDED in this section. You need to clarify
whether this is a recommendation to the implementer or the deployer:
if to the deployer, how do they control this and what are the
implications for the implementation? if to the implementer, what are
the implications for interoperability?

And in any case, using RECOMMENDED is like using SHOULD. You need to
give an indication of what alternatives exist (that's easy) and how
and why the alternative might be chosen.

49. Section 6.2 has...

| 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.
|
| The RECOMMENDED Locator-Block size for the REPLACE-C-SID flavor can
| be 48, 56, 64, 72, or 80 bits, depending on the needs of the
| operator.

For both cases of RECOMMENDED, you are allowing other LBL values, but
you are not giving any guidance about how/why another value would be
chosen.

For REPLACE-C-SID you say "can be" which doesn't sit right with
"RECOMMENDED".

50. Another use of RECOMMENDED in 6.3 that needs discussion of variation
of the suggested values.

51. Section 6.4 has

| An SR segment endpoint node instantiating a NEXT-C-SID or REPLACE-
| C-SID flavored SID SHOULD install the corresponding FIB entry to
| match only the Locator and Function parts of the SID (i.e., with a
| prefix length of LBL + LNL + FL).

This "SHOULD" needs guidance on the alternative (MAY): what is it,
why might it be chosen.

The MAY in the subsequent paragraph is good in that it gives a reason,
but I note that it is "in addition" which suggests that it might not
apply if the SHOULD in this paragraph is violated.

52. 7.1 and source node SID validation

I'm glad to see this section, but I'm puzzled. Perhaps I am missing
something. When a SID is advertised or explicitly configured for use
by a source node, I think it just shows up as a 128-bit blob. Have I
missed something important?

If, for example per draft-ietf-idr-segment-routing-te-policy section
2.4.4.2.2 the SID is presented without interpretation, I don't see
how the source node can verify the SIDs. How does it know the various
LBL, LNL and FL (things that are well known at the node that initiated
the SID)?

Is there a requirement that the SID Structure TLV is used with the
advertisement? Indeed, this seems to be implied in the example in 7.2,
but you should call this out much higher up the document as an
absolute "requirement that the source node is able to determine the
structure of the SID, for example using the SID Structure TLV [ref]."
Oh, wow, I found this in Section 9. Well, I suppose the implementer
is supposed to read the whole document, but some more text or a cross-
reference would help.

53. In 7.2

| An SR source node MAY compress a segment list when it includes NEXT-
| C-SID and/or REPLACE-C-SID flavor SIDs.

I appreciate that compression is optional even when C-SID flavor SIDs
are in use. However, I think this sentence is too terse.
- How/why would a source node decide to compress a segment list?
- What does a source node do if it decides to not compress? (that's
easy to answer)
- Are there any implications in the network to the choice that the
source node makes?
- In the case where the segment list is provided by a third party
(e.g., by a controller), can it be supplied already compressed?
See the final paragraph in Section 9.
- The "and/or" here turns out to be very important. I think it is the
first hint in the document that both flavors can be present in the
same SID stack. It needs more discussion.

54. 7.2 gives guidance of how to do compression by means of an example,
and I think that is OK. But there are a few lines that are contained
within the example that are, I think, normative.

Thus:
- s/with this method//
- Move to an earlier section of the document

55. 7.2

| When the compression method encounters a series of consecutive
| eligible NEXT-C-SID or REPLACE-C-SID flavored SIDs, it compresses the
| series as follows.

You have to love the English language! The use of "or" here is
ambiguous because it implies that the series {NEXT-C-SID,
REPLACE-C-SID, NEXT-C-SID} is eligible for compression. Fix as...

| When the compression method encounters a series of consecutive
| eligible NEXT-C-SID flavored SIDs or a series of consecutive
| REPLACE-C-SID flavored SIDs, it compresses the series as follows.

Funnily, however, your example process does not require a series of
consecutive eligible C-SIDs, and will work in the case of precisely
one such C-SID. Recognising that, makes my question go away (although,
obviously, it results in no compression).

56. 7.2

| S01. Initialize a C-SID container as the first segment in the
| uncompressed segment list, with all argument bits set to 0,
| and initialize the remaining capacity to AL

Could this usefully say how to initialize the LB and LNF fields in
the first segment in the segment list? What does it mean to refer to
"as the first segment in the uncompressed segment list"?

Is the resolution to my two questions s/as/equal to/ ?

On the other hand, it seems extreme to explicitly say that the
argument bits are set to 0 because of the constraint you have already
stated as:

| * The SID argument value is 0.

57. The description of the process in 7.2 for REPLACE-C-SID seems to be
incomplete. It says...

| * For a series of REPLACE-C-SID flavor SIDs of the same C-SID length
| in the uncompressed segment list, the first segment is not
| compressed. The SR source node then initializes an empty C-SID
| container with all bits set to 0 and inserts consecutive C-SIDs
| from the uncompressed segment list into the container. When the
| container is full, it is pushed on the compressed segment list and
| a new container is initialized.

This doesn't handle the case of the end of the sequence when:
- if the C-SID container is empty, it is not pushed
- the (not empty) not full C-SID container is pushed.

58. I don't think you should reproduce text from another RFC like you
do in 7.2. It makes it delicate in the case of any (accidental)
differences, and vulnerable to errata or modifications of the source
RFC. A reference should be enough.

If you feel you must include the quote, can you mark it in some way so
that it is possible to see where the quote ends and this document's
text resumes. (Using the pipe '|' is often a neat trick.)

59. In 7.4

| 2. A series of REPLACE-C-SID flavor SIDs MUST contain at least two
| elements.

Why? Surely it works perfectly well if the series has just one such
SID and it is presented in the SID list? In this case, there is no
distinction between this and the SID being in the list in uncompressed
form.

This requirement places a considerable burden on the component that
computes the SR path because it has to look ahead at the next hop
before it works out which SID to use at the current hop. Further, it
is not clear how a source node would behave it was supplied with a SID
list by an external party, and that list contained a single REPLACE-C-
SID flavor SID.

60. In 7.4

| 3. End of a C-SID sequence can be indicated by:

Is there also a case where the sequence is at the end of the segment
list?

61. In 7.4 there are multiple examples of text like...

| When receiving a 128-bit SID with NEXT-C-SID flavor, LNL=16, FL=16 or
| 0, AL=128-LBL-NL-FL and the value of argument is all 0, the source
| node marks the SID supporting 16-bit C-SID.

a. In just one case you have "marks the SID as supporting" but I think
all cases should use that additional "as" to make it clear.
b. In all cases, it is not clear where this marking is done.

62. I had to read Section 8 a few times.

The End.XPS is a new SID behavior defined in this document. While you
then go on to define how it may be given a C-SID flavor, the End.XPS
SID is not anything to do with compression: it is a new concept and I
don't see why you have "hidden" it in this document. Indeed, it
doesn't seem to be mentioned in the Implementation Status section or
in the IANA section, so perhaps it is "ambitious" to include it in
this document.

While you say that the "End.XPS behavior described in this section is
OPTIONAL" I think that is a bit confusing. It implies that the
described processing is optional. I think you could use "The use of
the End.XPS SID behavior is OPTIONAL." But you do go on to write...

| This section defines an optional solution and SID behavior allowing
| for the use of different Locator-Blocks between routing domains.

So how do you handle the scenario described if you don't use this
optional solution and an End.XPS SID behavior?

63. Section 8

s/corresponding the Locator-Block/corresponding to the Locator-Block/

64. Section 8

| Note: the way the Locator-Block B2 of the next routing domain is
| known is out of scope of this document. As examples, it could be
| learnt via configuration, or using a signaling protocol either with
| the peer domain or with a central controller (e.g. Path Computation
| Element (PCE)).

This could use a citation for PCE. Perhaps, RFC 4655, but perhaps the
PCEP document that provides this feature.

65. In Section 8

| When End.XPS SID behavior is used, the restriction on the C-SID
| length for the REPLACE-C-SID flavor is relaxed and becomes: all SID
| the are part of a C-SID sequence *within a domain* MUST have the same
| C-SID length LNFL.

a. Please add a back reference to where the C-SID length restriction
is defined.
b. The English is a bit mangled. Perhaps, "all SIDs that are part..."
c. The use of * to highlight text is non-standard and, I think, not
needed.
d. "within a domain" is ambiguous because all SIDs are always within
a domain. Perhaps, "within the same domain"?

66. Section 9

| The SR segment endpoint node MUST set the SID argument bits to 0 when
| advertising a locally instantiated SID of this document in the
| routing protocol (e.g., IS-IS [RFC9352], OSPF
| [I-D.ietf-lsr-ospfv3-srv6-extensions], or BGP-LS
| [I-D.ietf-idr-bgpls-srv6-ext]).

a. "SID of this document" is probably "SID with a NEXT-C-SID or
REPLACE-C-SID flavor"

b. What happens if a node receives an advertisement of a SID with one
of these flavors but does not support the advertised flavor? That
may be answered with a simple "handled as described in..."

c. What happens if a node receives an advertisement of a SID with one
of these flavors but the argument bits are non-zero? The answer is
is not to be found in other documents because those documents don't
describe those flavors (but you might have "MUST treat the received
advertisement as malformed/unsupported and process it as described
in Section x.y of RFC abcd."

67. Section 9

| Signaling the SRv6 SID Structure is REQUIRED for all the SIDs
| introduced in this document.

a. I think you mean "for both SID flavors introduced in this
document".

b. What does a receiver do if it gets an advertisement of one of a
SID with one of these SID flavors but without an indication of the
SRv6 SID Structure? Again, possibly, "MUST treat the received
advertisement as malformed/unsupported and process it as described
in Section x.y of RFC abcd."

68. Section 9

| The length values
| in the SRv6 SID Structure advertisement MUST match the format of the
| SID on the SR segment endpoint node.

Given that it is the segment endpoint node that is responsible for
doing the advertisement, I think this should read...

| The SR segment endpoint node initiating the advertisement of a SID
| with NEXT-C-SID or REPLACE-C-SID flavor set the length values in the
| SRv6 SID Structure in the advertisement to match the format of the
| SID.

69. Section 9

| A local C-SID MAY be advertised in the control plane individually or
| in combination with a global C-SID instantiated on the same SR
| segment endpoint node, with the End behavior, and the same Locator-
| Block and flavor as the local C-SID.

a. This "MAY" is a bit confusing. It would normally be assumed to
apply to the combination "a or b". You might fix this with
s/individually/either individually/

b. When you have a choice like this, you should give the implementer
a clue about how to choose. Maybe it is a free choice, maybe it is
a configuration knob, maybe there are optimisations or benefits to
one approach or the other.

70. Section 9

For a segment list computed by a controller and signaled to an SR
source node (e.g., via BGP [I-D.ietf-idr-segment-routing-te-policy]
or PCEP [I-D.ietf-pce-segment-routing-ipv6]), the controller provides
the ordered segment list comprising the uncompressed SIDs to the SR
source node. The SR source node may then compress the segment list
as described in Section 7.

I asked a question in #53 about whether the compression could be done
by a controller. You seem to be saying that it cannot, but I don't see
why that is the case. Compression is non-trivial processing, and there
could be implementation benefits from placing it in a controller.

However, it is possible that your thinking is that the source node
would like to hold an uncompressed SID list for diagnostic purposes.
In any case, how does a source node process if it receives a SID list
that has already been compresses? In some cases, how would it even
know!

Actually, there is an implicit architectural requirement here that
sets the control as determining the path as a SID list, but the source
node as responsible for listening to the SID advertisements to know
what flavors (and SID structures) have been advertised in order to
process the compression. Why do you force this separation? Why can't
the controller listen to all the advertisements?

71. In 10.1 you, again and several times, have "a SID of this document"
and I think you need to say "a SID with a NEXT-C-SID or REPLACE-C-SID
flavor."

72. 10.1

| When pinging a SID of this document via a segment list, the SR source
| node constructs the IPv6 packet as described in Section 7 and
| computes the ICMPv6 checksum using the IPv6 destination address as it
| is expected to appear at the ultimate destination of the packet.

I appreciate you calling this out, and I think it is the topic of a
thread on the mailing list, but this checksum comment seems odd to me.
I went back to RFC 9259 to see what it has to say about checksums in
the case of a ping with a segment list, and found nothing (especially
in Appendix A).

If this is cunning magic, then the reader could use a clue about what
is going on.

73. 10.2

| Section 5.4 of [RFC8754] defines the logic that an SR source node
| should follow to determine the ultimate destination of an invoking
| packet containing an SRH.

Why is this "should follow"? RFC 8754 does not seem to make this
optional. I suggest s/should follow/follows/

I presume that the logic in this section is exactly what would be
required in 10.1 to determine the ultimate DA that is being used to
compute the checksum?

74. I wonder what the plans are for the draft referenced from Section 11
I-D.clad-spring-srv6-srh-compression-illus appears to have expired
some long time ago and has only had nit changes since it was first
posted in October 2021. Is the content even consistent with this draft
which has constantly evolved over the last two years?

Clearly, that draft is not normative and only supplies illustrations,
but if it is deemed helpful to have illustrations, something needs to
change. Alternatively, perhaps Section 11 should be cut.

75. I think Section 12 could really use some more details.
For example:
- Do you expect deployments to restrict a single SR-domain to use at
most one flavor of C-SID?
- If not, it would be useful to have a section in the document
making a clear description of the processing when both flavors
are present. I think it "just works" but a little more text
describing how/why this is the case would help. And compare with
Section 4 where there is a recommendation to limit to a single
flavor in a single domain - certainly, the "ease of operation"
mentioned in that section deserves to be called out here.
- Do you make a distinction between SR-domain and AS/routing domain
as described in Section 8?

76. You might add a note to the top of Section 13 to remind the RFC
Editor to clean up the many references from this section when it is
deleted.

77. I found Section 14 to be very sparse and so a little unbelievable.
Surely Section 8 introduces some Security concerns? Should you
have an Informative reference to draft-li-spring-srv6-security-
consideration or some similar ongoing work to address the continued
concerns about SR security.

78. Is Section 15 (IANA) missing registration for End.XPS with and
without flavors?

79. I think Section 15 needs to also request that the Change Controller
is changed to "IETF" for all of the code points.

80. I think that three references need to be moved from 17.2 to 17.1
because of how they are used in the text. I don't think any of these
causes a problem.
8200
9256
9259

81. Hoping that Appendix A will be resolved

82. I wonder whether you need some clarification in the document to
describe if you can have multiple C-SID flavors of the same SID
advertised and, if so, how a path selection node might decide which
flavor (including the non-CSID flavor) to use at any hop as it
builds the path.

83. It would be really good to include a section on future compatibility.

a. What consideration should be given to future SID endpoint behaviors
in terms of making them compressible using the flavors here?

b. Are there any comments the document should include about inventing
future C-SID flavors

_______________________________________________
spring mailing list
spring@ietf.org<mailto:spring@ietf.org>
https://www.ietf.org/mailman/listinfo/spring

Disclaimer

This e-mail together with any attachments may contain information of Ribbon Communications Inc. and its Affiliates that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.