Early Review of draft-ietf-idr-sdwan-edge-discovery-17
review-ietf-idr-sdwan-edge-discovery-17-intdir-early-fressancourt-2024-10-30-00

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/
<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.