Re-, Thank you for the review. Please see inline. Cheers, Med > -----Message d'origine----- > De : tom petch [mailto:daedulus@xxxxxxxxxxxxx] > Envoyé : mercredi 3 octobre 2018 13:57 > À : tom petch; ietf > Cc : softwires@xxxxxxxx; softwire-chairs@xxxxxxxx; jiangsheng@xxxxxxxxxx; > draft-ietf-softwire-yang@xxxxxxxx > Objet : Re: Last Call: <draft-ietf-softwire-yang-06.txt> (YANG Modules for > IPv4-in-IPv6 Address plus Port Softwires) to Proposed Standard > > And some technical comments on this I-D as a result of which, I do not > think this is ready to progress; perhaps no show stoppers, just a lot of > changes are needed IMHO. The more I look into this I-D, the less I > understand (which may be my ignorance of YANG). > > This I-D is concerned with three tunnel types (Lw4o6, MAP-E, MAP-T). In > several places, you have > augment "/if:interfaces/if:interface" { > when "if:type = 'ianaift:tunnel'"; > This will augment for all tunnel types, not just those of this I-D. I > think you should have your own three (?) derived identities for the > three specific tunnels described here, all in the common module. [Med] Good point. A new parameter called tunnel-type is now added to the interface YANG module (augments 8343). > > The three YANG modules are for Customer Premises Equipment, Border Relay > and a common module. Both the first two define two features, binding and > algorithm, binding for Lw4o6 and algorithm for MAP-E/MAP-T - I do not > know if/how this duplication of features will work (and as I said > before, I am confused about support for 'MAP-E and MAP-T' versus 'MAP-E > or > MAP-T' both of which phrases appear in the I-D). As with tunnel types, > should > there be three features, should they be in the common module? (yes, and > yes) [Med] The feature indicates the support of MAP-E and/or MAP-T. the data plane clause is supposed to hint whether this is about MAP-E (encapsulation) or MAP-T (transaltion). I do agree that defining features for each of these two may be more simple. > > 2.2 > 'they should be imported here, as needed. ' > import is a YANG technical term as used in this I-D so I think the use > of it here wrong [Med] Fixed. > > ' The CE must already have minimal IPv6 configuration' > Could that also be IPv4 which, by definition, is going to be widespread > in the deployment? [Med] A+P mechanisms assumes that no IPv4 configuration is provided because A+P is meant to provide IPv4 service continuity over IPv6. > > 3.2 > softwire-path-mru: > what is the point of this field? I looked at RFC7596, RFC7597 and > RFC7599 > and could find no reference thereto. I went back to their references, > such as RFC4821, RFC2473, RFC1981, RFC6346 ... - no joy. I had thought > to suggest a reference was called for but seeing the complete absence of > any connection, I think that a substantial explanation is called for. > [Med] I completely lost the context why the document ended to have this in. I'm sure Ian has a record about this. > Likewise, I note that references to MTU are qualified with IPv4 but > references to MRU are not; rather, this object > 'set the maximum IPv6 softwire packet size that can be received ...' > when > 'the implementation is unable to calculate the correct IPv4 size ' > Really? > > 'IPv6 prefix type' or ''IPv6 address type'. > Well, no It can be > type ipv6-prefix or ipv6-address > [Med] Yeah! > This description of the ietf-softwire-ce module describes objects that > are not in the ietf-softwire-ce module, which confuses me. Rather they > are in the ietf-softwire-common module. I think you need a description > of the objects in the ietf-softwire-common module first and then moving > on to the two specific modules. The sense I get is that while splitting > into three makes sense, the consequences have not been thought through. [Med] Not sure which part you are referring to. Can you please explicit your comment? Thank you. > > The descriptions of objects here are (mostly) good, but do not appear in > the YANG module. Those in the YANG module are shallow by comparison and > should be at least as comprehensive as those in the body of the I-D; the > YANG module has to be able to survive on its own. > > 'The version number is incremented ' > Any constraints on the increment? one, a hundred, a million...? [Med] This is implementation-specific. As indicated in this example, a timestamp would be sufficient. > > 4.2 > As with 3.2, the descriptions here of the objects look fine, those in > the description clauses of the YANG module do not; they are thin by > comparison. > > 5 > leaf br-ipv6-addr { > mandatory true; > "The IPv6 address for of the binding BR."; > This is not quite English. [Med] Fixed. > And since this object is MAP-E only, should it be, can it be, mandatory? [Med] this is of lw4over6. Hence, the mandatory clause. > > leaf id { > type uint32; > mandatory true; > description "Algorithm Instance ID"; > Any constraints on how this 32-bit integer will be used? [Med] No, there is no constraint defined in existing RFCs. > > 6 > > leaf softwire-num-max { type uint32; > description > "The maximum number of softwires that can be created on > the binding BR."; > Any restriction on the range of this. Can it be zero? [Med] No restriction is set because we don't have such recommendation in softwire RFCs. Setting it to zero is equivalent to disabling the instance. > > / "Enables the generation of ICMPv6 errors messages/ > "Enables the generation of ICMPv6 error messages/ > [Med] Fixed. Thank you. > leaf icmpv4-rate { > leaf icmpv6-rate { > Can these validly be zero? Also, I think that there should be a > recommended value (the Transport Area are keen on such things) [Med] We'd love to add a reco here, but absent any authoritative RFC, we are avoiding to add it. > > leaf id { > type uint32; > mandatory true; > description "id"; > Not the most helpful of descriptions [Med] Yes, indeed. > > 'This must be initialized when the BR instance is configured' > Perhaps > "This must be reset to the current date-and-time when the BR instance is > configured" > [Med] This is better. > 'Notify the client that a specific binding entry has been > expired/invalid. > Perhaps > 'Notify the client that a specific binding entry has expired or is > invalid.' > [Med] Fixed. > leaf date { > type yang:date-and-time; > description "Timestamp to the algorithm"; > Not a helpful description IMHO > [Med] Updated the text. > leaf name { > type string; > description "The name for the instance."; > ditto; what is the namespace, how is the name used? > [Med] This is used to uniquely distinguish instances by their alias/name. > "Embedded Address (EA) bits are the IPv4 EA-bits in the IPv6 > address identify an IPv4 prefix/address (or part thereof) or > a shared IPv4 address (or part thereof) and a port-set > identifier. > This is not quite English [Med] s/identify/identifying. > > 8 > The IESG like to see TLS1.3 RFC8446 here [Med] Done. > > I have yet to review the examples, but if my suggestion above result in > changes, then the examples will change significantly. > > Tom Petch [Med] Many thanks for the detailed review .
