Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
From: Jan Lindblad <janl@tail-f.com>
In-Reply-To: <00d401d4bfc8$273c1380$4001a8c0@gateway.2wire.net>
Date: Sat, 9 Feb 2019 08:48:22 +0100
Cc: "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: <01887BB2-429A-4230-A278-7577546F93D8@tail-f.com>
References: <154955530928.5208.12848706122029839234@ietfa.amsl.com>
 <00d401d4bfc8$273c1380$4001a8c0@gateway.2wire.net>
To: tom petch <daedulus@btconnect.com>
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/4PtmHDkpjeL6zjF9GCb6DyViY5k>
Subject: Re: [pim] Yangdoctors telechat review of
 draft-ietf-pim-igmp-mld-yang-10
Precedence: list

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.
>=20
> 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
>=20
>=20
>> Reviewer: Jan Lindblad
>> Review result: Ready with Issues
>>=20
>> 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.
>>=20
>> 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:
>>=20
>> #3 Top level structures not optional
>>=20
>> 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.
>>=20
>>   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.
>>=20
>> 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.
>>=20
>> #4 Unclear meaning of optional leaves
>>=20
>> 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.
>>=20
>> This happens with many leafs, but a couple of examples:
>>=20
>>     leaf max-entries {
>>       if-feature global-max-entries;
>>       type uint32;
>>       description
>>         "The maximum number of entries in IGMP or MLD.";
>>     }
>>=20
>> 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?
>>=20
>>     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.";
>>     }
>>=20
>> If no source policy is specified, does that mean there is no policy?
>>=20
>> There are many more.
>>=20
>> #8 What policy name?
>>=20
>> 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?
>>=20
>> If this name can be chosen freely without considering some other =
list,
> it's all
>> fine.
>>=20
>>       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. ";
>>=20
>> #9 Default for version
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> The authors should
>>     leaf version {
>>       type uint8 {
>>         range "1..3";
>>       }
>>       default 2;
>>=20
>> #10 Inaccurate description or expression
>>=20
>> 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 =3D '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?
>>=20
>>           leaf interface-name {
>>             type if:interface-ref;
>>             must "/if:interfaces/if:interface[if:name =3D =
current()]/"
>>                + "ip:ipv4" {
>>               description
>>                 "The interface must have IPv4 enabled.";
>>=20
>> If ipv4 needs to be enabled, the must expression should be
>>=20
>>             must "/if:interfaces/if:interface[if:name =3D =
current()]/"
>>                + "ip:ipv4/ip:enabled =3D 'true'" {
>>=20
>> If ipv4 needs to be configured, the description should say "The
> interface must
>> have IPv4 configured (but not necessarily enabled)."
>>=20
>> The same logic applies to MLD and ipv6 as well.
>>=20
>=20