Lars Eggert has entered the following ballot position for draft-ietf-ipsecme-iptfs-14: Discuss
When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.) Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ for more information about how to handle DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-ipsecme-iptfs/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- # GEN AD review of draft-ietf-ipsecme-iptfs-14 CC @larseggert Thanks to Peter E. Yee for the General Area Review Team (Gen-ART) review (https://mailarchive.ietf.org/arch/msg/gen-art/VKlfYh3uoGomO4_Lv8e6kltl36g). ## Discuss ### Section 2.4.1, paragraph 3 ``` For similar reasons as given in [RFC7510] the non-congestion- controlled mode should only be used where the user has full administrative control over the path the tunnel will take. This is required so the user can guarantee the bandwidth and also be sure as to not be negatively affecting network congestion [RFC2914]. In this case, packet loss should be reported to the administrator (e.g., via syslog, YANG notification, SNMP traps, etc.) so that any failures due to a lack of bandwidth can be corrected. ``` There is a lot more nuance and there are a lot more restrictions in RFC7510 that also apply here. If a non-congestion-controlled mode is desired, especially without even using RFC8084 circuit breakers, similar mandatory text needs to be crafted for this mechanism. (I would also strongly suggest to require circuit breakers here.) ### Section 2.4.2, paragraph 0 ``` 2.4.2. Congestion-Controlled Mode ``` This mode adds a LOT of complexity to this mechanism. Is this really needed? Could not RFC8229 be used instead of defining an entirely separate congestion control scheme for (padded/fragmented) ESP? ### Section 2.4.2.1, paragraph 1 ``` In additional to congestion control, implementations MAY choose to define and implement circuit breakers [RFC8084] as a recovery method of last resort. Enabling circuit breakers is also a reason a user may wish to enable congestion information reports even when using the non-congestion-controlled mode of operation. ``` This makes no sense. If implemented in addition to CC, circuit breakers will never fire, because a functioning congestion control algorithm will always maintain a send rate below the circuit breaker threshold. What would make sense is to use circuit breakers in the non-congestion-controlled case, to provide a safety mechanism in cases the network provisioning changes for an active tunnel. ### Section 3.1, paragraph 0 ``` 3.1. ECN Support ``` There is a lot more nuance to this, as described in RC6040. This document needs to follow that existing standard rather than define another variant. ### Section 6.1.2, paragraph 9 ``` RTT: A 22-bit value specifying the sender's current round-trip time estimate in microseconds. The value MAY be zero prior to the sender having calculated a round-trip time estimate. The value SHOULD be set to zero on non-AGGFRAG_PAYLOAD-enabled SAs. If the RTT is equal to or larger than 0x3FFFFF the value MUST be set to 0x3FFFFF. ``` This can only encode RTTs of up to around four seconds. That is too little for Internet usage. (Same concern about other 22-bit microsecond values below.) ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- ## Comments ### Section 2, paragraph 0 ``` 2. The AGGFRAG Tunnel ``` This description in this section doesn't seem to accurately reflect the availability of a congestion-controlled mode of operation, it's only about CBR. ### Section 2.2.3.1, paragraph 1 ``` When the tunnel bandwidth is not being fully utilized, a sender MAY pad-out the current encapsulating packet in order to deliver an inner packet un-fragmented in the following outer packet. The benefit ``` If this MAY is not followed, the tunnel isn't a CBR tunnel anymore. Permitting that seems to contradict one of the main premises of this approach? ### Section 2.2.5.2, paragraph 0 ``` 2.2.5.2. ECN value ``` RFC6040 has updated the guidance in RFC4301 for ECN (see above, too.) ### Section 2.4.2, paragraph 1 ``` The required inputs for the TCP friendly rate control algorithm described in [RFC5348] are the receiver's loss event rate and the sender's estimated round-trip time (RTT). These values are provided by IP-TFS using the congestion information header fields described in Section 3. In particular, these values are sufficient to implement the algorithm described in [RFC5348]. ``` RFC5348 like most IETF congestion control mechanisms are sender-side. Is there a good reason to flip this around and do the computation on the receiver, which complicates the actual use of RFC5348? ### Section 2.4.2, paragraph 2 ``` the available tunnel bandwidth. An implementation choosing to always send the data MAY also choose to only update the LossEventRate and RTT header field values it sends every RTT though. ``` Why? Sending known stale data is worse than not sending any data. ### Section 4.1, paragraph 1 ``` Bandwidth is a local configuration option. For non-congestion- controlled mode, the bandwidth SHOULD be configured. For congestion- controlled mode, the bandwidth can be configured or the congestion control algorithm discovers and uses the maximum bandwidth available. No standardized configuration method is required. ``` For congestion-controlled mode, what is the point of configuring bandwidth? The CC algorithm will not use this at all. ### Section 4.3, paragraph 1 ``` Congestion control is a local configuration option. No standardized configuration method is required. ``` I don't understand what this is supposed to express? ### Section 6.1.2, paragraph 8 ``` E: A 1-bit value that if set indicates that Congestion Experienced (CE) ECN bits were received and used in deriving the reported LossEventRate. ``` What is the reason for signaling this? CC does not depend on this. ### Inclusive language Found terminology that should be reviewed for inclusivity; see https://www.rfc-editor.org/part2/#inclusive_language for background and more guidance: * Term `traditional`; alternatives might be `classic`, `classical`, `common`, `conventional`, `customary`, `fixed`, `habitual`, `historic`, `long-established`, `popular`, `prescribed`, `regular`, `rooted`, `time-honored`, `universal`, `widely used`, `widespread` * Term `native`; alternatives might be `built-in`, `fundamental`, `ingrained`, `intrinsic`, `original` ## Nits All comments below are about very minor potential issues that you may choose to address in some way - or ignore - as you see fit. Some were flagged by automated tools (via https://github.com/larseggert/ietf-reviewtool), so there will likely be some false positives. There is no need to let me know what you did with these suggestions. ### Section 2.4.2, paragraph 1 ``` practice RECOMMENDS that the algorithm conform to [RFC5348]. ``` "RECOMMENDS" is not an RFC2119 keyword. ### Grammar/style #### Section 2.2.3, paragraph 5 ``` mbers will not work for this document so the sequence number stream MUST incr ^^^ ``` Use a comma before "so" if it connects two independent clauses (unless they are closely connected and short). #### Section 2.2.3, paragraph 5 ``` w sizes. This is because packets outside of the smaller window but inside the ^^^^^^^^^^ ``` This phrase is redundant. Consider using "outside". #### Section 2.2.3.1, paragraph 3 ``` load is called an empty payload. Currently this situation is only applicable ^^^^^^^^^ ``` A comma may be missing after the conjunctive/linking adverb "Currently". #### Section 2.2.3.1, paragraph 4 ``` [RFC4301], AGGFRAG mode may and often will be encapsulating more than one IP ^^^^^^^^^^^^^ ``` The adverb "often" is usually put between "will" and "be". #### Section 2.2.7, paragraph 1 ``` e tunnel will take. This is required so the user can guarantee the bandwidth ^^^ ``` Use a comma before "so" if it connects two independent clauses (unless they are closely connected and short). #### Section 2.5, paragraph 3 ``` to have a round-trip time estimate. Thus the sender communicates this estimat ^^^^ ``` A comma may be missing after the conjunctive/linking adverb "Thus". #### Section 6.1.1, paragraph 7 ``` value of zero indicates no loss. Otherwise the loss event rate is 1/LossEven ^^^^^^^^^ ``` A comma may be missing after the conjunctive/linking adverb "Otherwise". #### Section 6.1.3.3, paragraph 3 ``` ffecting network congestion in a predictable way, and if it would be, then n ^^^^^^^^^^^^^^^^^^^^ ``` Consider replacing this phrase with the adverb "predictably" to avoid wordiness. ## Notes This review is in the ["IETF Comments" Markdown format][ICMF], You can use the [`ietf-comments` tool][ICT] to automatically convert this review into individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT]. [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md [ICT]: https://github.com/mnot/ietf-comments [IRT]: https://github.com/larseggert/ietf-reviewtool _______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
