Re: [Gen-art] Genart last call review of draft-ietf-bess-evpn-prefix-advertisement-10

[Alissa Cooper <[e-mail]>]

Elwyn, thanks for your review. I entered a No Objection ballot and pointed to your review there.

Alissa

> On Apr 14, 2018, at 6:59 AM, Elwyn Davies <[e-mail]> wrote:
> 
> Reviewer: Elwyn Davies
> Review result: Almost Ready
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-bess-evpn-prefix-advertisement-10
> Reviewer: Elwyn Davies
> Review Date: 2018-04-14
> IETF LC End Date: 2018-04-10
> IESG Telechat date: 2018-05-10
> 
> Summary:  Almost ready.  My main concerns are the lack of a good introduction
> and a rather weak definition of the format of the new EVPN route option
> (looking back on RFC 7342, I think this could be said about that document
> also!).  This is very technical material and  a good introduction would help
> readers who are not  already deeply into the Data Center area to understand
> what is going on.  Also this document has some remaining vestiges of being
> written like an academc paper (some were fixed in the previous revision),
> particularly the spurious notion of having 'conclusions' (material actually
> deserves to be in the Intro)
> 
> Major issues:
> 
> None
> 
> Minor issues:
> Lack of a proper introduction: An introduction should precede the terminology
> section and needs to be somewhat clearer about the context (presumably
> multi-tenant data centers). A reference to RFC 7365 which describes the data
> center model in which the EVPN mechanism is expected to be employed would be
> very useful. A somewhat expanded version of the text in s2 would be a good
> basis for the introduction. The ss2.1 and 2.2 with a short new header would
> become a new section (3) which is the Problem Statement.
> 
> s5: Associated with my previous comment... An RFC is not a scientific paper and
> does not have 'conclusions'.  On reading s5, it strikes me that this text would
> actually make quite a nice part of the introduction, probably interpolated
> after the first paragraph of the current s2.
> 
> Terminology import: The current s1 contains a number of terms that are actually
> defined in RFC 7365 (Data Center, Tenant System, Network Virtualization Edge,
> etc). Pointing to RFC 7365 s1.1 would be helpful.
> 
> s1, VNI: There is some difference between the usage of VNI in RFC 8365 (Section
> 5.1), in RFC 7365 (s3.1.2) where it means Virtual Network Instance and in this
> draft (... Identifier). This is potentially confusing to the naive reader and
> definitely confusing with the usage of VNI in item (4) of s4.1 where the VNI is
> the VXLAN Network Identifier. It would be better if VNI meant the same thing in
> all this closely related work. Please review all instances of VNI in the draft
> to make sure they are talking about the same thing.
> 
> s2.1:
>>   [RFC7432] is used as the control plane for a Network Virtualization
>>   Overlay (NVO3) solution in Data Centers (DC),
> The acronym NVO3 is used here as opposed to NVO which is used elsewhere in the
> document. NVO3 is used in RFC 7365 to refer to an overlay with an L3 underlay
> network. It is not quite clear to me whether this is a typo with EVPN NOT being
> an NVO3 example or whether the sentence really ought to refer to RFC 7365 and
> explicitly say "Network Virtualization Overlay over Layer 3 tunnels". In any
> case you can't use an RFC as a control plane - they don't come with knobs on
> ;-) ! Suggest: NEW: [RFC7432] describes how a BGP MPLS-based Ethernet VPN
> (EVPN) can provide the control plane for a Network Virtualization Overlay [over
> Layer 3 Tunnels] (NVO[3]) solution in Data Centers (DC), ENDS If this is a real
> NVO3 then probably the NVO3 acronym should be used in the rest of the draft in
> place of NVO.
> 
> s3.1: The encoding of the IP Prefix Route encoding is both underspecified and
> to some extent confusing. [I note that this is, in part, inherited from RFC
> 7342, where I consider Section 7 to be grossly underspecified.] Specifically: -
> Figure 3: Expand RD on first use. - 1st bullet after Fig 3: The usage of RD
> appears to be defined on a per route type basis in RFC 7342. Accordingly I
> don't think it is sufficient to refer to how it is done in RFC 7342. - 2nd
> bullet after Fig 3: s/byte/octet/ for consistency - 3rd bullet: Encoding not
> specified (presumably unsigned integer) - 4th bullet: The alignment of the
> prefix bits in the field is not specified (presumably left aligned). The second
> sentence about the size of the field is unnecessary and confusing if the total
> length specification is given clearly. - 6th bullet: The alighment/encoding of
> the field is not specified (high order doesn't have any meaning without this).
> - 7th bullet: It would be better to have the length specification as the first
> bullet - this then clarifies the length possibilities of the IP Prefix and GW
> IP address fields. Given that the field lengths are given in octets it would be
> clearer to specify the total length of the BGP EVPN NLRI variable portion in
> octets (and to repeat in s3 as in RFC 7342 that the length is the length of the
> varible portion in octets). Thus the length specification would become: NEW: o
> The Length field of the BGP EVPN NLRI for an EVPN IP Prefix route MUST be
> either 34 (if IPv4 addresses are carried) or 58 (if IPv6 addresses are
> carried). The IP Prefix and Gateway IP Address MUST be from the same IP address
> family
> 
> Nits/editorial comments:
> Global: s/i.e. /i.e., /g, s/e.g. /e.g., /g
> 
> Global: Section cross references: s/section/Section/
> 
> Global: s/route-target/Route Target/ (c.f. definition in RFC 4364 - it might be
> useful to reference RFC 4364 in the Terminology section where Route Target is
> mentioned.)
> 
> Abstract: s/EVPN/The BGP MPLS-based Ethernet VPN (EVPN - RFC 7432) mechanism/
> 
> Abstract: Expand NVO on first use and point to RFC 7365.
> 
> s1: Mention that many of these terms are fully defined in RFC 7365.
> 
> s1, RT-2: Add reference to RFC 7432 Section 7.
> 
> s1, RT-5: Note that this is defined in this draft and point to s3.
> 
> s1, MAC-VRF and IP-VRF: The terminology definitions define these as tables, but
> the usage mostly treats them as entities. For example in the BD terminology
> defintion; later in para 1 of s2 we have "IP-VRF tables" - which would mean a
> "route table table" if the terminology definition is taken as correct. I think
> they need to be defined as entities and a check of all usages made to ensure
> that the wording reflects this.
> 
> s1, GARP: Add an illustrative reference to RFC 5227. Arguably since there isn't
> a separate GARP protocol and there is only one usage, it might be better to
> remove this and expand the usage in s4.2.
> 
> s1: The terms VXLAN, nvGRE and VTEP need to be defined. Also Ethernet A-D route
> (see first comment on s3 below).
> 
> s2, last para: This document doesn't provide justification - it wouldn't exist
> if the new route type wasn't justified. Suggest: OLD: Once the need for a new
> EVPN route type is justified, sections 3, 4 and 5 will describe this route type
> and how it is used in some specific use cases. NEW: Then Section 3 describes
> the format of an additional option for the BGP EVPN NLRI (see [RFC7432] Section
> 7) used to carry advertisements of routes using an IP prefix and introduces the
> concept of an Overlay Index, describing how it is used with recursive routing
> resolution to control the egress path associated with a given IP prefix.
> Section 4 describes a set of use cases where the Overlay Index mechanism solves
> certain problems encountered in multi-tenant Data Center implementations.
> Section 5 summarises the distinction between the single IP address EVPN route
> type (RT-2, defined in [RFC742]) and the IP Prefix route type defined in this
> document. ENDS
> 
> s2.1. para 1: Expand TOR acronym (probably needs a terminology entry).
> 
> s2.1, para 2:
>> If the term Tenant System (TS) is used to designate a physical or
>>   virtual system
> What else would it designate (given the definition in RFC 7365)?
> 
> s2.1, 3rd bullet after Fig 1: s/depending on who the master is./depending on
> whichsystem is the master./
> 
> s2.1, para after bullets after Fig 1: Expand ESI on first appearance.
> 
> a2.1, last two paras: These contain RFC 2119 keywords (MUST/MAY respectively)
> which cannot be used in a requirements outline. Suggest replacing them with
> "need to"/"could" respectively.
> 
> s2.2, para 2: s/section 2.1/Section 2.1/
> 
> s2.2, para 3: s/E.g.:/For example,/, s/1k/1000/ (2 places).
> 
> s2.2, next to last para: Need to expand NLRI on first occurrence.
> 
> s3, para 1: s/The current/The/ (It won't change in RFC 7432 by definition of
> RFCs).
> 
> s3, after Figure 2:
>>   Where the route type field can contain one of the following specific
>>   values (refer to the IANA "EVPN Route Types" registry):
>> 
>>   + 1 - Ethernet Auto-Discovery (A-D) route
>> 
>>   + 2 - MAC/IP advertisement route
>> 
>>   + 3 - Inclusive Multicast Route
>> 
>>   + 4 - Ethernet Segment Route
> This is not future proof {and it probably won't be accurate by the time this
> draft becomes an RFC) and there is no value in repeating it here. Please delete
> it. Note that this removes the expansion of A-D, so will need to add Ethernet
> A-D route to Terminology.
> 
> s3, after Figure 2:
> OLD:
> This document defines an additional route type that IANA has added to
> the registry, and will be used for the advertisement of IP Prefixes:
> 
> + 5 - IP Prefix Route
> NEW:
> This document defines an additional route type (RT-5) in the IANA EVPN Route
> Types registry [EVPNRouteTypes], to be used for the advertisement of EVPN
> routes using IP Prefixes:
> 
> Value: 5
> Description: IP Prefix Route
> ENDS
> The reference [EVPNRouteTypes] points to https://www.iana.org/assignments/evpn
> 
> s3.1, last para: s/Router's/router's/
> 
> s3.1, extra piece: There are various constraints on the values of fields
> in the RT-5 variable data:
> - The limitations on the value of IP Prefix length and the total length of the
> data depending on the address family of IP Prefix and GW IP Address. - The
> possible values of the fields depending on the Overlay Index type as set out in
> Table 1. The behaviour of receivers if an RT-5 route that breaks these
> constraints is received needs to be defined (error behaviour). This might be a
> useful point to emphasise that certain data combinations would imply a
> withdrawal of the route rather than an advertisement as described in the notes
> to Table 1 and elsewhere.
> 
> s3.2, para 13: s/sectin 2.2/Section 2.2/
> 
> s4.1, para 2: The shorthand SN1/24 which I take it implies subnet SN1 using a
> 24 bit prefix, needs to be explained on first usage since SN1 is not
> immediately obviously an IP address. Suggest: s?subnet SN1/24?subnet SN1, which
> uses a 24 bit IP prefix (written as SN1/24 in future),?
> 
> s4.1, item (3), 1st bullet: VXLAN and VTEP need to be defined (suggested adding
> to terminology above).
> 
> s4.2, para 1: s/section 2.1 and 2.2/Sections 2.1 and 2.2/
> 
> s4.2, last para: s/GARP message/using a gratuitous ARP REPLY message as
> explained in [RFC5227]/ (and remove the GARP erminology entry as suggested
> above).
> 
> s4.3, para 5 and bullet (2): I suspect AD here should be Ethernet A-D. If not
> expand AD on first use - it isn't in terminology (unlike AC). But see previous
> notes on Ethernet A-D.
> 
> s4.3, para 6: s/in a similar way as/in a similar way to/
> 
> s4.3, item (5), bullet 1: Where can I find a definition of the 'MAC disposition
> model' and 'VNI disposition model'- Google didn't help me. :-)
> 
> s4.4, para 7 (et seq): For consistency: s/ip-lookup/IP-lookup/g and
> s/mac-lookup/MAC-lookup/ (2 places) (and s/ip and mac lookups/IP- and
> MAC-lookups/)
> 
> s4.4, para 8: Expansion of SBD is not required as it is in Terminology (and
> might be confusing).
> 
> s4.4, para 9:
>> has a IRB interfaces that
>>   connect the SBD to the IP-VRF.
> Should this be 'has IRB interafces that connect' or 'has an IRB interface that
> connects'? If the plural is meant then in the next sentence s/The IRB
> interface's/Each IRB interface's/ (I assume the IP/MAC addresses would be
> different for each IRB but can't be sure).
> 
> s4.4.1, bullets (c) and (d): s/layer-3/layer 3/ (and two further instances in
> s4.4.2 and s4.4.3).
> 
> s4.4.1, last para: s/like in/as in/
> 
> s4.4.3, bullet (c):
> OLD:
> and there is a requirement to save IP addresses on those interfaces.
> NEW:
> and there is a requirement to limit the number of IP addresses used.
> END
> 
> s4.4.3, last para: s/Interface-ful with SBD IRB model/Interface-ful with
> unnumbered SBD IRB model/. PS This paragraph does not appear to add value - I
> assume it is there to some extent for symmetry with ss4.4.1.and 4.4.2.
> 
> s6, para 3: s/any action/any actions/ or maybe, s/any action that end up/any
> action that ends up/
> 
> s7: This should be redrafted as a request for allocation rather than a report
> of a previous action. The current allocation is temporary.
> 
> s8.1: idnits reports that EVPN-OVERLAY is now RFC 8365.
> 
> 
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art