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

[Xufeng Liu <[e-mail]>]

Hi Tom,

Thanks for pointing out these additional quirks. We will fix them in the
next revision.
Best,
- Xufeng

On Tue, Feb 12, 2019 at 7:20 AM tom petch <[e-mail]> wrote:

> Jan
>
> Thanks for that.  I see grouping as a necessary evil to be used with
> restraint, like subroutines and procedures in code, a restraint that I
> would like to be exercised more:-(
>
> So, I now see how the groupings are used under an augment/when which
> makes them conditional on either IGMP or MLD and so not ambiguous as I
> first thought.
>
> Having sorted that, five more quirks I see for the authors to address.
>
> Introduction
> no mention of NMDA as required by RFC8407
>
>
> /interfaces/interface/interface-name {
>    must "/if:interfaces/if:interface[if:name = current()]/"
>                    + "ip:ipv4" {
> description  "The interface must have IPv4 enabled.";
>
> looks wrong to me.  If you want IPv4 enabled, then you need
> /ip:enabled
> set to true
>
>
>
>             "This augmentation is only valid for a control-plane
>              protocol instance of IGMP (type 'mld').";
>
> looks odd to me when the augment has
>         when "derived-from-or-self(rt:type, 'igmp-mld:mld')" {
>
>
>
>       rpc clear-igmp-groups {
>             description
>               "Name of the IGMP interface.
>                If it is not specified, groups from all interfaces are
>                cleared.";
>
> taken at face value, this clears all MLD groups as well as IGMP groups;
> is this intended?  If not, suggest
>
>                If it is not specified, groups from all IGMP interfaces
> are
>                cleared.";
>
> ditto
>       rpc clear-mld-groups {
>
>
> Tom Petch
>
> ----- Original Message -----
> From: "Jan Lindblad" <[e-mail]>
> Sent: Saturday, February 09, 2019 7:48 AM
>
> Tom,
>
> > 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
>
> As you know I'm not the author of this YANG, but I can explain how it
> works.
>
> >      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.
>
> No, this construct will not force an implementation to do both igmp and
> mld. There are other features to control which protocols to implement
> (feature-igmp, feature-mld). Leaf enable is modeled in a grouping (you
> can think of it as a macro). This grouping is used once in igmp and once
> in mld, so there are two separate enable leafs if both features are
> implemented.
>
> The single global-admin-enable feature does force an implementation that
> has both igmp and mld to support the global enable leaf either for both
> protocols, or for none of them. It's not possible to support global
> enable for one protocol but not the other, when both protocols are
> implemented.
>
> > 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
>
> I have no idea what SSM is, and the module does little to help on that
> front. Good point.
>
> /jan
>
> > ----- 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.
>
>