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

[Xufeng Liu <[e-mail]>]

Hi Jan,

Thanks for the review. We have posted the updated
https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-yang-11 to address
these issues.

Some of the details are in-line below.

Best regards,
- Xufeng

On Thu, Feb 7, 2019 at 11:01 AM Jan Lindblad <[e-mail]> wrote:

> 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.
>
[Xufeng]: The node "control-plane-protocol" where the augmentation is
applied is a list entry, which can already achieve what a presence
container can do. I don't think that we need to make igmp and mld presence
containers in this case. The mandatory leaves are already possible because
of the list statement.

>
> #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.
>
[Xufeng]: We examined the model, and added descriptions to these occasions.

>
> #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. ";
>
[Xufeng]: Yes. In this model, this name can be chosen freely without
considering some other list. The model that defines such entries are not
yet available, and may be specified by a separate model later. When that
happens, the system would need to verify against such a model.

>
> #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;
>
[Xufeng]: The version range is defined as “1..3”, so this model can only be
used for the versions of IGMP protocols current already specified by IETF.
In the future, if new versions of IGMP protocols are introduced, this model
cannot be used as is. We will need to add new features to the model, the
version range range will need to be modified, and the default version can
be changed at that time.

>
> #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.
>
[Xufeng]: The intention is the described as the must statement, and the
descriptions are not accurate. We have fixed the description as suggested.