Early Review of draft-ietf-pim-msdp-yang-01
review-ietf-pim-msdp-yang-01-yangdoctors-early-rahman-2018-01-12-00

Request Review of draft-ietf-pim-msdp-yang-01
Requested rev. 01 (document currently at 06)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2017-12-13
Requested 2017-11-22
Requested by Mehmet Ersue
Authors Xufeng Liu, Zheng Zhang, Anish Peter, Mahesh Sivakumar, Feng Guo, Pete McAllister
Draft last updated 2018-01-12
Completed reviews Yangdoctors Early review of -01 by Reshad Rahman (diff)
Comments
Requested by YANG secretary on behalf of PIM chairs.
Assignment Reviewer Reshad Rahman
State Completed
Review review-ietf-pim-msdp-yang-01-yangdoctors-early-rahman-2018-01-12
Reviewed rev. 01 (document currently at 06)
Review result On the Right Track
Review completed: 2018-01-12

Review
review-ietf-pim-msdp-yang-01-yangdoctors-early-rahman-2018-01-12

YANG Doctor review of draft-ietf-pim-msdp-yang-01 by Reshad Rahman

1 module in this draft:
- ietf-msdp@2017-08-30.yang

No YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.53).

0 examples are provided in this draft (section 3.12 of draft-ietf-netmod-rfc6087bis-15)

Module ietf-msdp@2017-08-30.yang:
- Too many features (17)! Every piece of config has an if-feature statement. Some of the configs (timers?) should be part of most/basic implementations, for other config (e.g. authentication) I can see why a feature would be used.
- “import ietf-yang-types” should have a reference to RFC6991 (see section 4.7 of rfc6087bis-15) 
- “import ietf-inet-types” should have a reference to RFC6991
- “import ietf-routing” should have a reference to RFC8022
- “import ietf-interfaces” should have a reference to RFC7223
- "import ietf-ip" should have a reference to RFC7277
- "import ietf-key-chain" should have a reference to RFC8177
- organization s/"...PIM( Protocols for IP Multicast ) Working Group"/"...PIM (Protocols for IP Multicast) Working Group"?
- Remove WG Chairs from contact information as per Appendix C of rfc6087bis-15 
- No copyright in the module description, see Appendix of 6087bis-15 for a module description example
- Module description must contain reference to RFC, see Appendix C of rfc6087bis-15
- grouping authentication-container. key-chain and password both use if-feature peer-key-chain.
- grouping connect-source. The name is not very descriptive. Should this be something along the lines of tcp-connection-source?
- grouping global-state-attributes has nothing
- Some of the descriptions are pretty terse. e.g. for rpf-peer it says "RPF peer.". In a case like this consider adding more descriptive text or a reference to the proper section in RFC3618
- peer-as (Autonomous System Number) is defined as type string, should be of type as-number in ietf-inet-types?
- Not sure why peer-as is needed. Don't see it in RFC3618.
- keepalive-interval depends on holdtime-interval. There should be "if-feature peer-timer-holdtime" under leaf keepalive-interval or change the must statement to (assuming we keep the 2 features):
  must "(not ../holdtime-interval) or (. > 1 and . < ../holdtime-interval)".
- leaf up-time: s/sa cache/SA cache/
- leaf up-time, what's meant by "up time" in the description? Is it time it's been created?
- description for leaf expire seems wrong.
- leaf peer-learned-from, change description from "The address of peer that we learned this SA from ." to "The address of the peer that we learned this SA from."
- Groupings are used for data which is used only once. Is this done on purpose or was the intention to use those groupings more than once?
- augment of control-plane-protocols is incorrect. There should be an identity msdp with base "rt:routing-protocol" and then augment "/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol" with a when statement. Take a look at OSPF YANG for an example.
- RPC leaf group, I thought we had a type for IP multicast address? If not, it should be done?
- s/msdp/MSDP/
- In rpc msdp-clear-peer, s/Clears the session to the peer./Clears the TCP connection to the peer./
- In rpc msdp-clear-sa-cache, why have the enum '*' for source-addr. Can't the same technique as for peer-address be used?
- msdp prefix not needed in rpc names
- MSDP peers are configured in a mesh-group, did the authors consider adding state per mesh-group, e.g. all the peers in a particular mesh-group?

General:
- Per Appendix B of rfc6087bis-15: "that all YANG modules containing
imported items are cited as normative reference". So RFCs 6991, 7223,
7277, 8022 and 8177 should be included in the normative reference
section.
- Section 3 "the irrelevant information", add a reference/explanation for what the irrelevant information is. s/the irrelevant information/irrelevant information/?
- Section 5 should give a brief description of what the RPCs do.
- Section 6 any plans for notifications? If not, just say so.
- Need Security Considerations, see sections 3.7 and 6 of rfc6087bis-15
- Need IANA Considerations, see section 3.8 of rfc6087bis-15
- Need license in YANG module, see appendix B of rfc6087bis-15

Nits:
- Section 4 s/Sa-cache/SA-cache/