Re: [Softwires] Benjamin Kaduk's Discuss on draft-ietf-softwire-yang-14: (with DISCUSS and COMMENT)

[<[e-mail]>]

Hi Benjamin, 

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk [mailto:[e-mail]]
> Envoyé : mardi 8 janvier 2019 20:38
> À : The IESG
> Cc : [e-mail]; Sheng Jiang; softwire-
> [e-mail]; [e-mail]; [e-mail]
> 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.