Re: [pim] Genart last call review of draft-ietf-pim-igmp-mld-yang-10

[Xufeng Liu <[e-mail]>]

Hi Dale,

Thanks for the review. We have posted the updated
https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-yang-11 to address
these issues.

Some details are in-line below.

Best regards,
- Xufeng

On Tue, Feb 5, 2019 at 10:50 PM Dale Worley <[e-mail]> wrote:

> Reviewer: Dale Worley
> Review result: Ready with Nits
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document:  draft-ietf-pim-igmp-mld-yang-10
> Reviewer:  Dale R. Worley
> Review Date:  2019-02-05
> IETF LC End Date:  2019-02-08
> IESG Telechat date:  not known
>
> Summary:
>
>    This draft is basically ready for publication, but has nits
>    that should be fixed before publication.
>
>    I do not have the expertise to review the contents of the Yang
>    module itself.  Fortunately, the Yang Doctor can review that.
>
> Minor issues:
>
>    This draft has a number of exposition issues that should be fixed.
>
> Abstract
>
>    This document defines a YANG data model that can be used to
>    configure and manage Internet Group Management Protocol (IGMP) and
>    Multicast Listener Discovery (MLD) devices.
>
> Both here and in the Introduction, it would be better to say "devices
> that implement IGMP and MLD" or something like that, since IGMP and
> MLD are protocols, not classes of devices.
>
> Table of Contents
>
>    2. Design of Data model......................................... 4
>       2.1. Scope of model ......................................... 4
>       2.2. Optional capabilities .................................. 4
>       2.3. Position of address family in hierarchy ................ 5
>    3. Module Structure ............................................ 5
>       3.1. IGMP Configuration and Operational state ............... 5
>       3.2. MLD Configuration and Operational State ................ 8
>
> It looks like the current style would capitalize "model",
> "capabilities", "state", etc.
>
> [Xufeng]: Fixed.

>
> 1. Introduction
>
>    This model will support
>    the core IGMP and MLD protocols, as well as many other features
>    mentioned in separate IGMP and MLD RFCs.
>
> "will support" needs clarifying.  Does the model defined by this
> document "support many other features", or is this a prediction that
> the model will eventually be extended to do so?  Indeed, the
> Introduction should make a clear statement of what is and is not
> supported by this version of the model.
>
[Xufeng]: Fixed. Should be present, not future.

>
> 1.3. Prefixes in Data Node Names
>
>    Otherwise,
>    names are prefixed using the standard prefix associated with the
>
> The tail of this sentence is missing.
>
[Xufeng]: Fixed.

>
> 2. Design of Data model
> 2.1. Scope of model
>
>    The model covers IGMPv1 [RFC1112], IGMPv2 [RFC2236], IGMPv3
>    [RFC3376] and MLDv1 [RFC2710], MLDv2 [RFC3810].
>
> This should be stated in the Introduction as well.
>
[Xufeng]: Added.

>
>    The configuration of IGMP and MLD features, and the operational
>    state fields and RPC definitions are not all included in this
>    document of the data model.
>
> As written, this says that the model covers some unspecified subset of
> IGMP and MLD features.  Is it possible to characterize what is
> included and what is not?  Otherwise, the reader would have to work
> through the model to check on every specific item they were interested
> in.
>
[Xufeng]: This section has been expanded to describe these features.

>
>    This model can be extended, though the
>    structure of what has been written may be taken as representative of
>    the structure of the whole model.
>
> What does this mean?  Like any Yang model, this model can be extended,
> by anyone who chooses to do so.  But how does "what has been written"
> represent or constrain the structure of an extended model?
>
[Xufeng]: Reworded.

>
> 2.2. Optional capabilities
>
>    The main design goals of
>    this document are that any major now-existing implementation may be
>    said to support the basic model, [...]
>
> Probably more correct to say "[...] may be said to support the
> facilities described by the basic model [...]".
>
[Xufeng]: Reworded.

>
>    There is also value in widely-supported features being standardized,
>    to save work for individual vendors, [...]
>
> And probably more importantly, so that the features can be accessed in
> a standardized way.
>
[Xufeng]: Added such a phrase.

>
> 2.3. Position of address family in hierarchy
>
>    The current document contains IGMP and MLD as separate schema
>    branches in the structure. The reason for this is to make it easier
>    for implementations which may optionally choose to support specific
>    address families. And the names of objects may be different between
>    the IPv4 (IGMP) and IPv6 (MLD) address families.
>
> It seems like the qualification of IGMP == IPv4 and MLD == IPv6 should
> be done in the first sentence rather than the last.
>
[Xufeng]: Added clarification sentences at the beginning of this section.

>
> 3.1. IGMP Configuration and Operational state
>
> It seems like this section has a first part which applies to IGMP and
> MLD equally (though it only talks about IGMP), and a second part which
> is a summary of the IGMP module.  Perhaps they should be split into
> two sections?
>
[Xufeng]: Moved the paragraphs common to both IGMP and MLD to Sec 3. Put
IGMP specifics in Sec 3.1, and MLD specifics in Sec 3.2.

>
>        Interface-global: Only including configuration data nodes that
>    IGMP configuration attributes are applicable to all the interfaces
>    whose interface-level corresponding attributes are not existing,
>    with same attributes' value for these interfaces.
>
> This sentence seems to have either extra words or missing words.
>
[Xufeng]: Rephrased.

>
> "SSM" seems to show up a lot but isn't defined.  Is it part of IGMP/MLD?
>
[Xufeng]: Added to Sec 1.1.

>
> 3.2. MLD Configuration and Operational State
> 3.3. IGMP and MLD RPC
>
>    IGMP and MLD RPC clears the specified IGMP and MLD group membership.
>
> This is awkwardly phrased.  Perhaps, "IGMP and MLD each have one RPC
> which clears the group membership [database? table?] for that
> protocol."
>
[Xufeng]: Rephrased.

>
> 4. IGMP and MLD YANG Modules
>
> The use of empty lines isn't consistent in the module definition.
>
[Xufeng]: Went through the module and fixed them.

>
> 5. Security Considerations
>
>    These subtrees are all under
>
>    /rt:routing/rt:control-plane-protocols
>    /rt:control-plane-protocol/igmp:
>
> Is the trailing "/igmp:" meaningful?
>
> And parallel cases later in the section.  As far as I can see, "igmp:"
> etc. is part of the root node name of the subtree, not attached to
> the path above it.
>
[Xufeng]: The “igmp” is meant to indicate the node under
“control-plane-protocol”. The prefix “igmp:” before “global” and
“interfaces” is the name space, but it should be “igmp-mld:” instead,
because igmp and mld share the same module and name space. Fixed these
tokens in the document.

>
>    Unauthorized access to any data node of these subtrees can adversely
>    affect the membership records of multicast routing subsystem on the
>    local device.
>
> -- and some similar cases.  The scope of the phrase "these subtrees"
> is unclear.
>
[Xufeng]: The phrase “these subtrees” indicates all the trees listed above.
This paragraph is part of the template. Is it ok for us to modify this
paragraph?

>
> 6. IANA Considerations
>
>     This document registers the following namespace URIs in the IETF XML
>     registry [RFC3688]:
>
>    --------------------------------------------------------------------
>
>    URI: urn:ietf:params:xml:ns:yang:ietf-igmp-mld
>
>    Registrant Contact: The IESG.
>
>    XML: N/A, the requested URI is an XML namespace.
>
>    --------------------------------------------------------------------
>
> RFC 3688 section 3.2 includes:
>
>    ns -- XML Namespaces [W3C.REC-xml-names] are named by a URI.  [...]
>       Thus, the
>       registered document will be either the specification or a
>       reference to it.  [...]
>
> It seems to me that the "XML" field of this registration should be:
>
>    XML: RFC XXXX
>
> to provide the name of the registered specification of the namespace.
>
[Xufeng]: Section 14 in [RFC6020] registers two URIs for the YANG and YIN
XML namespaces in the IETF XML registry [RFC3688]. After it, all YANG
modules currently use this format.