Hi Benjamin, Thank you for the review.
Please see inline. Cheers, Med > -----Message d'origine----- > De : Benjamin Kaduk [mailto:[email protected]] > Envoyé : mardi 8 janvier 2019 20:38 > À : The IESG > Cc : [email protected]; Sheng Jiang; softwire- > [email protected]; [email protected]; [email protected] > Objet : Benjamin Kaduk's Discuss on draft-ietf-softwire-yang-14: (with > DISCUSS and COMMENT) > > Benjamin Kaduk has entered the following ballot position for > draft-ietf-softwire-yang-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/iesg/statement/discuss-criteria.html > for more information about IESG DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-softwire-yang/ > > > > ---------------------------------------------------------------------- > DISCUSS: > ---------------------------------------------------------------------- > > This document has 7 listed authors/editors. Since, per RFC 7322, documents > listing more than five authors are unusaul, and seven is greater than five, > let's talk about the author count. > [Med] Will leave this one to our AD. > The binding-table-versioning container's "version" leaf is of type uint64 > but the in-module description indicates that it is a timestamp. If it is > actually supposed to be a timestamp, then the units and zero time need to > be specified, but it seems more likely that this should just be described > as an abstract version number, if I understand the prose text about this > container correctly. > [Med] Thank you for catching this one. There is a copy/paste bug: OLD: container binding-table-versioning { description "binding table's version"; leaf version { type uint64; description "Timestamp when the binding table was activated. A binding instance may be provided with binding entries that may change in time (e.g., increase the size of the port set). When an abuse party presents an external IP address/port, the version of the binding table is important because, depending on the version, a distinct customer may be identified. The timestamp is used as a key to find the appropriate binding table that was put into effect when an abuse occurred. "; } leaf date { type yang:date-and-time; description "Timestamp of the binding table"; reference "RFC7422: Deterministic Address Mapping to Reduce Logging in Carrier-Grade NAT Deployments"; } } NEW: container binding-table-versioning { description "binding table's version"; leaf version { type uint64; description "A version number for the binding table."; } leaf date { type yang:date-and-time; description "Timestamp when the binding table was activated. A binding instance may be provided with binding entries that may change in time (e.g., increase the size of the port set). When an abuse party presents an external IP address/port, the version of the binding table is important because, depending on the version, a distinct customer may be identified. The timestamp is used as a key to find the appropriate binding table that was put into effect when an abuse occurred. "; reference "RFC7422: Deterministic Address Mapping to Reduce Logging in Carrier-Grade NAT Deployments"; } } > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Please expand CE on first usage. > > Section 4.1 > > It feels a little strange to put something as generic as > /if:interfaces/if:interface/if:statistics:sent-ipv4-packets in the > ietf-softwire-ce module. Are these counters likely to be useful for other > (non-softwire?) tunneling techniques? [Med] Some of these counters may be applicable to some other tunneling techniques, but not all of them. As such, these counters cannot be considered as generic. If in the future, a YANG module is to be defined for some tunneling technique and similar counters are also applicable fro that technique, that module can use the traffic-stat grouping defined in draft-ietf-softwire-yang. > > Section 5.2 > > o softwire-num-max: used to set the maximum number of softwire > binding rules that can be created on the lw4o6 element > simultaneously. This paramter must not be set to zero because > this is equivalent to disabling the BR instance. > > This seems to leave it ambiguous whether a server should reject an attempt > to set it to zero, or accept it but diable the BR instance. [Med] The text is clear, IMO. Furthermore, the range of allowed values is called out explicitly in the module: leaf softwire-num-max { type uint32 { range "1..max"; } ... > > Section 7 > > leaf enable-hairpinning { > type boolean; > default "true"; > description > "Enables/disables support for locally forwarding > (hairpinning) traffic between two CEs."; > reference "Section 6.2 of RFC7596"; > > Is a global toggle sufficient or would there be cases where more > fine-grained control would be needed? > [Med] A+P is designed to reduce as much as possible the per-subscriber state at the network/BR. Requiring fine-grained control would require some extra state to be maintained, which is not desired. Having the general parameter is sufficient. > Section 8 > > container algo-versioning { > [...] > leaf date { > > type yang:date-and-time; > description > "Timestamp when the algorithm instance was activated. > > An algorithm instance may be provided with mapping > rules that may change in time (for example, increase > the size of the port set). When an abuse party > presents an external IP address/port, the version > of the algorithm is important because depending on > the version, a distinct customer may be identified. > > nit: "abuse party" is probably not a term that everyone is familiar with. > (similarly in br-instances) [Med] We used "abuse" in reference to what is discussed in RFC6269 : https://tools.ietf.org/html/rfc6269#section-13.1. We may add a pointer to that section if you think this is useful. > > Section 9 > > Is there any possibility of a situation where the > invalid-/added/modified-entry notifications cause a substantial amount of > notification traffic (i.e., a DoS level of traffic)? > [Med] This is in theory possible if the BR is under the control of a non-authorized/misbehaving entity. The DDoS can be softened by defining a notification interval, but given that this interval parameter can be disabled or set to a low value by the misbehaving entity, the same problem will be observed. _______________________________________________ Softwires mailing list [email protected] https://www.ietf.org/mailman/listinfo/softwires
