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

[Jan Lindblad <[e-mail]>]

Yingzhen,

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

That was impressively fast :-)

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

Excellent. I think that was a really good change, improving both efficiency and clarity.

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

Since you confirm my understanding that both values are needed for an entry to make sense, I would actually suggest that you mark both as   mandatory true;   The mandatory concept is also useful for config false data. In this case it means the server MUST return these values for each entry. This will make it abundantly clear to all that every entry needs to have both. Users that are using this model can file a bug report to their vendors etc if they see something else, rather than a drawn out conversation about what is allowed in this list.


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

Great.

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

Indentation looks perfect to me now.
 
>> 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.

Nice.

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

My comment here is truly a nit, so I'm not going to ask you to do any change, but in principle I think the security considerations should not be a place for template text. This section is potentially very important and should be tailored to the situation at hand.

Best Regards,
/jan