[yang-doctors] Yangdoctors early review of draft-ietf-pim-msdp-yang-01

[Reshad Rahman <[e-mail]>]

Reviewer: Reshad Rahman
Review result: On the Right Track

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

1 module in this draft:
- [e-mail]

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 [e-mail]:
- 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/