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 < nore...@ietf.org> 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. 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".
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
