[mpls] AD review of draft-ietf-mpls-ldp-p2mp

[Adrian Farrel <[e-mail]>]

Hi,

I have performed an AD review of your draft.

Don't panic!

I review all drafts that I am responsible for before putting them forward for
IETF last call. The main objective is to catch nits and minor issues that would
show up during the last call or in IESG review. The intention is to help polish
your document and make sure it is clean and shiny so that other reviewers will
stick to the technical details.

Most of my comments are pretty trivial, but there are quite a few of them and a
couple need a bit of discussion. I think this merits a quick respin of the
document before I issue the IETF last call. As soon as I see a new revision
posted, I'll set the ball in motion.

Of course, all of my issues are up for discussion.

Thanks for the work,
Adrian

---

As noted in the write-up, RFC 4664 is an unused reference. I think it can simply
be removed unless it was intended to be added to the final paragraph of Section
1.

---

Abstract

Some consistency in hyphenation, please. I think you need to adopt
"point-to-multipoint" and "multipoint-to-multipoint" as in the title.

---

Acronyms
Need to expand them on first use in the main text (Abstract doesn't  count). I
find:

(LDP is a permitted acronym - no action needed)
LSP
LSR

---

Please s/draft/document/ in the text so that when it is published as an RFC the
text is accurate.

---

Section 1

   Only a single copy of the packet will be sent
   on any link traversed by the MP LSP (see note at end of
   Section 2.4.1).

Is this strictly true in all cases. are there not fringe cases, just as there
were in P2MP RSVP-TE, where a branch is logically made upstream of a common link
because of limited branching capabilities at the downstream end of the link?

---

Section 1.2

   Leaf node:  A Leaf node can be either an Egress or Bud LSR when
      referred in the context of a P2MP LSP.  In the context of a MP2MP
      LSP, an LSR is both Ingress and Egress for the same MP2MP LSP and
      can also be a Bud LSR.

I think...
s/an LSR is both Ingress and Egress/a leaf is both Ingress and Egress/

---

Section 2.1

The figure in this section appears to state that the S-bit must always be set to
1. That means that the capability cannot be withdrawn. I have no issue with
that, but I think that if this is you intention, you must say that the S-bit
must always be set to 1, the capability cannot be withdrawn, and the processing
if S=0 is received. OTOH, if you allow withdrawal, you can leave "S" in the
figure and refer to RFC5561 for the meaning.

---

Pedantic point I don't propose you address (unless you have more spare than you
should have)...

The Opaque Value field is not truly opaque, since we can look into it and find a
series of opaque elements.

---

Section 2.2

Can you explain to me why the working group wanted to allow the P2MP FEC to use
any address family from IANA's "Address Family Numbers" registry to identify the
root LSR?

---

Section 2.2

   The combination of (Root Node Address,
   Opaque Value) uniquely identifies a P2MP LSP within the MPLS network.

With the current specification as shown in the figure, this is not completely
accurate since two addresses from different families could have the same root
node address value.

So your text should refer to the combination of {Root Node Address type, Root
Node Address, Opaque Value).

---

2.3

OLD
   Type:  The Type of the LDP MP Opaque Value Element basic type is to
      be assigned by IANA.
NEW
   Type:  The Type of the LDP MP Opaque Value Element. IANA maintains a
      registry of basic types (see Section 11).

---
                        
2.3

We don't generally define empty TLVs and objects.

Can you convince me of the need for the Extended Type? Are you predicting a
large number of FCFS opaque values?. You are surely not expecting many standards
track types. 

---

2.3

OLD
   Extended Type:  The Extended Type of the LDP MP Opaque Value Element
      extended type is to be assigned by IANA.
NEW
   Extended Type:  The Extended Type of the LDP MP Opaque Value Element.
      IANA maintains a registry of extended types (see Section 11).

---

A point of pedantry, but you repeatedly refer to the "Label Map" message and (of
course :-) it is really the "Label Mapping" message.

---

2.4

   2.  P2MP Label Map <X, Y, L>: a Label Map message with a FEC TLV with
       a single P2MP FEC Element <X, Y> and Label TLV with label L.

       Label L MUST be allocated from the per-platform label space (see
       [RFC3031] section 3.14) of the LSR sending the Label Map Message.

While you are at liberty to make this "MUST" requirement with the WGs support,
it would be good if you gave the reason rather than "sneaking" it in. I assume
that you want to do this to make FRR easier, but it is not immediately clear why
P2MP is in any way different from P2P on upstream interfaces. Or maybe it is
because you want to allow the choice of downstream interface to rest with the
upstream LSR (per 2.4.1.2).

Can you add a short clause to say why the per-platform label space is used?

---

2.4.1

   The remainder of this section specifies the procedures for
   originating P2MP Label Map messages and for processing received P2MP

s/The remainder of this/This/

---

You have two instances (spot the block copy!) of "label map" in lower case.

---

2.4.1.1

   A node Z that
   wants to join a MP LSP <X, Y> determines the LDP peer U which is Z's
   next-hop on the best path from Z to the root node X. If there is more
   than one such LDP peer, only one of them is picked.  U is Z's
   "Upstream LSR" for <X, Y>.      

   When there are several candidate upstream LSRs, the LSR MAY select
   one upstream LSR.

The "MAY" seems to be in contradiction to the previous paragraph that appears to
say that the LSR MUST select exactly one upstream LSR.

---
                        
2.4.1.1

CRC32 is not a well-known acronym and would benefit from a reference.

---

2.4.1 and 2.4.1.1

It seems that it is important to be precise about what is meant by the "opaque
value" (Y). This is particularly important in 2.4.1.1 because there is an
interop dependency relying on the input to the hash function. You might mean:
- the whole opaque value TLV
- the opaque value field of the opaque value TLV
  (i.e., the concatenation of the whole opaque value elements)
- the value field of the opaque value element
- the concatenation of the value fields from each of the value fields
  of the opaque value elements

Please add text to clarify (at least for the CRC32 case).

---

2.4.1.4

Trivially...

   Assuming its old forwarding state was
   L'-> {<I1, L1> <I2, L2> ..., <In, Ln>}, its new forwarding state
   becomes L'-> {<I1, L1> <I2, L2> ..., <In, Ln>, <I, L>}.

Misses the case that I=Ii for some value of i.

---

2.4.2.1

   If a leaf node Z discovers (by means outside the scope of this
   document) that it has no downstream neighbors in that LSP, and that
   it has no need to be an egress LSR for that LSP, then it SHOULD send

Just some subtle re-wording needed. We *do* know the means by which Z discovers
it has no downstream neighbors -- that is what this document is about! The
parentheses apply to the second clause (that it has no need to be an egress).

---

2.4.2.2.

In the light of the procedure set out in 2.4.3 and 8., should you recommend that
propagation of Label Withdraw should be delayed to prevent full LSP teardown
when there is a small change in the path of the trunk of an LSP? Maybe a fringe
case we can leave to implementations?

---

Section 3

Many of the nits in section 2 apply here, too.

---

Section 3

While it is possible that the root of an MP2MP LSP is also a leaf and can act as
a data source, I think that...

   An MP2MP LSP is much like a P2MP LSP in that it consists of a single
   root node, zero or more transit nodes and one or more leaf LSRs
   acting equally as Ingress or Egress LSR. 

Should read "two or more leaf LSRs" since the semantic change from P2MP is that
leaf nodes are ingresses as well as egresses, and it would make no sense to have
only one.

---

Section 3

I can't help feeling that some figures would help understanding the path that
packets are expected to take on an MP2MP LSP since this is key to working out
what label state is installed.

----

5.1

OLD
   Type:  The type of the LDP MP Status Value Element is to be assigned
      by IANA.
NEW
   Type:  The type of the LDP MP Status Value Element. IANA maintains a
      registry of status value types (see Section 11).


And in the figure s/Type(TBD)/Type/

---

5.2.  LDP Messages containing LDP MP Status messages

   The LDP MP status message may appear either in a label mapping
   message or a LDP notification message.

I think s/status message/status TLV/  throughout.

---

5.2.1

   An LDP MP status TLV sent in a notification message must be
   accompanied with a Status TLV.

This is a statement of fact not a piece of new protocol spec, so you are correct
to use lower case "must". But you should include a reference at this point.

---

7.1.  Root node redundancy - procedures for P2MP LSPs

   Since all leafs have set up P2MP LSPs to all the roots, they are
   prepared to receive packets on either one of these LSPs.  However,
   only one of the roots should be forwarding traffic at any given time,
   for the following reasons: 1) to achieve bandwidth savings in the
   network and 2) to ensure that the receiving leafs don't receive
   duplicate packets (since one cannot assume that the receiving leafs     
   are able to discard duplicates).  How the roots determine which one
   is the active sender is outside the scope of this document.

The "should" seems strong since I know of deployed networks where 1+1 style P2MP
transmission is performed and the application layer is responsible for sorting
out duplicates that arise on failover. 1+1 is chosen in the face of network cost
because of the high level of reliability that is wanted.

---

8.3

Same issue with the S-bit apparently always set meaning the capability cannot be
withdrawn.

---

Section 9

This is not clear. The figure shows "Type=mLDP". There are two FEC Elements
defined in the document so I think you need...

OLD

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Typed Wcard   | Type = mLDP   |   Len = 2     |      AFI      ~
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      ~               |
      +-+-+-+-+-+-+-+-+

   Type Wcard:  As specified in [RFC5918]


   Type:  mLDP FEC Element Type as documented in this draft.

NEW

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Typed Wcard   |     Type      |   Len = 2     |      AFI      ~
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      ~               |
      +-+-+-+-+-+-+-+-+

   Type Wcard:  As specified in [RFC5918]


   Type:  The type of FEC Element Type. Either the P2MP FEC Element 
        or the MP2MP FEC Element using the values defined for those
        FEC Elements when carried in the FEC TLV as defined in this
        document

---

Section 10

I am a bit disappointed by the Security Section. While it is true that the
security relationships and techniques between LDP peers are not changed, and the
imperatives are the same, you have introduced a new type of service that has a
new attack vector.

In P2P LSPs, the sender has some control over the receiver, but this is less the
case in leaf-initiated join to P2MP LSPs. I think this section should at least
note that there is no way for a root to validate the leafs of the P2MP tree.
Probably the only security you can offer is that;
- the leafs all form part of the same trusted network
- the opaque values could be made unguessably large
- the opaque values could be distributed in a secure way

---

Section 11

Can you please add...

   The requested code point values listed below have been allocated by
   IANA through early allocation.

...between...

      The allocation policy for this space is 'Standards Action with
      Early Allocation'

...and...

   This document requires allocation of three new code points from the
   IANA managed LDP registry "Forwarding Equivalence Class (FEC) Type
   Name Space".  The values are: