Hi, all, One additional transport suggestion:
- it would be useful to include recommended configurations for TCP connections. Given these are multi-byte request/response exchanges, Nagle should be disabled, e.g.
Joe On 2/13/2012 2:00 PM, Joe Touch wrote:
Hi, all, I've reviewed this document as part of the transport area directorate's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors for their information and to allow them to address any issues raised. The authors should consider this review together with any other last-call comments they receive. Please always CC tsv-...@ietf.org if you reply to or forward this review. This request was received Feb. 2, 2012. This document describes an extension to DHCPv4 for the bulk query and transfer of current lease status over TCP. The following transport issues were noted, and are significant: UPDATES- The document updates DHCP with support for TCP, and as such this document seems like it should UPDATE RFC2131 PORT USE- Although DHCPv4 has an assigned TCP port, this document should clearly indicate that there is no other specified use of that port other than the bulk lease query described in this document It should further explain why no other existing DHCP exchanges are not valid on the TCP port. CONNECTION MANAGEMENT- The document describes the use of TCP connections for bulk transfer, but needs to be more specific about which side (relay client or server) closes the connection, under what circumstances, and with what mechanism (e.g., graceful CLOSE vs. ABORT, as per RFC 793) sec 7.3 indicates some conditions for terminating connections; this section should indicate which side performs this, and by what method (CLOSE, ABORT) this sec also allows connections to stay open after all pending transactions are complete (MAY); the rationale for this should be given, or the connection MUST be closed. the same issue applies to sec 7.8 and 8 throughout; sec 8.5 is particularly problematic on this issue because it discusses aborting a request using in-band data, which may not be available if the connection is closed using ABORT. the state of other pending connection shsould be discussed too. TIMEOUTS- Sec 6.3 defines a timeout for the TCP connection; is this intended to supercede any TCP timeout? or is it intended to be the min of the TCP timeout and the specified one? This section should more carefully explain behavior when a connection is dropped and the reason - e.g., timeout, abort, close. INTERLEAVING- sec 7.7 says that the server MUST interleave replies; there doesn't seem a valid reason for this requirement. clearly the receiver MUST tolerate interleaved replies. having the server interleave replies is relevant only if each request/reply has its own timeout -- which should be overridden if there is another response in progress anyway. This issue should be more clearly explained and motivated. There were some other issues noted in this document. These comments appear below, and although not focused on transport issues, they represent significant issues that IMO should be addressed as well. NOTE - regarding some terminology issues, I did not survey current DHCP RFCs for consistency, but IMO these terms should be corrected even if they are then inconsistent with existing specs. Joe ----------- Major non-transport issues: - In many places the doc allows inaction to substitute for either positive or negative confirmation. IMO, this invites implementation errors, and should be avoided. E.g., status return codes, data source option missing, query-for-all-configured-IPs, etc. - the data source option reserved codes need more detailed specification. if these are intended for future use, then they MUST be ignored by the receiver (at least). if they are to mean anything at all, at least one bit (typically all of them) MUST be set to zero by the transmitter for implementations that do not support any of the component fields. further, the length of this option MUST be 1 - The protocol supports bulk transfer that is not data driven. This could represent a security vulnerability by exposing information that may not be on the data path (and thus already accessible) to a relay agent. This should be discussed in the security considerations section. - Integer quantities should be referred to as "unsigned 32-bit integers" throughout. - "VPN" is used throughout to refer to "private" addresses (RFC 1918); a VPN is not just private addressing. - (this is a nit with all IDs, FWIW) SHOULD and SHOULD NOT are used throughout without context. Any time SHOULD is used instead of MUST (or SHOULD NOT rather than MUST NOT), it is useful to explain a viable exception. If no such exception exists, the rationale for selecting SHOULD over MUST should be included. - It would be useful to explain why STATUS-CODE strings MUST NOT be null-terminated; is that a protocol violation, or are you just saying that NULLs are valid in these strings? the description should be clear that the string field describes the string length without any termination characters. - start-time-of-state is expressed as an offset from base-time; this appears to be the only time indicated as an offset rather than as an absolute. That inconsistency invites implementation errors; IMO, this should be absolute too. - option lengths: throughout, the doc refers to option lengths as being "an octet"; they are *indicated* in one octet. Some are constant (e.g., DCHP-STATE), some allow the contents of the octet to vary. Again, this is an *unsigned integer* octet. - some of the information provided (in DHCP-STATE) goes beyond that of DHCP. It would be useful to motivate the need for this information in a bulk query, and why it is not similarly available for nodes using UDP (e.g., as an extension to DHCPv4, not just to the bulk transfer command). again, absence of state information should not indicate state. State should always be expressed explicitly. these states are further meaningless without a state diagram explaining them. if such a diagram is not possible (as noted at end of sec 6.2.7), then the states are irrelevant and the option should not be included. - in sec 6.2.9 the term "not allowed" should be explained - are they reserved for future use and thus ignored? or are they "not allowed" - in which case the doc should indicate handling if they appear. - sec 7.4 states that the clock skew of zero is desired; this assumes E2E delays under 1sec. An explanation of why this is desired should be given, as well as the consequences of it not. Other non-transport issues: - The document includes definitions and references to irrelevant deployment and implementation issues, notably DSLAMs, concentrators, and access concentrators. These should not appear formally; they should be used only to usefully illustrate currently intended uses. - The doc refers to "real-time", which could imply requirements not supported. This should be replaced with "rapid". - "absolute time" *indicates* (rather than contains) the number of seconds... - "third party agent" should be explained, i.e., neither DHCP client nor DHCP server. - "downstream" and "upstream" should be defined as both away from the server *and towards the client* ("away from the server" has two directions). - sec 3 should refer to relays, and returning the entire set or individual bindings; there is no reason to explain the goals in terms of access concentrators. - sec 3.2 appears to provide contradictory advice - caching is required, but should be avoided? it would be useful to resolve this inconsistency. - sec 3.3 refers to 'fast path', but this term doesn't make sense in this context because fast-path is a forwarding issue. it would be useful to explain what you mean, and pick a different term ----
_______________________________________________ Ietf mailing list Ietf@ietf.org https://www.ietf.org/mailman/listinfo/ietf