Re: [tsvwg] David Black's WGLC review of draft-ietf-tsvwg-rfc6040update-shim-08

["Black, David" <[e-mail]>]

Inline ...  two action items:


  1.  idnits - update IKEv2 reference, and check whether pre-5378 boilerplate is required.
  2.  RFC 8085 on UDP Guidelines is a BCP, and should be updated directly for that reason (vs. indirectly via an update of RFC 6040).  No other RFC that cites RFC 6040 would be affected, unless it's also a BCP.

Thanks, --David

From: Bob Briscoe <[e-mail]>
Sent: Friday, May 17, 2019 6:55 PM
To: Black, David; [e-mail]
Subject: Re: [tsvwg] David Black's WGLC review of draft-ietf-tsvwg-rfc6040update-shim-08


[EXTERNAL EMAIL]
David,

As promised, here's my response to the 'Minor' half of your review...
On 09/05/2019 02:08, Black, David wrote:
idnits found a couple of minor things:

  *   Current reference for IKEv2 is RFC 7296 instead of RFC 5996
  *   If any text was copied from pre-5378 RFCs, an additional boilerplate disclaimer is required.

[David>]  Please check these, especially whether the pre-5378 boilerplate is required.

Thanks, --David

From: Black, David
Sent: Wednesday, May 8, 2019 8:50 PM
To: [e-mail]<mailto:[e-mail]>
Subject: David Black's WGLC review of draft-ietf-tsvwg-rfc6040update-shim-08

This is a solidly written draft on updating tunnel protocols that use shim headers to propagate ECN according to RFC 6040.

Unfortunately, I found a major issue with the updates [1] along with a few minor issues (easily dealt with) and a bunch of editorial items.

The major issue [1] on updates creating non-compliant implementations looks like it will need WG discussion on what should be done, as the "right thing to do" appears to be in potential conflict with deployed "running code".

--- Major ---

[snip]

--- Minor ---

[A] Updated RFCs.

Need to be explicit about what the updates are to each of the updated RFCs.
Each update to each RFC within S.5.1 says exactly what text in the original RFC is updated and exactly what it becomes. Surely it couldn't be more explicit?


The updates are scattered throughout the draft - a possible approach is to
add a section that summarizes the update(s) to each RFC and provides pointers
to the section(s) that contain the update(s).
The normative updates for each RFC are all in section 5.1. under the subsection for each protocol. That's the only place those interested in that RFC will look. So that's where it is safest to put each update.

All updates are extremely explicit, and all concentrated in one subsection, so I'm not sure how these criticisms have arisen.

[David>] Ok for now.


[B] Section 4

      *  if it is known that the tunnel egress does not support
         propagation of the ECN field (RFC 6040, RFC 4301 or the full
         functionality mode of RFC 3168)

The parenthetical listing of RFCs is unclear - I think these all do support
ECN field propagation, so edits are needed to make this clear.

Sry, I've changed it to read:
      *  if it is known that the tunnel egress does not support any
         of the RFCs that define propagation of the ECN field (RFC 6040,
         RFC 4301 or the full functionality mode of RFC 3168)

[David>] Ok.


[C] Section 4

   For instance, copying the
   ECN field as a side-effect of copying the DSCP is a seriously unsafe
   bug that risks breaking the feedback loops that regulate load on a
   tunnel.

This text comes immediately after the text in issue [1].  The problem with
this text is that it's implicitly assuming that the tunnel egress discards
the outer header including any congestion indication that it may contain.

That needs to be stated/explained.
You're right. Without context this text is dangerous. Thx. I've changed it to:

If a tunnel ingress does not have any ECN logic, copying the ECN field as a side-effect of copying the DSCP is a seriously unsafe bug that risks breaking the feedback loops that regulate load on a tunnel.

Note, it didn't say it /will/ break; it just says it /risks/ breaking. The point is, if it doesn't have ECN logic, it cannot have checked the egress ECN capability. Indeed, if the ingress doesn't have ECN logic it's quite likely the egress doesn't either, assuming both ends of many tunnels are deployed at the same time.

[David>] I would add at the end "as it risks an ECN-unaware tunnel egress discarding ECN congestion indications as a consequence of discarding outer headers of packets whose inner header indicates ECN capability." to make the risk explicit to those who aren't as deeply involved in ECN as the two of us.


[D] Section 5

   Section 3.1.11 of the UDP usage guidelines [RFC8085] already explains
   that a tunnel that encapsulates an IP header directly within a UDP/IP
   datagram needs to follow RFC 6040 when propagating the ECN field
   between inner and outer IP headers.  The requirements in Section 4
   update RFC 6040 so, by reference, they automatically update RFC 8085
   to add the important but previously unstated requirement that, if the
   UDP tunnel egress does not, or might not, support ECN propagation, a
   legacy UDP tunnel ingress has to clear the outer IP ECN field to
   0b00, e.g. by configuration.

That's too clever/subtle - this draft should explicitly update RFC 8085.
Not happy about this. Followed to its logical conclusion, as well as updating RFC6040, this draft would then need to update every RFC that already normatively references RFC6040 (e.g. GUE, Geneve, etc).
[David>] No, RFC 8085 is different, as it's our primary UDP guidelines document for all protocol designers, not a specification of a specific protocol.  This is indicated by RFC 8085 being a BCP, whereas the protocol RFCs are not.

Wouldn't such an update get kicked out during IESG review, as a duplicate update? Certainly it might make the ID nits checker issue a high-pitched scream as it runs round a tight loop of "Updates" headers.
[David>] I think clarity wins over process here.  Update the BCP, please.

So, instead, how about stating the above para in the opposite order as follows:
   This document indirectly updates the UDP usage guidelines [RFC8085]
   to add the important but previously unstated requirement that, if the
   UDP tunnel egress does not, or might not, support ECN propagation, a
   legacy UDP tunnel ingress has to clear the outer IP ECN field to
   0b00, e.g. by configuration. Section 3.1.11 of RFC 8085 already explains
   that a tunnel that encapsulates an IP header directly within a UDP/IP
   datagram needs to follow RFC 6040 when propagating the ECN field
   between inner and outer IP headers. And the requirements in Section 4
   update RFC 6040 so, by reference, they indirectly update RFC 8085.

[David>] I think RFC 8085 needs to be explicitly updated, as we are definitely changing its BCP recommendations here.


[E] Have the L2TP changes been reviewed by L2TP experts?
Of course. Each protocol modification was done in collaboration with an expert on that protocol. I wouldn't have felt confident to modify all these protocols otherwise.

  *   L2TP update: Carlos was particularly helpful, but also Ignacio, and it was all properly discussed on the L2TPEXT list
  *   Similarly, CAPWAP in OPSAWG with Margaret Wasserman/Cullen, Benoit, Warren Kumari, Mahalingam Mani, Dorothy Gellert, Dorothy Stanley, Michael Montemurro. But once I dug into CAPWAP, it turned out not to require any changes.
  *   GRE was covered by INT-AREA, particularly Mohamed Boucadair. Early on INT-AREA and Alia Atlas did significant reviewing to find which protocols it should cover as well.
  *   See the ACKs section for the names of all the significant helpers.



[F] Have the AMT changes been reviewed by AMT experts?
Yup - text suggested by Jake Holland (also ACK'd).

Herding all these cats has involved a fair amount of work and cajoling, particularly 'cos it's unfamiliar territory for many people, and it's rarely their priority.

[David>] Thanks, just needed to double-check.  Once we get this draft revised, I'll send a note to the relevant protocol email lists for awareness.





--- Editorial ---

Abstract

It surveys widely deployed IP tunnelling
   protocols separated by such shim header(s)

separated by -> that use

Section 4

   Permanently zeroing the outer ECN field is safe, but it is not
   sufficient to claim compliance with RFC 6040 because it does not meet
   the aim of introducing ECN support to tunnels (see Section 4.3 of
   [RFC6040]).

"Permanently" is not the right word..  Suggest rephrasing start of paragraph to

   Zeroing the ECN field in the outer header of all packets is safe, ...
I've taken all the above verbatim.


IANA Considerations

The "[TO BE REMOVED: ..." text is not the "right thing to do" - do this instead:

OLD
   IANA is requested to assign the following L2TP Control Message
   Attribute Value Pair:
NEW
   IANA is requested to assign the following L2TP Control Message
   Attribute Value Pair in the Layer Two Tunneling Protocol "L2TP"
   registry (https://www.iana.org/assignments/l2tp-parameters/l2tp-<https://www..iana.org/assignments/l2tp-parameters/l2tp->
   parameters.xhtml):
END

IANA will edit this text after performing the assignment and can
remove the link if appropriate.
I followed RFC5226 "Guidelines for Writing an IANA Considerations Section in RFCs" verbatim:

         When

         referring to an already existing registry, providing a URL to

         precisely identify the registry is helpful.  All such URLs,

         however, will be removed from the RFC prior to final

         publication.  For example, documents could contain: [TO BE

         REMOVED: This registration should take place at the following

         location:  http://www.iana.org/assignments/foobar-registry]

[David>] Ok, this is editorial-only, so no change is fine ...

Bob



Thanks, --David
----------------------------------------------------------------
David L. Black, Senior Distinguished Engineer
Dell EMC, 176 South St., Hopkinton, MA  01748
[phone] New    Mobile: [phone]
[e-mail]<mailto:[e-mail]>
----------------------------------------------------------------




--

________________________________________________________________

Bob Briscoe                               http://bobbriscoe.net/