Last Call Review of draft-ietf-nvo3-yang-cfg-05
review-ietf-nvo3-yang-cfg-05-yangdoctors-lc-lindblad-2021-09-16-00

This is the YANG Doctor review of draft-ietf-nvo3-yang-cfg. In my judgement the
draft is not ready for last call. This is not to say that there isn't plenty of
good work that has gone into this draft and YANG module, but since I have been
unable to compile it, due to unclear dependencies, the fundamental requirements
for performing a review aren't met.

As soon as you import another module, you become dependent on that module and
inherit any problems it may have. If the modules you depend upon are not stable
enough, it may not be viable to perform a last call review. As long as the
modules you depend upon are not published, I would think you cannot go through
with publication of your own module either.

There's always a bit of guesswork involved when a YANG module imports
unpublished modules. By looking in the IETF Git repository, under
"standards/ietf/RFC" as well as under
"experimental/ietf-extracted-YANG-modules", i.e. the section for automatically
extracted YANG modules from drafts, I was able to guess at which modules that
are transitively referenced. Based on this, I believe we will need to have
stable versions of the following modules before the draft-ietf-nvo3-yang-cfg
can proceed. Experimental modules are marked (*).

iana-bfd-types.yang (*)
iana-if-type.yang
ietf-bfd-types.yang (*)
ietf-bgp-common-multiprotocol.yang (*)
ietf-bgp-common-structure.yang (*)
ietf-bgp-common.yang (*)
ietf-bgp-l3vpn.yang (*)
ietf-bgp-neighbor.yang (*)
ietf-bgp-peer-group.yang (*)
ietf-bgp-policy.yang (*)
ietf-bgp-rib-attributes.yang (*)
ietf-bgp-rib-tables.yang (*)
ietf-bgp-rib-types.yang (*)
ietf-bgp-rib.yang (*)
ietf-bgp-types.yang (*)
ietf-bgp.yang (*)
ietf-inet-types.yang
ietf-interfaces.yang
ietf-ip.yang
ietf-key-chain.yang
ietf-l2vpn.yang (*)
ietf-netconf-acm.yang
ietf-network-instance.yang
ietf-pseudowires.yang (*)
ietf-routing-policy.yang (*)
ietf-routing-types.yang
ietf-routing.yang
ietf-tcp-common.yang (*)
ietf-tcp.yang (*)
ietf-yang-schema-mount.yang
ietf-yang-types.yang

Since I already did a first reading and found a few things, I'll list them here
for the WG's benefit rather than me holding them to myself.

1. YANG 1.1

The draft says "YANG [RFC6020] is a data definition language that was
introduced to". Since you are using yang-version 1.1, it would be more
appropriate with a reference to RFC 7950.

2. Identity equality tests

There are a couple of places in the YANG that check that an interface is of Nve
type. Currently this is done using an equality test. While that can work, a
better and more future proof way is to use derived-from-or-self(). So change

 must "(/if:interfaces/if:interface[if:name=current()]/if:type='Nve')";

to

 must
 "derived-from-or-self(/if:interfaces/if:interface[if:name=current()]/if:type,
 'Nve')";

3. Behavioral constraints defined in English text

          list static-peer {
            key "peer-ip";
...
            leaf out-vni-id {
              type uint32 {
                range "1..16777215";
              }
              description
                "The ID of VNI for outbound. Do not support separate deletion.";
            }

I'm not completely clear as to the "Do not support separate deletion" is
supposed to mean. If every static-peer must have an out-vni-id, it should be
marked mandatory. If this is supposed to mean that an out-vni-id may be added,
but not deleted, that violates one of the basic principles in YANG. The
validity of a configuration should not depend on anything else than the
configuration itself. In particular, the validity of an upcoming configuration
should not depend on the prior configuration.

If you don't want out-vni-id to be mandatory, but also don't want people to be
able to delete it without deleting the static-peer, I would recommend making
out-vni-id part of the key for static-peer, and ensure it can take a value that
essentially means none. For example like this:

          list static-peer {
            key "peer-ip out-vni-id";
...
            leaf out-vni-id {
              type union {
                type enumeration {
                  enum none;
                }
                type uint32 {
                  range "1..16777215";
                }
              }
              description
                "The ID of VNI for outbound. Do not support separate deletion.";
            }

4. Repetition of config false

In many places within the module, there are config false statements inside
elements that are already config false. While this is perfectly legal, it is
superfluous. Once a container or other element is marked config false,
everything below that point will always be config false. The IETF convention is
to not write config false in more places than necessary.

      container peers {
        config false;     <--- Everything within/below container peers is now
        config false description
          "Operational data of remote NVE address in a VNI.";
        list peer {
          key "vni-id source-ip peer-ip";
          config false;     <--- Superfluous

5. Weak data format

      leaf up-time {
        type string {
          length "1..10";
        }
        config false;
        description
          "The continuous time as NVO3 tunnel is reachable.";
      }

YANG strings are great when used for names that the client can choose
arbitrarily. They are not so great when they encode information that is not a
name, unless there is a detailed description of the encoding. Since the
encoding of the up-time leaf is not specified, the contents will not be
interoperable. Either remove this leaf, or better, describe the format of the
contents so that it can be decoded. Changing the type from string to some time
type might be a good option.

6. Identities conventionally start with lowercase letters

By convention, all identities defined by IETF start with a lower case letter.
Citing the principle of least surprise, I would recommend changing

  identity Nve {

to

  identity nve {

7. RPC or Action

In YANG 1.1, rpc:s that pertain to a certain element in the data tree can be
modeled as actions instead. This often leads to a more coherent model with less
text. In this module, that seems to be the case for the two rpc:s defined.
Therefore I would suggest changing

  rpc reset-vni-peer-statistic {
    description
      "Clear traffic statistics about the VXLAN tunnel.";
    input {
      leaf vni-id {
        type uint32 {
          range "1..16777215";
        }
        mandatory true;
        description
          "The ID of the VNI.";
      }
      leaf peer-ip {
        type inet:ip-address-no-zone;
        mandatory true;
        description
          "The address of the remote NVE.";
      }
      leaf direction{
        type direction-type;
        mandatory true;
        description
          "Traffic statistics direction for the tunnel.";
      }
    }
  }

to

        list statistic {
          key "vni-id peer-ip direction";
...
          action reset-vni-peer-statistic {
            description
              "Clear traffic statistics about the VXLAN tunnel.";
          }

Best Regards,
/jan