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

[John Scudder <[e-mail]>]

Hi Alvaro,

(And note that I’m replying as a co-author now.)

Comments inline. Note that in most cases I went from memory and didn’t go back and re-do a line-by-line check of the document, so while I believe my reply is correct, it’s possible I might have made a mistake somewhere.

I found a number of overlooked points as I worked through this reply, so we’ve issued -17 to correct them. This reply is with respect to that version. It’s currently awaiting manual posting; I’ve cc’d Alvaro and Sue on the manual posting request so they have copies already if needed.

—John

> On Feb 21, 2020, at 7:47 AM, Alvaro Retana <[e-mail]> wrote:
> 
> 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://urldefense.com/v3/__https://mailarchive.ietf.org/arch/msg/idr/yuIXr3HpPEp7v_k08XpdhXRN6GM__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzyw06SK0g$ 

We added 5566 and 5640 to the list of obsoleted RFCs, along with an explanation.

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

Fixed. Type codes, lengths are given in-line throughout, definition of Tunnel Extended Community is inlined.

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

Done.

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

Added Section 1.4, "Use Case for The Tunnel Encapsulation Attribute”.

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

This is completely redone. See Section 3.1.1, “Validating the Address Field”.

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

This is also redone. The outline is that 3.1.1 is what talks about how to determine what origin ASes to care about and when, and the Security Considerations section mentions that OV + BGPsec is a way to validate that the origin ASes you got are true.

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

We re-did this some, but the whole “belonging” thing was addressed otherwise, see above. The Scoping section basically says “don’t leak across EBGP by default”.

> 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

I’m pretty sure we simply accepted all the nits. I’m not going to flag each one with a “done”. If there are exceptions I’ll address them. Same for “minor”.

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

Really the only one that seemed especially amenable to a forward reference was the Tunnel Egress Endpoint sub-TLV, we added that reference. Otherwise, it doesn’t work all that well AFAICT — this section is basically a precis of the whole following document; really, the reader can just consult the table of contents. 

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

Deleted.

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

Clarified that we also refer to this as “Tunnel TLV”. We considered just making it “Tunnel TLV” throughout but decided not to.

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

Moved it, put in a forward reference.

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

There’s MUST NOT language now. There’s also language in Section 12 that says if you put another protocol sub-TLV inside an X-in-Y tunnel, that protocol sub-TLV MUST be disregarded.

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

I believe this has been cleaned up throughout, also I believe we have cleaned up all the “reserved” stuff to say send as zero, disregard on receipt.

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

Section 12:

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

> ...
> 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. :-(

Fixed naming. Added type. Also added type everywhere else you noted it, I won’t ack each of these individually.

> [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).

Done.

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

Now called ’Tunnel Egress Endpoint Sub-TLV’

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

I believe the answer to this is “it’s not forbidden, but it’s not specified in this document”, just like it says in the text you’ve quoted. If someone wants to spec it, they should write a draft.

> [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..."

I’m a little hesitant to try to provide a template for extension documents. Seems like a pretty heavy lift. If you (or others) think there are a few pithy sentences that will be valuable for this purpose, I’m all ears.

> [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?

I don’t think so, I think it should be considered unrecognized. Added text to that effect.

> 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 ??

Done.

> 383	   In this case, the length field of Tunnel Endpoint sub-TLV must
> 384	   contain the value 10 (0xa).
> 
> [major] s/must/MUST ??

Done.

> 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 ??

This whole paragraph was removed; it’s redundant with this new paragraph:

   o  The IP address in the sub-TLV's address subfield is listed in the
      relevant Special-Purpose IP Address Registry [RFC6890] as either
      not a valid destination, or not forwardable.

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

Per the rewrite, it’s “malformed”.

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

Resolved by removing the AS number entirely.

> [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?

This is all now captured in Section 3.1.1.

> 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

Done.

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

I think the rewrite eliminates this.

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

Done.

> 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?

Referenced RFC 6890.

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

This is not relevant any more, however 3.1.1 as written is optional, so not “MTI". I don’t think it’s non-deterministic, though — you either do it as written, or you don’t.

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

I think we’ve got these overlap cases consolidated into Section 12 now. (The former Section 11.)

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

Done.

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

Done, same for all your other “spec the length” comments, I won’t reply to them individually.

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

Done.

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

We simply dropped “valid”. As you say, it didn’t add anything. Same for all your later “valid” comments, not acked individually.

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

New text:

   o  If the V bit is not set, and the BGP UPDATE message has AFI/SAFI
      other than Ethernet VPNs (EVPN) then the NVGRE tunnel cannot be
      used.

> ...
> 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.
> 
> [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).
> 
> 
> ...
> 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.

Fixed.

> ...
> 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.
> 
> [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).
> 
> 604	      M: This bit is set to 1 to indicate that a valid MAC Address is
> 605	      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.

Added this text:

   o  If the V bit is not set, and the BGP UPDATE message has AFI/SAFI
      other than Ethernet VPNs (EVPN) then the VXLAN tunnel cannot be
      used.

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

Fixed.

> ...
> 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.
> 
> [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).
> 
> 
> 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?

We removed the sentence beginning “That is”. The document should now be clear that tunnels of the form X-in-Y are exclusively for the use of traffic of type X, whereas other tunnels can carry different protocols as indicated by the protocol sub-TLV.

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

We broke this out into section 5.

> 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?

Done.

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

Done.

> [] 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

Done.

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

You use the sub-TLV length field and hope for the best. This is made clear by the first paragraph of Section 12.

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

I think we discussed previously that the words just have their usual English meanings and are clear in context of RFC 3032’s use of “top” and “bottom”.

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

Normatively referenced 3032, 5462 for field definitions. Those seemed like the right ones.

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

Changed to “… of all the entries being pushed MUST be cleared."

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

Left as 3032, 5462. They are now 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.

Done.

> 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?

Rewritten as:

   The TTL (Time to Live) field of each label stack entry SHOULD be set
   to 255, unless changed to some other non-zero value 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 to some other non-zero value.
   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, either 255 or some other value as determined by
   policy as discussed above.

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

We hacked the text slightly (to “permitted”) but I don’t think it really addresses your point. Then again, the text you quote from 8669 says “not defined”, it doesn’t say “forbidden”. So I think given it’s a gray area in 8669, it’s OK for it to be a gray area here too.

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

As we discussed earlier, although it may seem like a picky point, we’re not actually re-specifying.

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

Yes, it would. :-)

Seriously, it’s already a long document and none of us was excited to write the example. We’d gladly accept a contribution.

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

Done.

> 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

Done.

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

It’s probably better that you just re-read this section. As I mention above, we rewrote it to simplify all this “well you could do it this way, or that” and just made the community way the mandatory one.

> 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"?

Yep. See above.

> (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?

There’s no issue about preference — if they aren’t the same, then by definition they encode two different tunnels, and both tunnels are available for use. I’m not sure what you mean by “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.

Fixed.

> [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."

Done.

> [] 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://urldefense.com/v3/__https://mailarchive.ietf.org/arch/msg/idr/-45ujNcRzWFWTRnBL1NBcbPoKgo__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzzSNYlWig$ 

Done, even if it is a little weird to define an empty registry. Also renamed the “Reserved” field to be “Flags” in the Color Extended Community section and added a note that it has to be propagated unmolested.

> [] 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.

Sigh, reserved fields are attractive nuisances. The problem with reserved fields is they don’t have a defined format until someone defines one. In the example above, the authors of draft-ietf-idr-segment-routing-te-policy have done so. In some other case, though, another creative person might use a two-byte reserved field to encode (say) a two-byte metric, in which case it wouldn’t be all that helpful to have defined a bitfield registry for it. I think the document that invents a use for the field should build the registry for it.

I did review the various reserved fields and I didn’t feel prescient enough to know what kind of registries to create for them, sorry. :-(

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

Now says:

   The Color Value field is encoded as 4 octet value by the
   administrator and is outside the scope of this document.  For the use
   of this Extended Community please see Section 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.

Done. 

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

Now says:

      *  The tunnel is specified in a TLV whose Tunnel Egress Endpoint
         sub-TLV identifies an IP address that is reachable.  This IP
         address may be reachable via one or more forwarding tables.
         Local policy may determine these forwarding tables and is
         outside the scope of this document.  The reachability condition
         is evaluated as per [RFC4271].

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

Done.

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

Agreed. It’s gone.

> 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

Done.

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

Done.

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

It would be possible to be excruciatingly correct by interpolating something like "(either literally, or virtually as in the case of an Encapsulation Extended Community, see XREF)”. It seems to me like taken in context the text is clear though and the extra words would just get in the way.

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

Removed.

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

Now says:

   If a Tunnel Encapsulation attribute specifies several tunnels, the
   way in which a router chooses which one to use is a matter of policy,
   In addition to the reachability to the address of the egress endpoint
   of the tunnel, other policy factors MAY be used to determine the
   feasibility of the tunnel.  The policy factors are beyond the scope
   of this document.


> [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).

I think this is resolved now.

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

Should be clear now that “feasible” means that which is defined in the previous section:

   The presence of the Tunnel Encapsulation attribute affects the BGP
   best route selection algorithm.  If a route includes the Tunnel
   Encapsulation attribute, and if that attribute includes no tunnel
   which is feasible, then that route MUST NOT be considered resolvable
   for the purposes of Route Resolvability Condition [RFC4271] section 9.1.2.1
.
> 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.

Done.

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

Neither the previous text, nor your text, are exactly right, because it really comes down to an implementation issue. Changed the title to s/Infinite Stacking/Mutual Recursion/ and changed the text about “an infinite number of encapsulation headers” to say "It is easy to see that this can have no good outcome” which IMO is both clear and sufficient.

> 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

Done. Also all following ones, I’m going to stop acking these individually.

> [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?

We removed that bit.

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

Rewrote more generically.

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

We removed the “valid” stuff, but that’s not the point here. You’re pointing out the more general fact that if someone subverts the control plane, they can cause us to send packets to the wrong place, with the wrong headers. This is true, but I don’t see how it’s unique or special to the V bit compared to everything else.

In general it’s my understanding that Security Considerations sections should identify considerations that are introduced by the present spec, that differ from what’s gone before, we don’t have to recapitulate every vulnerability in the history of networking. “If you hack the control plane, you can cause bad things” seems to be in the latter category. The SC section does try to focus on diversion/subversion attacks that are unique to this solution and what can be done to mitigate them.

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

Refactored as you hint.

> 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?

Now says

   o  If the TLV does not contain an Embedded Label Handling sub-TLV, or
      if it contains an Embedded Label Handling sub-TLV whose value is
      2, the embedded label is copied into the lower 3 octets of the
      virtual network identifier field of the encapsulation header.

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

Now says

   … Avoidance of this
   unfortunate outcome is a matter of network planning and design, and
   is outside the scope of this document.

Which kind of sucks, but it’s reality, as you hint.

> 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?

It’s BGP, nothing is safe. ;-) But seriously, I’m not sure what normative thing we would say about this.

> 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)?

What’s special about option b in this context is that in option b, the label value associated with the prefix is overwritten at the inter-AS boundary. So, if the tunnel is crossing into a different AS, you’re probably not going to be happy if you use the label it offers you — it’s kind of a “cross the streams” case of the control planes of the two different ASes, without the remapping that’s supposed to happen at the option b border. I think in general the profile of cases where you get in trouble, is when you signal a tunnel endpoint from one namespace into a different one. As long as the names (addresses, labels, SIDs, whatever) you’ve signaled are only exposed at the endpoints, it’s all good, but if they’re going to be exposed in transit, you’d better be sure they’re being interpreted in the correct namespace or things will go sideways.

I changed “not supported” to “not recommended”, which is probably more accurate. 

> [] "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.

We added some text to address this, also made filtering by default specific to EBGP.

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

I think we’ve resolved this with the 3.1.1 rework.

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

We renamed this to “Validation and Error Handling”. We didn’t do a total shred-and-relocate job, but did slim it down some. I do see your point, but I’m not eager to bulk up section 6 even more, and that’s the most logical place to move the stuff to. That section is long enough already, and it probably does the reader no favors to clutter it up with exception cases and caveats. I did add a forward reference though.

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

Fixed, now it just says to do treat-as-withdraw if the lengths are broken.

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

Done.

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

We just turned the whole works into treat-as-withdraw.

> [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"?

Fixed.

> [] 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.

Do we? I think this may be a case of “perfect is the enemy of good”.

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

Done.

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

Removed the example.

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

Changed to MUST.

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

I don’t agree — as you’ve noted elsewhere in your review, it’s better to spec something in one place; and there’s already a place where we spec what to do with unrecognized sub-TLVs (and it’s more than just “be ignored”). And since that place is literally one paragraph above, it seems not too onerous for the reader.

> [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?

Rewritten without reference to AFI/SAFI:

   In general, if a TLV contains a sub-TLV that is malformed (e.g.,
   contains a length field whose value is not legal for that sub-TLV),
   the sub-TLV should be treated as if it were an unrecognized sub-TLV.
   This document specifies one exception to this rule -- if a TLV
   contains a malformed Tunnel Egress Endpoint sub-TLV (as defined in
   Section 3.1), the entire TLV MUST be ignored, and MUST be removed
   from the Tunnel Encapsulation attribute before the route carrying
   that attribute is distributed.

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

Restructured as suggested.

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

Reordered as suggested.

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

Done.

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

Done. In the course of doing this, also reorganized the IANA section a little bit, to remove the repetitive “IANA is asked to…”. They know we’re asking them, we can just say it once, and without so much painful passive voice.

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

Fixed.

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

Done.

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

Fixed, renamed as “Fred”. Only joking.

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

Removed Prefix SID, and the others, since as you say, they’re already in there.

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

Done.

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

Done.

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

Done.

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

Done.

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

Added a reference to RFC 4272’s coverage, also discussed it in line.

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

I think this is covered by the rewrite of 3.1.1 and the Security Considerations section.

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

Again, I think this is cleared up by the rewrite.

> 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?

That text is gone.

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

That text is gone.

> 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?

That text is gone.

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

That text is gone.

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

That text is gone.

> [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?

That text is gone.

> 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

All your reference notes are taken care of.

> 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://urldefense.com/v3/__https://www.rfc-editor.org/info/rfc4760__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzyUC899-A$ >.
> 
> == 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://urldefense.com/v3/__https://www.rfc-editor.org/info/rfc5512__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzzh4qtmgg$ >.
> 
> [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://urldefense.com/v3/__https://www.rfc-editor.org/info/rfc5566__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzw5cMSHmQ$ >.
> 
> [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://urldefense.com/v3/__https://www.rfc-editor.org/info/rfc7510__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzy8w5rDGA$ >.
> 
> [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://urldefense.com/v3/__https://www.rfc-editor.org/info/rfc2474__;!!NEt6yMaO-gk!SkdzZX_sX8T7ecXyiST0nkn-gI4-tbIWX0j3GLlXwCaSEUDih59SOzwPU8MNdA$ >.
> 
> [major] This reference should be Normative.