Segment Routing with the MPLS Data Plane
draft-ietf-spring-segment-routing-mpls-22

(1) Section 1.  Shouldn’t RFC8402 be “[RFC8402]”?

(2) Section 1.  First use of the acronym, but above there was no “Segment Routing (SR)”.

(3) Section 2.1.  Editorial.  s/,…,etc/, etc./

(4) Section 2.4.  The variable name in the text is “Size” and “size” in the equation.  They should be consistent.

(5) Section 2.4.  Per Step #5 of the collision resolution procedure, could you please clarify what is a “non-zero algorithm”?

(6) Section 2.5.1.  Could you please clarify what “better administrative distance means”.  Per the tie-breaking rules above, I assume you mean it is a lower administrative distance.

(7) Section 2.5.1, for the sub-bullets under “The fields of each FEC are encoded as follows”, consider adding periods to the first two bullets:

s/significant bits are set to zero/ significant bits are set to zero./

s/the numerical value for IPv6/the numerical value for IPv6./

(8) The Security Considerations references [RFC8402] to lay out the assumed trust model and a few of the possible implications (malicious looping, evasion of access control, hiding of the source of DoS attacks) if this model is violated.  I concur on the language in the reference.  I believe there are a few more things to explicitly point out:

** Per the implications, routing traffic through an observation point controlled by the attacker is another key privacy and integrity concern.

** Per the already stated implication, “malicious loop” feels too soft as the overall availability of the network can be affected (e.g., loops, slowing traffic down).

FWIW, I’m not raising this to a DISCUSS because there is no normative language to address these issues, only additional proposed cautionary language of the threat.

Thank you for a well written / easy to understand document.

I have a few suggestions / nits. Please note: If the WG has already discussed these, and come to other decisions, I'm fine with that... 

1:  "In order to have a node segment to reach the node, a network operator SHOULD configure at least one node segment per routing instance, topology, algorithm. "
Perhaps: "segment per (routing instance, topology, algorithm)." (or "set of ..", or "combination of..." or something - not quite sure how best to word, but this seems slightly confusing).

2:  "If the SRGB of a node does not conform to the structure specified in this section or to the previous two rules, then this SRGB MUST be completely ignored by all routers in the routing domain and the node MUST be treated as if it does not have an SRGB."
Shouldn't this be logged somewhere?

" The rules applicable to the SRGB are also applicable to the SRLB, except rule that says that the SRGB MUST only be used to instantiate global SIDs  into the MPLS forwarding plane. The recommended, minimum, or maximum size of the SRGB and/or SRLB is a matter of future study"
3: I think there is a missing word(s) between 'except rule' - perhaps "for the"?
4: Missing period.

5: "For the purpose of incoming label collision resolution, a routing instance is identified by a single incoming label downloader to FIB." - is downloader the right word here? 

6: "If the derived numerical value varies for the same configuration, then an implementation SHOULD make numerical value used to identify a routing instance configurable."
This is a philosophical point, but it seems like I might always want to be able to configure this -- perhaps just "Implementations SHOULD make...? "

7: "This document defines the default tie breaking rules that SHOULD be implemented. An implementation MAY choose to support different tie-breaking rules and MAY use one of the these instead of the default tie-breaking rules. All routers in a routing domain SHOULD use the same tie-breaking rules to maximize forwarding consistency."
Is there any reason not to require that all implementations implement these rules (mandatory to implement)? I don't want to end up in a situation where I buy boxes from Vendor X, and then cannot add Vendor Y, because they don't share a set of rules.

8:  "R2 is the next-hop along the shortest path towards R8. By applying the steps in Section 2.8 the outgoing label downloaded to R1’s FIB corresponding to the global SID index 8 is 1008 because the SRGB of R2 is [1000,5000] as shown in Figure 2." - I was initially confused by the [1000,5000] as it isn't represented like this in the figure. Perhaps change either this text, or the text in the figure?

It is time to publish this one.

Nits
----

In section 2, "The flooding mechanisms of link state IGPs fits very well with label stacking on ingress" while I do not disagree with the statement, it is somehow out of the blue: either explain shortly why or delete the sentence if so obvious. Note: or even keep it like it is now.

Section 2.5.1, binary numbers 100 and 110 should probably explicitly qualified as binary representations.

Some typos in name or spacing in sections 6 and 7

§2.1:

>  The MCC in the network downloads
>  different MPLS labels/SIDs to the FIB for different forwarding
>  behaviors

Please expand "FIB" on first use.

Thank you for addressing (most of) my Discuss points!
I leave it to the responsible AD to decide whether any action is needed
on the author count.

[original COMMENT section preserved below; thank you for addressing
the comments therein as well]

It seems that we're introducing something of a new concept in this
document of "routing instance" as something with a numerical identifier.
(That is, this does not appear in RFC 8402 or RFC 3031, in terms of
what references I might expect to be in scope.)  Am I just missing some
other reference where this is introduced?  If not, maybe it is worth
mentioning in a terminology section.

[I think some of these section-by-section notes were spotted already;
I didn't get a chance to deduplicate.]

Section 2

   In order to have a node segment to reach the node, a network operator
   SHOULD configure at least one node segment per routing instance,
   topology, algorithm. [...]

nit: maybe "per tuple of [...]"?

Section 2.2

   o  The label value MUST be unique within the router on which the MCC
      is running. i.e. the label MUST only be used to represent the SID
      and MUST NOT be used to represent more than one SID or for any
      other forwarding purpose on the router.

Maybe I'm misreading the intent, but "MUST be unique" seems like it's a
requirement from core MPLS and need not be restated.

Section 2.3

                                                               The rules
   applicable to the SRGB are also applicable to the SRLB, except rule
   that says that the SRGB MUST only be used to instantiate global SIDs
   into the MPLS forwarding plane. [...]

nit: "except the rule"

Section 2.4

I'd consider writing the algorithm in real code (python?) rather than
abstract pseudocode.  In some cases (though probably not here?)
pseudocode makes it easy to miss edge cases that need to be specified in
order for things to be interoperably implementable.

Section 2.5

   MPLS Architecture [RFC3031] defines Forwarding Equivalence Class
   (FEC) term as the set of packets with similar and / or identical
   characteristics which are forwarded the same way and are bound to the
   same MPLS incoming (local) label. In Segment-Routing MPLS, local
   label serves as the SID for given FEC.

nit: there's some missing (in)definite articles here; "The MPLS
Architecture", "the local label", "a given FEC".  (And it probably reads
better as "defines the term [FEC]" than putting "term" after the name of
the term.

   o  (Prefix, Routing Instance, Topology, Algorithm [RFC8402]), where a
      topology identifies a set of links with metrics. For the purpose
      of incoming label collision resolution, the same Topology
      numerical value SHOULD be used on all routers to identify the same
      set of links with metrics. [...]

Is the IGP going to help me satisfy this SHOULD or is it more of a
pie-in-the-sky sort of thing?

Section 2.5.1

   This document defines the default tie breaking rules that SHOULD be
   implemented. An implementation MAY choose to support different tie-
   breaking rules and MAY use one of the these instead of the default
   tie-breaking rules. All routers in a routing domain SHOULD use the
   same tie-breaking rules to maximize forwarding consistency.

I didn't think through this hard enough to come up with a specific
scenario that would fail, but it seems like there could be bad failure
modes when forwarding consistency is not maintained.  That would perhaps
suggest a "MUST" requirement to use the same rules, and perhaps even
announcement of an identifier for what rules are in use, so that peers
can detect an inconsistency.

   The default FEC administrative distance order starting from the
   lowest value SHOULD be

I think it would be nice if we could get this to be an "is" rather than
a "SHOULD be", especially since at present we offer no guidance on
actually constructing the required 8-bit numerical values.

   The numerical sort across FECs SHOULD be performed as follows:

It seems like the first two top-level bullets here are not necessarily
part of the procedure itself, but rather some ancillary information
about how to compute values used as part of the procedure.  I don't know
if, editorially speaking, the presentation could be improved by
reframing how these are discussed.

       o All prefixes are represented by (128 + 8) bits.

            . A prefix is encoded in the most significant bits and the
               remaining bits are set to zero.

            . The prefix length is encoded before the prefix in a field
               of size 8 bits.

This description seems needlessly confusing.  Couldn't we write it as
(8+128) bits, and put the sub-bullet for the prefix length before the
other sub-bullet, so that they appear in the order the bits are encoded?

   o  Encode the remaining set of FECs as follows

       o Prefix, Routing Instance, Topology, Algorithm: (Prefix Length,
         Prefix, routing_instance_id, Topology, SR Algorithm,)

       o (next-hop, outgoing interface): (next-hop,
         outgoing_interface_id)

      o (number of adjacencies, list of next-hops in ascending
         numerical order, list of outgoing interface IDs in ascending
         numerical order). This encoding is used to encode a parallel
         adjacency [RFC8402]

       o (Endpoint, Color): (Endpoint_address, Color_id)

       o (IP address): This is the encoding for a mirror SID FEC. The IP
        address is encoded as described above in this section

I think this needs to say a little bit more about what is being
presented.  The part before the colon is what we're using to label a
category of FECs, and the part after the colon is how it is encoded?
There might be a more formal description language to describe the
encoding rules used, and also the (number of adjacencies, list of
next-hops) bullet point doesn't have a colon.

We also don't specify that big-endian (network byte order) is used.

   o  Select the FEC with the smallest numerical value

If I understand correctly, we are encoding these FECs to byte strings,
but different types of FEC get encoded as different length byte strings.
How do we then interpret these byte strings as numerical values?

Section 2.6

                                                    However to minimize
   the chance of misforwarding, a FEC that loses its incoming label to
   the tie-breaking rules specified in Section 2.5 MUST NOT be
   installed in FIB with an outgoing segment routing label based on the
   SID corresponding to the lost incoming label.

It's not entirely clear to me how actionable this requirement is.
That is, is the entity instaslling the FIB entry always going to know
that the outgoing label was "based on" the incoming (non-)label?

Section 2.7.1

Setting TTL and TC improperly can have security considerations.
This document does not discuss those, nor does RFC 8402 (the only
reference listed from this document's security considerations).

Section 4

"OAM" is not listed as "well-known" at
https://www.rfc-editor.org/materials/abbrev.expansion.txt and would
typically qualify for expansion on first usage.

Section 5

[see also comment on Section 2.7.1]

Should we mention that different routers can get different results from
the tie-breaking rules in case of skew in IGP convergence?

Appendix A.1

   The packet arrives at router R2. Because the top label 1008
   corresponds to the IGP SID "8", which is the prefix-SID attached to
   the prefix 192.0.2.8/32 owned by the node R8, then the instruction
   associated with the SID is "forward the packet using all ECMP/UCMP
   interfaces and all ECMP/UCMP next-hop(s) along the shortest/useable
   path(s) towards R8". Because R2 is not the penultimate hop, R2
   applies the CONTINUE operation to the packet and sends it to R3 using
   one of the two links connected to R3 with top label 1008 as specified
   in Section 2.10.1.

"one of the two links" seems inconsistent with the claimed "using all
ECMP/UCMP interfaces and all ECMP/UCMP next-hop(s)".

                                                            Because R3
   is the penultimate hop, we assume that R3 performs penumtimate hop
   popping, which corresponds to the NEXT operation, then sends the
   packet to R8. [...]

This chain of causality doesn't follow.  We assume that R3 performs PHP
-- the fact that in this flow R3 is the penultimate hope does not factor
into that assumption.

Appendix A.2.5

   Since both FECs are from the same MCC, they have the same default
   admin distance. So we compare FEC type code-point. FEC1 has FEC type
   code-point=120, while FEC2 has FEC type code-point=130. Therefore,
   FEC1 wins.

nit: It feels a little strange to call these code-points when there's no
registry and they're locally assigned per site policy.

Appendix A.2.6

   FEC1 and FEC2 both use dynamic SID assignment. Since both FECs are
   from the same MCC, they have the same default admin distance. So we
   compare FEC type code-point. Both FECs have FEC type code-point=120.
   So we compare address family. Since IPv4 is preferred over IPv6, FEC1
   wins.

side note: It's a little surprising that "IPv4 is preferred over IPv6"
did not get any objections at IETF LC.  (Example 13 has the same
property.)

Noting Mirja's comment asking why is this not Informational, I agree with the current track "PS" as it defines
(using RFC2119 keywords) SR-MPLS procedures for an MPLS dataplane.

Section 2
(my previous Discuss point)
- 1.    The text in Section 1 states “An implementation SHOULD
check that an IGP node-SID is not associated with a prefix that is owned by more than one router
within the same routing domain. If so, it SHOULD NOT use this Node-SID, MAY use another one if
available, and SHOULD log an error”.

Sasha suggested MAY/s/SHOULD or MUST,  saying MUST aligns with Section 3.2/RFC8402, which uses
the wording "MUST NOT" be used by another router.

While the document was changed to "SHOULD",  my point was that I agreed with Sasha on this,
to align with RFC8402, it needs to be a "MUST".

Though reading later in RFC8402's Section 9 Manageability Considerations, I see it uses a "SHOULD".
So I'll defer to the authors/working group on their preference.

On my previous Discuss, I asked how does an implementation "check"?
In RFC8402's Manageability Considerations, it says "In addition to the allocation policy/tooling that the
operator will have in place, an implementation SHOULD protect the network in case of conflict detection
by providing a deterministic resolution approach." So while I prefer RFC8402's more explicit operational
guidance vs. "check", I'll defer to the authors. My concern is not so much for MPLS operators, this is nothing
new, but to say something more accurate than "check" in an RFC.

Nit (overall)
I was surprised/disappointed there was no alignment on terminology with RFC8402. For example, RFC8402
defines terms for SR MPLS, e.g. SR-MPLS, but this document doesn't use any of RFC8402's defined MPLS terms.

Suggestion at minimum a fix for the Abstract:
Segment Routing (SR) leverages the source routing paradigm.
/s/
Segment Routing (SR) leverages the source routing paradigm as defined in [RFC8402].
And:
This document specifies the forwarding behavior to allow instantiating SR over the
MPLS dataplane.
/s/
This document specifies the forwarding behavior to allow instantiating SR over the
MPLS dataplane (SR-MPLS).

Nit: Section 2
I had difficulty parsing the first bullet:
From a control plane perspective, [RFC3031] does not mandate a single signaling protocol.  Segment Routing makes
use of various control plane protocols such as link state IGPs [I-D.ietf-isis-segment-routing-extensions],
[I-D.ietf-ospf-segment-routing-extensions] and [I-D.ietf-ospf-ospfv3-segment-routing-extensions]. The flooding
mechanisms of link state IGPs fits very well with label stacking on ingress. Future control layer protocol and/or
policy/configuration can be used to specify the label stack.
/suggest/
From a control plane perspective, [RFC3031] does not mandate a single control protocol or use of a control protocol. 
Segment Routing makes use of various control plane protocols such as link state IGPs [I-D.ietf-isis-segment-routing-extensions],
[I-D.ietf-ospf-segment-routing-extensions] and [I-D.ietf-ospf-ospfv3-segment-routing-extensions].
Future control layer protocols are not precluded and/or management policy/configuration can be used to specify the label stack.

Given this document does not introduce any change in the MPLS data plane, I think it could also be published as informational.

* Section 2

"including for TI-LFA".

Add a reference to draft-bashandy-rtgwg-segment-routing-ti-lfa at first use?

* Section 2.4

Calling "temp" something more descriptive would have been helpful (say base_index).

* Section 2.5.1.

"Address Family represented by 8 bits, where IPv4 encoded as 100 and IPv6 is encoded as 110."

Suggest rewording to say IPv4 is represented by the value 4 and IPv6 is represented by the value 6 (unless you actually meant to use the decimal values 100 and 110, in which case ignore this comment).