A YANG Module for IS-IS Reverse Metric
draft-ietf-lsr-yang-isis-reverse-metric-06

I'll echo the polish Ben Kaduk suggested on structuring the security considerations of the sensitive read and write nodes in Section 4.

whole-lan is only applicable to multi-access interfaces.  I was expecting something similar to how "priority" is defined (in the main module), but I can't find that here.  Am I missing something?

Thanks for the simple and easy-to-read document!
My comments are pretty minor.

Section 2.2

     grouping reverse-metric-if-config-data {
       description "IS-IS reverse metric config data.";
       container reverse-metric {
         description "IS-IS reverse metric data.";
         uses reverse-metric-data;
         leaf exclude-te-metric {
           type boolean;
           default false;
           description
             "If true and there is a TE metric defined for this
              interface then do not send the TE metric sub-TLV in the
              reverse metric TLV.";
           reference "RFC8500, Section 3.5";
         }
       }
     }

     grouping tlv16-reverse-metric {
       description "IS-IS reverse metric TLV data.";
       container reverse-metric {
         description "IS-IS reverse metric TLV data.";
         uses reverse-metric-data;
         leaf te-metric {
           type uint32;
           description
             "The TE metric value from the sub-TLV if present.";
           reference "RFC8500, Section 3.5";
         }
       }
     }

Please confirm that Section 3.5 of RFC 8500 is the right reference for
both of these; I didn't really see much there that lined up well with
these descriptions.

       container reverse-metric {
         description "Announce a reverse metric to neighbors.";
         uses reverse-metric-if-config-data;
         container level-1 {
           description
             "Announce a reverse metric to level-1 neighbors.";
           uses reverse-metric-if-config-data;
         }
         container level-2 {
           description
             "Announce a reverse metric to level-2 neighbors.";
           uses reverse-metric-if-config-data;
         }
       }

Are the interactions between the toplevel
"reverse-metric-if-config-data" and the per-level uses of that grouping
adequately specified by the core draft-ietf-isis-yang-isis-cfg such that
we don't need to repeat them here?  (I think it probably is.)

Section 4

   These are the subtrees and data nodes and their sensitivity/
   vulnerability:

I think the intent of the security considerations template is that we
specifically talk about what bad things would happen if some
unauthorized entity was writing values to the ("config true") nodes, or
reading values from the ("config false") nodes.  The current text here
seems to just be listing the nodes without saying what happens if
they're misconfigured or the contents are leaked to an unauthorized
party.  (On first glance, it seems like the security considerations of
RFC 8500 basically cover everything that we would need to say.  It's
short, so we could repeat the content, or we could say that these YANG
nodes correspond directly to the RFC 8500 functionality and the
considerations of the functionality are described in RFC 8500.)

Section 5

The core NETCONF/RESTCONF RFCs may not need to be classified as
normative (we only reference them from the security considerations
boilerplate), but there's not much harm in leaving them here.

Appendix A

Thank you for including the examples!

Thanks to Vijay Gurbani for their General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/vTI3euK_FgI2nI-8482WikVehC8).

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 2.2. , paragraph 11, nit:
> eudo-node LSP for this interface. Otherwise it will only increment the metric
>                                   ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Otherwise".

Section 2.2. , paragraph 15, nit:
> ic announcements from neighbors. By default reverse metric handling is disab
>                                  ^^^^^^^^^^
Did you mean: "By default,"?

These URLs point to tools.ietf.org, which is being deprecated:
 * https://tools.ietf.org/html/rfcXXXX
 * https://tools.ietf.org/html/draft-ietf-isis-yang-isis-cfg-42
 * https://tools.ietf.org/wg/lsr/

Hi Chris,

Thanks for this YANG module.

Nit:
- Copyright statement in the YANG module should presumably be updated to 2021, to match the revision entry.

A few comments on the YANG model:
- For the interface config, reverse-metric turns up twice in the path.  You could perhaps drop it from the
  grouping so that it only appears once?
- Would it be helpful to make the top level reverse-metric container have presence?  This might make more
  sense if constraints are ever added to validate that a metric is specified at the top level, or under at
  least one of the levels.
- Am I right in assuming that that the level-1/level-2 config is effectively hierarchical and would override
  the config under the reverse-metric grouping?  E.g., is it allowed to specify a metric at the top level,
  and the whole-lan flag only under the level-1?  If so, would it be helpful to document this hierarchical
  behaviour in the description for the fields?
- There is a default assigned to exclude-te-metric, but no default assigned to whole-lan and allow-unreachable.
  If the config is not hierarchical, then should these all have defaults? If the config is hierarchical then
  perhaps they should not have any defaults, and the use statement for the top level reverse-metric grouping
  should refine them with default values?  Assuming that the descriptions make their hierarchical nature
  clear?

Security Considerations:
Would it also be helpful to include a reference back to the security considerations of the base ISIS YANG
module, since the concerns that apply to metrics there would seem to mostly also apply here.

References:
- Probably need to add XML and JSON references or otherwise change the requests to edit-config or RESTCONF requests.
- XML reference can be as per RFC 8342, JSON should probably be to RFC 7951.

A.1.  Example Enable XML
Suggest retitling to: Enablement Example Using XML YANG Instance Data"

A.2.  Example Use XML
Suggest retitling to: "Usage Example using XML YANG Instance Data"

A.3.  Example JSON
Suggest retitling to: "Usage Example using JSON YANG Instance Data"

Thanks,
Rob