Re: [Softwires] Last Call: <draft-ietf-softwire-yang-06.txt> (YANG Modules for IPv4-in-IPv6 Address plus Port Softwires) to Proposed Standard

[<[e-mail]>]

Re-,

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : tom petch [mailto:[e-mail]]
> Envoyé : mercredi 3 octobre 2018 13:57
> À : tom petch; ietf
> Cc : [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> 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 .