Re: [Gen-art] Genart last call review of draft-ietf-masque-connect-udp-12

[David Schinazi <[e-mail]>]

Hi Christer,

Al and I had a productive conversation on one of the points that you had
brought up,
so I wanted to mention that here, see inline below.

Thanks,
David

On Tue, May 31, 2022 at 3:53 PM David Schinazi <[e-mail]>
wrote:

> Hi Christer,
>
> Thank you for your detailed review. I've written up the following PR to
> address your comments:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-udp/pull/173
> I also added additional comments inline below.
>
> Thanks,
> David
>
>
> On Sat, May 28, 2022 at 10:53 AM Christer Holmberg via Datatracker <
> [e-mail]> wrote:
>
>> GENERAL:
>> --------
>>
>> QG_1:
>>
>> The document contains a mix of "SHALL", "MUST", "SHALL NOT" and "MUST
>> NOT", but
>> I can't see any specifc pattern based on whether "SHALL (NOT)" or "MUST
>> (NOT)"
>> is used.
>>
>
> I tend to use SHALL for active requirements and MUST for reactive
> requirements.
> Since RFC 2119 defines those terms as strictly identical, this doesn't
> harm comprehension.
>
> ABSTRACT:
>> ---------
>>
>> QA_1:
>>
>> The text says:
>>
>> "This document describes how to proxy UDP in HTTP,"
>>
>> To me "proxy UDP in HTTP" sounds a little weird. Isn't "tunneling UDP over
>> HTTP" more appropriate? That terminology is also used later in the
>> document.
>>
>> (The same comment applies to "proxy TCP in HTTP")
>>
>
> The "proxy" terminology is the simple way of referring to these mechanisms
> which is suitable to casual readers. The "tunnel" terminology is more
> pedantic
> and best suited for readers expert in HTTP terminology. This is why we
> differentiate using the two by saying "more specifically".
>
> Section 1:
>> ----------
>>
>> Q1_1:
>>
>> The text says:
>>
>>    "While HTTP provides the CONNECT method (see Section 9.3.6 of [HTTP])
>>    for creating a TCP [TCP] tunnel to a proxy, it lacks a method for
>>    doing so for UDP [UDP] traffic."
>>
>> I suggest to remove the "it lacks..." part, because the following
>> paragraph
>> says that the document defines such method.
>>
>
> I've added "prior to this specification" to get the best of both worlds.
>
> Q1_2:
>>
>> The text says:
>>
>>   "This protocol supports all versions of HTTP"
>>
>> I think the text should say which is the latest supported, as there is no
>> guarantee future versions of HTTP will be supported.
>>
>
> Switched to "all existing versions of HTTP".
>
> Section 1.1:
>> ------------
>>
>> Q1_1_1:
>>
>> The txt says:
>>
>>    "Note that, when the HTTP version in use does not support multiplexing
>>    streams (such as HTTP/1.1), any reference to "stream" in this
>>    document represents the entire connection."
>>
>> I suggest to replace "Note that," with e.g., "In this document,", as in
>> the
>> paragraph above.
>>
>
> I personally prefer "note that". I'll let the RFC Editor have final say on
> such details.
>
> Section 2:
>> ----------
>>
>> Q2_1:
>>
>> I suggest placing the example URIs after the requirements and procedures
>> for
>> creating the URL.
>>
>
> As a first introduction to a concept, I personally find examples easier to
> understand than this very long list of requirements.
>
> Q2_2:
>>
>> The text says:
>>
>>    "If the client detects that any of the requirements above are not met
>>    by a URI Template, the client MUST reject its configuration and fail
>>    the request without sending it to the UDP proxy.  While clients
>>    SHOULD validate the requirements above, some clients MAY use a
>>    general-purpose URI Template implementation that lacks this specific
>>    validation."
>>
>> It sounds strange to say "MUST reject" while saying "SHOULD validate but
>> MAY
>> use a general-purpose URI Template implementation".
>>
>> I suggest to say "SHOULD validate the URI Template and reject it if it
>> does not
>> fulfil the requirements above".
>>
>
> The MUST is prefixed by an if clause. Conceptually this means: "if you
> notice an issue
> you have to report it, but you don't have to explicitly check for issues".
> What you propose
> would be "you should check and might report findings" which is not as
> strong.
>

We've now changed the text to match both the order you were suggesting and
the
normative requirements that I had intended:

    Clients SHOULD validate the requirements above; however, clients MAY
use a
    general-purpose URI Template implementation that lacks this specific
validation.
    If a client detects that any of the requirements above are not met by a
URI
    Template, the client MUST reject its configuration and fail the request
without
    sending it to the UDP proxy.

Section 3:
>> ----------
>>
>> Q3_1:
>>
>> (This comment might apply to other parts of the document too)
>>
>> The text says:
>>
>>   "To initiate a UDP tunnel associated with a single HTTP stream,
>>    clients issue a request containing the "connect-udp" upgrade token."
>>
>> I suggest to refer to a single entity for the procedure, i.e., "client"
>> instead
>> of "clients".
>>
>
> Agreed. I've changed this and some other parts of the draft.
>
> Section 3.1:
>> ------------
>>
>> Q3_1_1:
>>
>> The text says:
>>
>>    "Upon receiving a UDP proxying request:
>>
>>    *  if the recipient is configured to use another HTTP proxy, it will
>>       act as an intermediary: it forwards the request to another HTTP
>>       server.  Note that such intermediaries may need to reencode the
>>       request if they forward it using a version of HTTP that is
>>       different from the one used to receive it, as the request encoding
>>       differs by version (see below).
>>
>>    *  otherwise, the recipient will act as a UDP proxy: it extracts the
>>       "target_host" and "target_port" variables from the URI it has
>>       reconstructed from the request headers, and establishes a tunnel
>>       by directly opening a UDP socket to the requested target."
>>
>> It is unclear whether the rest of the Section text applies to both of
>> these
>> bullets, or only one of them.
>>
>
> The rest of the section specifically mentions the "UDP proxy" and that only
> applies to the second bullet. But perhaps we can do better, would you have
> a textual suggestion here?
>
> Also, the "(see below)" part in the first bullet refers to the following
>> Sections, but it is unclear. If refering to another section, I suggest
>> using
>> explicit Section references instead of "below".
>>
>
> I tried spelling out the four related subsections and it became unwieldy
> and less clear than "below".
>
> Sections 3.2, 3.3, 3.4 and 3.5:
>> -------------------------------
>>
>> I suggest to use sub-sections, with e.g., the following structure:
>>
>> 3.2.     HTTP/1.1 Usage
>> 3.2.1.   Request
>> 3.2.2.   Response
>> 3.3.     HTTP/2 and HTTP/3 Usage
>> 3.3.1.   Request
>> 3.3.2.   Response
>>
>
> I personally prefer the current structure.
>
> Section 4:
>> ----------
>>
>> The text says:
>>
>>   "This protocol..."
>>
>> What protocol? :) As this is the first sentence of the Section, please be
>> specific.
>>
>
> Agreed. Replaced "this protocol" with "The mechanism for proxying UDP in
> HTTP defined in this document".
>