[yang-doctors] Yangdoctors telechat review of draft-ietf-pim-igmp-mld-yang-10

[Jan Lindblad <[e-mail]>]

Reviewer: Jan Lindblad
Review result: Ready with Issues

This is my YANG-doctor review of draft-ietf-pim-igmp-mld-yang-10. In the
spring, I did an early review of the -02, and in the early fall a last call
review of the -07 version. The module has progressed significantly since then.
In particular the two main issues with -07 have been fixed, which is great!
There are still a few things that should be looked at, however.

Since some of the current issues are the same as addressed in earlier comments,
I will reuse the numbering from the -07 review, and add a few points at the end:

#3 Top level structures not optional

If-feature statements have been introduced so that an implementor can choose
whether to implement igmp and/or mld separately. This is very good. RFC 8349,
which the current module augments, recommend that the top level containers be
presence containers. This is currently not the case, and I see no reason why
they aren't.

   It is also RECOMMENDED that protocol-specific data nodes be
   encapsulated in an appropriately named container with presence.  Such
   a container may contain mandatory data nodes that are otherwise
   forbidden at the top level of an augment.

At this time, there are no mandatory leafs under the top level containers,
which makes this recommendation less of a concern, but if a mandatory leaf is
introduced (e.g. see #9 below), making the top level containers presence
containers will be essential.

#4 Unclear meaning of optional leaves

While a lot of default values have been introduced (great!), there are still
many optional configuration leafs with no default value, not mandatory, not
self-evident and no description of what absence signifies. A few words in the
description might be all it takes to fix this.

This happens with many leafs, but a couple of examples:

     leaf max-entries {
       if-feature global-max-entries;
       type uint32;
       description
         "The maximum number of entries in IGMP or MLD.";
     }

If no max-entries is configured, what is the prescribed behavior? Will all
implementations agree that there is no max then? If so, maybe mention that in
the description?

     leaf source-policy {
       if-feature intf-source-policy;
       type leafref {
         path "/acl:acls/acl:acl/acl:name";
       }
       description
         "Name of the access policy used to filter sources.
          A device can restrict the length
          and value of this name, possibly space and special
          characters are not allowed.";
     }

If no source policy is specified, does that mean there is no policy?

There are many more.

#8 What policy name?

In leaf ssm-map-group-policy, the description begins with "Name of the policy
...". Does this mean that the policy name can be chosen freely, or does the
policy name entered here have to match some policy name specified somewhere
else?

If this name can be chosen freely without considering some other list, it's all
fine.

       leaf ssm-map-group-policy {
         type string;
         description
           "Name of the policy used to define ssm-map rules.
            A device can restrict the length
            and value of this name, possibly space and special
            characters are not allowed. ";

#9 Default for version

Leaf version is given a default of 2; this sounds excellent. This clarifies
what protocol version to use even if the operator doesn't specify. Note that a
default can never be changed, however, so only do this if 2 will feel like a
good number even 10 years from now.

If no suitable default value that withstands time can be selected, another
option is to make it mandatory for the client to configure. Note that in such a
case #3 above becomes very important to resolve.

The authors should
     leaf version {
       type uint8 {
         range "1..3";
       }
       default 2;

#10 Inaccurate description or expression

In the interface-name leaf, there is a must statement and description which
contradict each other. The must statement requires that the ipv4 container is
configured, while the description says ipv4 should be enabled. It is possible
to configure the ipv4 container but have ipv4/enabled = 'false', so the must
expression only cares whether the ipv4 presence container is configured, not if
it is enabled. Which one reflects the desired behavior?

           leaf interface-name {
             type if:interface-ref;
             must "/if:interfaces/if:interface[if:name = current()]/"
                + "ip:ipv4" {
               description
                 "The interface must have IPv4 enabled.";

If ipv4 needs to be enabled, the must expression should be

             must "/if:interfaces/if:interface[if:name = current()]/"
                + "ip:ipv4/ip:enabled = 'true'" {

If ipv4 needs to be configured, the description should say "The interface must
have IPv4 configured (but not necessarily enabled)."

The same logic applies to MLD and ipv6 as well.