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

[Jan Lindblad via Datatracker <[e-mail]>]

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

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?

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.";
           }

Nit #4: Indentation

The indentation is a bit off. I'd suggest running the module through a pretty
printer, like pyang -f yang.

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..";
     }

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.

Best Regards,
/jan