Re: [Lsr] Robert Wilton's No Objection on draft-ietf-lsr-yang-isis-reverse-metric-04: (with COMMENT)

[Christian Hopps <[e-mail]>]

Hi Robert,

tl;dr all comments addressed :)

new version published:

  Diff: https://www.ietf.org/rfcdiff?url2=draft-ietf-lsr-yang-isis-reverse-metric-06
  URL: https://www.ietf.org/archive/id/draft-ietf-lsr-yang-isis-reverse-metric-06.txt

Comments inline as well...

Robert Wilton via Datatracker <[e-mail]> writes:

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> 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.

Updated to 2022.

> 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?

Dropped from grouping.

> - 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.

This would require specifying a default reverse metric metric value, and there would be no way to enable reverse metric at only 1 of 2 levels on a level-1-2 interface. I have expanded the description under the interface config augment container node to the following:

    container reverse-metric {
      description
        "Announce a reverse metric to neighbors. The configuration
         is hierarchical and follows the same behavior as defined
         for 'Per-Level' values in the augmented base module.

         Reverse metric operation is enabled by the configuration of
         a reverse-metric metric value at either the top level or
         under a level-specific container node. If a reverse-metric
         metric value is only specified under a level-specific
         container node then operation is only enabled at the
         specified level.";

> - 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?

This hierarchical structure is documented in the base model. In addition to the text added in the description noted in the previous question, I've added the following text under "YANG Module" section as well:

  This YANG module uses the same "Per-Level" hierarchical configuration
  structure as is defined in the augmented base module.

> - 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?

I have added a top level refinement to add default false for both flags.

> 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.

Added a reference to the base YANG I-D.

> 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.

Done.

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

Done.

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

Done.

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

Done.

Thanks,
Chris.

> Thanks,
> Rob