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

From: Jan Lindblad <[email-masked]>
Date: Thu, 07 Feb 2019 08:01:49 -0800

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.

---

From: tom petch <[email-masked]>
Date: Fri, 8 Feb 2019 16:07:12 +0000

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" <[email-masked]>
Sent: Thursday, February 07, 2019 4:01 PM

> ....". Does this mean that the policy name can be chosen freely, or
does the

---

From: Jan Lindblad <[email-masked]>
Date: Sat, 9 Feb 2019 08:48:22 +0100

Tom,

> understand theYANG module, about the repeated use of IGMP and/or MLD as

As you know I'm not the author of this YANG, but I can explain how it works.

> course blown out of the water by including separate features for IGMP

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.

> referenced and in a sense, this is another protocol to add to IGMP and

I have no idea what SSM is, and the module does little to help on that front. Good point.

/jan

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sat, 9 Feb 2019 08:52:46 +0100

They have a feature to control a global enable leaf???

/js

On Sat, Feb 09, 2019 at 08:48:22AM +0100, Jan Lindblad wrote:
> _______________________________________________
> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>

---

From: Jan Lindblad <[email-masked]>
Date: Sat, 9 Feb 2019 09:41:20 +0100

Jürgen,

> They have a feature to control a global enable leaf???

That's right. It's called "global-admin-enable" and is used for nothing else. But I guess features are cheap.

/jan

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sat, 9 Feb 2019 09:58:47 +0100

If there are individual enable leafs, then the combined enable leaf is
(i) not needed and (ii) very easy to implement and clearly not
justifying a feature.

/js

---

From: Jan Lindblad <[email-masked]>
Date: Sat, 9 Feb 2019 12:13:27 +0100

> (i) not needed and (ii) very easy to implement and clearly not

There is one global enable leaf for igmp, another for mld. Both are conditioned on the same feature. Currently the containers igmp and mld are not presence containers, so these global switches may have some use. Presence containers would be better (as pointed out in my review), but that does not preclude enable leafs.

/jan

---

From: Xufeng Liu <[email-masked]>
Date: Mon, 11 Feb 2019 19:00:45 -0500

Hi Tom,

The intended usage of the feature igmp, mld, and global-admin-enable is as
what Jan explained. Various combinations of these features provide a
certain level of flexibility.

Hi Jan,

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

Thanks,
- Xufeng

On Sat, Feb 9, 2019 at 6:13 AM Jan Lindblad <[email-masked]> wrote:

> There is one global enable leaf for igmp, another for mld. Both are
> conditioned on the same feature. Currently the containers igmp and mld are
> not presence containers, so these global switches may have some use.
> Presence containers would be better (as pointed out in my review), but that
> does not preclude enable leafs.
>
> /jan
>
>
> >>>> 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.
> >>>>
> >>>>>> spring, I did an early review of the -02, and in the early fall a
> last
> >>>>> call
> >>>>>> Since some of the current issues are the same as addressed in
> earlier
> >>>>> comments,
> >>>>>> If-feature statements have been introduced so that an implementor
> can
> >>>>> choose
> >>>>>> which makes this recommendation less of a concern, but if a
> mandatory
> >>>>> leaf is
> >>>>>> While a lot of default values have been introduced (great!), there
> are
> >>>>> still
> >>>>>> self-evident and no description of what absence signifies. A few
> words
> >>>>> in the
> >>>>>> 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 {
> >>>>>> In leaf ssm-map-group-policy, the description begins with "Name of
> the
> >>>>> policy
> >>>>>> If this name can be chosen freely without considering some other
> list,
> >>>>> it's all
> >>>>>> fine.
> >>>>>>
> >>>>>> default can never be changed, however, so only do this if 2 will
> feel
> >>>>> like a
> >>>>>> option is to make it mandatory for the client to configure. Note
> that
> >>>>> in such a
> >>>>>> In the interface-name leaf, there is a must statement and
> description
> >>>>> which

---

From: tom petch <[email-masked]>
Date: Tue, 12 Feb 2019 12:20:41 +0000

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" <[email-masked]>
Sent: Saturday, February 09, 2019 7:48 AM

Tom,

> understand theYANG module, about the repeated use of IGMP and/or MLD
as

As you know I'm not the author of this YANG, but I can explain how it
works.

> course blown out of the water by including separate features for IGMP

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.

> referenced and in a sense, this is another protocol to add to IGMP and

I have no idea what SSM is, and the module does little to help on that
front. Good point.

/jan

last
are
words
Will
the
list,

---

From: Xufeng Liu <[email-masked]>
Date: Wed, 13 Feb 2019 17:18:50 -0500

Hi Tom,

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