Subject: [Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06

From: Martin Björklund <[email-masked]>
Date: Mon, 15 Oct 2018 01:59:59 -0700

Reviewer: Martin Björklund
Review result: Ready with Issues

This is my YANG doctor review of draft-ietf-softwire-yang-06.  The
review focuses on YANG-specifics only, since I am not familiar with
the technology that is modelled.

o  I would like to see all Tom Petch's comments in his three replies
   to the IETF LC for this document addressed.  Especially his comment
   about using ianatf:tunnel.

o  All three YANG modules should follow the common template for IETF
   YANG modules found in
   https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis.

   Specifically, fix the authors list and copyright text.

o  The term "instance" is used to mean - I think - the "device".  I
   didn't find this term in the RFCs 7596, 7597 or 7599.  I suggest
   you use some other term, since "instance" is quite generic, and is
   often used to refer to instances of YANG data nodes.

   Also, does the term "instance" mean the same thing in
   "algo-instance"?  And in "br-instances"?  "bind-instance"?

o  In ietf-softwire-common:

  grouping algorithm-instance {
    description
      "Indicates that the instance supports the MAP-E and MAP-T
      function. The instance advertises the MAP-E/MAP-T feature
      through the capability exchange mechanism when a NETCONF
      session is established.";

  This description does not seem right.  A grouping can't indicate
  anything.  Also, what is "the MAP-E/MAP-T feature"?

o  In ietf-softwire-ce:

  A similar description is found here:

        container algo-instances {
          description
            "Indicates that the instances supports the MAP-E and MAP-T
            function. The instances advertise the MAP-E/MAP-T
            feature through the capability exchange mechanism
            when a NETCONF session is established.";

  same comments apply.

o  In ietf-softwire-common:

    container algo-versioning {
      description "algorithm's version";
      leaf version {
        type uint64;
        description "Incremental version number for the algorithm";
      }
      leaf date {
        type yang:date-and-time;
        description "Timestamp to the algorithm";
      }
    }

  Maybe these descriptions are crystal clear to someone who knows the
  technology.  If so, perhaps add a reference to help the rest of us?

o  In general, it would help if the definitions had "reference"
   statements to the relevant sections in the underlying RFCs.

o  In ietf-softwire-ce:

  notification softwire-ce-event {
    if-feature binding;
    description "CE notification";
    leaf ce-binding-ipv6-addr-change {
      type inet:ipv6-address;
      mandatory true;
      description
        "If the CE's binding IPv6 address changes for any reason,
        it should notify the NETCONF client.";
    }

  Perhaps rewrite the description to:

    This notification is generated whenever the CE's binding IPv6
    address changes for any reason.

o  In ietf-softwire-br:

  You have:

            leaf softwire-num-max
            leaf softwires-payload-mtu
            leaf softwire-path-mru

   Any reason it isn't "softwire-payload-mtu"?

o  In ietf-softwire-br:

          list bind-instance {
            key "id";
            [...]
            container binding-table-versioning { ... }
            leaf id { ... }

  I suggest you put the leaf "id" before the container; it is common
  practise to put the key leafs first.

  Also, the leaf id has the description:

      description "An instance identifier.";

  This again can be confused with YANG's "instance-identifier".

  Also, it seems each "instance" has a numerical id (the key), but
  also a name (a string).  Maybe there are protocol-reasons to do
  this, but if not, did you consider using the "name" as key instead?

o  In ietf-softwire-br:

  notification softwire-binding-instance-event {
    if-feature binding;
    description "Notifications for binding instance.";

  notification softwire-algorithm-instance-event {
    if-feature algorithm;
    description "Notifications for algorithmic instance.";

  I suggest that the descriptions are updated to describe when these
  notifications are generated.

o  Appendix A

  The examples have:

    <softwire-config xmlns="urn:ietf:params:xml:ns:yang:ietf-softwire-br">

  which isn't present in the module.  Maybe a leftover?

o  Formatting

  To save the RFC editor some cycles, I suggest you format the modules
  consistently wrt. whitespaces/newlines and indentation.  One option
  is to run:

    pyang -f yang --keep-comments <filename>

  and then manually fix the long leafref paths that pyang can't handle
  properly at the moment.

/martin

---

From: <[email-masked]>
Date: Tue, 23 Oct 2018 05:59:53 +0000

Hi Martin,

Thank you for the review. 

We released a new revision -08 to address your comments. We will double check the formatting and issue another iteration if needed. 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Martin Björklund [mailto:[email-masked]]
> Envoyé : lundi 15 octobre 2018 11:00
> À : [email-masked]
> Cc : [email-masked]; [email-masked]; [email-masked]
> Objet : Yangdoctors last call review of draft-ietf-softwire-yang-06
> 

[Med] This is fixed in -07. A new tunnel-type parameter is defined to handle this. 

>    Specifically, fix the authors list and copyright text.

[Med] Done. 

> 
> o  The term "instance" is used to mean - I think - the "device".

[Med] It is used to mean function rather than device. A device may enable multiple instances of the same function. 

  I
>    "algo-instance"?  And in "br-instances"?  "bind-instance"?
[Med] algo-instance means an instance of type algorithm. br-instances denotes a set of br instances, and bind-instance means an instance of type binding. 

>   anything.  Also, what is "the MAP-E/MAP-T feature"?

[Med] Fixed.

>             when a NETCONF session is established.";

[Med] Fixed. 

>   technology.  If so, perhaps add a reference to help the rest of us?

[Med] This is used for logging purposes. A reference to RFC7422 is added. 

>     address changes for any reason.

[Med] OK.

>    Any reason it isn't "softwire-payload-mtu"?

[Med] No. Fixed already in -07.

>   practise to put the key leafs first.

[Med] Fixed. 

>   This again can be confused with YANG's "instance-identifier".

[Med] chnaged to:  

              "A binding instance identifier.

               This identifier can be automatically assigned
              or explicitly configured.";

>   this, but if not, did you consider using the "name" as key instead?

[Med] id/name is inspired from how NAT instances are managed (see RFC7659). The name is optional. 

>   notifications are generated.

[Med] Done.  

>   which isn't present in the module.  Maybe a leftover?

[Med] Ooops. We used to have that before moving to an nmda-compliant module.

>   and then manually fix the long leafref paths that pyang can't handle

[Med] Will double check this one.

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 23 Oct 2018 10:05:15 +0200 (CEST)

Hi,

<[email-masked]> wrote:
> We released a new revision -08 to address your comments. We will
> double check the formatting and issue another iteration if needed.

Thank's for addressing my comments.  See inline and at the end for
some new comments on -08.

> [Med] This is fixed in -07. A new tunnel-type parameter is defined

I think that the addition of identities for various tunnel types that
derive from ift:tunnel is the right thing to do.

However, since these identities derive from ift:tunnel, the
augmentation of ietf-interfaces in ietf-interface-tunnel is not
needed.  Instead, the new identities should be used as if:type
directly.  For example, instead of:

  <interface xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">
    <name>lw4o6-wan</name>
    <type>ianaift:tunnel</type>
    <tunnel-type
      xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type">
      iana-tunnel-type:aplusp
    </tunnel-type>
    ...
  </interface>

you should do:

  <interface>
      xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type">
    <name>lw4o6-wan</name>
    <type>iana-tunnel-type:aplusp</type>
    ...
  </interface>

So, I think you should remove the ietf-tunnel-type module.

An additional comment on the identities in iana-tunnel-type; for each
identity, I think you should add a "reference" statement that points
to the RFC(s) that define the tunnel.  (available in the IANA registry
at
https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-5)

> [Med] It is used to mean function rather than device. A device may
> enable multiple instances of the same function.

But you have for example in the description of "binding-mode":

      This feature indicates that the instance functions as a binding
      based softwire instance.

And in container algo-instances you have:

            The instances advertise the MAP-E/MAP-T
            feature through the capability exchange mechanism
            when a NETCONF session is established."

Unless your intentation is that one "instance" == one "function" ==
one NETCONF server, then this text is not correct.

So I am a bit confused - if the device advertises the feature
"binding-mode" it means that "it functions as a binding based softwire
instance".  Maybe you mean something along the lines of

      This feature indicates that the network element can function as
      one or more binding based softwire instances.

(I don't know if you want to use the term "network element" or
something else.)

Also there is similar text for the features map-e and map-t.

Anyway, if this meaning of the word "instance" is defined somewhere,
I suggest you add a reference to that other doc; or else explain this
meaning in 1.1.

> [Med] algo-instance means an instance of type
> algorithm. br-instances denotes a set of br instances, and
> bind-instance means an instance of type binding.

I could guess that.  I think the issue is when the word "instance" is
used unqualified. 

> >             when a NETCONF session is established.";

This text is still present, with a minor change:

            "Indicates that the instances supports the MAP-E and/or MAP-T

But since the container "algo-instances" is a non-presence container,
it can't "indicate" anything.  This text needs to be rephrased.

> [Med] This is used for logging purposes. A reference to RFC7422 is added. 

Ok.  I still don't really understand how it is supposed to be used.

When you write "version number for the algorithm", do you mean
"version number for this algo-instance"?

These are config true leafs; should they be config false and
internally managed by the server?  If not, I suppose an operator can
set them to any suitable values?   If so, what does "incremental
version number" really mean?  Is the server supposed to reject a value
that is not "incremental"?

[...]

>    RFC7659). The name is optional.

Well, in MIBs instances are normally identified with integers b/c of
how the protocol (SNMP) works.  In YANG modules, we usually use a
"name" that is a string to identify instances.  With a string, the
operator can choose meaningful names, and use them in other leafrefs,
instead of having to remember how the names are mapped to integers.

(Compare ifIndex (MIB) w/ interface/name (YANG))

So I suggest you use "name" as key.

o  I also note that you have changed the name of some lists in -08,
   e.g., list "bind-instance" is now list "bind-instances"
   (plural). Another example is:

          +--rw algo-instances
             +--rw algo-instances* [id]

   I think you should change these back to singluar; that's what YANG
   modules typically use.

/martin

---

From: <[email-masked]>
Date: Tue, 23 Oct 2018 09:40:16 +0000

Re-,

Please see inline. 

Cheers,
Med

> De : Martin Bjorklund [mailto:[email-masked]]
> Envoyé : mardi 23 octobre 2018 10:05
> À : BOUCADAIR Mohamed TGI/OLN
> Cc : [email-masked]; [email-masked]; draft-ietf-softwire-
> [email-masked]; [email-masked]
> Objet : Re: Yangdoctors last call review of draft-ietf-softwire-yang-06
> 
> Hi,
> 

[Med] OK, thanks

> needed. 

[Med] ietf-interface-tunnel tries to preserve a similar structure as in https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib, but you are right we can get rid of it. 

 Instead, the new identities should be used as if:type

[Med] OK. 

> https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-5)

[Med] I guess you meant https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-6. Fixed in my local copy, except form IPHTTPS for which we don't have an RFC.

>       one or more binding based softwire instances.

[Med] This is it. Updates when appropriate. 

> (I don't know if you want to use the term "network element" or

[Med] Network element is fine. 

> Also there is similar text for the features map-e and map-t.

[Med] Yes. 

> I suggest you add a reference to that other doc; or else explain this

[Med] added to the terminology section: 

   A network element may support one or multiple instances of a softwire
   mechanism; each of these instances may have its own configuration and
   parameters. 

> I could guess that.  I think the issue is when the word "instance" is

[Med] Updated to avoid unqualified "instances" 

> it can't "indicate" anything.  This text needs to be rephrased.

[Med] Updated accordingly. 

> "version number for this algo-instance"?

[Med] What is meant is:

          "Incremental version number for the mapping 
           algorithm rules provided to the algorithm instance"; 

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. The timestamp is used as a key to find the appropriate algorithm that was put into effect when an abuse occurred. 

Updated the description among these lines. 

> internally managed by the server? 

[Med] This can be generated by the server or set by an operator. 

 If not, I suppose an operator can
> that is not "incremental"?

[Med] What is important is to have a unique version number, how is set is not important. Incremental seems to be straightforward, but one may envisage other ways to manage versions. I deleted "incremental". 

> So I suggest you use "name" as key.

[Med] If you think this is better, I'm fine with that. 

>    I think you should change these back to singluar; that's what YANG

[Med] Actually, this was a comment from Tom. Perhaps, we misunderstood it. OK to change it back if this is the recommended practice.

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 23 Oct 2018 15:30:03 +0200 (CEST)

Hi,

Thanks for the quick update!

I have looked at -11, and have just a few minor comments.

o  Section 5.1

  Maybe the tree diagram needs to be re-generated; at least:

           |     +--rw bind-instance* [id]

  should be

           |     +--rw bind-instance* [name]

o  Section 8

            leaf softwire-num-max {
              type uint32;
              must ". >= 1";

  This should be:

            leaf softwire-num-max {
              type uint32 {
                range "1..max";
              }

o  Section 8

  Since you now have "name" as key in the lists, is the leaf "id"
  still needed?

/martin

---

From: <[email-masked]>
Date: Tue, 23 Oct 2018 13:42:27 +0000

Re-,

Fixed the first two ones in my local copy. 

id is optional. I'm maintaining it because it is already used by some implementations. 

Thank you again for the review. Much appreciated!

Cheers,
Med

> Envoyé : mardi 23 octobre 2018 15:30
> > [Med] ietf-interface-tunnel tries to preserve a similar structure as in
> https://www.iana.org/assignments/ianaiftype-mib/ianaiftype-mib, but you are
> right we can get rid of it.
> >
> > [Med] I guess you meant https://www.iana.org/assignments/smi-numbers/smi-
> numbers.xhtml#smi-numbers-6. Fixed in my local copy, except form IPHTTPS for
> which we don't have an RFC.
> >
> > >
> > >
> > 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. The timestamp is used as a key to find the appropriate algorithm
> that was put into effect when an abuse occurred.
> >
> > [Med] What is important is to have a unique version number, how is set is
> not important. Incremental seems to be straightforward, but one may envisage
> other ways to manage versions. I deleted "incremental".
> >
> > >
> > > [...]
> > >
> > [Med] Actually, this was a comment from Tom. Perhaps, we misunderstood it.
> OK to change it back if this is the recommended practice.
> >
> > >
> > >
> > > /martin
> >

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 24 Oct 2018 08:31:45 +0200 (CEST)

Hi,

> id is optional. I'm maintaining it because it is already used by some

Ok, but these implementations propably need to change anyway since the
"id" is no longer the key.

I will just point out that having one key and another integer based
identifier that doesn't serve any purpose looks a bit odd.  But if the
WG thinks that it is needed then that's fine (although in this case I
suggest you add some text to the descriptions of these leafs that
explain what the purpose is).

I had a closer look at the iana-tunnel-type module and the
instructions to IANA, and I think that you could make some minor
clarificiations:

In the module description, you have:

        This module contains a collection of YANG data types defined
        by IANA and used for tunnel types.

perhaps write:

        This module contains a collection of YANG identities defined
        by IANA and used as interface types for tunnel interfaces.

And in the IANA Considerations section you have:

   "base":        Contains the value of the tunnel type in lowercase.

maybe instead

   "base":        Contains the string "ift:tunnel".

/martin

> > > [Med] I guess you meant
> > > https://www.iana.org/assignments/smi-numbers/smi-

---

From: <[email-masked]>
Date: Wed, 24 Oct 2018 06:50:37 +0000

Hi Martin,

Please see inline. 

Cheers,
Med

> Envoyé : mercredi 24 octobre 2018 08:32

[Med] Yes.  

>         by IANA and used as interface types for tunnel interfaces.

[Med] Works for me. 

>    "base":        Contains the string "ift:tunnel".

[Med] That text is referring to the table in https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-6. 

We can change the text for better clarity to: 

"base":        Contains the name assigned to the tunnel type, in lowercase.

> > > > >       xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-
> tunnel-
> > > type">
> > > > >       iana-tunnel-type:aplusp
> > > > >     </tunnel-type>
> > > > >     ...
> > > > >   </interface>
> > > > >
> > > > > you should do:
> > > > >
> > > > >   <interface>
> > > > >       xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-
> tunnel-
> > > type">
> > > > >     <name>lw4o6-wan</name>
> > > > > > >             "Indicates that the instances supports the MAP-E and
> MAP-
> > > T

---

From: tom petch <[email-masked]>
Date: Wed, 24 Oct 2018 15:57:46 +0000

Martin

I am still catching up with all the changes, and it is that time - IETF
meeting time - when the world seems to spin a little faster yielding
four revisions in a day!

One query for you; are you ok with the map-e and map-t features being
defined separately in each module?  I had thought to put them in the
common module and then import them, using a prefix.

I had also thought to have a map feature from which map-e and map-t were
derived but that seems unnecessarily complicated.

Tom Petch

----- Original Message -----
From: "Martin Bjorklund" <[email-masked]>
To: <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>;
<[email-masked]>; <[email-masked]>
Sent: Wednesday, October 24, 2018 7:31 AM
Subject: Re: Yangdoctors last call review of draft-ietf-softwire-yang-06

Hi,

perhaps write:

maybe instead

/martin

> > > > > > Cc : [email-masked];
[email-masked];
> > > > [email-masked]
> > > > > >    to the IETF LC for this document addressed.  Especially
his
> > comment
> > > > > [Med] This is fixed in -07. A new tunnel-type parameter is
defined
> > > > > to handle this.
> > > [Med] ietf-interface-tunnel tries to preserve a similar structure
as
> > > in
> > > >   <interface
xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">
> > > >     <type>ianaift:tunnel</type>
xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-
> > > >   <interface>
xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-
> > > > to the RFC(s) that define the tunnel.  (available in the IANA
registry
https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-
> > numbers-5)
> > > > > >             "Indicates that the instances supports the MAP-E
and MAP-
> > > > > [Med] This is used for logging purposes. A reference to
RFC7422 is
> > added.
> > > [Med] What is important is to have a unique version number, how is
set
> > > is
> > > > > [Med] id/name is inspired from how NAT instances are managed
(see
> > > > >    RFC7659). The name is optional.
> > > >
> > > [Med] Actually, this was a comment from Tom. Perhaps, we
misunderstood
> > > it.