[bess] Re: Rtgdir early review of draft-ietf-bess-evpn-geneve-07

["Boutros, Sami" <[e-mail]>]

Thanks Jonathan for the review,

I updated the documents to address your concerns.

And submitted https://www.ietf.org/archive/id/draft-ietf-bess-evpn-geneve-08.txt

Please see comments inline.

Reviewer: Jonathan Hardwick
Review result: Has Issues

Document: draft-ietf-bess-evpn-geneve-07
Reviewer: Jon Hardwick
Review Date: 5 June 2024

Summary:
I have some minor concerns about this document that I think should be resolved
as part of the normal WGLC process.

Comments:
---

Section 1 - please could you also mention that you are adding a new Ethernet
Option TLV to Geneve (i.e. the thing in section 4) to account for BUM traffic
and split-horizon control?

SB:> Added the following

   <t>Furthermore, the document defines a new ethernet option TLV to handle

   BUM traffic, etree root and leaf indication, and split horizon.</t>
SB:>

Section 1 final paragraph - Introduction section is not the place for normative
statements. Suggest removing this sentence ("a transmitting NVE MUST NOT...")
as you say the same thing in 5.1.

SB:>
I prefer to keep it, given that we added to section 1 a description of section 4.
SB:>

Section 4.1 - since Length field is given in 4-byte multiples, please could you
change "length=4" to either "length=4 octets" or "length=0x1"? Similar for
"length=8".

SB:> ok

Section 4.1 - The B,L,R bits are being defined where these three bits were
previously reserved, is that right?  I assume you are doing this just for
option-class=Ethernet and that they continue to be reserved for other option
classes?  Given that these reserved bits are part of the common Option TLV
header I'm not sure it's correct to give them a meaning just for one Option TLV
type and not others.  This is likely to lead to confusion, at least.  I suggest
that they should be part of the Option TLV payload.  That is, make the Ethernet
Option TLV always like Figure 2 (8 octets long), with the new flags in the Rsvd
field, and an extra flag to indicate whether the ESI label is included or not.

Section 4.1 - I wonder if you should use a different letter for the
Root-Indication flag, as R is commonly used to mean "Reserved" (particularly by
RFC 8926).
SB:> I took your suggestion, and changed the Root-indication bit to be H instead of R.

Section 4.1 - "Type is set to EVPN-OPTION with value = 0" - do you just mean "
Type is set to 0"?

SB:> Correct.

Section 5 para 2 - typo "Typei"

Section 5.1 diagram - I believe that length is always 2 octets and never 1
octet, since the type is in the range 192-252 (any type >128 has a 2-octet
length per RFC 9012).

Section 5.1 - Regarding the following sentence:
BEGIN QUOTE
   An NVE receiving the above sub-TLV, MUST send Geneve packets to the
   originator NVE with only the option TLVs the receiver NVE is capable
   of receiving, and following the same order.
END QUOTE

Does the "receiver NVE" in this sentence (2nd line) refer to the NVE that
receives the Geneve Tunnel Option Types sub-TLV or to the NVE that receives the
subsequent Geneve packets (that is, the one referred to in the same sentence as
the "originator NVE")?  I think it must be the latter - "receiver NVE" and
"originator NVE" in this sentence are the same NVE.  If so, that's confusing!
Here is a suggested rewording.

BEGIN NEW:
   An NVE informs its peers which Geneve option TLVs it can receive
   by including the first 4 bytes of each option TLV in the Geneve Tunnel
   Option Types sub-TLV.  The peers MUST send Geneve packets to this
   NVE with only the option TLVs that it has specified here, following the
   same order.
END NEW:

SB:> Updated the text.

Section 5.1 - the final sentence needs rewording because it makes two different
normative statements. OLD:
  "The above sub-TLV(s) MAY be included with only Ethernet A-D per-ES routes"
NEW:
  "The above sub-TLV(s) MAY be included with Ethernet A-D per-ES routes and
  MUST NOT be included with other routes."

SB:> Updated the text.
Thanks,
Sami