Re: [spring] Chair Review of draft-ietf-spring-srv6-srh-compression-11

[Alvaro Retana <[e-mail]>]

On February 29, 2024 at 12:50:46 PM, Francois Clad wrote:

Francois:

Hi!

> We have integrated those changes as part of revisions -12 and -13 of the
> document. Please find our detailed replies inline.

I have put comments below as well, and deleted any parts were we agree
or no more discussion is needed.

BTW, it looks like my review may have been truncated: you replied up
to line 1203, but it keeps going.  Take a look at the complete review
here: https://mailarchive.ietf.org/arch/msg/spring/scPK_7Cc8-G2aKbDWQV3sQnC9VA/

Thanks!

Alvaro.


...
> > Operational Considerations/Guidance
> >
> > Dhruv brought up [1] the point of providing "guidance on when to use
> > which flavor and with which C-SID lengths". I fully agree! The
> > document contains (mostly in §4) recommendations, for example, about
> > LBL and C-SID lengths, even if any other value is possible. IOW, the
> > possibilities are endless! Please provide more operational
> > considerations on how an operator can evaluate their needs and select
> > the appropriate flavor/value for their deployment.
>
> We added a paragraph providing such operational guidance in revision -12, and
> further clarified it in revision -13.
>
> | SRv6 is intended for use in a variety of networks that require
> | different prefix lengths and SID numbering spaces. Each of the two
> | flavors introduced in this document comes with its own
> | recommendations for Locator-Block and C-SID length, as specified in
> | Section 4.1 and Section 4.2. These flavors are best suited for
> | different environments, depending on the requirements of the network.
> | For instance, larger C-SID lengths may be more suitable for networks
> | requiring ample SID numbering space, while smaller C-SID lengths are
> | better for compression efficiency. The two compression flavors allow
> | the compressed segment list encoding to adapt to a range of
> | requirements, with support for multiple compression levels. Network
> | operators can choose the flavor that best suits their use case,
> | deployment design, and network scale.

I don't consider this paragraph enough.  I understand that "different
environments, depending on the requirements of the network" *may*
result in a different choice.  I want to understand how "operators can
choose the flavor that best suits their use case, deployment design,
and network scale."

Let me illustrate with an example.  The text above says that "larger
C-SID lengths may be more suitable for networks requiring ample SID
numbering space, while smaller C-SID lengths are better for
compression efficiency".  Ok, what are my choices?

   §4.1: "An implementation MUST support...a 16-bit C-SID length (LNFL) for
          NEXT-C-SID flavor SIDs, and may support any other...C-SID length."

   §4.2: "This document defines the REPLACE-C-SID flavor for 16-bit and 32-bit
          C-SID lengths (LNFL). An implementation MUST support a 32-bit C-SID
          length for REPLACE-C-SID flavor SIDs."

Judging from the *required* implementations, it looks like if an
operator wants a C-SID length that is "better for compression
efficiency" then it should use the NEXT-C-SID flavor (because 16-bit
may only be supported there).

However, if we assume that vendors will offer other options, for
example 16-bit C-SIDs for the REPLACE-C-SID flavor, what next?  Which
C-SID should I chose?

I know that this document is not a deployment guide and that it cannot
cover all cases.  But the current guidance is basically non-existent
given the large amount of choice.



...
> > [major] "SHOULD use a consistent Locator-Block length and C-SID length
> > for all SIDs of the SR domain"
> >
> > When is it ok to not be consistent? IOW, why is this recommended and
> > not required? What are the effects of not being consistent?
>
> Deployments may use any Locator-Block and C-SID length. However, the C-SID
> compression relies on common Locator-Block and consistent C-SID length, so
> using inconsistent ones, while possible, will lead to reduced compression
> efficiency.

Please add this information to the draft.  Note that clarifying this
option is good operational guidance.  Also, it appears several times
in the document.

"Because you can doesn't mean you should."



...
> > 362 An SR segment endpoint node instantiating a SID with the NEXT-C-SID
> > 363 flavor MUST accept any Argument value for that SID.
> >
> > [major] Does this also mean that any future behavior cannot make use
> > of an Argument? IOW, behaviors like End.DT2M cannot be used with the
> > NEXT-C-SID flavor. If so, please be explicit about it.
>
> This statement does not impose any requirement on future behaviors.
>
> The argument of the NEXT-C-SID flavor SIDs defined in this document only
> contains the following C-SIDs in the container, for which an endpoint node
> must accept any value.
>
> A future document defining another NEXT-C-SID flavor SID whose argument
> contains other pieces of information will need to define the structure of
> that argument and acceptable values. Most likely, the part of the argument
> carrying the following C-SIDs will follow the same rule as stated here.

The text doesn't point at the requirement (MUST) applying to only the
SIDs in this document.  If that is the case then please be explicit.



...
> > 377 4.1.1. End with NEXT-C-SID
> > ...
> > 384 The below pseudocode is inserted between lines S01 and S02 of the SRH
> > 385 processing in Section 4.1 of [RFC8986]. In addition, this pseudocode
> > 386 is executed before processing any extension header that is not an
> > 387 SRH, a Hop-by-Hop header or a Destination Option header, or before
> > 388 processing the upper-layer header, whichever comes first.
> >
> > [major] "In addition..."
> >
> > This sentence is not needed because S01 says "When an SRH is
> > processed", so we're already processing the SRH. Also, this sentence
> > is paraphrasing the ordering in §4/rfc8200 -- which makes it
> > unnecessary as the behavior is already specified elsewhere.
> >
> > Furthermore, Appendix A.1 shows the pseudocode being executed "before
> > processing the upper-layer header". However, that upper-layer header
> > would only be processed *after* the SRH is processed (rfc8200) -- so
> > doing it again is unnecessary.
> >
> > Please remove both the sentence above and the extra step in A.1
> > *before* the upper-layer header.
> >
> > ** Note that other descriptions in this section also contain the same
> > text and should be modified in the same way (including the
> > appendices).
> >
>
> The SRH processing is performed when the packet contains an SRH. The sentence
> that you quoted (and the corresponding step described in appendix) covers the
> cases where it does not. This may occur when the SRH is omitted in the SRv6
> encapsulation (section 4.1 of RFC 8754 or section 5 of RFC 8986) or when it
> is removed before the ultimate destination (section 4.16.1 of RFC 8986).

I see.

This point is related to the still-open issue of L4 checksums when no
SRH is present.  If that discussion results in a requirement to always
carry the SRH then we'll need to remove the text; otherwise, something
will need to be added to §6.5 about it.



> > 390 N01. If (DA.Argument != 0) {
> > 391 N02. If (IPv6 Hop Limit <= 1) {
> > 392 N03. Send an ICMP Time Exceeded message to the Source Address,
> > 393 Code 0 (Hop limit exceeded in transit),
> > 394 interrupt packet processing and discard the packet.
> > 395 N04. }
> >
> > [major] Why are the other checks not done? For example, why are SL
> > not checked? I understand that if the previous node didn't change it
> > then it should be ok -- but it may not!
>
> This pseudocode specifies a dataplane behavior implemented in the forwarding
> path of routers, where every operation matters. For that reason, sanity
> checks are strictly limited to the fields used as part of the packet
> processing. For instance, the value in the Segments Left field is only
> validated when it is used or modified.

Ok -- the problem remains.  Without the SL check, for example, packets
could be forwarded that shouldn't.  Given the performance expectation,
it would be good to add a note about any potential risk/threat.



...
> > 547 4.2. REPLACE-C-SID Flavor
> > ...
> > 597 The RECOMMENDED Locator-Block lengths (LBL) for REPLACE-C-SID flavor
> > 598 SIDs are 48, 56, 64, 72, or 80 bits, depending on the needs of the
> > 599 operator.
> >
> > 601 The REPLACE-C-SID flavor supports both 16- and 32-bit C-SID lengths
> > 602 (LNFL). A C-SID length of 32-bit is RECOMMENDED.
> >
> > 604 Any other Locator-Block and C-SID length selection is possible, but
> > 605 may lead to suboptimal C-SID encoding in the C-SID containers (e.g.,
> > 606 presence of padding bits).
> >
> > [major] The first two of the three paragraphs above suggest the use of
> > specific values, but it is not until the third paragraph that the
> > reasons become clear -- and it is clarified that any length selection
> > is possible. This makes the initial paragraphs a little misleading
> > because it gives the impression that only specific lengths are
> > supported.
> >
> > Suggestion:
> >
> > The REPLACE-C-SID flavor supports any Locator-Block and C-SID
> > length selection. LBL values of 48, 56, 64, 72, or 80 bits,
> > and C-SID lengths of 16- or 32-bits are RECOMMENDED to avoid
> > suboptimal C-SID encoding in the C-SID containers (e.g.,
> > presence of padding bits).
>
> We clarified the requirements and recommendations in revision -13.
>
> | The REPLACE-C-SID flavor SIDs support any Locator-Block length (LBL),
> | depending on the needs of the operator, as long as it does not exceed
> | 128-LNFL-ceiling(log_2(128/LNFL)) (ceiling(x) is the least integer
> | greater than or equal to x [GKP94]), so that enough bits remain
> | available for the C-SID and Argument. A Locator-Block length of 48,
> | 56, 64, 72, or 80 bits is RECOMMENDED for address planning reasons.

Related to the operational guidance  I'm looking for -- what "address
planning reasons"?  How does my address planning influence the
decision?

Looks like there is no required LBL implementation support.  IOW, the
RECOMMENDED values are from the deployment point of view, right?  If
so, then there's no interoperability-related need to use normative
language.  s/RECOMMENDED/recommended



...
> > 840 The SR segment endpoint node obtains the value Arg.FE2 from the 16
> > 841 most significant bits of DA.Argument if DA.Arg.Index is zero, or from
> > 842 the 16 least significant bits of the next position in the current
> > 843 C-SID container (Segment List[Segments Left][DA.Arg.Index-1])
> > 844 otherwise (DA.Arg.Index is non-zero).
> >
> > [?] Where does the 16-bit value come from? rfc8986 doesn't specify
> > the size of Arg.FE2, and the related applications don't seem to match
> > in length. What am I missing?
>
> This document sets the length of Arg.FE2 to 16 bits for the End.DT2M with
> REPLACE-C-SID SID. We clarified this in revision -13.
>
> | The value of Arg.FE2 is 16-bit long. The SR segment endpoint node
> | obtains the value Arg.FE2 from the 16 most significant bits of
> | DA.Argument if DA.Arg.Index is zero, or from the 16 least significant
> | bits of the next position in the current C-SID container (Segment
> | List[Segments Left][DA.Arg.Index-1]) otherwise (DA.Arg.Index is non-
> | zero).

s/The value of Arg.FE2 is 16-bit long./The value of Arg.FE2 is 16-bit
long when used with the REPLACE-C-SID C-SID.



...
> > 963 5.4. Recommended Installation of C-SIDs in FIB
> >
> > 965 An SR segment endpoint node instantiating a NEXT-C-SID or REPLACE-
> > 966 C-SID flavor SID SHOULD install the corresponding FIB entry to match
> > 967 only the Locator and Function parts of the SID (i.e., with a prefix
> > 968 length of LBL + LNL + FL). Any other mean of identifying a locally
> > 969 instantiated SID is possible as long as it is compliant with
> > 970 Section 4.3 of [RFC8754] and accepts all valid Argument values for
> > 971 the SID.
> >
> > [major] §4.3/rfc8754 doesn't use normative language. It uses a
> > general statement that allows for different implementations:
> >
> > Without constraining the details of an implementation, the SR segment
> > endpoint node creates Forwarding Information Base (FIB) entries for its
> > local SIDs.
> >
> > It seems to me that rfc8754 already covers what wants to be conveyed
> > in this document: the FIB entry has to uniquely identify the segment
> > endpoint.
> >
> > As written, the text raises several questions:
> >
> > When is it ok to not "install the corresponding FIB entry to match
> > only the Locator and Function parts of the SID"? IOW, why is this
> > action recommended and not required? Note that the entry has to at
> > least cover "a prefix length of LBL + LNL + FL".
> >
> > The other means refer to §4.3/rfc8754, which (as shown above) says
> > that anything (including "install the corresponding FIB entry to match
> > only the Locator and Function parts of the SID") is ok. This takes us
> > back to my original point: rfc8754 already covers what this section
> > wants to convey and it is not needed.
> >
> > "all valid Argument values" -- most of the SIDs used don't use an
> > Argument, so which are the "valid Argument values"? s/all valid
> > Argument values/any Argument value
>
> In general, an implementation could identify a locally instantiated SRv6 SID
> with argument by installing multiple /128 FIB entries, one for each valid
> argument value. However, such method is not suited for NEXT-C-SID and
> REPLACE-C-SID flavor SIDs given than any argument value is valid. We
> clarified this in revision -13.
>
> | Section 4.3 of [RFC8754] defines how an SR segment endpoint node
> | identifies a locally instantiated SRv6 SID. To ensure that any valid
> | argument value is accepted, an SR segment endpoint node instantiating
> | a NEXT-C-SID or REPLACE-C-SID flavor SID SHOULD install a
> | corresponding FIB entry that matches only the Locator and Function
> | parts of the SID (i.e., with a prefix length of LBL + LNL + FL).

My opinion is still that rfc8754 already covers what this section
wants to convey.

If you want to keep it you should maintain the "spirit" of the text in
§4.3/rfc8754 and avoid using normative language (s/SHOULD
install/installs), OR, explain when is it ok to not "install a
corresponding FIB entry...". IOW, why is this action recommended and
not required?



...
> > 1094 The segment list that the SR source node pushes onto the packet MUST
> > 1095 comply with the rules in Section 6.3 and Section 6.4 and result in a
> > 1096 path equivalent to the original segment list.
> >
> > [major] "MUST...result in a path equivalent to the original segment list"
> >
> > How is a "path equivalent to the original" defined? The next
> > paragraph mentions "a compressed segment list of equal or shorter
> > length than the uncompressed segment list". What does the length
> > refer to -- the number of Segment Lists in the SRH, the size of the
> > SRH, or something else?
>
> We clarified in revision -13 that this is "the same forwarding path".
>
> The length of a segment list is the number of elements that it contains.
>
> > 1098 If an SR source node chooses to compress the segment list, one method
> > 1099 is described below for illustrative purposes. Any other method
> > 1100 producing a compressed segment list of equal or shorter length than
> > 1101 the uncompressed segment list is compliant.

The requirement is: "MUST...result in the same forwarding path as the
original segment list."  BUT a method that produces "a compressed
segment list of equal or shorter length than the uncompressed segment
list is compliant".

I understand the intent, but given that it is a requirement, how can
"the same forwarding path" be enforced?  If the path is loose, how can
"the same forwarding path" be guaranteed?

Suggestion>

   The segment list that the SR source node pushes onto the packet MUST
   comply with the rules in Section 6.3 and Section 6.4 and should result
   in the same forwarding path as the original segment list.



...
> > 1107 * When the compression method encounters a series of one or more
> > 1108 consecutive compressible NEXT-C-SID flavor SIDs, it compresses the
> > 1109 series as follows. A SID with the NEXT-C-SID flavor is
> > 1110 compressible if its structure is known to the SR source node and
> > 1111 its Argument value is 0.
> >
> > [major] Unlike the REPLACE-C-SID flavor (below), there's no check
> > equivalent to ConCheck for the NEXT-C-SID flavor SIDs. Why isn't that
> > needed?
>
> A similar check is performed inline on line S03 of the first pseudocode in this section.

Yes, similar, but not the same.  ConCheck explicitly checks if the
same SID structure is shared -- why isn't that needed here?

[]