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

[Yingzhen Qu <[e-mail]>]

Hi Jan,

Please see my questions below inline about the "mandatory" statement.

Thanks,
Yingzhen

On Wed, Dec 20, 2023 at 8:07 AM Jan Lindblad <[e-mail]> wrote:

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

[Yingzhen]: although both the msd-type and msd-value are needed for an
entry to make sense, it's possible for a router to not return any entry. My
understanding of a leaf to be mandatory means the leaf has to exist with a
value. Please correct me if I'm wrong.

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