[bess] Warren Kumari's No Objection on draft-ietf-bess-evpn-etree-13: (with COMMENT)

[Warren Kumari <[e-mail]>]

Warren Kumari has entered the following ballot position for
draft-ietf-bess-evpn-etree-13: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-bess-evpn-etree/



----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

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