Re: [RTG-DIR] Rtgdir early review of draft-dmk-rtgwg-multisegment-sdwan-05

[Yingzhen Qu <[e-mail]>]

Hi Adrian,

Thanks for this very nice review. I really appreciate it.

A chair note, this draft has been presented in RTGWG sessions for a couple
of times. We, the RTGWG chairs, did communicate with the NVO3 chairs about
initiating an adoption call in RTGWG, and the adoption call email will be
copied to NVO3 as well.

Authors,

Please work with Adrian to address his comments. We'll start an adoption
call only after all major issues are resolved.

Thanks,
Yingzhen


On Sat, Feb 10, 2024 at 1:38 PM Adrian Farrel via Datatracker <
[e-mail]> wrote:

> Reviewer: Adrian Farrel
> Review result: Has Issues
>
> Hello,
>
> draft-dmk-rtgwg-multisegment-sdwan-05.txt
>
> I have been selected to do a routing directorate "early" review of this
> draft.
>
> The routing directorate will, on request from the working group chair,
> perform an "early" review of a draft before it is submitted for
> publication to the IESG. The early review can be performed at any time
> during the draft's lifetime as a working group document. The purpose of
> the early review depends on the stage that the document has reached.
>
> This review was requested by the RTGWG chairs as part of the process for
> considering this draft for working group adoption.
>
> For more information about the Routing Directorate, please see
> https://wiki.ietf.org/en/group/rtg/RtgDir
>
> Although these comments are primarily for the use of the RTGWG chairs,
> it would be helpful if you could consider them along with any other
> adoption poll or mailing list comments that you receive, and strive to
> resolve them through discussion or by updating the draft.
>
> Document: draft-dmk-rtgwg-multisegment-sdwan-05.txt
> Reviewer: Adrian Farrel
> Review Date: 10 February 2024
> Intended Status: Standards Track
>
> = Summary =
>
> I believe this document is suitable to be a candidate for an adoption
> poll in RTGWG.
>
> I have some concerns about this document that I think should be resolved
> during the adoption process (or before).
>
> = Overview =
>
> The IETF has documented GENEVE in the Standards Track RFC 8926. It is a
> generic encapsulation for network virtualization.
>
> This document proposes using GENEVE to encapsulate IPsec packets between
> customer premises equipment (CPEs) and data centre gateways as it is
> carried across a variety of underlay networks in support of "SD-WAN"
> connectivity services. It is suggested tht this is necessary so that key
> transit relay points do not need to decrypt and re-encrypt the packets,
> allowing IPsec to run correctly "edge-to-edge" between CPEs.
>
> I assume there is a reason why this document was not taken to NVO3, but
> it seems wise to let them know about the proposal to adopt it in RTGWG.
> Otherwise, it seems that this draft could be in scope for RTGWG
> according to the charter.
>
> The problem statement is somewhat woolly and is better understood from
> the examples than from the Introduction. However, the examples seem to
> offer credible scenarios and requirements. What is not clear from the
> examples or the introduction is why GENEVE is an appropriate solution
> rather than any of the many other tunnelling encapsulations that exist.
> It is not until Section 4 that we discover why GENEVE is suggested as a
> solution. I have no doubt that using GENEVE in this way could be made to
> work (modulo some fixes to the proposal in the draft), and if that is
> what the WG wants to work on, then it seems to be OK.
>
> = Comments =
>
> The document is relatively readable notwithstanding my concerns about
> clarity of motivation. I feel that it could be a lot shorter and cleaner
> (possibly the authors are too close to the problem?), but this is
> something the WG should be able to polish.
>
> There are a lot of nits in this document. Nits are not significant for
> determining adoption and might not need to be fixed before adoption by a
> WG (this is a matter for the chairs to determine), but their presence
> makes the document much harder to review.
>
> = Major Issues =
>
> 4.1 has
>
>    The Protocol Type (16 bits) = 50 (ESP) [RFC4303] indicates
>    that IPsec ESP encapsulated data are appended at the end of
>    the GENEVE header.
>
> My understanding of RFC 8926 is that you put an Ethertype in the
> Protocol Type field.
>
>    Protocol Type (16 bits):  The type of protocol data unit appearing
>       after the Geneve header.  This follows the Ethertype [ETYPES]
>       convention, with Ethernet itself being represented by the value
>       0x6558.
>
> The value 50 (x32) would thus be interpreted as an IEEE802.3 Length
> Field. I *think* what you are meant to do is put the ESP after an IP
> header (per RFC 4303) and then use the appropriate Ethertype in the
> GENEVE header. This probably also makes life a lot easier because the
> encapsulating IP header tells the gateway a lot about the intended
> destination of the packet (for example, you wouldn't need the SD-WAN
> Tunnel Endpoint Sub-TLV and the SD-WAN Tunnel Originator Sub-TLV).
>
> = Minor Issues =
>
> Section 1 lists a number of ways to connect an enterprise to a cloud DC.
> It says that these are described in [Net2Cloud] but a quick search does
> not yield matching terms. It would be helpful if the terminology was
> aligned and a more specific (section) reference was given.
>
> The section introduces "the IPsec encrypted packets" without mentioning
> where these are flowing and why IPsec is being used.
>
> A lot of Section 1 seems to be describing reasons for an architecture
> (which would make use of the proposed solution) and not simply speaking
> to the problem at hand that motivates the solution. Doesn't the
> architectural discussion belong in [Net2Cloud]?
>
> All in all, it is very hard to determine a clear motivation for this
> work from the text until we get to Section 3.4.
>
> ---
>
> 2.
>
>    SD-WAN      An overlay connectivity service that optimizes
>                transport of IP Packets over one or more Underlay
>                Connectivity Services by recognizing applications
>                (Application Flows) and determining forwarding
>                behavior by applying Policies to them. [MEF-70.1]
>
> To be clear, and notwithstanding anything in MEF70.1, it is not
> possible (or desireable) to identify and applications. What is
> possible, using the fields highlighted in MEF70.1, is to select
> traffic flows and apply steering policies to them to direct them
> towards interfaces or tunnel endpoints so that they are carried
> across the underlay.
>
> ---
>
> 3.1 points to the use of cloud security functions ans SaaS features. If,
> however, end-to-end encryption is in use through IPsec, and that is
> carried in GENEVE so that the gateways do not need to decrypt and re-
> encrypt, the availability of those security functions will be limited
> (because they cannot see into the payload packet). Doesn't that mean
> that these pieces of this use case are lost?
>
> ---
>
> I think 3.3 says, "overlays are useful," and then 3.4 says "tunnels are
> useful."
>
> ---
>
> You need much more clarity on your choice to not set the C bit in the
> option types. You should state that this is a deliberate choice and
> describe how the packets will be processed if the option type is not
> recognised.
>
> Won't you need a registry for these option types?
>
> ---
>
> 4.6 and 4.7
>
> Obviously, you are going to have to specify these TLVs in a future
> version. For the include case, you are going to have to say whether this
> is an ordered list, whether the inclusion is mandatory, and whether the
> list is strict or loose. You may also have to worry about the option
> length in the GENEVE header especially in the case of IPv6.
>
> ---
>
> 5.5 What do you mean by an invalid address?
>
> ---
>
> 4.5 has...
>    The Control Plane protocol is out of the
>    scope of this document.
>
> And yet...
>
>    7. Control Plane considerations
>
> ---
>
> I'm curious why your new Multi Segment SD-WAN Sub-TLVs registry has 0
> and 255 as reserved.
>
> = Nits =
>
> The very first thing I do when asked to review a document is to run idnits
> It is not hard (just click a button).
>
> There is rarely an excuse for any version of a draft to have nits, but when
> an author is requesting action (such as adoption or last call) then they
> really have a duty to fix all of the nits first.
>
> idnits reports
>
>   Checking nits according to https://www.ietf.org/id-info/checklist :
>
> ----------------------------------------------------------------------------
>
>   == There are 7 instances of lines with non-RFC6890-compliant IPv4
> addresses
>      in the document.  If these are example addresses, they should be
> changed.
>
>   == There are 5 instances of lines with private range IPv4 addresses in
> the
>      document.  If these are generic example addresses, they should be
> changed
>      to use any of the ranges defined in RFC 6890 (or successor):
> 192.0.2.x,
>      198.51.100.x or 203.0.113.x.
>
>
>   Miscellaneous warnings:
>
> ----------------------------------------------------------------------------
>
>   == The document seems to lack the recommended RFC 2119 boilerplate, even
> if
>      it appears to use RFC 2119 keywords -- however, there's a paragraph
> with
>      a matching beginning. Boilerplate error?
>
>      (The document does seem to have the reference to RFC 2119 which the
>      ID-Checklist requires).
>   -- The document date (January 24, 2024) is 13 days in the past.  Is this
>      intentional?
>
> ---
>
> The layout of the file is a bit of a mess in places. This does not make
> reviewing it any easier. The problems are around layout of boilerplate
> and the use of bullets. (Probably an artefact of using the Word template
> but surely easy to fix before submission?
>
> The running footer is also broken probably for the same reason.
>
> ---
>
> As a matter of style, the Title, Abstract, and main text should not use
> abreviations (SD-WAN, DC, CPE, GW) without expanding them first. This
> should happen in each place they are used for the first time (i.e., the
> main text cannot inherit from the Abstract, and the Abstract cannot
> inherit from the Title.
>
> ---
>
> Please decide on "cloud DC" or "Cloud DC"
>
> ---
>
> I'm not sure that wikipedia is the best reference, but if you want to
> use it, make it a proper reference.
>
> ---
>
> Clean your terms to only mention ones you actually use.
>
> ---
>
> I don't think you should reproduce the GENEVE header from RFC 8926.
> Although you probably have this right, you risk issues if you
> accidentally introduce a discrepency.
>
> You can just say, "The GENEVE header is defined in Section 3 of
> [RFC8926]."
>
> ---
>
> Please use consistent case for "TBD"
>
>
>
>