Re: [nvo3] [Last-Call] Yangdoctors last call review of draft-ietf-nvo3-yang-cfg-05

[tom petch <[e-mail]>]

On 16/09/2021 08:18, Liubing (Remy) wrote:
> Hello Jan,
>
> Many thanks for your review. We will make related changes in the next version.

Bing

This e-mail caught my eye as I was looking at the recent revision of 
nvo3-yang.

You might also want to fix

IANA Considerations
Security Considerations
Copyright
Contact
Organisation
etc
none of which conform to YANG Guidelines!

They are admin details which have to be fixed sometime and for me, the 
sooner the better.  I have a mindset that says if the admin is wrong, 
then the technical aspects probably are as well:-)

And whether you use 'action' or 'RPC' I would always add
nacm:default deny-all
as a basic security precaution.

I agree with all Jan's comments.

Tom Petch

> Best regards,
> Bing
>
> -----邮件原件-----
> 发件人: Jan Lindblad via Datatracker [mailto:[e-mail]]
> 发送时间: 2021年9月16日 15:11
>
> Reviewer: Jan Lindblad
> Review result: Not Ready
>
> 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
>
>
>