I am an assigned INT directorate reviewer for draft-ietf-idr-sdwan-edge-discovery-17.txt. These comments were written primarily for the benefit of the Internet Area Directors. Document editors and shepherd(s) should treat these comments just like they would treat comments from any other IETF contributors and resolve them along with any other Last Call comments that have been received. For more details on the INT Directorate, see https://datatracker.ietf.org/group/intdir/about/ . Based on my review, if I was on the IESG I would ballot this document as DISCUSS. I have the following DISCUSS level issues: The text is quite long, and in its 17th version. Yet, I think its structure needs some rework, because of some inconsistencies and missing elements. For instance: * In section 3.3.1, two types of encoding for the Client Route UPDATE messages are given, "Barebones" and Tunnel Encaps Attribute, while they are described only in section 5. * In section 3.4, the behavior of the RR mentions authorized BGP peers, but the document does not give an idea about where and how those authorized BGP peers are set. * In section 4.2, the text mentions that the RR speaks to the BGP clients over IPSec while section 3.4 mentions a secure transport session (e.g. TLS) * In section 8, the text refers to the IPSec-SA-ID, and assumes it has been set but very little details are given in the document as to how this ID is set, negotiated or provisioned in the IPSec SA endpoints. References are made to other RFCs, but the text would benefit from a description of the mechanism the authors are considering using. * In section 11, the text describes "walled garden SD-WAN deployments" but does not state the differences with a more open SD-WAN deployment. In general, the document tends to give the detail of the formatting used in the messages and TLVs before describing the protocol mechanism using those messages. The general understanding of the protocol's mechanism would be improved if this were done the other way round. On a technical standpoint, I have some questions regarding the design of the TLVs described in the document: * In section 6.1 about the SD-WAN NLRI, it is mentioned that "Route type outside of 1 are out of scope for this document": Are other route types used in private or experimental deployments? What is the justification for using 2 bytes to carry a route type that is associated a single value in the document's context? * In section 7 about the Extended port attribute TLV, 1 byte is dedicated to both the NAT type and the Encap type to encode 7 and 2 possibilities while the transport network ID and the Routing Domain ID need to be globally unique, which requires space: wouldn't it be possible to reallocate some space from the 2 first fields to give more space to the two later fields? * In the same section, is it always necessary to carry the Public IP and Public port fields? Can a flag be used to signal the presence of those two fields if necessary? * In section 7.1.1, in the design of the Underlay Network Transport subsubTLV, why are the connection and port type described rather than their underlying properties (bandwidth, cost, latency, jitter, packet loss...)? Describing the underlay link's characteristics in a more abstract way could help the TLV be more generic. * In section 8.5 about the simplified IPSec SA sub-TLV, is it necessary to allocate a full byte to both the Transform and the IPSec mode fields? The following are other issues I found with this document that SHOULD be corrected before publication: It is to be noted that Figure 1 extensively uses public IP(v4) addresses. Proper private addresses should be used for the addresses in the SD-WAN realm, while public underlay addresses should be clearly marked as example addresses not to be reused in public deployments. The following are minor issues (typos, misspelling, minor text improvements) with the document: * In page 3, is "[MP_REACH_NLRI]" a reference to a document, or a specific section to a document? * In page 3, reference to "[MEF-70.1]" is missing in the document. * In page 6, section 3.3, in the sentence "There are two different sequences of BGP UPDATE messages are used for SD-WAN Edge Discovery.", either remove "There are" or the second "are". * In page 6, section 3.3, there is a "./" at the end of a paragraph. * In page 6, section 3.3.1, is "[Encap-EC]" a reference to a document, or a specific section to a document? * In page 7, section 3.3.1, the address given in "d) port-based IPsec tunnel [160.0.0.1 - 172.0.0.1]" is not consistent with Figure 1 which uses 170.0.0.1 (and please consider the previous remark done on the adressing scheme used in Figure 1). * In page 8, section 3.3.1, the captions of Figures 2 and 3 should mention the encoding method used for the depicted Client Routes Update messages. * In page 8, section 3.2.2, "parameter" should replace "paramater". * In page 13, section 5.1, which document is targeted in the sentence "The validation for the Tunnel Egress Endpoint uses the validation in section 6, 8, and 13 applied to the NextHop."? * In page 14, section 5.4, "the" is missing in "An SD-WAN VPN ID is same as a client VPN...". * In page 14, section 5.5, "which" should be removed from the sentence "SD-WAN edge node which can be reached by either an MPLS path or an IPsec path within the hybrid SD-WAN tunnel." * In page 14, section 6, a verb is missing in "The hybrid SD-WAN underlay tunnel UPDATE is to advertise the detailed...". * In page 14, section 6, "with" should be removed from "The TEA can include with SubTLVs for Extended Port attribute..." * In page 17, section 7, the first sentence of the section should be reformulated, as it is difficult to understand. * In page 18, in Figure 11, the content of the Type box is unclear. * In page 22, section 9, the reference to figure 10 is wrong, and should indicate Figure 13. * In page 24, section 8.2, the text does not mention the "Rekey Counter" field. * In page 26, section 8.4, the sentence "The Reserved os ignored upon receipt and set to zero upon transmission." should be corrected. * In page 28, section 8.5, "none-length:" should be replaced by "nonce-length:" (or even "Nonce-length:", which implies adding capital letters in the fields described in Figure 18). * In page 31, section 10.2, one "s" should be removed in "asssociation". * In page 32, section 10.2, one "may" should be removed in "... SD-WAN Edge Node may may help select...". * In page 33, section 10.2.2, the last "e" should be removed in "associationes". * In page 33 and 34, section 10.2.2, the formatting of the messages described should be consistent. * In page 34, section 11, "to" needs to be added in "BGP connection has be established...". The sentence "SD-WAN edge nodes to advertise its properties to their peers..." needs to be corrected.