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

Jan Lindblad <janl@tail-f.com> Sat, 09 February 2019 08:41 UTC

Return-Path: <janl@tail-f.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 0A220130F2F; Sat, 9 Feb 2019 00:41:27 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
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 fluc-5L8jRYg; Sat, 9 Feb 2019 00:41:24 -0800 (PST)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 3C4F1130F34; Sat, 9 Feb 2019 00:41:24 -0800 (PST)
Received: from [192.168.1.102] (213-67-237-150-no99.tbcn.telia.com [213.67.237.150]) by mail.tail-f.com (Postfix) with ESMTPSA id 7C9CB1AE012C; Sat, 9 Feb 2019 09:41:21 +0100 (CET)
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
From: Jan Lindblad <janl@tail-f.com>
In-Reply-To: <20190209075246.6gqnbmsrxvle5nx4@anna.jacobs.jacobs-university.de>
Date: Sat, 9 Feb 2019 09:41:20 +0100
Cc: tom petch <daedulus@btconnect.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-Transfer-Encoding: quoted-printable
Message-Id: <FECB8944-1B7E-4EB3-8BCE-16E413F31849@tail-f.com>
References: <154955530928.5208.12848706122029839234@ietfa.amsl.com> <00d401d4bfc8$273c1380$4001a8c0@gateway.2wire.net> <01887BB2-429A-4230-A278-7577546F93D8@tail-f.com> <20190209075246.6gqnbmsrxvle5nx4@anna.jacobs.jacobs-university.de>
To: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>
X-Mailer: Apple Mail (2.3445.102.3)
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/91t-oPNpuNXUt7LZVlWj-enChTE>
Subject: Re: [pim] [yang-doctors] 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: Sat, 09 Feb 2019 08:41:27 -0000

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



> On Sat, Feb 09, 2019 at 08:48:22AM +0100, Jan Lindblad wrote:
>> 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.
>>>> 
>>> 
>> 
>> _______________________________________________
>> yang-doctors mailing list
>> yang-doctors@ietf.org
>> 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/>
>