Hi Alvaro, Thanks for the review. Please refer to my replies to your comments marked with [Ali] inline. I have incorporated them in rev09 of the draft that has just been published. Please let me know if you have any other comments.
Regards, Ali From: BESS <bess-boun...@ietf.org> on behalf of Alvaro Retana <aretana.i...@gmail.com> Date: Monday, December 4, 2017 at 1:49 PM To: "bess-cha...@ietf.org" <bess-cha...@ietf.org> Cc: Thomas Morin <thomas.mo...@orange.com>, "bess@ietf.org" <bess@ietf.org> Subject: [bess] AD Review of draft-ietf-bess-evpn-overlay-08 Dear Authors: I just finished reading this document (finally!). I have a some comments (see below) which I think should be easy to address. I also have some bigger issues that we’ll need the Chairs’ help to solve: (1) Coordination with nv03 For the Chairs: Except for some clarification comments [1] related to the current version, I see no other traffic in the nvo3 list related to this document. Was there some other coordination that I’m missing? I am not questioning having this document in bess (that’s perfectly fine); I’m just raising the needed coordination with other WGs, from the Charter: "The working group will also coordinate with...NVO3 working group for coordination of protocols to support data center VPNs.” [Ali] The chairs have already addressed the coordination with NOV3. I just want to add that the initial versions of this draft were presented in NOV3 WG long time ago; however, because this draft is about the control plane, the discussions have been happening in the BESS WG. Also as Martin mentioned, there have been some tutorial preso and a applicability draft written for NOV3 consumption and presented over past couple of IETF meetings at NOV3 WG. What about Geneve (draft-ietf-nvo3-geneve)? The nvo3 WG is focusing on Geneve as the Standard encapsulation, but this document doesn’t even mention it. [Ali] Geneve has become a standard just recently and a few months ago Thomas mentioned to us that we should have coverage for it and we turned around quickly and published a draft for it to show how this draft with little extension can be used for Geneve encapsulation. (2) Document Status Why is this a Standards Track document? The Abstract/Introduction say that “this document describes how Ethernet VPN (EVPN) can be used as an NVO solution and explores applicability of EVPN functions and procedures.” -- if it’s just a description (as the text clearly is), and not a technical specification, then why it is not Informational? [Ali] As suggested by Thomas and Martin, I will change the wording to say “specifies” instead of “describes”. This document is definitely a Standard Track document because it specifies procedures, EVPN route formats, and operations for different service types for NV overlay. I can see how we could call it an Applicability Statement (rfc2026) and still publish it in the Standards Track. If we want to go that way, we would need some minor updates to make it clear that this is an Applicability Statement and is not intended to stand in for a Technical Specification. I am not clear on the process as it related to possible DownRefs (see below), but I’m willing to Last Call an Applicability Statement in the Standards Track…if that is what you want. [Ali] I have updated the draft by adding the following sentence to the abstract to clarify the point that this document is a Technical Specification: “This document also specifies new multi-homing procedures for split-horizon filtering and mass-withdraw. It also specifies EVPN route constructions for VxLAN / NvGRE encapsulations and ASBR procedures for multi-homing NV Edge devices.” Thanks! Alvaro. [1] https://mailarchive.ietf.org/arch/msg/nvo3/cd0hOfb966ROcL4t8JCrBD28bBg/?qid=c9f632dc5d6aab5e4b22972bb242baf0 Major: M1. Section 5.1.2.1 (Auto Derivation of RT) shows a “packet format” to illustrate how the RT can be auto-derived. Without any context, the description doesn’t make much sense: the fields are not described in order, the reader is expected to know about global and local administrative fields, the “A-bit” (or the Type field) are not mentioned in the description, people could probably guess that “D-ID” means “domain-id”, there’s no indication of what to do with that “packet format”, etc. Please clean the description up, include clearer explanations and some references can’t hurt. [Ali] Done. M1.2. What about 4-byte ASNs? [Ali] In such case, RT cannot be auto-derived. I have added the following paragraph to the end of 5.1.2: “It should be noted that RT auto-derivation is applicable for 2-octet AS numbers. For 4-octet AS numbers, RT needs to be manually configured since 3-octet VNI fields cannot be fit within 2-octet local administrator field.” M2. From 5.1.3 (Constructing EVPN BGP Routes): “...routes MAY be advertised with multiple encapsulation types as long as they use the same EVPN multi-homing procedures (section 8.3.1, Split Horizon)…”. Is Split Horizon an example, or the only multi-homing procedure that should be considered? Please be clear. [Ali] removed this paragraph from this section because there is a dedicated section 6 for “EVPN with Multiple Data Plane Encapsulations”. Added the following paragraph for clarification to this section: “When a PE advertises multiple supported encapsulations, it MUST advertise encapsulations that use the same EVPN procedures including procedures associated with split-horizon filtering described in section 8.3.1. For example, VxLAN and NvGRE (or MPLS and MPLS over GRE) encapsulations use the same EVPN procedures and thus a PE can advertise both of them and can support either of them or both of them simultaneously. However, a PE MUST NOT advertise VxLAN and MPLS encapsulations together because a) MPLS field of EVPN routes is set to either a MPLS label for a VNI but not both and b) some of EVPN procedures (such as split-horizon filtering) are different for VxLAN/NvGRE and MPLS encapsulations.” M3. From 5.2 (MPLS over GRE): M3.1. “...when it is used in conjunction with EVPN the GRE key field SHOULD be present, and SHOULD be used to provide a 32-bit entropy field.” What are the cases when it is ok not to include the field, or use it for that purpose? IOW, why are these SHOULDs not MUSTs? Or maybe, when is the key field needed? [Ali] The reason it is a “SHOULD” because, the MPLS over GRE encap still works without the key field set to the entropy value; however, if that’s done, then ECMP won’t work well in the network. Also, the core routers in the network may not support ECMP based on GRE key and that’s another reason for “SHOULD”. M3.2. "The Checksum and Sequence Number fields are not needed and their corresponding C and S bits MUST be set to zero.” This is different than what rfc4023 specifies (not a problem). If you’re specifying the setting of the bits, then you should be more prescriptive with whether the fields are included of not; suggestion: "The Checksum and Sequence Number fields MUST NOT be included and the corresponding C and S bits in the GRE Packet Header MUST be set to zero.” [Ali] done. M4. In 7.1 (Impact on EVPN BGP Routes & Attributes for VXLAN/NVGRE Encapsulation) M4.1. These 2 MUSTs seem out of place as they are used to explain, and not in a Normative way: “ ..the RD must be a unique value per EVI or per NVE as described in [RFC7432]. In other words, whenever there is more than one administrative domain for global VNI, then a unique RD MUST be used, or whenever the VNI value have local significance, then a unique RD MUST be used.” s/MUST/must [Ali] done. M4.2. This SHOULD is also out of place because the standardization was already done in rfc7432 (this document is just mentioning it): “...as noted in section 8.6 of [RFC7432]...the single-homing ingress NVE SHOULD be able to…”. s/SHOULD/should [Ali] done. M4.3. Section 10.2.1 then points back at the text in 7.1 using another SHOULD…again, s/SHOULD/should [Ali] done. M5. 7.2 (Impact on EVPN Procedures for VXLAN/NVGRE Encapsulation) also includes a superfluous SHOULD: “...as noted in section 8.6 of [RFC7432]...a single-homing ingress NVE SHOULD implement…”. s/SHOULD/should [Ali] done. M6. The introductory paragraph in Section 8.1 (EVPN Multi-Homing Features) says that it is used to "recap the multi-homing features of EVPN to highlight the encapsulation dependencies. The section only describes the features and functions at a high-level. For more details, the reader is to refer to [RFC7432].” However some of the 8.1.* sub-sections contain Normative language. Is that Normative language specifying new behavior introduced in this document, or highlighting the recap from rfc7432? If there’s no new behavior, then please use lower cases. If there is new behavior, then it would be nice to clarify that in 8.1. [Ali] done. M7. In 8.3.1 (Split Horizon), this MUST is out of place because it is not specifying anything: “...other means of performing the split-horizon filtering function MUST be devised.” s/MUST/must [Ali] done. M8. References M8.1. TUNNEL-ENCAP (aka draft-ietf-idr-tunnel-encaps) should be Normative, which btw is marked to Obsolete rfc5512; I mention this because both are listed in several parts, so you should take rfc5512 out. [Ali] Please refer to Thomas explanation on this which is copied here for your convenience: “This was debated on a while ago, and this is somehow the conclusion of the discussion: https://mailarchive.ietf.org/arch/msg/bess/z1J2VD9rtCQC7NHnmi_4tz_bR_w Copy-paste: ---- We'll also add idr-tunnel-encaps a Informative reference. With respect to Tunnel Encap Extended Community (which is the only part of idr-tunnel-encap used by evpn-overlay draft), idr-tunel-encap draft itself references RFC 5512. During the course of WG LC and RFC editorship of evpn-overlay draft, if we see that idr-tunnel-encap is progressing fast, then we can drop the reference to RFC 5512 and make the reference to idr-tunnel-encap Normative. Otherwise, we'll keep both references with RFC 5512 as Normative and idr-tunnel-encap as Informative. ---- The question probably is whether or not idr-tunnel-encap progress is sufficient.” M8.2. These should also be Normative: RFC7348, NVGRE, VXLAN-GPE, RFC4023 [Ali] Please refer to Thomas explanation on this which is copied here for your convenience: “My process fu is weakening, but if this specification is standard track (and I believe it should be), I believe it can't normatively depend on Informative specs and some of the above are in this category.” Minor: P1. Please use the new Normative Language boilerplate (rfc8174). [Ali] done. P2. From 7.1 (Impact on EVPN BGP Routes & Attributes for VXLAN/NVGRE Encapsulation): “...reduces the required routes and attributes to the following subset of four out of eight”. Please either list the attributes that are not needed, or put in a reference to where those can be found. (rfc7432 ??) [Ali] done. P3. From 8.3.1 (Split Horizon): "In order to prevent unhealthy interactions…”. I would really like to see more here: what does “unhealthy interactions” mean? Can it result in loops or lost traffic or some other “bad” thing? I note that even through you “prohibit” the configuration, you don’t go as far as mandating that it not to be used (MUST NOT), which makes me want to understand more the potential effects…if it happens, what are the risks? [Ali] Changed the text to: “In order to allow proper operation of split-horizon filtering among the same group of multi-homing PE devices, a mix of PE devices with MPLS over GRE encapsulations running [RFC7432] procedures for split-horizon filtering on the one hand and VXLAN/NVGRE encapsulations running local-bias procedures on the other on a given Ethernet Segment MUST NOT be configured.” P4. In 8.3.3 (Unknown Unicast Traffic Designation): “...the ingress PE MAY use a flag-bit in the VxLAN header to indicate BUM traffic type. Bit 6 of flag field in the VxLAN header is used for this purpose per section 3.1 of [VXLAN-GPE].” Suggestion: “…the ingress PE MAY set the BUM Traffic Bit (B bit) [VXLAN-GPE] to indicate BUM traffic.” [Ali] done. P5. Security Considerations: I find that the suggestion of IPSec may be out of place in this document; this document pretty much just talks about how to use EVPN, it doesn’t really introduce new capabilities, so unless IPSec is mentioned in rfc7432 (which is not — and not even mentioned in this section), or recommended in rfc4271 (which is not) then I would refrain from including such recommendation here. In fact, I think that pointing at the Security Considerations of existing RFCs should be enough. [Ali] Sounds good. Done. P5.1. The reference to rfc4301 (beyond what VXLAN/NVGRE) mention seems like you’re trying too hard. I would just put explicit references to the encapsulations since they should be dealing with security themselves. [Ali] Done. P6. References: [KEYWORDS] shows up twice. I think that the reference to rfc4271 can be made Informative. [Ali] Removed the 2nd [KEYWORDS] and moved RFC4271 to informative. Nits: N1. Section 3 (Terminology) literally transcribes several definitions from rfc7432/rfc7365 — while it is ok, I personally prefer just pointing at the definitions: it’s just easier to have the other RFCs be the authoritative source and not rely on maintaining the definitions in sync should they ever change. Suggestion: “The reader is expected to be familiar with the terminology in …”. [Ali] I prefer to keep it as is for easier read. N2. s/the VNI value have local significance/the VNI value has local significance [Ali] Done. N3. s/it is recommend/it is recommended [Ali] Done. N4. Please be consistent in naming references, some list the RFC number, while others a name... [Ali] Done.
_______________________________________________ BESS mailing list BESS@ietf.org https://www.ietf.org/mailman/listinfo/bess
