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

Xufeng Liu <xufeng.liu.ietf@gmail.com> Mon, 29 April 2019 15:59 UTC

Return-Path: <xufeng.liu.ietf@gmail.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1489C120362; Mon, 29 Apr 2019 08:59:48 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.998
X-Spam-Level:
X-Spam-Status: No, score=-1.998 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id PoiXhtF1WODr; Mon, 29 Apr 2019 08:59:44 -0700 (PDT)
Received: from mail-it1-x12e.google.com (mail-it1-x12e.google.com [IPv6:2607:f8b0:4864:20::12e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B991A120336; Mon, 29 Apr 2019 08:59:44 -0700 (PDT)
Received: by mail-it1-x12e.google.com with SMTP id s3so16998400itk.1; Mon, 29 Apr 2019 08:59:44 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/iZw2Z5d0RqicNcI1zlCZ/Fs/NwSbrt9dQOYSohgf/c=; b=Z8TGKOFVezsNrsxmr0wxz4QQ2qe0T34mMnC3wn23z87t0Z5CMQ8HPH8jp/rheZmIrL Cv+LmHML+VZ2F8OVdt2CQ4PnmTo7wmQ07TCnU3A+yRnnOvP2GklFl/a5//D7bLA7dTTR YqliMwykt8rbGHTKA+3dEk4KJP5qOCDAlymnFMGMQoAg2TeRViwClbI54QHq8dTpzPRO McMSYcrnvPgOVWdR4LG6pqfd9UioXQOh0KyG2ufUi6MWW7Un23dMWJ9lk1E3CK/n4lLb bAmr1okqtjCrvLHX5S/ws4047GUDDPEIONVEdAj6MV+/gEyf+MnDnu74kXa2Dm4+J1Zh p48Q==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/iZw2Z5d0RqicNcI1zlCZ/Fs/NwSbrt9dQOYSohgf/c=; b=AqueSuOxus0BYl9Hq1DRljWdjpsdxCiI3ZC4Wp+yD+1Y0m0XEv/5XudJuH6TdI088u gHRBe5a0l5SnPIcGiu/E6j9CEwX158bLjx9JMGZCxj37HNz8DyLHPgtniR+1NhC6k7mi pPHF7jnDsL5+NAUKbIA/1FtjtEzuJs7c2PyJzcXgtrtGGhwTes5CSt39YKW+kdDjrYlh WvcT7xpO96foi9+USiGQ+AQnPI92s+qk3RW281c/FaU6cidtivCxYWzuLMFS75FtrEiN DTlGIXMvV0kTlHpTfAykx5RrJesYoAnFgAdocV05OMVfQJqzvI8ai2THQ4gVgGXWvqiG IRMw==
X-Gm-Message-State: APjAAAVK63ph7rUutNJrn6OECubxR0Cw1tbemSHeDYxLe0GdwosVPWfl V3GFLyheYJEcwoEQ/iDB1gA9ciSFmlQYDv/FY5s=
X-Google-Smtp-Source: APXvYqyyR+VbCFhkAmwEz/vwqm06eEJtCUZyaCCids+yETTnPW6oAfMolEHrz83bIL4glXmeu0Ngc2rbDhrvI8Wsz+w=
X-Received: by 2002:a24:4682:: with SMTP id j124mr19612837itb.90.1556553583958; Mon, 29 Apr 2019 08:59:43 -0700 (PDT)
MIME-Version: 1.0
References: <154955530928.5208.12848706122029839234@ietfa.amsl.com> <00d401d4bfc8$273c1380$4001a8c0@gateway.2wire.net> <01887BB2-429A-4230-A278-7577546F93D8@tail-f.com> <050101d4c2cd$2832f760$4001a8c0@gateway.2wire.net> <CAEz6PPTHE7ruwtRY7Zc5ce6DjxQbkAZfQjcgzz+zyZ6Y-KkrKA@mail.gmail.com>
In-Reply-To: <CAEz6PPTHE7ruwtRY7Zc5ce6DjxQbkAZfQjcgzz+zyZ6Y-KkrKA@mail.gmail.com>
From: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Date: Mon, 29 Apr 2019 11:59:33 -0400
Message-ID: <CAEz6PPSaMdSnPFvqDa8eHNNS0szfQmyRTD32=RULvdcYJPBb+A@mail.gmail.com>
To: tom petch <daedulus@btconnect.com>
Cc: Jan Lindblad <janl@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "pim@ietf.org" <pim@ietf.org>, "draft-ietf-pim-igmp-mld-yang.all@ietf.org" <draft-ietf-pim-igmp-mld-yang.all@ietf.org>
Content-Type: multipart/alternative; boundary="0000000000007220ca0587ad5f23"
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/r6Z7iJCoqjvWd26Ycs5utBJsUZU>
Subject: Re: [pim] Yangdoctors telechat review of draft-ietf-pim-igmp-mld-yang-10
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 29 Apr 2019 15:59:48 -0000

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 <xufeng.liu.ietf@gmail.com>;
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 <daedulus@btconnect.com>; 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" <janl@tail-f.com>;
>> 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" <janl@tail-f.com>;
>> > 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.
>>
>>