Ethernet-Tree (E-Tree) Support in Ethernet VPN (EVPN) and Provider Backbone Bridging EVPN (PBB-EVPN)
draft-ietf-bess-evpn-etree-14

[ I wrote this review back on Aug 8th, but the balloting wasn't open then - I believe that there has been a new revision since, and so some of these may already have been addressed. ] 


I'm balloting No Objection, but I do have significant reservations; 
because of the number of nits the document is quite hard to read - a number of these required re-reading the sentence multiple times, guessing at what the sentence is trying to says, etc. Many of these are really obvious (e.g see nit #14 below) and it makes me concerned that the document hasn't been sufficiently reviewed. 

In addition, the RFC Editor would have caught these, but it doesn't seem reasonable to rely on them to make documents readable, nor to waste multiple reviewers time addressing what could easily have been caught with a simple read though. 


Questions / Notes:
1: The document has 6 authors (one listed as an author) and a contributors section -- is there a substantive difference between those above the fold and those below it?

2: This document uses a number of acronyms without expanding them on first use.

3: Section 8.1 Considerations for PMSI Tunnel Types
"The status of 0x7F may only be
   changed through Standards Action [RFC5226]." - what makes 0x7F special? What is it reserved for?

4: The shepherd writeup says:  
Q: (16) Will publication of this document change the status of any
existing RFCs? 
A: No change of the status of existing RFCs.

I believe that this is incorrect, especially when compared with the Q/A #17.

5: The IANA considerations section seems to be incorrect / transition isn't really described.
The current registry says:
0xFB-0xFE Reserved for Experimental Use

This document changes that to be:
0x7B-0x7E      Reserved for Experimental Use 

While it "only" a change to an experimental range, by their very nature you don't know if anyone is using the current range. I think that the document needs to be much more clear about the fact that it is changing this, and also what the implications are for people who are already using e.g: 0xFB - from what I can see, it could break existing deployments.

Nits:

1: Section 2.1 Scenario 1: Leaf OR Root site(s) per PE
O: In such scenario, using tailored BGP Route Target (RT) import/export
   policies among the PEs belonging to the same EVI, can be used to
   restrict the communications among Leaf PEs.
P: In such scenario, tailored BGP Route Target (RT) import/export
   policies among the PEs belonging to the same EVI can be used to
   restrict the communications among Leaf PEs.
C: Redundant 'using' when coupled with 'can be used'

2: Section  2.2 Scenario 2: Leaf OR Root site(s) per AC
O: Approach (A) would require the same data plane enhancements as
   approach (B) if MAC-VRF and bridge tables used per VLAN, are to
   remain consistent with [RFC7432] (section 6).
C: This doesn't really parse -- I cannot tell if it is just an extraneous comma (after VLAN) or if it is that subject for "used per VLAN" is unclear.

3:
O: Given that both approaches (A) and (B) would require exact same data-
   plane enhancements, 
P: Given that both approaches (A) and (B) would require the same data-
   plane enhancements, 
C: grammar

4: 
O: It should be noted that if one wants to use RT constrain
   in order to avoid MAC advertisements 
P: It should be noted that if one wants to use RT constraints
   in order to avoid MAC advertisements 
C: Assuming "constraints"; if not, needs more rewording.

5: 
O: For this scenario, if for a given EVI, significant number of PEs have
   both Leaf and Root sites attached, even though they may start as
   Root-only or Leaf-only PEs, then a single RT per EVI should be used.
P: If, for a given EVI, a significant number of PEs have both Leaf and Root sites attached (even though they may start as Root-only or Leaf-only PEs), then a single RT per EVI should be used.
C: Probably cleaner would just be to drop the "For this scenario"; I don't think that the reader would take this a generalization, but your views may differ.

6: 2.3 Scenario 3: Leaf OR Root site(s) per MAC
I think that this may be better titled as "2.3 Scenario 3: Leaf OR Root site(s) per MAC address" -- without the 'address' it wasn't clear for the first bit if you actually intended MAC or if this was a typo for AC. Purely a nit.

7: 
O: This scenario is not covered in both [RFC7387] and [MEF6.1]; 
P: This scenario is not covered in either [RFC7387] or [MEF6.1]; 
C: At least I'm assuming you meant either!

8: Section 3.1 Known Unicast Traffic
O: Since in EVPN, MAC learning is performed in control plane via
P: Since in EVPN, MAC learning is performed in the control plane via
C: Or perhaps "by the control plane"

9: 
O:  thus providing very efficient filtering and avoiding sending known unicast
   traffic over MPLS/IP core to be filtered
P:  thus providing very efficient filtering and avoiding sending known unicast
   traffic over the MPLS/IP core to be filtered

10:
O: This new Extended Community MUST be advertised with MAC/IP
   Advertisement route.
P: This new Extended Community MUST be advertised with MAC/IP
   Advertisement routes.
C: s/route/routes/ (or similar)

11: Section 3.2 BUM Traffic
O: This specification does not provide support for filtering BUM
   (Broadcast, Unknown, and Multicast) traffic on the ingress PE because
   it is not possible to perform filtering of BUM traffic on the ingress
   PE, as is the case with known unicast described above, due to the
   multi-destination nature of BUM traffic.
P: This specification does not provide support for filtering BUM (Broadcast, Unknown, and Multicast) traffic on the ingress PE; due to the multi-destination nature of BUM traffic, is is not possible to perform filtering of the same on the ingress PE.
C: Parenthesis make it a bit easier to read, but the whole sentence should be rewritten; "This specification doesn't do X because it is not possible to do X due to Y" sounds odd, even more so being a run on sentence.

12:
O: Other mechanisms for identifying root or leaf (e.g., on a per MAC address basis) is beyond the scope of this document.
P: Other mechanisms for identifying root or leaf (e.g., on a per MAC address basis) are beyond the scope of this document.
C: Plural alignment.

13: Section 3.2.1 BUM traffic originated from a single-homed site on a leaf AC
O: along with an Ethernet A-D per ES route
C: A-D is used without expansion. I'm assuming Auto-Discovery, but this is not helpful to readers.

14: 3.2.3 BUM traffic originated from a multi-homed site on a leaf AC
O: In such scenarios,  If a multicast
P: In such scenarios, if a multicast
C: Things like this (there are multiple) make the reader wonder how well this was reviewed. This has been in the document since -03 (Oct 2015), so it isn't simply a "rush to squeeze it in" event.

15: Section 3.3.1 E-Tree with MAC Learning
O: For the scenario described in section 2.1 (or possibly section 2.2), these routes are imported ...
P: For the scenario described in section 2.1 (and possibly the scenario in section 2.2), these routes are imported ...
C: The original sounds a lot like you are not clear which section you are referring to.

16:
O: To support multicast/broadcast from Leaf to Root sites, ingress
   replication should be sufficient for most scenarios where there are
   only a few Roots (typically two). Therefore, in a typical scenario, a
   root PE needs to support both ...
C: Throughout the document you are using a mix of 'Root' vs 'root', 'Leaf' vs 'leaf' -- there doesn't seem to be much consistency.

17: Section 4 Operation for PBB-EVPN
O: In PBB-EVPN, the PE advertises a Root/Leaf indication along with each
   B-MAC Advertisement route, to indicate whether the associated B-MAC
   address corresponds to a Root or a Leaf site.
C: Please fix the commas - I think you just need to delete the second (or perhaps put the "along with each" bit in parens) - the current text is confusing.

18: 
O: In such multi-homing scenarios where an Ethernet Segment has
   both Root and Leaf ACs, ...
P: In multi-homing scenarios where an Ethernet Segment has
   both Root and Leaf ACs, ...

19: 
O: it is assumed that While different ACs (VLANs) on the same ES
C: See #14. 

20: Section 4.1 Known Unicast Traffic
O: On the ingress PE, the C-MAC destination address lookup yields, ...
C: C-MAC is used without expansion - please insert reference (probably to RFC 7623)

21: Section 4.3 E-Tree without MAC Learning
O: In scenarios where the traffic of interest is only Multicast and/or
   broadcast, the ...
P: In scenarios where the traffic of interest is only multicast and/or
   broadcast, the ...
C: Please choose a single capitalization for Broadcast and Multicast and stick to it -- there are around 5 Multicast and 6 multicast.

22: Section 5.2 PMSI Tunnel Attribute
O: This document uses all other remaining fields per
   existing definition. 
   P: This document does not redefine any other terms.
C: This still ready poorly -- I think according to existing definitions (also, plural matching).

23:
O: When receiver ingress-replication label is needed, the high-order bit
   of the tunnel type field (Composite Tunnel bit) is
C: I think "When a receiver ingress-replication label is needed" or "When receiver ingress-replication labels are needed" - whatever the case, I think you are missing a word or using the wrong one.

24: 
O: When this Composite Tunnel bit is set, the "tunnel identifier" field
   would begin with a three-octet label,
P: When this Composite Tunnel bit is set, the "tunnel identifier" field
   begins with a three-octet label,

25: 
O: PEs that don’t understand the
   new meaning of the high-order bit would treat the tunnel type
C: As before, delete "would"

26: Section 7  Security Considerations
O: Furthermore, this document provides additional
   security check by allowing sites
P: Furthermore, this document provides an additional
   security check by allowing sites
C: Or "checks"...

Section 3.3.2 says:

   The PEs implementing an E-Tree service need not perform MAC learning
   when the traffic flows between Root and Leaf sites are mainly
   multicast or broadcast.

Does this mean to say "mainly"? I would have expected "only", as in section 4.3.  In particular, if "mainly" is correct, I'm unsure how unicast traffic is supposed to be handled. Is it simply flooded out (modulo filters) in the same way as broadcast traffic? If that's the intention, I think some additional text here saying as much would be useful.

----

Section 5.1:

   The reserved bits SHOULD be set to zero by the transmitter and SHOULD
   be ignored by the receiver.

The "SHOULD" here seems that it might make assigning meaning to these bits in the future problematic. If implementations decide to either assign local meaning to these bits, or decide that they don't need to be initialized, then future IETF specs that try to use them might be in for some pretty nasty deployment surprises. If these need to be "SHOULD" instead of "MUST," please add some motivating text to the document for the sake of people who might want to extend the protocol in the future.

----

The IANA handling of "Composite Tunnel" seems problematic: although several values in this "Reserved for Composite Tunnel" range have well-defined values (e.g., 0x81 means "RSVP-TE P2MP LSP with composite tunnel"), they look unallocated/reserved in the resulting table. I think what you really want to do here is update the introductory text for the table to make it clear that values now take the range 0x00 - 0x7F and modify 0x7B through 0x7F as you've proposed doing.

On top of this, I have the same concerns as Warren does regarding the impact of this change on in-the-field use of experimental tunnel types. I think the only reasonable way to retrofit this mechanism onto the existing system would be to to say that the "Composite Tunnel" bit MUST be ignored for tunnel types 0x7B-0x7E, and possibly allocate some additional experimental codepoints (e.g., 0x77-0x7A) so that people can run experiments with tunnel types that also include composite tunnel behavior.

Nit: typo Unkonwn appears multiple times in the document.

I share the questions about the prohibition of leaf-to-leaf communication. There's a fair amount of text enforcing such a requirement, but very little if any about why.

MEF doesn't stand for Metro Ethernet Forum any longer.

4.2 Broadcast, Unkonwn, and Multicast (BUM) Traffic
Nit: unknown

I'm reading the leaf to leaf prohibition as a routing optimization that isn't intended for security purposes, but could help prevent routing loops and leakage.  There is a little text early on in the document to describe the purpose, but I agree the text should be made a bit more clear.

Mostly editorial comment:
Reading sections 3 and 4 it not clear to me if this is providing an actual implementation specification or just reasoning. If the former is indented I would recommend to use more active and normative language. Also I would recommend to have section 5 before 3 and 4 because they use these new flags/fields.

Please also consider the comments provided by the new gen-art review (thanks Dale!) which probably go in the same direction and suggest some good concrete improvements.