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

[Bob Briscoe <[e-mail]>]

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.
>
> Thanks, --David
>
> *From:* Black, David
> *Sent:* Wednesday, May 8, 2019 8:50 PM
> *To:* [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.

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



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

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

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.

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.



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

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




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/