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

From: Alissa Cooper <[email-masked]>
Date: Wed, 9 May 2018 16:04:01 -0400

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 <[email-masked]> 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
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art

---

From: "Rabadan, Jorge (Nokia - US/Mountain View)" <[email-masked]>
Date: Fri, 18 May 2018 19:24:13 +0000

Elwyn,

Thank you very much for your thorough review.
We addressed all your comments.. see below for more details, with [JORGE].

Thx
Jorge

-----Original Message-----
From: Elwyn Davies <[email-masked]>
Date: Saturday, April 14, 2018 at 12:59 PM
To: "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Genart last call review of draft-ietf-bess-evpn-prefix-advertisement-10
Resent-From: <[email-masked]>
Resent-To: <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, <[email-masked]>, Zhaohui Zhang <[email-masked]>, <[email-masked]>
Resent-Date: Saturday, April 14, 2018 at 12:59 PM

    Reviewer: Elwyn Davies

    

    
    Major issues:
    
    None
    
    Minor issues:
[JORGE] OK, I shifted the intro and terminology sections, and expanded the introduction as you suggested.
    
[JORGE] ok, removed the conclusions section and took the content to the introduction.
    
    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.
[JORGE] Added this in the terminology section:
"This document also assumes familiarity with the terminology of
 [RFC7432], [RFC8365] and [RFC7365]."

    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.
[JORGE] Done. I also added:
   "VNI: Virtual Network Identifier. As in [RFC8365], the term is used as
      a representation of a 24-bit NVO instance identifier, with the
      understanding that VNI will refer to a VSID in NVGRE, or VNI in
      VXLAN, etc. unless it is stated otherwise."

[JORGE] Good point, but, actually since this document is based on the work done in [RFC8365] I think we should be consistent with it. And [RFC8365] uses NVO, so I replaced NVO3 by NVO and added NVO to the terminology.

    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
[JORGE] ok, I took most of them. I'm not convinced we need to add the alignment.. I haven't seen that in any related spec, and definitively not on the RFCs this document takes as normative references.
    
    Nits/editorial comments:
[JORGE] done.
    
[JORGE] done.
    
[JORGE] done.

[JORGE] done.
    
[JORGE] done.
    
    s1: Mention that many of these terms are fully defined in RFC 7365.
[JORGE] added this: "This document also assumes familiarity with the terminology of [RFC7432], [RFC8365] and [RFC7365]."
    
[JORGE] done.

[JORGE] done.
    
[JORGE] They are defined as in RFC7432 and RFC4364 respectively. However, to avoid confusion, I also added that they the instantiation of an EVI or Layer-3 VPN in an NVE/PE.
    
[JORGE] done.
    
    s1: The terms VXLAN, nvGRE and VTEP need to be defined. Also Ethernet A-D route
    (see first comment on s3 below).
[JORGE] done. A-D route should be taken care by the sentence suggesting familiarity with terms in RFC7432.

[JORGE] yes, this paragraph is already modified based on the feedback of another reviewer along the same lines. 
    
[JORGE] done.
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] done.
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] done
    
[JORGE] done
    
[JORGE] done.
    
    s3, after Figure 2:
    OLD:
    
    + 5 - IP Prefix Route
    NEW:
    
    Value: 5
[JORGE] done.
    
    s3.1, last para: s/Router's/router's/
[JORGE] the extended community is called like that, so I left it that way.
    
[JORGE] added: "certain data combinations would imply a
    to Table 1 and elsewhere."
    
[JORGE] done.
    
[JORGE] done.
    
[JORGE] done.
    
[JORGE] done.
    
[JORGE] ok, done
    
[JORGE] fixed
    
[JORGE] fixed
    
    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. :-)
[JORGE] I changed the wording and added reference to RFC7432.
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed

    s4.4.3, bullet (c):
    OLD:
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed
    
[JORGE] fixed