[Idr] AD Review of draft-ietf-idr-tunnel-encaps-15

Alvaro Retana <aretana.ietf@gmail.com> Fri, 21 February 2020 12:47 UTC

From: Alvaro Retana <aretana.ietf@gmail.com>
MIME-Version: 1.0
Date: Fri, 21 Feb 2020 04:47:14 -0800
Message-ID: <CAMMESsw09LGWWhqyJ_0=jRimUN+_UuCjaXHCdqF9zkpaxSQgVQ@mail.gmail.com>
To: "draft-ietf-idr-tunnel-encaps@ietf.org" <draft-ietf-idr-tunnel-encaps@ietf.org>
Cc: John Scudder <jgs@juniper.net>, idr-chairs@ietf.org, "idr@ietf. org" <idr@ietf.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/S1nyz1sewA_Xqf63W_vz8c4L8Z0>
Subject: [Idr] AD Review of draft-ietf-idr-tunnel-encaps-15
Precedence: list

Dear authors:

Here is my review of draft-ietf-idr-tunnel-encaps-15. Thank you for
all the work that you and the WG have put into it.

While this is a good document, I do have a significant number of
comments (see details in-line below). I want to highlight a few of
them up front.

I will wait for the major issues to be addressed before starting the IETF LC.

Thanks!!

Alvaro.

(1) Obsoleting other work

This document deprecates the Encapsulation SAFI because it "has never
been used in production". What about other work that specifies the
use of the Encapsulation SAFI? Should they also be Obsoleted by this
document? [Yes, I know there was a long discussion on the list, which
resulted in the text in §1.4.]

Both rfc5566 (mentioned in §1.4) and rfc5640 specify the use of the
Encapsulation SAFI. Is there interest in Updating those RFCs? If
not, then it seems like the easy thing to do is to Obsolete that work
with this document. If there is, then I agree with the text in §1.4
in that the effort would be out of scope of this document. In either
case, I think that §1.4 raises more questions than it answers and is
not needed in this document.

John: As Shepherd, can you please ping the WG (you may have to reach
out to bess as well) about potential interest in Updating
rfc5566/rfc5640. I won't hold this document waiting for the
resolution, but having an idea of any interest would be nice.
Interest has been expressed for rfc5566bis [1], but didn't see
anything related to rfc5640. BTW, the mailing list contains
discussion about other IPsec-related drafts that use tunnel-encap; I'm
not sure of their relationship (if any) to rfc5566.

[1] https://mailarchive.ietf.org/arch/msg/idr/yuIXr3HpPEp7v_k08XpdhXRN6GM

(2) Underspecification.

A big part of this document is dedicated to specifying sub-TLVs...but
in none of those descriptions is the Type of the sub-TLV indicated!
Please add all the basic description of a sub-TLV in one place, and
don't force the user to search for the types elsewhere: in the IANA
Considerations sometimes, but in the registry in others. Please don't
forget that this document Obsoletes rfc5512, so anything that was
specified there needs to be re-specified here.

(3) Use of Normative Language.

While using rfc2119 keywords is not always necessary, they help
clarify the behavior required for interoperability. Cases where the
keywords are not used (e.g. rfc8200) often result in multiple
interpretations. This document is inconsistent in the use of
Normative Language: it is used sometimes, but not always. Please be
consistent! Recommendation: use it! I pointed out some cases, but
the whole document would benefit from a re-read.

(4) Use Cases

rfc5512 gave an overview of the use case (mostly intra-AS), but this
document doesn't mention use cases either as an introduction to the
problem being solved or in the form of Operational Considerations.
Sections 9 and 10 would be a good start for that type of section, but
having a general description of the problem upfront (in the
Introduction maybe) would be ideal. Again, because this document
Obsoletes rfc5512, then we can't really rely on the information there.
I'm looking for a few paragraphs, mostly directed at readers that may
not be familiar with rfc5512 and the potential applications.

(5) When does an IP address "belong" to an AS?

§3.1 uses this condition as a way to determine if the Tunnel Endpoint
sub-TLV is malformed:

o It can be determined that the IP address in the sub-TLV's address
subfield does not belong to the non-zero AS whose number is in the
its Autonomous System subfield. (See section Section 13 for
discussion of one way to determine this.)

IOW, for the Tunnel Endpoint sub-TLV to be formed correctly the IP
address has to "belong" to the AS listed.

§13 (Security Considerations) is very tentative about how to determine
if an IP address "belongs" to the AS listed. In fact, the text there
starts by suggesting that "BGP Origin Validation [RFC6811] can be used
to obtain assurance that the given IP address belongs to the given
AS", but after pointing at some flaws concludes that:

For these reasons, BGP Origin Validation should not be relied upon
exclusively, and the filtering procedures of Section 10 should always
be in place.

§10 (Scoping) says that "any BGP speaker that understands the
attribute MUST be able to filter the attribute from incoming/outgoing
BGP UPDATE messages"...but it doesn't say anything about the topic of
"belonging", or what these filters should consider when active.
Presumably, the filters can be used to avoid receiving Tunnel Endpoint
sub-TLV with IP addresses that don't belong to the AS listed...but
that is not mentioned anywhere, much less which would be the criteria
to make that filtering decision.

The effect of the above is that the document doesn't really specify
how to determine that the Tunnel Endpoint sub-TLV is not malformed.

My suggestion is to not make "belonging" part of the criteria for a
well-formed sub-TLV. The validity of the address in terms of
"belonging" is important and should be highlighted as it is in §13.
Consider a stronger requirement for using Origin Validation (SHOULD
?). Also, the deployment models (and use cases, as mentioned above),
including the "well-defined scope" from §10 should be clear
throughout.

[Line numbers from idnits.]

...
13 Abstract

15 RFC 5512 defines a BGP Path Attribute known as the "Tunnel
16 Encapsulation Attribute". This attribute allows one to specify a set
17 of tunnels. For each such tunnel, the attribute can provide the
18 information needed to create the tunnel and the corresponding
19 encapsulation header. The attribute can also provide information
20 that aids in choosing whether a particular packet is to be sent
21 through a particular tunnel. RFC 5512 states that the attribute is
22 only carried in BGP UPDATEs that have the "Encapsulation Subsequent
23 Address Family (Encapsulation SAFI)". This document deprecates the
24 Encapsulation SAFI (which has never been used in production), and
25 specifies semantics for the attribute when it is carried in UPDATEs
26 of certain other SAFIs. This document adds support for additional
27 tunnel types, and allows a remote tunnel endpoint address to be
28 specified for each tunnel. This document also provides support for
29 specifying fields of any inner or outer encapsulations that may be
30 used by a particular tunnel.

[nit] s/that have the "Encapsulation/that use the "Encapsulation

...
142 1.1. Brief Summary of RFC 5512
...
161 o BGP speaker R1 has received and installed UPDATE U;

[minor] To be consistent with rfc4271: s/installed UPDATE U/selected
UPDATE U for local use

...
171 o R1's best path to D is a BGP route that has R2 as its next hop;

[minor] rfc4271 consistency: s/best path/best route/g

...
179 1.2. Deficiencies in RFC 5512
...
191 o There is no way to use the Tunnel Encapsulation attribute to
192 specify the tunnel egress endpoint address of a given tunnel;
193 [RFC5512] assumes that the tunnel egress endpoint of each tunnel
194 is specified as the NLRI of an UPDATE of the Encapsulation-SAFI.

[nit] s/Encapsulation-SAFI/Encapsulation SAFI

...
209 1.3. Brief Summary of Changes from RFC 5512

[minor] It would be very nice if forward references were provided
below -- to the section where the changes/enhancements are specified.

211 In this document we address these deficiencies by:

[style nit] Please don't write in first person. Instead: "This
document addresses these deficiencies by:". There is similar text in
other places.

...
233 One of the sub-TLVs defined in [RFC5512] is the "Encapsulation sub-
234 TLV". For a given tunnel, the encapsulation sub-TLV specifies some
235 of the information needed to construct the encapsulation header used
236 when sending packets through that tunnel. This document defines
237 encapsulation sub-TLVs for a number of tunnel types not discussed in
238 [RFC5512]: VXLAN (Virtual Extensible Local Area Network, [RFC7348]),
239 VXLAN-GPE (Generic Protocol Extension for VXLAN,
240 [I-D.ietf-nvo3-vxlan-gpe]), NVGRE (Network Virtualization Using
241 Generic Routing Encapsulation [RFC7637]), and MPLS-in-GRE (MPLS in
242 Generic Routing Encapsulation [RFC2784], [RFC2890], [RFC4023]).
243 MPLS-in-UDP [RFC7510] is also supported, but an Encapsulation sub-TLV
244 for it is not needed.

[nit] s/encapsulation sub-TLV/Encapsulation sub-TLV/g

...
265 [RFC5512] defines a Tunnel Encapsulation Extended Community, that can
266 be used instead of the Tunnel Encapsulation attribute under certain
267 circumstances. This document addresses the issue of how to handle a
268 BGP UPDATE that carries both a Tunnel Encapsulation attribute and one
269 or more Tunnel Encapsulation Extended Communities.

[nit] s/Community, that/Community that

271 1.4. Impact on RFC 5566

273 [RFC5566] uses the mechanisms defined in [RFC5512]. While this
274 document obsoletes [RFC5512], it does not address the issue of how to
275 use the mechanisms of [RFC5566] without also using the Encapsulation
276 SAFI. Those issues are considered to be outside the scope of this
277 document.

[major] See my related comments up front. This section is not needed.

279 2. The Tunnel Encapsulation Attribute
...
287 0 1 2 3
288 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
289 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
290 | Tunnel Type (2 Octets) | Length (2 Octets) |
291 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
292 | |
293 | Value |
294 | |
295 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

297 Figure 1: Tunnel Encapsulation TLV Value Field

[minor] This TLV is called the Tunnel TLV all over the text...except
here, where it is not given a name in the description text. Note that
besides the title of the figure above, "Tunnel Encapsulation TLV" is
used in one other place too...

299 o Tunnel Type (2 octets): identifies a type of tunnel. The field
300 contains values from the IANA Registry "BGP Tunnel Encapsulation
301 Attribute Tunnel Types".

303 Note that for tunnel types whose names are of the form "X-in-Y",
304 e.g., "MPLS-in-GRE", only packets of the specified payload type
305 "X" are to be carried through the tunnel of type "Y". This is the
306 equivalent of specifying a tunnel type "Y" and including in its
307 TLV a Protocol Type sub-TLV (see Section 3.4.1) specifying
308 protocol "X". If the tunnel type is "X-in-Y", it is unnecessary,
309 though harmless, to include a Protocol Type sub-TLV specifying
310 "X".

[minor] This is a good paragraph, but it may be more relevant in §3.4.1.

[major] "...only packets of the specified payload type "X" are to be
carried through the tunnel of type "Y"." Is this intended to be a
Normative statement? Note that §3.4.1 says that the "protocol type
sub-TLV...indicate the type of the payload packets that may be
encapsulated with the tunnel parameters that are being signaled in the
TLV." In neither case are rfc2119 keywords used, but given that both
mechanisms are intended to be equivalent, it would be great if the
text was also equivalent.

[major] rfc5512 said that "Unknown types are to be ignored and skipped
upon receipt." Is that still the case?

...
320 +--------------------------------+
321 | Sub-TLV Type (1 Octet) |
322 +--------------------------------+
323 | Sub-TLV Length (1 or 2 Octets) |
324 +--------------------------------+
325 | Sub-TLV Value (Variable) |
326 +--------------------------------+

328 Table 1: Tunnel Encapsulation Sub-TLV Format

[minor] s/Table 1/Figure 2 Please renumber the other Figures.

330 o Sub-TLV Type (1 octet): each sub-TLV type defines a certain
331 property about the tunnel TLV that contains this sub-TLV.

[major] Does this text from rfc5512 apply?

When the TLV is being processed by a BGP speaker that will be
performing encapsulation, any unknown sub-TLVs MUST be ignored and
skipped. However, if the TLV is understood, the entire TLV MUST
NOT be ignored just because it contains an unknown sub-TLV.

This text uses rfc2119 keywords, but the text about unknown TLVs
(above, also from rfc5512) doesn't. If adding the text back in,
please be consistent on the wording.

...
348 3.1. The Tunnel Endpoint Sub-TLV

[major] The Type of this sub-TLV is not specified anywhere. Note that
§12.4 talks about "Tunnel Egress Endpoint"...the names are obviously
inconsistent. :-(

[major] The length of a sub-TLV is specified above, but it would be
ideal if a general statement for *this* sub-TLV was made; something
along the lines of "the length MUST be 6 + length of the AF...".
This would make the specification of the sub-TLV being malformed
easier to do later (and for future extensions).

350 The Tunnel Endpoint sub-TLV specifies the address of the endpoint of
351 the tunnel, that is, the address of the router that will decapsulate
352 the payload. It is a sub-TLV whose value field contains three sub-
353 fields:

[nit] "sub-field" is used here (and in a couple of other places), but
"subfield" is used the most. Please be consistent.

...
359 3. an address sub-field, whose length depends upon the Address
360 Family.

[nit] s/address/Address/g Capitalize to denote the name of the
field, and not simply an address.

362 0 1 2 3
363 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
364 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
365 | Autonomous System Number |
366 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
367 | Address Family | Address ~
368 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +
369 ~ ~
370 | |
371 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

373 Figure 2: Tunnel Endpoint Sub-TLV Value Field

[minor] Please reorder the text below to specify the fields in order.

[suggestion] Some parts of the text talk about a "tunnel endpoint
address", but it may not be obvious to everyone that the Address field
of the Tunnel Endpoint sub-TLV is that "tunnel endpoint address".
Suggestion: rename the field to "Tunnel Endpoint Address".

375 The Address Family subfield contains a value from IANA's "Address
376 Family Numbers" registry. In this document, we assume that the
377 Address Family is either IPv4 or IPv6; use of other address families
378 is outside the scope of this document.

[major] Given that EVPN (AFI/SAFI 25/70) is valid according to §5, and
this document assumes only IPv4/IPv6, does that mean that the tunnel
endpoint cannot be a MAC address, ESI, etc..? That is not specified
anywhere. [Warning: my EVPN is rusty.]

[major] Because there are special considerations depending on the AF,
consider adding something along the lines of: "Documents specifying
the use of other AFs are expected to..."

[major] If only IPv4/IPv6 are in scope, what should a receiver do if
another AF is received? Should the sub-TLV be considered malformed?

380 If the Address Family subfield contains the value for IPv4, the
381 address subfield must contain an IPv4 address (a /32 IPv4 prefix).

[major] s/must/MUST ??

383 In this case, the length field of Tunnel Endpoint sub-TLV must
384 contain the value 10 (0xa).

[major] s/must/MUST ??

386 If the Address Family subfield contains the value for IPv6, the
387 address sub-field must contain an IPv6 address (a /128 IPv6 prefix).
388 In this case, the length field of Tunnel Endpoint sub-TLV must
389 contain the value 22 (0x16). IPv6 link local addresses are not valid
390 values of the IP address field.

[major] s/must/MUST ??

[major] What if a link local address is received?

392 In a given BGP UPDATE, the address family (IPv4 or IPv6) of a Tunnel
393 Endpoint sub-TLV is independent of the address family of the UPDATE
394 itself. For example, an UPDATE whose NLRI is an IPv4 address may
395 have a Tunnel Encapsulation attribute containing Tunnel Endpoint sub-
396 TLVs that contain IPv6 addresses. Also, different tunnels
397 represented in the Tunnel Encapsulation attribute may have Tunnel
398 Endpoints of different address families.

[nit] In this case "Tunnel Endpoints" is used to refer to the Address
field in the sub-TLV, which is described as "the address of the
endpoint of the tunnel" (in the first paragraph of this section).
This section mostly used "address" to refer to the same field, which
is called Address anyway. Please be consistent. I would prefer it if
s/Tunnel Endpoint/address of the endpoint of the tunnel -- if you
rather use "Tunnel Endpoint", then perhaps consider changing the name
of the field.

...
404 The AS number in the sub-TLV MUST be the number of the AS to which
405 the IP address in the sub-TLV belongs.

[major] What if it doesn't? What does "belong" mean? Please put a
forward reference to §13.

[major] Should the UPDATE with the Tunnel Encapsulation Attribute be
originated by the same AS that originated the route to the destination
reflected in the Address field? Should the UPDATE with the Tunnel
Encapsulation Attribute be originated by the same ASN in this sub-TLV?
Are there valid cases where other ASes would/should originate a
Tunnel Encapsulation Attribute used to send traffic somewhere else?

407 There is one special case: the Tunnel Endpoint sub-TLV MAY have a
408 value field whose Address Family subfield contains 0. This means
409 that the tunnel's egress endpoint is the UPDATE's BGP next hop. If
410 the Address Family subfield contains 0, the Address subfield is
411 omitted, and the Autonomous System number field is set to 0.

[major] rfc4271/rfc4760 consistency: s/UPDATE's BGP next hop/address
of the next hop

[minor] To keep consistent with the text above, please specify the length (6).

[major] Why isn't the ASN kept to indicate the AS to which the address
in the NEXT_HOP belongs to? I ask mostly because the specification
above about the ASN says that it "MUST be the number of the AS...",
which implies that there are no exceptions. If at this point
(assuming implementations) the ASN has to be 0, then please use
"SHOULD" in the initial specification.

413 If any of the following conditions hold, the Tunnel Endpoint sub-TLV
414 is considered to be "malformed":

416 o The sub-TLV contains the value for IPv4 in its Address Family
417 subfield, but the length of the sub-TLV's value field is other
418 than 10 (0xa).

420 o The sub-TLV contains the value for IPv6 in its Address Family
421 subfield, but the length of the sub-TLV's value field is other
422 than 22 (0x16).

424 o The sub-TLV contains the value zero in its Address Family field,
425 but the length of the sub-TLV's value field is other than 6, or
426 the Autonomous System subfield is not set to zero.

[minor] Is there any way to generalize these conditions? If general,
then we don't need to worry about future AFs clearly specifying what a
malformed sub-TLV is.

Suggestion>
...the Length of the sub-TLV is not 6 + the length of the AF...as specified
above.

428 o The IP address in the sub-TLV's address subfield is not a valid IP
429 address (e.g., it's an IPv4 broadcast address).

[major] What is a "valid IP address"? The text above says that
link-local are not allowed...and that a host address is expected...and
that the address must "belong"...now you added broadcast addresses.
Anything else? Is a multicast destination "valid"? Is there a
reference that can be added here to avoid having to define it?

431 o It can be determined that the IP address in the sub-TLV's address
432 subfield does not belong to the non-zero AS whose number is in the
433 its Autonomous System subfield. (See section Section 13 for
434 discussion of one way to determine this.)

[major] "one way" I hope that it is the MTI way -- otherwise, the
determination of the sub-TLV being malformed is not deterministic.

436 If the Tunnel Endpoint sub-TLV is malformed, the TLV containing it is
437 also considered to be malformed, and the entire TLV MUST be ignored.
438 However, the Tunnel Encapsulation attribute MUST NOT be considered to
439 be malformed in this case; other TLVs in the attribute MUST be
440 processed (if they can be parsed correctly).

[major] "sub-TLV is malformed, the TLV containing it is also
considered to be malformed, and the entire TLV MUST be ignored." §11
adds: "the entire TLV MUST be ignored, and MUST be removed from the
Tunnel Encapsulation attribute". Please specify things only once!
Instead of specifying what to do in this section, put a reference to
§11 and take care of it there.

442 When redistributing a route that is carrying a Tunnel Encapsulation
443 attribute containing a TLV that itself contains a malformed Tunnel
444 Endpoint sub-TLV, the TLV MUST be removed from the attribute before
445 redistribution.

[minor] This paragraph seems to say the same thing as §11, but please
take it out and specify things only once.

[nit] rfc4271 consistency s/redistribute/distribute/g

447 See Section 11 for further discussion of how to handle errors that
448 are encountered when parsing the Tunnel Encapsulation attribute.

NEW>
Error Handling is detailed in Section 11.

...
454 3.2. Encapsulation Sub-TLVs for Particular Tunnel Types

456 This section defines Tunnel Encapsulation sub-TLVs for the following
457 tunnel types: VXLAN ([RFC7348]), VXLAN-GPE
458 ([I-D.ietf-nvo3-vxlan-gpe]), NVGRE ([RFC7637]), MPLS-in-GRE
459 ([RFC2784], [RFC2890], [RFC4023]), L2TPv3 ([RFC3931]), and GRE
460 ([RFC2784], [RFC2890], [RFC4023]).

[minor] The sub-TLV is called "Tunnel Encapsulation" here, but simply
"Encapsulation" almost everywhere else. Please be consistent.

[minor] It looks like the only reference for MPLS-in-GRE should be
rfc4023...which should then not be a reference for GRE.

...
465 There are also tunnel types for which it is not necessary to define
466 an Encapsulation sub-TLV, because there are no fields in the
467 encapsulation header whose values need to be signaled from the tunnel
468 egress endpoint.

[minor] If I'm following...all Encapsulation sub-TLVs have the same
Type; the indication of the encapsulation itself is indicated by the
Tunnel Type of the TLV, right? Please remind people at this point of
that fact.

[major] The Type of this sub-TLV is not specified anywhere.

470 3.2.1. VXLAN

[major] The Length of this version of the sub-TLV is fixed, please
indicate so in this section.

472 This document defines an encapsulation sub-TLV for VXLAN tunnels.
473 When the tunnel type is VXLAN, the following is the structure of the
474 value field in the encapsulation sub-TLV:

[nit] "When the tunnel type is VXLAN..." It would be nice to remind
the reader what the value is. If you add the value here, please do so
in the other sections as well.

476 0 1 2 3
477 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
478 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
479 |V|M|R|R|R|R|R|R| VN-ID (3 Octets) |
480 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
481 | MAC Address (4 Octets) |
482 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
483 | MAC Address (2 Octets) | Reserved |
484 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

486 Figure 3: VXLAN Encapsulation Sub-TLV

488 V: This bit is set to 1 to indicate that a "valid" VN-ID (Virtual
489 Network Identifier) is present in the encapsulation sub-TLV.
490 Please see Section 8.

[minor] The use of "valid" (specially as above in quotations) is
misleading/confusing as the validity (as in the VN-ID being correct,
etc.) is not guaranteed. It seems to me that, at best, this bit is
simply indicating the fact that the sender intended to include a VN-ID
(vs some random bits).

492 M: This bit is set to 1 to indicate that a valid MAC Address is
493 present in the encapsulation sub-TLV.

[minor] The use of "valid" is misleading/confusing as the validity (as
in the MAC address being correct, or even existing!) is not
guaranteed. It seems to me that, at best, this bit is simply
indicating the fact that the sender intended to include a MAC address
(vs some random bits).

[major] It is not clear to me whether setting both the V and M bits is
a valid configuration.

...
501 VN-ID: If the V bit is set, the VN-id field contains a 3 octet VN-
502 ID value. If the V bit is not set, the VN-id field MUST be set to
503 zero.

[nit] s/VN-id/VN-ID/g

[minor] "MUST be set to zero." To avoid errors, add "...and ignored
by the receiver."

505 MAC Address: If the M bit is set, this field contains a 6 octet
506 Ethernet MAC address. If the M bit is not set, this field MUST be
507 set to all zeroes.

[minor] "MUST be set to all zeroes." To avoid errors, add "...and
ignored by the receiver."

...
529 o See Section 8 to see how the VNI field of the VXLAN encapsulation
530 header is set.

[minor] Suggestion>
Section 8 describes how the VNI field of the VXLAN encapsulation header is
set.

...
538 3.2.2. VXLAN-GPE

[major] The Length of this version of the sub-TLV is fixed, please
indicate so in this section.

540 This document defines an encapsulation sub-TLV for VXLAN tunnels.
541 When the tunnel type is VXLAN-GPE, the following is the structure of
542 the value field in the encapsulation sub-TLV:

[minor] s/VXLAN tunnels/VXLAN-GPE tunnels

544 0 1 2 3
545 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
546 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
547 |Ver|V|R|R|R|R|R| Reserved |
548 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
549 | VN-ID | Reserved |
550 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

552 Figure 4: VXLAN GPE Encapsulation Sub-TLV

[minor] Most of the document uses "VXLAN-GPE". Please be consistent
with the form used in I-D.ietf-nvo3-vxlan-gpe.

[minor] Please explain the content of the fields in order.

554 V: This bit is set to 1 to indicate that a "valid" VN-ID is
555 present in the encapsulation sub-TLV. Please see Section 8.

...
570 VN-ID: If the V bit is set, this field contains a 3 octet VN-ID
571 value. If the V bit is not set, this field MUST be set to zero.

[minor] "MUST be set to zero." To avoid errors, add "...and ignored
by the receiver."

[major] The Reserved field is not described.

...
580 o See Section 8 to see how the VNI field of the VXLAN-GPE
581 encapsulation header is set.

[minor] Suggestion>
Section 8 describes how the VNI field of the VXLAN-GPE encapsulation header
is set.

583 3.2.3. NVGRE

[major] The Length of this version of the sub-TLV is fixed, please
indicate so in this section.

...
589 0 1 2 3
590 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
591 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
592 |V|M|R|R|R|R|R|R| VN-ID (3 Octets) |
593 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
594 | MAC Address (4 Octets) |
595 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
596 | MAC Address (2 Octets) | Reserved |
597 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

599 Figure 5: NVGRE Encapsulation Sub-TLV

601 V: This bit is set to 1 to indicate that a "valid" VN-ID is
602 present in the encapsulation sub-TLV. Please see Section 8.

604 M: This bit is set to 1 to indicate that a valid MAC Address is
605 present in the encapsulation sub-TLV.

[major] It is not clear to me whether setting both the V and M bits is
a valid configuration.

...
613 VN-ID: If the V bit is set, the VN-id field contains a 3 octet VN-
614 ID value. If the V bit is not set, the VN-id field MUST be set to
615 zero.

[minor] "MUST be set to zero." To avoid errors, add "...and ignored
by the receiver."

617 MAC Address: If the M bit is set, this field contains a 6 octet
618 Ethernet MAC address. If the M bit is not set, this field MUST be
619 set to all zeroes.

[minor] "MUST be set to all zeroes." To avoid errors, add "...and
ignored by the receiver."

...
641 o See Section 8 to see how the VSID (Virtual Subnet Identifier)
642 field of the NVGRE encapsulation header is set.

[minor] Suggestion>
Section 8 describes how the VSID (Virtual Subnet Identifier) field of the
NVGRE encapsulation header is set.

644 3.2.4. L2TPv3

[major] The Length of this version of the sub-TLV is not fixed but
known (4-12), please indicate so in this section.

...
671 The length of the cookie is not encoded explicitly, but can be
672 calculated as (sub-TLV length - 4).

674 3.2.5. GRE

[major] The Length of this version of the sub-TLV is fixed, please
indicate so in this section.

...
687 GRE Key: 4-octet field [RFC2890] that is generated by the
688 advertising router. The actual method by which the key is
689 obtained is beyond the scope of this document. The key is
690 inserted into the GRE encapsulation header of the payload packets
691 sent by ingress routers to the advertising router. It is intended
692 to be used for identifying extra context information about the
693 received payload.

[minor] "The actual method by which the key is obtained is beyond the
scope of this document." That is a true statement, but I think that
the right thing to do here is to simply point at rfc2890 and leave the
burden there (I know that document says that it is out of scope too).

...
698 3.2.6. MPLS-in-GRE

[major] The Length of this version of the sub-TLV is fixed, please
indicate so in this section.

...
711 GRE-Key: 4-octet field [RFC2890] that is generated by the
712 advertising router. The actual method by which the key is
713 obtained is beyond the scope of this document. The key is
714 inserted into the GRE encapsulation header of the payload packets
715 sent by ingress routers to the advertising router. It is intended
716 to be used for identifying extra context information about the
717 received payload. Note that the key is optional. Unless a key
718 value is being advertised, the MPLS-in-GRE encapsulation sub-TLV
719 MUST NOT be present.

721 Note that the GRE tunnel type defined in Section 3.2.5 can be used
722 instead of the MPLS-in-GRE tunnel type when it is necessary to
723 encapsulate MPLS in GRE. Including a TLV of the MPLS-in-GRE tunnel
724 type is equivalent to including a TLV of the GRE tunnel type that
725 also includes a Protocol Type sub-TLV (Section 3.4.1) specifying MPLS
726 as the protocol to be encapsulated. That is, if a TLV specifies
727 MPLS-in-GRE or if it includes a Protocol Type sub-TLV specifying
728 MPLS, the GRE tunnel advertised in that TLV MUST NOT be used for
729 carrying IP packets.

[major] "...if it includes a Protocol Type sub-TLV specifying MPLS,
the GRE tunnel...MUST NOT be used for carrying IP packets." The
example in §3.4.1 is along the same lines as this phrase (3 TLVs, 1
Protocol Type sub-TLV in each, 3 tunnels); but §11 says that this
sub-TLV "may appear zero or more times in a given Tunnel TLV". IOW, a
second Protocol Type sub-TLV could indicate IP. Which one is correct?

...
735 3.2.7. IP-in-IP

737 When the tunnel type of the TLV is IP-in-IP, it does not have Virtual
738 Network Identifier. See for Section 8.1 Embedded Label handling on
739 IP-in-IP tunnels.

[nit] s/have Virtual Network Identifier/have a Virtual Network Identifier

[] This sub-section seems to be out of place: §3.2 defines the Tunnel
Encapsulation sub-TLVs, but the IP-in-IP encapsulation doesn't seem to
need one (?). Also, I'm not sure what the paragraph is about
anyway...

741 3.3. Outer Encapsulation Sub-TLVs
...
753 If an outer encapsulation sub-TLV occurs in a TLV for a tunnel type
754 that does not use the corresponding outer encapsulation, the sub-TLV
755 is treated as if it were an unknown type of sub-TLV.

[major] Should this behavior be Normative?

757 3.3.1. IPv4 DS Field

759 Most of the tunnel types that can be specified in the Tunnel
760 Encapsulation attribute require an outer IP encapsulation. The IPv4
761 Differentiated Services (DS) Field sub-TLV can be carried in the TLV
762 of any such tunnel type. It specifies the setting of the one-octet
763 Differentiated Services field in the outer IP encapsulation (see
764 [RFC2474]). The value field is always a single octet.

[major] What is the type??

[major] What about IPv6? Can this same sub-TLV be used in the IPv6
case? It would need to be renamed... Note that rfc2474 is applicable
to both. [This point is bound to be noticed by other IESG members.]

[] Note also that draft-ietf-ospf-encapsulation-cap, which specifies
the same functionality as this document for OSPF (both OSPFv2 and
OSPFv3), relies heavily on the sub-TLVs defined here, including this
one...which seems to be expected to be used for both IPv4/IPv6
endpoints.

766 3.3.2. UDP Destination Port

768 Some of the tunnel types that can be specified in the Tunnel
769 Encapsulation attribute require an outer UDP encapsulation.
770 Generally there is a standard UDP Destination Port value for a
771 particular tunnel type. However, sometimes it is useful to be able
772 to use a non-standard UDP destination port. If a particular tunnel
773 type requires an outer UDP encapsulation, and it is desired to use a
774 UDP destination port other than the standard one, the port to be used
775 can be specified by including a UDP Destination Port sub-TLV. The
776 value field of this sub-TLV is always a two-octet field, containing
777 the port value.

[major] What is the type??

...
781 3.4.1. Protocol Type Sub-TLV

[major] What is the type??

783 The protocol type sub-TLV MAY be included in a given TLV to indicate
784 the type of the payload packets that may be encapsulated with the
785 tunnel parameters that are being signaled in the TLV. The value
786 field of the sub-TLV contains a 2-octet value from IANA's ethertype
787 registry [Ethertypes].

[minor] The name of the registry is "ETHER TYPES" (written in all caps).

...
801 3.4.2. Color Sub-TLV

[major] What is the type??

803 The color sub-TLV MAY be encoded as a way to "color" the
804 corresponding tunnel TLV. The value field of the sub-TLV is eight
805 octets long, and consists of a Color Extended Community, as defined
806 in Section 4.3. For the use of this sub-TLV and Extended Community,
807 please see Section 7.

[major] s/color sub-TLV MAY be encoded/color sub-TLV MAY be used

809 Note that the high-order octet of this sub-TLV's value field MUST be
810 set to 3, and the next octet MUST be set to 0x0b. (Otherwise the
811 value field is not identical to a Color Extended Community.)

[minor] The first paragraph already says that the "value field of the
sub-TLV is eight octets long, and consists of a Color Extended
Community"; it is not necessary to repeat the value of the first two
octets because they area defined in §4.3.

813 If a Color sub-TLV is not of the proper length, or the first two
814 octets of its value field are not 0x030b, the sub-TLV should be
815 treated as if it were an unrecognized sub-TLV (see Section 11).

[minor] "is not of the proper length" Do you mean that the Length
field of the sub-TLV and the actual length of the Value fields don't
match, or that the Length field is set to anything else besides 8?
Please be specific.

[major] In either case, do you trust the Length field, or how do you
know where the sub-TLV ends so it can be ignored? See the comments in
§11.

817 3.5. Embedded Label Handling Sub-TLV
...
831 Suppose a Tunnel Encapsulation attribute is attached to an UPDATE of
832 an embedded address family, and it is decided to use a particular
833 tunnel (specified in one of the attribute's TLVs) for transmitting a
834 packet that is being forwarded according to that UPDATE. When
835 forming the encapsulation header for that packet, different
836 deployment scenarios require different handling of the embedded label
837 and/or the virtual network identifier. The Embedded Label Handling
838 sub-TLV can be used to control the placement of the embedded label
839 and/or the virtual network identifier in the encapsulation.

[minor] s/embedded address family/labeled address family

841 The Embedded Label Handling sub-TLV may be included in any TLV of the
842 Tunnel Encapsulation attribute. If the Tunnel Encapsulation
843 attribute is attached to an UPDATE of a non-labeled address family,
844 the sub-TLV is treated as a no-op. If the sub-TLV is contained in a
845 TLV whose tunnel type does not have a virtual network identifier in
846 its encapsulation header, the sub-TLV is treated as a no-op. In
847 those cases where the sub-TLV is treated as a no-op, it SHOULD NOT be
848 stripped from the TLV before the UPDATE is forwarded.

[minor] s/treated as a no-op/ignored/g (It may be clearer.)

[major] What is the type?

...
864 3.6. MPLS Label Stack Sub-TLV

866 This sub-TLV allows an MPLS label stack ([RFC3032]) to be associated
867 with a particular tunnel.

[major] What is the type?

869 The value field of this sub-TLV is a sequence of MPLS label stack
870 entries. The first entry in the sequence is the "topmost" label, the
871 final entry in the sequence is the "bottommost" label. When this
872 label stack is pushed onto a packet, this ordering MUST be preserved.

[minor] "topmost" and "bottommost" are not mentioned in rfc3032.
Please be consistent with the terminology. It is ok to introduce new
terminology, if needed, but please explain that as well.

874 Each label stack entry has the following format:

876 0 1 2 3
877 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
878 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
879 | Label | TC |S| TTL |
880 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

882 Figure 9: MPLS Label Stack Sub-TLV

[major] The fields of the sub-TLV are not explained...but the text
below simply starts talking about them. I realize these fields are
described elsewhere. Please include text pointing to where the fields
are defined. Note that rfc3032/rfc5462 are used below, but it seems
to me that the definition itself is done in rfc3270/rfc5129. These
references should be Normative.

884 If a packet is to be sent through the tunnel identified in a
885 particular TLV, and if that TLV contains an MPLS Label Stack sub-TLV,
886 then the label stack appearing in the sub-TLV MUST be pushed onto the
887 packet. This label stack MUST be pushed onto the packet before any
888 other labels are pushed onto the packet.

[minor] (redundant text) s/the label stack appearing in the sub-TLV
MUST be pushed onto the packet. This label stack MUST be pushed onto
the packet before any other labels are pushed onto the packet./the
label stack appearing in the sub-TLV MUST be pushed onto the packet
before any other labels are pushed onto the packet.

...
895 If the MPLS label stack sub-TLV is included in a TLV identifying a
896 tunnel type that uses virtual network identifiers (see Section 8),
897 the contents of the MPLS label stack sub-TLV MUST be pushed onto the
898 packet before the procedures of Section 8 are applied.

[nit] s/MPLS label stack sub-TLV/MPLS Label Stack sub-TLV/g

900 The number of label stack entries in the sub-TLV MUST be determined
901 from the sub-TLV length field. Thus it is not necessary to set the S
902 bit in any of the label stack entries of the sub-TLV, and the setting
903 of the S bit is ignored when parsing the sub-TLV. When the label
904 stack entries are pushed onto a packet that already has a label
905 stack, the S bits of all the entries MUST be cleared. When the label
906 stack entries are pushed onto a packet that does not already have a
907 label stack, the S bit of the bottommost label stack entry MUST be
908 set, and the S bit of all the other label stack entries MUST be
909 cleared.

[minor] This paragraph would ideally follow the description of the S bit.

[major] "When the label stack entries are pushed onto a packet that
already has a label stack, the S bits of all the entries MUST be
cleared." I'm assuming you mean all entries from the sub-TLV, and not
all entries in the resulting stack, right? I think that the text as
is could be misinterpreted.

911 By default, the TC (Traffic Class) field ([RFC3032], [RFC5462]) of
912 each label stack entry is set to 0. This may of course be changed by
913 policy at the originator of the sub-TLV. When pushing the label
914 stack onto a packet, the TC of the label stack entries is preserved
915 by default. However, local policy at the router that is pushing on
916 the stack MAY cause modification of the TC values.

[major] "([RFC3032], [RFC5462])" I think the correct references may
be rfc3270/rfc5129, and should be Normative.

[] While it is ok to not use rfc2119 keywords, the single use of "MAY"
to indicate an option without similar language for the rest isn't
great, specially starting with "by default". Consider something like
this:

Suggestion>
The TC (Traffic Class) field of each label stack entry SHOULD be set to 0,
unless changed by policy at the originator of the sub-TLV. When pushing the
label stack onto a packet, the TC of each label stack SHOULD be preserved,
unless local policy results in a modification.

918 By default, the TTL (Time to Live) field of each label stack entry is
919 set to 255. This may be changed by policy at the originator of the
920 sub-TLV. When pushing the label stack onto a packet, the TTL of the
921 label stack entries is preserved by default. However, local policy
922 at the router that is pushing on the stack MAY cause modification of
923 the TTL values. If any label stack entry in the sub-TLV has a TTL
924 value of zero, the router that is pushing the stack on a packet MUST
925 change the value to a non-zero value.

[] by default...

Suggestion>
The TTL (Time to Live) field of each label stack entry SHOULD be set to 255,
unless changed by policy at the originator of the sub-TLV. When pushing the
label stack onto a packet, the TTL of each label stack entry SHOULD be
preserved, unless local policy results in a modification. If any label
stack entry has a TTL value of zero, the router that is pushing the stack on
a packet MUST change the value to a non-zero value.

[major] "If any label stack entry in the sub-TLV has a TTL value of
zero, the router that is pushing the stack on a packet MUST change the
value to a non-zero value." From the rest of the paragraph, it could
be concluded that the value SHOULD be 255 -- is that a proper
conclusion? Given that local policy can change the value, is setting
the TTL to 0 a valid policy?

...
934 3.7. Prefix-SID Sub-TLV
...
942 In this document, we define a Prefix-SID sub-TLV. The value field of
943 the Prefix-SID sub-TLV can be set to any valid value of the value
944 field of a BGP Prefix-SID attribute, as defined in
945 [I-D.ietf-idr-bgp-prefix-sid].

[major] "set to any valid value of the value field of a BGP Prefix-SID
attribute" I read this phrase to mean that the contents of the
sub-TLV are only valid in the same cases where the BGP Prefix-SID
attribute would be. Is that the intent? If so, rfc8669 says that the
"BGP Prefix-SID attribute...can be attached to prefixes from
Multiprotocol BGP IPv4/IPv6 Labeled Unicast...other Address Family
Identifier (AFI) / Subsequent Address Family Identifier (SAFI)
combinations is not defined herein but may be specified in future
specifications", while this document mentions other AFI/SAFI
combinations (in §5) as allowed to carry the BGP Tunnel Encapsulation
attribute, and doesn't limit where this sub-TLV can exist (see the
first sentence in the next paragraph)... If the Prefix-SID Sub-TLV is
included in any AFI/SAFI other than IPv4/IPv6 Labeled Unicast, what
action should the receiver take?

947 The Prefix-SID sub-TLV can occur in a TLV identifying any type of
948 tunnel. If an Originator SRGB is specified in the sub-TLV, that SRGB
949 MUST be interpreted to be the SRGB used by the tunnel's egress
950 endpoint. The Label-Index, if present, is the Segment Routing SID
951 that the tunnel's egress endpoint uses to represent the prefix
952 appearing in the NLRI field of the BGP UPDATE to which the Tunnel
953 Encapsulation attribute is attached.

[major] Except for the first sentence, this paragraph says the same
thing as rfc8669. Please don't re-specify the behavior here (unless
it changes, of course); instead, point at the source.

955 If a Label-Index is present in the prefix-SID sub-TLV, then when a
956 packet is sent through the tunnel identified by the TLV, the
957 corresponding MPLS label MUST be pushed on the packet's label stack.
958 The corresponding MPLS label is computed from the Label-Index value
959 and the SRGB of the route's originator.

[minor] Please point at rfc8669 as the place where the computation of
the MPLS label is specified.

961 If the Originator SRGB is not present, it is assumed that the
962 originator's SRGB is known by other means. Such "other means" are
963 outside the scope of this document.

[minor] This paragraph is part of what rfc8669 already specifies ("MAY
contain the Originator SRGB TLV"), so it is not needed.

965 The corresponding MPLS label is pushed on after the processing of the
966 MPLS Label Stack sub-TLV, if present, as specified in Section 3.6.
967 It is pushed on before any other labels (e.g., a label embedded in
968 UPDATE's NLRI, or a label determined by the procedures of Section 8
969 are pushed on the stack.

[minor] It would be very nice to have an example of the most complex case.

971 The Prefix-SID sub-TLV has slightly different semantics than the
972 Prefix-SID attribute. When the Prefix-SID attribute is attached to a
973 given route, the BGP speaker that originally attached the attribute
974 is expected to be in the same Segment Routing domain as the BGP
975 speakers who receive the route with the attached attribute. The
976 Label-Index tells the receiving BGP speakers that the prefix-SID is
977 for the advertised prefix in that Segment Routing domain. When the
978 Prefix-SID sub-TLV is used, the BGP speaker at the head end of the
979 tunnel need even not be in the same Segment Routing Domain as the
980 tunnel's egress endpoint, and there is no implication that the
981 prefix-SID for the advertised prefix is the same in the Segment
982 Routing domains of the BGP speaker that originated the sub-TLV and
983 the BGP speaker that received it.

[minor] s/that the prefix-SID/what the prefix-SID

[nit] s/the BGP speaker at the head end of the tunnel/the receiving BGP speaker
This is the only place where "head end" is used.

985 4. Extended Communities Related to the Tunnel Encapsulation Attribute

987 4.1. Encapsulation Extended Community

[major] Please indicate the Type/sub-type, format, etc. Keep in mind
that this document is obsoleting rfc5512, so even if this community
was defined there, it needs to be fully specified here as well.

989 The Encapsulation Extended Community is a Transitive Opaque Extended
990 Community. This Extended Community may be attached to a route of any
991 AFI/SAFI to which the Tunnel Encapsulation attribute may be attached.
992 Each such Extended Community identifies a particular tunnel type. If
993 the Encapsulation Extended Community identifies a particular tunnel
994 type, its semantics are exactly equivalent to the semantics of a
995 Tunnel Encapsulation attribute Tunnel TLV for which the following
996 three conditions all hold:

[minor] (redundant text) s/Each such Extended Community identifies a
particular tunnel type. If the Encapsulation Extended Community
identifies a particular tunnel type,/Each such Extended Community
identifies a particular tunnel type,

[nit] "exactly equivalent" sounds redundant s/its semantics are
exactly equivalent to/its semantics are the same as

998 1. it identifies the same tunnel type,

[nit] s/tunnel type/Tunnel Type

...
1004 B. its "Address" subfield contains the same IP address that
1005 appears in the next hop field of the route to which the
1006 Tunnel Encapsulation attribute is attached

[major] rfc4271/rfc4760 consistency: s/same IP address that appears in
the next hop field/address of the next hop

1008 3. it has no other sub-TLVs.

1010 We will refer to such a Tunnel TLV as a "barebones" Tunnel TLV.

1012 The Encapsulation Extended Community was first defined in [RFC5512].
1013 While it provides only a small subset of the functionality of the
1014 Tunnel Encapsulation attribute, it is used in a number of deployed
1015 applications, and is still needed for backwards compatibility. To
1016 ensure backwards compatibility, this specification establishes the
1017 following rules:

1019 1. If the Tunnel Encapsulation attribute of a given route contains a
1020 barebones Tunnel TLV identifying a particular tunnel type, an
1021 Encapsulation Extended Community identifying the same tunnel type
1022 SHOULD be attached to the route.

1024 2. If the Encapsulation Extended Community identifying a particular
1025 tunnel type is attached to a given route, the corresponding
1026 barebones Tunnel TLV MAY be omitted from the Tunnel Encapsulation
1027 attribute.

1029 3. Suppose a particular route has both (a) an Encapsulation Extended
1030 Community specifying a particular tunnel type, and (b) a Tunnel
1031 Encapsulation attribute with a barebones Tunnel TLV specifying
1032 that same tunnel type. Both (a) and (b) MUST be interpreted as
1033 denoting the same tunnel.

[major] "MUST be interpreted" First of all, "interpreted" is not
something that can be Normatively enforced. Second, what does that
mean? I guess that the intent is that both community/TLV are expected
to represent the same tunnel. Is that it? If so, then it seems like
an action that the sender should ensure -- what should the receiver do
if it is not the case? The next paragraph says that the community is
preferred...so maybe that should be "3." and the text above should be
taken out.

1035 In short, in situations where one could use either the Encapsulation
1036 Extended Community or a barebones Tunnel TLV, one may use either or
1037 both. However, to ensure backwards compatibility with applications
1038 that do not support the Tunnel Encapsulation attribute, it is
1039 preferable to use the Encapsulation Extended Community. If the
1040 Extended Community (identifying a particular tunnel type) is present,
1041 the corresponding Tunnel TLV is optional.

[major] "However, to ensure backwards compatibility with applications
that do not support the Tunnel Encapsulation attribute, it is
preferable to use the Encapsulation Extended Community." I'm not sure
if that text is referring only to the barebones Tunnel TLV, or the
general case.

(1) This is one of those cases where using Normative language would be
clearer. Perhaps:

To ensure backwards compatibility with implementations that do not support
the Tunnel Encapsulation attribute, it is RECOMMENDED to use the
Encapsulation Extended Community.

This text is, I think, weak because the door is open to not use the
Extended Community. Should the "SHOULD/RECOMMENDED" be replaced by
"MUST/REQUIRED"?

(2) This section talks about the case where the barebones Tunnel TLV
and the Encapsulation Extended Community are the same. What about the
case where they are not? Should using the Tunnel TLV be preferred?
What about interoperability?

...
1054 4.2. Router's MAC Extended Community

1056 [I-D.ietf-bess-evpn-inter-subnet-forwarding] defines a Router's MAC
1057 Extended Community. This Extended Community provides information
1058 that may conflict with information in one or more of the
1059 Encapsulation Sub-TLVs of a Tunnel Encapsulation attribute. In case
1060 of such a conflict, the information in the Encapsulation Sub-TLV
1061 takes precedence.

[major] You're going to have to be more specific! Please explicitly
point out which is the "information that may conflict", and how the
conflict can happen.

[major] It seems to me that the last sentence may be ideal for
Normative language. For instance: "In case of such a conflict, the
information in the Encapsulation Sub-TLV MUST be used."

[] These are comments for both this document and
draft-ietf-bess-evpn-inter-subnet-forwarding -- I'm putting them here
because I'm reading this document (and have only skimmed
draft-ietf-bess-evpn-inter-subnet-forwarding), but to not forget them
later...

(1) It would be ideal if this document talked about the result of
using the Router's MAC Extended Community in the context of this
specification. IOW, if an Encapsulation sub-TLV is not present, then
is the result equivalent to including one? At first glance, it looks
like what is specified in this document and what is described in
§6.1.1 (draft-ietf-bess-evpn-inter-subnet-forwarding) is not
consistent.

(2) §6.1 (draft-ietf-bess-evpn-inter-subnet-forwarding) talks about
GENEVE, but this document doesn't, and there's no Tunnel Type for it
assigned... I'm not suggesting you need to cover GENEVE, just that
there seems to be an expectation in
draft-ietf-bess-evpn-inter-subnet-forwarding that cannot be satisfied.

(3) This may be a chicken-and-egg problem...
draft-ietf-bess-evpn-inter-subnet-forwarding talks about the use of
the Router's MAC Extended Community without considering cases when it
may be ignored (according to this section), or potentially other ways
of doing the same thing (using Encapsulation sub-TLVs). OTOH, this
section specifies not using the Router's MAC Extended Community
without accounting to the effect, or offering alternate mechanisms.
It seems to me that one of the two documents should maybe Update the
other, or at least consider the rest of the specification -- at first
glance, this document could serve as the base...

1063 4.3. Color Extended Community
...
1068 0 1 2 3
1069 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
1070 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1071 | 0x03 | 0x0b | Reserved |
1072 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
1073 | Color Value |
1074 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

1076 Figure 10: Color Extended Community

[minor] Please add some text related to how the Reserved field should
be handled: "MUST be set to zero by the sender and ignored by the
receiver", or something like that.

[major] Please ask IANA to create a registry for the Reserved bits.
Please take a look at this note:
https://mailarchive.ietf.org/arch/msg/idr/-45ujNcRzWFWTRnBL1NBcbPoKgo

[] Related: I haven't asked for other registries related to Reserved
fields. It might be a good idea to at least consider the likelihood
of potential requests/assignments and create registries where
appropriate.

[major] Please specify what the Color Value is. Note that "color
value" is not used anywhere else in the text...not even §7.

1078 For the use of this Extended Community please see Section 7.

1080 5. Semantics and Usage of the Tunnel Encapsulation attribute

1082 [RFC5512] specifies the use of the Tunnel Encapsulation attribute in
1083 BGP UPDATE messages of AFI/SAFI 1/7 and 2/7. That document restricts
1084 the use of this attribute to UPDATE messsages of those SAFIs. This
1085 document removes that restriction.

[nit] s/messsages/messages

...
1095 It has been suggested that it may sometimes be useful to attach a
1096 Tunnel Encapsulation attribute to a BGP UPDATE message that is also
1097 carrying a PMSI (Provider Multicast Service Interface) Tunnel
1098 attribute [RFC6514]. If the PMSI Tunnel attribute specifies an IP
1099 tunnel, the Tunnel Encapsulation attribute could be used to provide
1100 additional information about the IP tunnel. The usage of the Tunnel
1101 Encapsulation attribute in combination with the PMSI Tunnel attribute
1102 is outside the scope of this document.

[minor] It seems to me that identifying this relationship as out of
scope is not necessary: anything that is not specified in this
document is, by definition, out of scope. If not removed, then this
paragraph really doesn't provide useful information because it is
based on a suggestion, and it even provides a potential use, but then
it declares the use out of scope...why even mention it?

...
1108 When the Tunnel Encapsulation attribute is carried in an UPDATE of
1109 one of the AFI/SAFIs specified in the previous paragraph, each TLV
1110 MUST have a Tunnel Endpoint sub-TLV. If a TLV that does not have a
1111 Tunnel Endpoint sub-TLV, that TLV should be treated as if it had a
1112 malformed Tunnel Endpoint sub-TLV (see Section 3.1).

[nit] s/in the previous paragraph/above

[major] "each TLV MUST have a Tunnel Endpoint sub-TLV" Is there any
reason not to specify the sub-TLV as mandatory when it is defined
(§3.1)? I think that would be a better place.

...
1130 * The tunnel is of a type that can be used to carry packet P
1131 (e.g., an MPLS-in-UDP tunnel would not be a feasible tunnel for
1132 carrying an IP packet, UNLESS the IP packet can first be
1133 converted to an MPLS packet).

[nit] s/converted to/encapsulated in

1135 * The tunnel is specified in a TLV whose Tunnel Endpoint sub-TLV
1136 identifies an IP address that is reachable.

[major] How is reachability determined? Where (which table) should
the address be looked up in? In the sequence above, the destination
address of P and the address of the endpoint may be resolvable in
different tables...

[BTW, please also take a look at
draft-ietf-idr-bgp-bestpath-selection-criteria, which I think tries to
define a related, if not the same, concept.]

...
1141 If the Tunnel Encapsulation attribute contains several TLVs (i.e., if
1142 it specifies several tunnels), router R may choose any one of those
1143 tunnels, based upon local policy. If any tunnel TLV contains one or
1144 more Color sub-TLVs (Section 3.4.2) and/or the Protocol Type sub-TLV
1145 (Section 3.4.1), the choice of tunnel may be influenced by these sub-
1146 TLVs.

[minor] s/several tunnels/several feasible tunnels

1148 If a particular tunnel is not feasible at some moment because its
1149 Tunnel Endpoint cannot be reached at that moment, the tunnel may
1150 become feasible at a later time (when its endpoint becomes
1151 reachable). Router R should take note of this. If router R is
1152 already using a different tunnel, it MAY switch to the tunnel that
1153 just became feasible, or it MAY decide to continue using the tunnel
1154 that it is already using. How this decision is made is outside the
1155 scope of this document.

[minor] "Router R should take note of this." I'm not sure what the
router is expected to do from that sentence...also, the use of a
Normative "MAY" seems out of place since the behavior is our of scope
anyway.

Suggestion>
Reachability to the address of the endpoint of the tunnel may change over
time, directly impacting the feasibility of the tunnel. A router may start
using a newly feasible tunnel instead of an existing one. How this decision
is made is outside the scope of this document.

1157 In addition to the sub-TLVs already defined, additional sub-TLVs may
1158 be defined that affect the choice of tunnel to be used, or that
1159 affect the contents of the tunnel encapsulation header. The
1160 documents that define any such additional sub-TLVs must specify the
1161 effect that including the sub-TLV is to have.

[minor] This paragraph sounds out of place. Maybe it fits better in §3.

[major] "must specify the effect that including the sub-TLV is to have"

(1) Should this phrase be Normative?

(2) "the effect" with respect to what? It seems to me that defining a
new sub-TLV to have a specific role in the selection of which tunnel
to use already would include its effect (in that selection). IOW, I
don't think this paragraph is needed.

1163 Once it is determined to send a packet through the tunnel specified
1164 in a particular TLV of a particular Tunnel Encapsulation attribute,
1165 then the tunnel's egress endpoint address is the IP address contained
1166 in the sub-TLV. If the TLV contains a Tunnel Endpoint sub-TLV whose
1167 value field is all zeroes, then the tunnel's egress endpoint is the
1168 IP address specified as the Next Hop of the BGP Update containing the
1169 Tunnel Encapsulation attribute. The address of the tunnel egress
1170 endpoint generally appears in a "destination address" field of the
1171 encapsulation.

[minor] s/particular TLV/particular Tunnel TLV Please make sure the
TLV is always identified...

[major] rfc4271/rfc4760 consistency: s/IP address specified as the
Next Hop /address of the next hop

...
1180 Sending a packet through a tunnel always requires that the packet be
1181 encapsulated, with an encapsulation header that is appropriate for
1182 the tunnel type. The contents of the tunnel encapsulation header MAY
1183 be influenced by the Encapsulation sub-TLV. If there is no
1184 Encapsulation sub-TLV present, the router transmitting the packet
1185 through the tunnel must have a priori knowledge (e.g., by
1186 provisioning) of how to fill in the various fields in the
1187 encapsulation header.

[major] "The contents of the tunnel encapsulation header MAY be
influenced by the Encapsulation sub-TLV." Why is Normative language
used here? If the Encapsulation sub-TLV is present, I thought it
always plays a part. I think that the Normative MAY is out of place
because of the rest of the paragraph. s/MAY/may

[minor] "If there is no Encapsulation sub-TLV present..." I know this
section is talking about the Tunnel Encapsulation Attribute, but the
statement is true also if only the Encapsulation Extended Community is
included in the UPDATE.

1189 Whenever a new Tunnel Type TLV is defined, the specification of that
1190 TLV should describe (or reference) the procedures for creating the
1191 encapsulation header used to forward packets through that tunnel
1192 type. If a tunnel type codepoint is assigned in the IANA "BGP Tunnel
1193 Encapsulation Tunnel Types" registry, but there is no corresponding
1194 specification that defines an Encapsulation sub-TLV for that tunnel
1195 type, the transmitting endpoint of such a tunnel is presumed to know
1196 a priori how to form the encapsulation header for that tunnel type.

[nit] s/Tunnel Type TLV/Tunnel Type

[major] The registry is defined as FCFS, so there is no requirement to
have anything more than a message to IANA; expecting (or suggesting) a
specification here is not appropriate. Unless a change in the
registration policy is considered. I think this paragraph is then not
needed.

1198 If a Tunnel Encapsulation attribute specifies several tunnels, the
1199 way in which a router chooses which one to use is a matter of policy,
1200 subject to the following constraint: if a router can determine that a
1201 given tunnel is not functional, it MUST NOT use that tunnel. In
1202 particular, if the tunnel is identified in a TLV that has a Tunnel
1203 Endpoint sub-TLV, and if the IP address specified in the sub-TLV is
1204 not reachable from router R, then the tunnel MUST be considered non-
1205 functional. Other means of determining whether a given tunnel is
1206 functional MAY be used; specification of such means is outside the
1207 scope of this specification. Of course, if a non-functional tunnel
1208 later becomes functional, router R SHOULD reevaluate its choice of
1209 tunnels.

[major] "not functional" What does that mean? How can that be
determined? Is a "functional" tunnel the same as "feasible tunnel"
(from earlier in this section)? If so, please use consistent
terminology.

[major] The paragraph specifies the same thing as about 6-7 paragraphs
above (but calling it feasible). Please specify things only once.
IOW, this paragraph seems redundant (if functional and feasible are in
fact the same thing).

...
1241 6.1. Impact on BGP Decision Process

[nit] s/Impact on BGP Decision Process/Impact on the BGP Decision Process

1243 The presence of the Tunnel Encapsulation attribute affects the BGP
1244 bestpath selection algorithm. For all the tunnels described in the
1245 Tunnel Encapsulation attribute for a path, if no Tunnel Endpoint
1246 address is feasible, then that path MUST NOT be considered resolvable
1247 for the purposes of Route Resolvability Condition [RFC4271] section
1248 9.1.2.1.

[minor] rfc4271 consistency: s/bestpath/best route/

[minor] rfc4271 consistency: s/path/route/g

[major] "Tunnel Endpoint address is feasible" What does feasible mean
in this case? Is it the same thing as "valid" (§3.1)...or is it
"reachable" (§3.1/§5)? §5 talks about a feasible tunnel, but not a
feasible address... I'm assuming that in this case the feasibility
of the address is related to it being reachable (which is defined as
one of the criteria for a feasible tunnel)-- if that is the case,
where (which table) should it be resolved in? [This comment is
closely related to a similar one in §5.]

1250 6.2. Looping, Infinite Stacking, Etc.

1252 Consider a packet destined for address X. Suppose a BGP UPDATE for
1253 address prefix X carries a Tunnel Encapsulation attribute that
1254 specifies a tunnel egress endpoint of Y. And suppose that a BGP
1255 UPDATE for address prefix Y carries a Tunnel Encapsulation attribute
1256 that specifies a Tunnel Endpoint of X. It is easy to see that this
1257 will cause an infinite number of encapsulation headers to be put on
1258 the given packet.

[major] This case is an example of what rfc4271 calls a "mutually
recursive route":

Mutually recursive routes (routes resolving each other or themselves) also
fail the resolvability check. [rfc4271/§9.1.2.1]

IOW, that fact should be used here.

1260 This could happen as a result of misconfiguration, either accidental
1261 or intentional. It could also happen if the Tunnel Encapsulation
1262 attribute were altered by a malicious agent. Implementations should
1263 be aware of this. This document does not specify a maximum number of
1264 recursions; that is an implementation-specific matter.

[major] The potential attack is not an infinite number of headers, but
simply UPDATES that cannot be used because they are mutually
recursive.

1266 Improper setting (or malicious altering) of the Tunnel Encapsulation
1267 attribute could also cause data packets to loop. Suppose a BGP
1268 UPDATE for address prefix X carries a Tunnel Encapsulation attribute
1269 that specifies a tunnel egress endpoint of Y. Suppose router R
1270 receives and processes the update. When router R receives a packet
1271 destined for X, it will apply the encapsulation and send the
1272 encapsulated packet to Y. Y will decapsulate the packet and forward
1273 it further. If Y is further away from X than is router R, it is
1274 possible that the path from Y to X will traverse R. This would cause
1275 a long-lasting routing loop. The control plane itself cannot detect
1276 this situation, though a TTL field in the payload packets would
1277 presumably prevent any given packet from looping infinitely.

[nit] s/processes the update/processes the advertisement

[minor] s/presumably prevent/prevent

[minor] This paragraph would be better suited for the Security
Considerations section.

1279 These possibilities must also be kept in mind whenever the Tunnel
1280 Endpoint for a given prefix differs from the BGP next hop for that
1281 prefix.

[major] rfc4271/rfc4760 consistency: s/BGP next hop/address of the next hop

[minor] "must also be kept in mind" This phrase is not Normative, but
it seems to want to indicate that the router should do
something...what is it?

1283 7. Recursive Next Hop Resolution
...
1294 o the next hop of UPDATE U1 is router R2;

[major] rfc4271/rfc4760 consistency: s/next hop/address of the next hop

...
1305 However, suppose that one of the TLVs in U2's Tunnel Encapsulation
1306 attribute contains the Color Sub-TLV. In that case, packet P MUST
1307 NOT be sent through the tunnel identified in that TLV, unless U1 is
1308 carrying the Color Extended Community that is identified in U2's
1309 Color Sub-TLV.

[minor] s/identified/contained

...
1319 The procedures in this section presuppose that U1's next hop resolves
1320 to a BGP route, and that U2's next hop resolves (perhaps after
1321 further recursion) to a non-BGP route.

[major] rfc4271/rfc4760 consistency: s/next hop/address of the next hop

...
1336 8.1. Tunnel Types without a Virtual Network Identifier Field
...
1346 o If the TLV does not contain an Embedded Label Handling sub-TLV, or
1347 if it contains an Embedded Label Handling sub-TLV whose value is
1348 2, the embedded label is ignored completely. The tunnel is
1349 assumed to have terminated at the corresponding VRF.

[] Hmmm...this is the only place where a VRF is mentioned...so the
"corresponding VRF" may not be clear. At least, please expand VRF,
and add a short explanation.

...
1376 8.2.1. Unlabeled Address Families
...
1389 If the TLV identifying the tunnel contains an Encapsulation sub-TLV
1390 whose V bit is set, the virtual network identifier field of the
1391 encapsulation header is set to the value of the virtual network
1392 identifier field of the Encapsulation sub-TLV.

[major] §3.2.* talk about the VNI in the sub-TLV being "valid", but
there is no guarantee/check to confirm that. IOW, the receiver has to
trust the sender. There's an attack vector where a rogue sender, for
example, can send an invalid/nonexistent VNI (with the V bit set) and
result in mis-delivery or loss of the traffic. Please include
something to the effect in the Security Considerations section. [Note
that this applies to other places where the same information is used.]

...
1411 8.2.2.1. When a Valid VNI has been Signaled
...
1417 o If the TLV contains an Embedded Label Handling sub-TLV whose value
1418 is 1, then the virtual network identifier field of the
1419 encapsulation header is set to the value of the virtual network
1420 identifier field of the Encapsulation sub-TLV.

1422 The embedded label (from the NLRI of the route that is carrying
1423 the Tunnel Encapsulation attribute) appears at the top of the MPLS
1424 label stack in the encapsulation payload.

1426 o If the TLV does not contain an Embedded Label Handling sub-TLV, or
1427 if contains an Embedded Label Handling sub-TLV whose value is 2,
1428 the embedded label is ignored entirely, and the virtual network
1429 identifier field of the encapsulation header is set to the value
1430 of the virtual network identifier field of the Encapsulation sub-
1431 TLV.

[nit] s/or if contains/or it contains

[major] Both options above result in the same action: "...the virtual
network identifier field of the encapsulation header is set to the
value of the virtual network identifier field of the Encapsulation
sub-TLV."

1433 8.2.2.2. When a Valid VNI has not been Signaled
...
1449 o If the TLV does not contain an Embedded Label Handling sub-TLV, or
1450 if it contains an Embedded Label Handling sub-TLV whose value is
1451 2, the embedded label is copied into the virtual network
1452 identifier field of the encapsulation header.

[major] The length of the label and the VNI are different. How should
the label be encoded in the bigger field? I'm assuming that the
specification is in the encapsulation RFCs, right?

...
1459 9. Applicability Restrictions

1461 In a given UPDATE of a labeled address family, the label embedded in
1462 the NLRI is generally a label that is meaningful only to the router
1463 whose address appears as the next hop. Certain of the procedures of
1464 Section 8.2.2.1 or Section 8.2.2.2 cause the embedded label to be
1465 carried by a data packet to the router whose address appears in the
1466 Tunnel Endpoint sub-TLV. If the Tunnel Endpoint sub-TLV does not
1467 identify the same router that is the next hop, sending the packet
1468 through the tunnel may cause the label to be misinterpreted at the
1469 tunnel's egress endpoint. This may cause misdelivery of the packet.

[major] rfc4271/rfc4760 consistency: s/whose address appears as the
next hop/represented by the address of the next hop

[major] rfc4271/rfc4760 consistency: s/that is the next
hop/represented by the address of the next hop

1471 Therefore the embedded label MUST NOT be carried by a data packet
1472 traveling through a tunnel unless it is known that the label will be
1473 properly interpreted at the tunnel's egress endpoint. How this is
1474 known is outside the scope of this document.

[major] "the embedded label MUST NOT be carried...unless it is known
that the label will be properly interpreted...[which is] outside the
scope of this document." If outside the scope, then how can it be
Normatively enforced? IOW, the text can't mandate something (MUST
NOT) and then not offer a way to achieve it -- there's no way to test
for interoperability.

Is the only safe behavior if the Address field in the Tunnel Endpoint
sub-TLV and the address of the next hop are the same? Should that be
the Normative part of the specification?

1476 Note that if the Tunnel Encapsulation attribute is attached to a VPN-
1477 IP route [RFC4364], and if Inter-AS "option b" (see section 10 of
1478 [RFC4364]) is being used, and if the Tunnel Endpoint sub-TLV contains
1479 an IP address that is not in same AS as the router receiving the
1480 route, it is very likely that the embedded label has been changed.
1481 Therefore use of the Tunnel Encapsulation attribute in an "Inter-AS
1482 option b" scenario is not supported.

[major] "the Tunnel Endpoint sub-TLV contains an IP address that is
not in same AS as the router receiving the route" This phrase sounds
like a common scenario: tunnel endpoint is in a different AS. If this
is an issue for this case, why isn't it for other cases? What am I
missing? Is there a profile of use cases (where the ones above are
good examples of)?

[] "is not supported" What does that mean? Does it mean that the
attribute won't work in that scenario (there are a lot of "if"s in the
description)? Does it mean that the use is not recommended? Does it
mean that a specific implementation doesn't support it? ...

1484 10. Scoping

1486 The Tunnel Encapsulation attribute is defined as a transitive
1487 attribute, so that it may be passed along by BGP speakers that do not
1488 recognize it. However, it is intended that the Tunnel Encapsulation
1489 attribute be used only within a well-defined scope, e.g., within a
1490 set of Autonomous Systems that belong to a single administrative
1491 entity. If the attribute is distributed beyond its intended scope,
1492 packets may be sent through tunnels in a manner that is not intended.

1494 To prevent the Tunnel Encapsulation attribute from being distributed
1495 beyond its intended scope, any BGP speaker that understands the
1496 attribute MUST be able to filter the attribute from incoming BGP
1497 UPDATE messages. When the attribute is filtered from an incoming
1498 UPDATE, the attribute is neither processed nor redistributed. This
1499 filtering SHOULD be possible on a per-BGP-session basis. For each
1500 session, filtering of the attribute on incoming UPDATEs MUST be
1501 enabled by default.

1503 In addition, any BGP speaker that understands the attribute MUST be
1504 able to filter the attribute from outgoing BGP UPDATE messages. This
1505 filtering SHOULD be possible on a per-BGP-session basis. For each
1506 session, filtering of the attribute on outgoing UPDATEs MUST be
1507 enabled by default.

[major] The filtering described on this section implies a contiguous
set of ASes within the well-defined scope; the filters implied above
seem to be an all-or-nothing proposition. IOW, there doesn't seem to
be a possibility to accept the attribute in some updates and not
others, or to conditionally do so...maybe based on recommendations
from Origin Validation, for example. But then the example used (a
single administrative entity) seems to not require Origin Validation
anyway.

I find the deployment models to be contradicting...not clear...if I
trust my neighboring AS enough to accept the Tunnel Encapsulation
attribute, then maybe I don't need to verify that the address of the
endpoint belongs to them...

1509 11. Error Handling

[major] A significant portion of this section is not about error
handling, but about normal processing: for example "no significance to
the order in which the TLVs occur"... That part of the specification
belongs to the section where the TLV (in this case) is described.
Please move text to the appropriate sections.

1511 The Tunnel Encapsulation attribute is a sequence of TLVs, each of
1512 which is a sequence of sub-TLVs. The final octet of a TLV is
1513 determined by its length field. Similarly, the final octet of a sub-
1514 TLV is determined by its length field. The final octet of a TLV MUST
1515 also be the final octet of its final sub-TLV. If this is not the
1516 case, the TLV MUST be considered to be malformed. A TLV that is
1517 found to be malformed for this reason MUST NOT be processed, and MUST
1518 be stripped from the Tunnel Encapsulation attribute before the
1519 attribute is propagated. Subsequent TLVs in the Tunnel Encapsulation
1520 attribute may still be valid, in which case they MUST be processed
1521 and redistributed normally.

[major] "The final octet of a TLV MUST also be the final octet of its
final sub-TLV. If this is not the case...MUST be stripped from the
Tunnel Encapsulation attribute before the attribute is propagated."
Wait! To remove it, how do you know where the end of the TLV is: at
the end of the TLV length, or at the end of what seems to be the final
sub-TLV? It sounds that the assumption is that the Length of the TLV
is always correct...but that is clearly just an assumption that could
result in not removing the proper number of bits, and the next TLV (if
present) might not be able to be processed...

rfc7606/§4 (Attribute Length Fields) specifies that, when dealing with
errors in attribute length fields, "the "treat-as-withdraw" approach
MUST be used".

1523 If a Tunnel Encapsulation attribute does not have any valid TLVs, or
1524 it does not have the transitive bit set, the "Attribute Discard"
1525 procedure of [RFC7606] is applied.

[major] "does not have any valid TLVs" This is not a valid use of
Attribute Discard. This is the definition from rfc7606:

o Attribute discard: In this approach, the malformed attribute MUST
be discarded and the UPDATE message continues to be processed.
This approach MUST NOT be used except in the case of an attribute
that has no effect on route selection or installation.

It is clear to me, because of the specification in §6.1 (Impact on BGP
Decision Process), that the TLVs directly impact route
selection/installation.

[] "does not have the transitive bit set" The use of Attribute
Discard in this case is ok...from rfc7607/§3:

If the value of either the Optional or Transitive bits in
the Attribute Flags is in conflict with their specified
values, then the attribute MUST be treated as malformed and
the "treat-as-withdraw" approach used, unless the
specification for the attribute mandates different handling
for incorrect Attribute Flags.

1527 If a Tunnel Encapsulation attribute can be parsed correctly, but
1528 contains a TLV whose tunnel type is not recognized by a particular
1529 BGP speaker, that BGP speaker MUST NOT consider the attribute to be
1530 malformed. Rather, the TLV with the unrecognized tunnel type MUST be
1531 ignored, and the BGP speaker MUST interpret the attribute as if that
1532 TLV had not been present. If the route carrying the Tunnel
1533 Encapsulation attribute is propagated with the attribute, the
1534 unrecognized TLV MUST remain in the attribute.

[minor] "the unrecognized tunnel type MUST be ignored, and the BGP
speaker MUST interpret the attribute as if that TLV had not been
present." This sounds redundant to me...or is there a difference
between ignore and "interpret the attribute as if that TLV had not
been present"?

[] the handling of unrecognized stuff is not an error condition...it
may simply be a new/unknown thing... Please move the specification to
§2-3.

1536 If a TLV of a Tunnel Encapsulation attribute contains a sub-TLV that
1537 is not recognized by a particular BGP speaker, the BGP speaker MUST
1538 process that TLV as if the unrecognized sub-TLV had not been present.
1539 If the route carrying the Tunnel Encapsulation attribute is
1540 propagated with the attribute, the unrecognized TLV MUST remain in
1541 the attribute.

[minor] s/unrecognized TLV MUST remain/unrecognized sub-TLV MUST remain

[??] Is there a case where the result could be different tunnels?
For example, one router interprets one thing (partially) and another a
different result given the additional information? I would assume the
answer is yes given that we don't know the nature of the new sub-TLV.
We need text about the introduction of new sub-TLVs.

1543 If the type code of a sub-TLV appears as "reserved" in the IANA "BGP
1544 Tunnel Encapsulation Attribute Sub-TLVs" registry, the sub-TLV MUST
1545 be treated as an unrecognized sub-TLV.

[major] How does a router check the IANA registry? There is
currently only one Reserved value in the registry (0) -- it is then
better to simply specify what should be done if 0 is received.
However, because a Reserved value could be used for something in the
future [rfc8126], then I don't think it is necessary to explicitly
specify what to do in this case. Please remove the paragraph.

1547 In general, if a TLV contains a sub-TLV that is malformed (e.g.,
1548 contains a length field whose value is not legal for that sub-TLV),
1549 the sub-TLV should be treated as if it were an unrecognized sub-TLV.
1550 This document specifies one exception to this rule -- within a tunnel
1551 encapsulation attribute that is carried by a BGP UPDATE whose AFI/
1552 SAFI is one of those explicitly listed in the second paragraph of
1553 Section 5, if a TLV contains a malformed Tunnel Endpoint sub-TLV (as
1554 defined in Section 3.1), the entire TLV MUST be ignored, and MUST be
1555 removed from the Tunnel Encapsulation attribute before the route
1556 carrying that attribute is redistributed.

[minor] "e.g., contains a length field whose value is not legal..."
The length example is a bad one. As I mentioned above, rfc7606/§4
(Attribute Length Fields) specifies that, when dealing with errors in
attribute length fields, "the "treat-as-withdraw" approach MUST be
used".

[major] "should be treated as..." Should that be a Normative statement?

[minor] "be treated as if it were an unrecognized sub-TLV" Instead of
forcing the reader to check how an unrecognized sub-TLV is treated,
please simply go to the point: be ignored.

[nit] s/tunnel encapsulation attribute/Tunnel Encapsulation
attribute/g To be consistent with the rest of the text.

[major] "...one exception...a BGP UPDATE whose AFI/SAFI is...listed
in...Section 5, if a TLV contains a malformed Tunnel Endpoint
sub-TLV...the entire TLV MUST be ignored, and MUST be removed from the
Tunnel Encapsulation attribute" So...the "exception" applies to all
the currently supported AFI/SAFIs... If no other AFI/SAFIs are
specified at this time, why is this "exception" not the norm?

1558 Within a tunnel encapsulation attribute that is carried by a BGP
1559 UPDATE whose AFI/SAFI is one of those explicitly listed in the second
1560 paragraph of Section 5, a TLV that does not contain exactly one
1561 Tunnel Endpoint sub-TLV MUST be treated as if it contained a
1562 malformed Tunnel Endpoint sub-TLV.

[major] There is text below (penultimate paragraph) that starts to
specify the same thing ("sub-TLVs...MUST NOT occur more than
once")...so this paragraph is really an exception to the behavior
specified below. First, please specify the behavior only once (see
§3.1). Also, consider structuring the specification so that general
behavior is specified first and then the specifics.

1564 A TLV identifying a particular tunnel type may contain a sub-TLV that
1565 is meaningless for that tunnel type. For example, perhaps the TLV
1566 contains a "UDP Destination Port" sub-TLV, but the identified tunnel
1567 type does not use UDP encapsulation at all. Sub-TLVs of this sort
1568 MUST be treated as a no-op. That is, they MUST NOT affect the
1569 creation of the encapsulation header. However, the sub-TLV MUST NOT
1570 be considered to be malformed, and MUST NOT be removed from the TLV
1571 before the route carrying the Tunnel Encapsulation attribute is
1572 redistributed. (This allows for the possibility that such sub-TLVs
1573 may be given a meaning, in the context of the specified tunnel type,
1574 in the future.)

[nit] s/"UDP Destination Port"/UDP Destination Port

[minor] "That is, they MUST NOT affect the creation of the
encapsulation header. However, the sub-TLV MUST NOT be considered to
be malformed, and MUST NOT be removed from the TLV before the route
carrying the Tunnel Encapsulation attribute is redistributed." It
seems to me that these sentences are redundant and not needed. If you
want to make sure that "ignored" is translated into all the rest,
please specify it once and forget it.

[minor] "(This allows for the possibility...in the future.)" Please
remove this sentence and don't speculate in a Standards Track
document.

...
1581 The following sub-TLVs defined in this document MUST NOT occur more
1582 than once in a given Tunnel TLV: Tunnel Endpoint (discussed above),
1583 Encapsulation, IPv4 DS, UDP Destination Port, Embedded Label
1584 Handling, MPLS Label Stack, Prefix-SID. If a Tunnel TLV has more
1585 than one of any of these sub-TLVs, all but the first occurrence of
1586 each such sub-TLV type MUST be treated as a no-op. However, the
1587 Tunnel TLV containing them MUST NOT be considered to be malformed,
1588 and all the sub-TLVs MUST be propagated if the route carrying the
1589 Tunnel Encapsulation attribute is propagated.

[major] "MUST NOT occur more than once in a given Tunnel TLV: Tunnel
Endpoint (discussed above)...If a Tunnel TLV has more than one of any
of these sub-TLVs, all but the first occurrence..." Related to the
comment above about the Tunnel Endpoint exception; please reorder the
text, and consider moving some of the specification to the place where
specific sub-TLVs are defined.

...
1597 12. IANA Considerations

[major] Please take a look at rfc8126/§8, and use this text to
introduce the section:

Because this document obsoletes RFC 5512, IANA is asked to change all
registration information that references [RFC5512] to instead reference this
document.

Adding this document as a reference (as in §12.2) is not appropriate.
If adding the text above, then some of the sections below (like §12.2)
are not needed.

[minor] This document creates several new registries related to the
sub-TLVs. Consider creating a new grouping (maybe "BGP Tunnel
Encapsulation Parameters") to include those and the existing Tunnel
Types and Sub-TLVs registries.

1599 12.1. Subsequent Address Family Identifiers

1601 IANA is requested to modify the "Subsequent Address Family
1602 Identifiers" registry to indicate that the Encapsulation SAFI is
1603 deprecated. This document should be the reference.

[nit] Please indicate the value (7).

[major] In this case "obsolete", and not "deprecated" is the right indication.

1605 12.2. BGP Path Attributes

1607 IANA has previously assigned value 23 from the "BGP Path Attributes"
1608 Registry to "Tunnel Encapsulation Attribute". IANA is requested to
1609 add this document as a reference.

[] This section is not needed...assuming the introductory text
suggested above is added.

1611 12.3. Extended Communities

1613 IANA has previously assigned values from the "Transitive Opaque
1614 Extended Community" type Registry to the "Color Extended Community"
1615 (sub-type 0x0b), and to the "Encapsulation Extended
1616 Community"(0x030c). IANA is requested to add this document as a
1617 reference for both assignments.

[] This section is not needed...assuming the introductory text
suggested above is added.

1619 12.4. BGP Tunnel Encapsulation Attribute Sub-TLVs
...
1637 o The values in the range 64-125 and 192-252 are to be allocated
1638 using the "First Come, First Served" registration procedure.

[nit] s/First Come, First Served/First Come First Served

1640 o The values in the range 126-127 and 253-254 are reserved for
1641 experimental use; IANA shall not allocate values from this range.

[minor] A Table instead of the sentences above would be great.

...
1646 6: Remote Endpoint

1648 IANA is requested to change the name of "Remote Endpoint" to
1649 "Tunnel Egress Endpoint".

[major] The rest of this document calls it the "Tunnel Endpoint"
sub-TLV (not Tunnel Egress Endpoint). Call it anything you want, but
please be consistent!

...
1658 11: Prefix SID

[major] The rest of this document calls it the "Prefix-SID" sub-TLV
(not Prefix SID). Note that changing the text here requires asking
IANA to update the same (which I think is the right thing to do).

1660 IANA has previously assigned codepoints from the "BGP Tunnel
1661 Encapsulation Attribute Sub-TLVs" registry for "Encapsulation",
1662 "Protocol Type", and "Color". IANA is requested to add this document
1663 as a reference.

[] This last paragraph is not needed...assuming the introductory text
suggested above is added.

1665 12.5. Tunnel Types
...
1671 IANA is requested to add this document as a reference for tunnel
1672 types 1 (L2TPv3), 2 (GRE), and 7 (IP in IP) in the "BGP Tunnel
1673 Encapsulation Tunnel Types" registry.

[] This last paragraph is not needed...assuming the introductory text
suggested above is added.

1675 12.6. Flags Field of Vxlan Encapsulation sub-TLV

[minor] s/Vxlan/VXLAN/g

...
1680 IANA is requested to add this document as a reference for flag bits V
1681 and M in the "Flags field of Vxlan Encapsulation sub-TLV" registry.

[major] Suggestion>

IANA is requested to create a new registry titled "..." in the ggg group.
The registration policy for the registry is ppp.

The initial values for this new registry are indicated below.

...Add a table showing the Bit Position, Flag Name and Reference....

1683 12.7. Flags Field of Vxlan-GPE Encapsulation sub-TLV
...
1688 IANA is requested to add this document as a reference for flag bit V
1689 in the "Flags field of Vxlan-GPE Encapsulation sub-TLV" registry.

[major] Suggestion>

IANA is requested to create a new registry titled "..." in the ggg group.
The registration policy for the registry is ppp.

The initial values for this new registry are indicated below.

...Add a table showing the Bit Position, Flag Name and Reference....

1691 12.8. Flags Field of NVGRE Encapsulation sub-TLV
...
1696 IANA is requested to add this document as a reference for flag bits V
1697 and M in the "Flags field of NVGRE Encapsulation sub-TLV" registry.

[major] Suggestion>

IANA is requested to create a new registry titled "..." in the ggg group.
The registration policy for the registry is ppp.

The initial values for this new registry are indicated below.

...Add a table showing the Bit Position, Flag Name and Reference....

1699 12.9. Embedded Label Handling sub-TLV
...
1705 IANA is requested to add this document as a reference for value of 1
1706 (Payload of MPLS with embedded label) and 2 (no embedded label in
1707 payload) in the "sub-TLV's value field of the Embedded Label Handling
1708 sub-TLV" registry.

[major] Suggestion>

IANA is requested to create a new registry titled "..." in the ggg group.
The registration policy for the registry is ppp.

The initial values for this new registry are indicated below.

...Add a table showing the Values, Name and Reference....

1710 13. Security Considerations

1712 The Tunnel Encapsulation attribute can cause traffic to be diverted
1713 from its normal path, especially when the Tunnel Endpoint sub-TLV is
1714 used. This can have serious consequences if the attribute is added
1715 or modified illegitimately, as it enables traffic to be "hijacked".

[major] Even if the attribute is added legitimately, the contents may
still divert the traffic in an undesired direction. This is the case
of a rogue node. Not only can the traffic be diverted to a location
the rogue router expects, but it can be sent to an unsuspecting node
-- this last scenario could result in overloading it, etc. Please
include text related to actions that a rogue node could initiate. Not
all of these actions have possible mitigation, but being aware of them
is important.

1717 The Tunnel Endpoint sub-TLV contains both an IP address and an AS
1718 number. BGP Origin Validation [RFC6811] can be used to obtain
1719 assurance that the given IP address belongs to the given AS. While
1720 this provides some protection against misconfiguration, it does not
1721 prevent a malicious agent from inserting a sub-TLV that will appear
1722 valid.

[major] The text in §3.1 points to this section on the topic of the
address belonging to an AS...and then that check is used Normatively
to define actions. However, the use of BGP Origin Validation is not
Normatively mandated; instead language such as "can be used" or "may
be advisable" is used. That is justified (below) by pointing out
issues...and further suggesting that BGPSEC (which is a not-deployed
protocol!) could be used.

The summary is that there is no Normative/mandatory mechanism to
validate that an IP address belongs to an AS, which is a requirements
elsewhere in the text. IOW, the specification is not complete and can
result in implementations basically doing anything they want. :-(

[major] Note also that rfc6811 talks about route validation...and not
validation or any other object (like an attribute). If Origin
Validation is in fact recommended (SHOULD), then we need a paragraph
or two detailing the mapping between the fields in the Tunnel Endpoint
sub-TLV and "Route Prefix" and "Route Origin ASN".

1724 Before sending a packet through the tunnel identified in a particular
1725 TLV of a Tunnel Encapsulation attribute, it may be advisable to use
1726 BGP Origin Validation to obtain the following additional assurances:

1728 o the origin AS of the route carrying the Tunnel Encapsulation
1729 attribute is correct;

1731 o the origin AS of the route to the IP address specified in the
1732 Tunnel Endpoint sub-TLV is correct, and is the same AS that is
1733 specified in the Tunnel Endpoint sub-TLV.

[major] This text points at a slightly different requirement than the
text in §3.1, which wants it to be "determined that the IP address in
the sub-TLV's address subfield does not belong to the...Autonomous
System", which somehow implies checking the validity of the IP/ASN
pair in the sub-TLV. The text above talks about "the route to the IP
address", which is different!

Validating the route to the IP address (and not the sub-TLV) is
better, clearer and already specified (rfc6811). The issue of the
Normative dependence still remains.

1735 One then has some level of assurance that the tunneled traffic is
1736 going to the same destination AS that it would have gone to had the
1737 Tunnel Encapsulation attribute not been present. However, this may
1738 not suit all use cases, and in any event is not very strong
1739 protection against hijacking.

[major] "this may not suit all use cases" Like what? What are the exceptions?

[major] "...is not very strong protection against hijacking." Do you
have a reference for that? Seriously, this document is not the place
to criticize other solutions...much less ones that seem to be
necessary for the functionality described here. Instead, please point
to the security considerations of rfc6811 and let the reader reach any
conclusion they may.

1741 For these reasons, BGP Origin Validation should not be relied upon
1742 exclusively, and the filtering procedures of Section 10 should always
1743 be in place.

[minor] Should this text be Normative?

1745 Increased protection can be obtained by using BGPSEC [RFC8205] to
1746 ensure that the route carrying the Tunnel Encapsulation attribute,
1747 and the routes to the Tunnel Endpoint of each specified tunnel, have
1748 not been altered illegitimately.

[nit] s/BGPSEC/BGPsec

[major] BGPsec does not guarantee that the route has not been
altered...just just that the UPDATE traversed a path [rfc8205/§8.1].
Specifically, BGPsec doesn't protect any attributes (other than the
BGPsec_PATH).

1750 If BGP Origin Validation is used as specified above, and the tunnel
1751 specified in a particular TLV of a Tunnel Encapsulation attribute is
1752 therefore regarded as "suspicious", that tunnel should not be used.

[major] "suspicious" is not one of the states in rfc6811.

[major] "tunnel should not be used" Should that be a Normative
statement. If yes, are there cases when it would be ok to use the
tunnel?

1754 Other tunnels specified in (other TLVs of) the Tunnel Encapsulation
1755 attribute may still be used.

[nit] s/(other TLVs of)/other TLVs of

...
1795 16.1. Normative References

1797 [I-D.ietf-idr-bgp-prefix-sid]
1798 Previdi, S., Filsfils, C., Lindem, A., Sreekantiah, A.,
1799 and H. Gredler, "Segment Routing Prefix SID extensions for
1800 BGP", draft-ietf-idr-bgp-prefix-sid-27 (work in progress),
1801 June 2018.

[major] s/I-D.ietf-idr-bgp-prefix-sid/rfc8669

...
1842 [RFC4760] Bates, T., Chandra, R., Katz, D., and Y. Rekhter,
1843 "Multiprotocol Extensions for BGP-4", RFC 4760,
1844 DOI 10.17487/RFC4760, January 2007,
1845 <https://www.rfc-editor.org/info/rfc4760>.

== Unused Reference: 'RFC4760' is defined on line 1842, but no explicit
reference was found in the text

1847 [RFC5512] Mohapatra, P. and E. Rosen, "The BGP Encapsulation
1848 Subsequent Address Family Identifier (SAFI) and the BGP
1849 Tunnel Encapsulation Attribute", RFC 5512,
1850 DOI 10.17487/RFC5512, April 2009,
1851 <https://www.rfc-editor.org/info/rfc5512>.

[minor] Because this document Obsoletes rfc5512, there shouldn't be a
Normative dependence.

1853 [RFC5566] Berger, L., White, R., and E. Rosen, "BGP IPsec Tunnel
1854 Encapsulation Attribute", RFC 5566, DOI 10.17487/RFC5566,
1855 June 2009, <https://www.rfc-editor.org/info/rfc5566>.

[minor] This reference can be Informative.

...
1864 [RFC7510] Xu, X., Sheth, N., Yong, L., Callon, R., and D. Black,
1865 "Encapsulating MPLS in UDP", RFC 7510,
1866 DOI 10.17487/RFC7510, April 2015,
1867 <https://www.rfc-editor.org/info/rfc7510>.

[minor] This reference can be Informative.

...
1883 16.2. Informative References
...
1896 [RFC2474] Nichols, K., Blake, S., Baker, F., and D. Black,
1897 "Definition of the Differentiated Services Field (DS
1898 Field) in the IPv4 and IPv6 Headers", RFC 2474,
1899 DOI 10.17487/RFC2474, December 1998,
1900 <https://www.rfc-editor.org/info/rfc2474>.

[major] This reference should be Normative.

[Idr] AD Review of draft-ietf-idr-tunnel-encaps-15 Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Keyur Patel
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Keyur Patel
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Susan Hares
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Gyan Mishra
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Rabadan, Jorge (Nokia - US/Mountain View)
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Gyan Mishra
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Gyan Mishra
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John E Drake
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Gyan Mishra
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Susan Hares
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… John Scudder
Re: [Idr] AD Review of draft-ietf-idr-tunnel-enca… Susan Hares