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

[Xufeng Liu <[e-mail]>]

Hi Tom,

We have posted the update
https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-yang-11 to address
these issues, along with others.
Thanks,
- Xufeng

On Wed, Feb 13, 2019 at 5:18 PM Xufeng Liu <[e-mail]>
wrote:

> 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
>>
> [Xufeng]: Added.

>
>>
>> /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
>>
> [Xufeng]: The description is not accurate. We should allow an IPv4
interface that is disabled. We have fixed description.

>
>>
>>
>>             "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?
>
> [Xufeng]: Sorry that it is not the intention. There are two separate RPCs:
one is clear-igmp-groups; the other is clear-mld-groups . rpc
clear-igmp-groups is used to clear IGMP groups, while rcp clear-mld-groups
is used to clear MLD groups. Added more clarifications to the descriptions.


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