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

[tom petch <[e-mail]>]

Jan

Agree with your comments but I had a more fundamental problem trying to
understand theYANG module, about the repeated use of IGMP and/or MLD as
in
      feature global-admin-enable {
Is that one or or the other or both? Or
        leaf enable {
          type boolean;           default false;
          description "true to enable IGMP or MLD in the routing
instance;
Is that one or the other or both?
And so on. I am uncertain how the phrases are meant to be interpreted.

The only way I could make sense of this was to assume that an
implementation must implement and support both IGMP and MLD; which is of
course blown out of the water by including separate features for IGMP
and MLD.

In  a similar vein, SSM is repeatedly mentioned, never explained, never
referenced and in a sense, this is another protocol to add to IGMP and
MLD

Tom Petch

----- Original Message -----
From: "Jan Lindblad" <[e-mail]>
Sent: Thursday, February 07, 2019 4:01 PM


> 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.
>