Re: [yang-doctors] Yangdoctors early review of draft-ietf-mpls-msd-yang-02

[Yingzhen Qu <[e-mail]>]

Hi Jan,

Thanks for the review and comments. I've uploaded version -03 to address
your comments. Please see my answers below inline.

Thanks,
Yingzhen


On Tue, Dec 19, 2023 at 7:52 AM Jan Lindblad via Datatracker <
[e-mail]> wrote:

> Reviewer: Jan Lindblad
> Review result: Ready with Issues
>
> Hi all,
>
> This is my YANG-Doctor Last-Call review of draft-ietf-mpls-msd-yang-02.
> This
> draft contains two small and focused modules. They seem to be in pretty
> good
> shape, but I do have a couple of issues and nits to discuss.
>
> Issue #1: Keyless list
>
> The list node-msds is keyless. This is legal for config false lists, but
> that
> something is legal does not necessarily make it a good idea. Is there a
> strong
> reason for not having a key here? Keyless lists are less efficient, as all
> clients will have to read the entire list every time. Unless there is a
> strong
> reason not to, I would suggest adding a key here.
>
> Maybe the msd-type could function as a key for this list, or will there
> ever be
> multiple msd entries of the same msd type? Multiple entries of the same
> msd-type are currently allowed, but I'm not sure what the interpretation of
> such a server response should be.
>
>          list node-msds {
>            leaf msd-type { ... }
>            leaf msd-value { ... }
>          }
>
[Yingzhen]: I added a key of "msd-type".

>
> Issue #2: Optional values
>
> Both msd-type and msd-value are modeled as optional. They have no default
> and
> there is no description text explaining what it means if either of them is
> missing. Maybe they should be made mandatory, as I expect the list entry is
> pretty uninformative unless both are present?
>

[Yingzhen]: It's meaningless if there is a msd-value without a msd-type.
Theoretically
they should always show up as a pair. These are read-only data, so I don't
think any
control is needed.

>
> Nit #3: Repetition
>
> The msd-type and msd-value leaf definitions are repeated twice in the
> module
> (except for a one letter difference in one description that probably is not
> intentional). I would suggest placing them in a grouping that is
> referenced in
> each list to avoid the repetition.
>
>            leaf msd-type {
>              type identityref {
>                base iana-msd-types:msd-base-mpls;
>              }
>              description
>                "MSD type";
>            }
>            leaf msd-value {
>              type uint8;
>              description
>                "MSD value, in the range of 0-255.";
>            }
>
> [Yingzhen]: I added a grouping for this as suggested.


> Nit #4: Indentation
>
> The indentation is a bit off. I'd suggest running the module through a
> pretty
> printer, like pyang -f yang.
>
> [Yingzhen]: Done. Please let me know if you still see issues.


> Nit #5: Typo in reference
>
> The revision statement reference has some typos.
>
>      revision 2023-10-22 {
>        description
>          "Initial Version";
>        reference
>          "RFC XXXX: YA YANG Data Model for MPLS MSD..";
>      }
>
> [Yingzhen]: fixed.


> Nit #6: Read only data and get-config
>
> In section 5, security considerations, there is a paragraph:
>
>    Some of the readable data nodes in the modules may be considered
>    sensitive or vulnerable in some network environments.  It is thus
>    important to control read access (e.g., via get, get-config, or
>    notification) to these data nodes.
>
> Since the data in these modules is read-only, it will never show up in
> get-config, so listing that operation may be somewhat superfluous.
>

[Yingzhen]:  The text is from the template. I'm not sure whether we should
change it. If so,
we can simply remove "(e.g., via get, get-config, or notification) ".

>
> Best Regards,
> /jan
>
>
>
>