Hi,

I was assigned to be the YANG doctor for this document. Here is my
review:

**** General comments

Overall, the module is in a good shape and integrates nicely with the
core routing data model [RFC 8022].

The I-D text is rather scarce, apart from the module it essentially
contains only boilerplate stuff. Some information about the context in
which this module is supposed to be used, design decisions etc. would
be certainly useful.

A non-normative appendix with a configuration example (instance data)
would also help people that are not fluent in YANG to understand what
data are included in this model.

Some parts of the model – "distribute-list", "redistribute", "bfd",
"authentication" on interfaces (or at least the "key" list) – are
probably not specific to RIP and could be used by other protocols as
well. If it is so, it would be better to model them separately as
general mechanisms.

Documents containing YANG modules that are imported by ietf-rip should
appear among normative references. One way to achieve this (which is
also useful by itself) is to include a table of namespace prefixes,
see e.g.

https://tools.ietf.org/html/rfc8022#section-2.3

**** Specific comments

     - Matching identities is done in the old YANG 1.0 way, for
       example:

       when "rt:type = 'rip:ripv2' or rt:type = 'rip:ripng'"

       This is fragile because the identities are matched based on
       string equality, so the result depends on the choice of the
       namespace prefix. Unless there is a strong reason not to use
       YANG 1.1, this is much simpler and more rubust

       when "derived-from(rt:type, 'rip:rip')"

     - In "redistribution", the must expressions are outright broken,
       e.g. for IS-IS:

       must "../../../../../rt:control-plane-protocol"
       + "[rt:name = current()]/type = 'isis'" { ... }

       First, the ietf-rip module also needs to import ietf-isis, and
       then the must statement can be rewritten like so:

       must "derived-from-or-self("
       + "../../../../../rt:control-plane-protocol"
       + "[rt:name = current()]/type, 'isis:isis')"

     - Some descriptions could mention that the parameter (feature
       etc.) only applies to RIP. For example, in

       feature interface-statistics {
         description
           "This feature indicates that the system supports collecting
            per-interface statistic data.";
       }

       I would add "... related to RIP." at the end.

     - Maybe it's absolutely clear to experts but the name and
       description of the "neighbor-configuration" feature look like
       the feature can make neighbors remotely configurable. I had to
       look up where it is used to find out what's the real
       meaning. Perhaps something like "explicit-neighbors" might be
       easier to understand?

     - The name of "no-supply" leaf looks strange. I checked a couple
       of CLIs and they use "passive", "passive-interface" or
       "send-options" for preventing RIP updates from being sent.

     - In "split-horizon" leaf, I would use "poison-reverse" enum
       rather than "poison".

Lada

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C