Re: [pim] AD Review of draft-ietf-pim-msdp-yang-06

[<[e-mail]>]

Hi Alvaro,






Thank you very much for your reminder!


We are doing the update work.


We'd like to submit the new version as soon as possible.






Thanks,


Sandy









原始邮件



发件人：AlvaroRetana <[e-mail]>
收件人：[e-mail] <[e-mail]>;
抄送人：[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;
日 期 ：2019年12月24日 14:19
主 题 ：Re: [pim] AD Review of draft-ietf-pim-msdp-yang-06




_______________________________________________
pim mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/pim


Hi!




It has been almost 2 months since I sent out this review, and I haven’t heard anything back.  I know that the document needs significant work — how is it going?




Thanks!




Alvaro.


On October 23, 2019 at 12:17:03 PM, Alvaro Retana ([e-mail]) wrote:


Dear authors:

I just finished reviewing this document.  I don't have a lot of comments on the model itself (please see below), but the document needs significant work to live up to rfc8407 (Guidelines for YANG Documents) and the specific requirements mentioned there for YANG documents.  In general, compare this document with draft-ietf-pim-igmp-mld-yang (which was written by almost the same author team).  Items from rfc8407 that are not addressed in this document include:

 "A terminology section MUST be present if any terms are defined in the
 document or if any terms are imported from other documents."  Is there no
 terminology defined in rfc3618 (or elsewhere) that is used in this document 
 and useful to highlight?

 "If YANG tree diagrams are used, then an informative reference to the YANG
 tree diagrams specification MUST be included in the document."

 "If the module or modules defined by the specification imports definitions
 from other modules...or are always implemented in conjunction with other
 modules, then those facts MUST be noted in the overview section.....Section 2.3
 of [RFC8349] for an example of this overview section."

 The Security Considerations "MUST be patterned after the latest approved
 template (available at
 <https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines>)."  See also 
 some comments on the Security Consideration section itself.

 "For every import or include statement that appears in a module contained in
 the specification that identifies a module in a separate document, a
 corresponding normative reference to that document MUST appear in the
 Normative References section."

 "For every normative reference statement that appears in a module contained in
 the specification that identifies a separate document, a corresponding
 normative reference to that document SHOULD appear in the Normative References
 section."

 "Each specification that defines one or more modules SHOULD contain usage
 examples, either throughout the document or in an appendix."


Please also check idnits:
https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-pim-msdp-yang-06.txt


I will wait for a revision, and for any major issues identified inline before starting the IETF LC.

Thanks!

Alvaro.


[Line numbers from idnits.]

2	PIM WG                                                       Xufeng. Liu
3	Internet-Draft                                            Volta Networks
4	Intended status: Standards Track                            Zheng. Zhang
5	Expires: October 25, 2019                                ZTE Corporation
6	                                                            Anish. Peter
7	                                                  Individual contributor
8	                                                       Mahesh. Sivakumar
9	                                                        Juniper networks
10	                                                               Feng. Guo
11	                                                     Huawei Technologies
12	                                                        Pete. McAllister
13	                                                     Metaswitch Networks

[nit] For all the authors, "The author's name (initial followed by family name)..." (rfc7322).


...
71	1.  Introduction
...
78	   This model is designed to be used along with other multicast YANG
79	   models such as PIM, which are not covered in this document.

[major] Is the PIM model required?  I didn't find an indication in the model that other models were needed.

[nit] A reference to the PIM model would be nice.

81	2.  Design of the Data Model

83	   This model imports and augments ietf-routing YANG model defined in
84	   [RFC8349].  Both configuration data nodes and state data nodes of
85	   [RFC8349] are augmented.  The configuration data nodes cover global
86	   configuration attributes and per peer configuration attributes.  The
87	   state data nodes include global, per peer, and source-active
88	   information.  The container "msdp" is the top level container in this
89	   data model.  The presence of this container is expected to enable
90	   MSDP protocol functionality.  No notification is defined in this
91	   model.

[nit] s/augments ietf-routing YANG model/augments the ietf-routing YANG model

93	module: ietf-msdp
94	  augment /rt:routing/rt:control-plane-protocols:
95	    +--rw msdp!
96	       +--rw global
97	       |  +--rw tcp-connection-source?   if:interface-ref
98	       |  +--rw default-peer* [peer-addr prefix-policy] {global-default-peer,global-default-peer-policy}?
99	       |  |  +--rw peer-addr        -> ./././peers/peer/address
100	       |  |  +--rw prefix-policy    string
101	       |  +--rw originating-rp
102	       |  |  +--rw interface?   if:interface-ref
103	       |  +--rw sa-filter
104	       |  |  +--rw in?    string
105	       |  |  +--rw out?   string

[major] The use of "string" to point at the policy (for sa-filter, prefix-policy) caught my attention.  Later the text says that "The definition of such a policy is outside the scope of this document."  Ok...draft-ietf-rtgwg-policy-model  (Routing Policy Model) does define the policy -- why is that work not used/referenced?  I noticed that there is an unused reference to I-D.ietf-rtgwg-policy-model, which was added in -05, when the "out of scope" text was also added.....


...
111	       |     +--rw authentication
112	       |     |  +--rw (authentication-type)?
113	       |     |     +--:(key-chain) {peer-key-chain}?
114	       |     |     |  +--rw key-chain?          key-chain:key-chain-ref
115	       |     |     +--:(password)
116	       |     |        +--rw key?                string
117	       |     |        +--rw crypto-algorithm?   identityref

[major] rfc3618 only requires Keyed MD5 support...which I assume corresponds to the "password" option.  Is that correct?  Is the MSDP operation with a key-chain specified somewhere else?  Are we missing a reference (in the model)?

118	       |     +--rw enable?                  boolean {peer-admin-enable}?
119	       |     +--rw tcp-connection-source?   if:interface-ref

[minor] Maybe it's my poor understanding of YANG...  It's not clear to me whether the global configuration for tcp-connection-source has precedence over the peer-specific configuration or not.  Given that tcp-connection-source is optional, and that several of the parameters below depend on the peer configuration, it seems to me that some information may not be present if only the global piece is configured.

[Again, this comment may simply reflect my misunderstanding of YANG.  If that is the case, just tell me and we can move on. :-) ]


...
212	5.  MSDP RPC

214	   The RPC part is used to define some useful and ordinary operations of
215	   protocol management.  Network manager can delete all the information
216	   from a given peer by using the clear-peer rpc.  And network manager
217	   can delete a given SA cache information by clear-sa-cache rpc.

[nit] s/manager/managers

[major] There is no NMDA-related statement.

219	6.  MSDP YANG model

[major] Add text extracting all the reference statements (to be listed in the References secion): "This module references..."  All these should be Normative.

221	<CODE BEGINS> file "ietf-msdp.yang"

[major] From rfc8407:

   The "<CODE BEGINS>" tag SHOULD be followed by a string identifying
   the file name specified in Section 5.2 of [RFC7950].  The name string
   form that includes the revision date SHOULD be used.  The revision
   date MUST match the date used in the most recent revision of the
   module.

IOW, the file name should be [e-mail]  Please make sure this matches the date in the revision statement below.


...
239	     import ietf-routing {
240	       prefix "rt";
241	       reference "RFC8022";

[major] s/8022/8349

242	     }

244	     import ietf-interfaces {
245	       prefix "if";
246	       reference "RFC7223";

[major] s/7223/8343

247	     }

249	     import ietf-ip {
250	       prefix "ip";
251	       reference "RFC7277";

[major] s/7277/8344


...
264	     organization
265	       "IETF PIM(Protocols for IP Multicast) Working Group";

[nit] s/PIM(Protocols/PIM (Protocols


...
289	        description
290	         "The module defines the YANG definitions for MSDP.

292	          Copyright (c) 2018 IETF Trust and the persons
293	          identified as authors of the code.  All rights reserved.

295	          Redistribution and use in source and binary forms, with or
296	          without modification, is permitted pursuant to, and
297	          subject to the license terms contained in, the Simplified
298	          BSD License set forth in Section 4.c of the IETF Trust's
299	          Legal Provisions Relating to IETF Documents
300	          (http://trustee.ietf.org/license-info).
301	          This version of this YANG module is part of RFC 3618; see
302	          the RFC itself for full legal notices.";

[major] "this YANG module is part of RFC 3618"  No, it is part of this RFC-to-be; use "RFC XXXX" instead.

304	     revision 2018-10-20 {

305	       description
306	         "Initial revision.";
307	       reference
308	         "RFC XXXX: A YANG Data Model for MSDP.
309	          RFC 3618: Multicast Source Discovery Protocol (MSDP).
310	          RFC 4624: Multicast Source Discovery Protocol (MSDP) MIB";
311	     }

[major] The only reference should be RFC XXXX.  The others are not where the module is included...


...
477	       container originating-rp {
478	         description
479	           "The container of Originating RP.";
480	         leaf interface {
481	           type if:interface-ref;
482	           must "/if:interfaces/if:interface[if:name = current()]/"
483	             + "ip:ipv4" {
484	             description
485	               "The interface must have IPv4 enabled.";
486	           }
487	           description
488	             "Reference to an entry in the global interface
489	              list.
490	              IP address of the interface is used in the RP field of
491	              an SA message entry. When Anycast RPs are used, all
492	              RPs use the same IP address. This parameter can be
493	              used to define a unique IP address for the RP of each
494	              MSDP peer.
495	              By default, the software uses the RP address of the
496	              local system.";

[nit] s/address of the interface is used/address of the interface used


...
555	       container timer {
556	         description "Timer attributes.";
557	         leaf connect-retry-interval {
558	           type uint16;
559	           units seconds;
560	           default 30;
561	           description "Peer timer for connect-retry,
562	                        SHOULD be set to 30 seconds.";
563	         }

[major] BCP14 is not referenced, nor is the template used.  This is the only instance of rfc2119 keywords that I found.  If you want to keep the Normative language, then please include the template and references.


...
591	       uses ttl-threshold;

[?] The previous entry (line 510) reads:

     uses ttl-threshold {
       if-feature global-ttl-threshold;
     }

Should this mention of ttl-threshold be similar?



...
639	       leaf is-default-peer {
640	         type boolean;
641	         config false;
642	         description "If this peer is default peer.";

[minor] s/If this peer is default peer./'true' if this peer is a default peer.


...
738	     grouping ttl-threshold {
739	       description "Attribute to configure TTL threshold.";
740	       leaf ttl-threshold {
741	         type uint8 {
742	           range 1.255;
743	         }
744	         description "Maximum number of hops data packets can
745	                      traverse before being dropped.";
746	       }
747	     } // sa-ttl-threshold

[nit] s/sa-ttl-threshold/ttl-threshold


...
806	           list peer {
807	             key "address";
808	             description
809	               "List of MSDP peers.";
810	             leaf address {
811	               type inet:ipv4-address;
812	               description
813	                 "The address of peer";

[nit] s/address of peer/address of the peer


...
955	7.  Security Considerations

[major] Please update this section with the appropriate references (using the latest template): https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines


...
970	   There are a number of data nodes defined in this YANG module that are
971	   writable/creatable/deletable (i.e., config true, which is the
972	   default).  These data nodes may be considered sensitive or vulnerable
973	   in some network environments.  Write operations (e.g., edit-config)
974	   to these data nodes without proper protection can have a negative
975	   effect on network operations.  For MSDP, the ability to modify MSDP
976	   configuration will allow the unexpected MSDP peer establishment and
977	   unexpected SA information learning and advertisement.  The "password"
978	   field is also a sensitive readable configuration, the unauthorized
979	   reading function may lead to the password leaking.  The security
980	   considerations of MSDP [RFC3618] are applicable.

[major] Please follow draft-ietf-pim-igmp-mld-yang (draft-ietf-ospf-yang is also a good example) in calling out specifics...before the Security ADs use a DISCUSS.

[major] Information about the readable nodes is missing.


...
989	8.  IANA Considerations

991	   The IANA is requested to assign two new URIs from the IETF XML
992	   registry ([RFC3688]).  Authors are suggesting the following URI:

[nit] s/([RFC3688])/[RFC3688]

994	   URI: urn:ietf:params:xml:ns:yang:ietf-msdp

996	   Registrant Contact: PIM WG

[minor] s/PIM WG/The IESG

998	   XML: N/A, the requested URI is an XML namespace

1000	   This document also requests one new YANG module name in the YANG
1001	   Module Names registry ([RFC6020]) with the following suggestion:

[nit] s/([RFC6020])/[RFC6020]


...
1022	11.  Normative References

1024	   [I-D.ietf-netmod-rfc6087bis]
1025	              Bierman, A., "Guidelines for Authors and Reviewers of YANG
1026	              Data Model Documents", draft-ietf-netmod-rfc6087bis-20
1027	              (work in progress), March 2018.

== Outdated reference: draft-ietf-netmod-rfc6087bis has been published as
   RFC 8407

[minor] This reference (and the one to rfc6087) are not used.


...
1047	   [RFC5246]  Dierks, T. and E. Rescorla, "The Transport Layer Security
1048	              (TLS) Protocol Version 1.2", RFC 5246,
1049	              DOI 10.17487/RFC5246, August 2008,
1050	              <https://www.rfc-editor.org/info/rfc5246>.

** Obsolete normative reference: RFC 5246 (Obsoleted by RFC 8446)


...
1070	   [RFC6536]  Bierman, A. and M. Bjorklund, "Network Configuration
1071	              Protocol (NETCONF) Access Control Model", RFC 6536,
1072	              DOI 10.17487/RFC6536, March 2012,
1073	              <https://www.rfc-editor.org/info/rfc6536>.

** Obsolete normative reference: RFC 6536 (Obsoleted by RFC 8341)


...
1079	   [RFC7223]  Bjorklund, M., "A YANG Data Model for Interface
1080	              Management", RFC 7223, DOI 10.17487/RFC7223, May 2014,
1081	              <https://www.rfc-editor.org/info/rfc7223>.

** Obsolete normative reference: RFC 7223 (Obsoleted by RFC 8343)

1083	   [RFC7277]  Bjorklund, M., "A YANG Data Model for IP Management",
1084	              RFC 7277, DOI 10.17487/RFC7277, June 2014,
1085	              <https://www.rfc-editor.org/info/rfc7277>.

** Obsolete normative reference: RFC 7277 (Obsoleted by RFC 8344)


1087	   [RFC8022]  Lhotka, L. and A. Lindem, "A YANG Data Model for Routing
1088	              Management", RFC 8022, DOI 10.17487/RFC8022, November
1089	              2016, <https://www.rfc-editor.org/info/rfc8022>.

** Obsolete normative reference: RFC 8022 (Obsoleted by RFC 8349)