Re: [yang-doctors] Yangdoctors early review of draft-ietf-dots-rfc8782-bis-00

[Ebben Aries <[e-mail]>]

On Sep 24 08:36 AM, [e-mail] wrote:
> Hi Ebben, all,
> 
> Thank you for the review. 
> 
> Please see inline. 
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Ebben Aries via Datatracker [mailto:[e-mail]]
> > Envoyé : jeudi 24 septembre 2020 03:38
> > À : [e-mail]
> > Cc : [e-mail]; [e-mail]
> > Objet : Yangdoctors early review of draft-ietf-dots-rfc8782-bis-00
> > 
> > Reviewer: Ebben Aries
> > Review result: On the Right Track
> > 
> > 2 modules in this draft:
> > - [e-mail]
> > - [e-mail]
> > 
> > YANG compiler errors or warnings (pyang 2.3.2, yanglint 1.9.2)
> > - No compiler errors or warnings however pyang 2.3.2 is currently
> > the only
> >   compiler verified to emit YANG sx:structure data in tree format.
> > Instance
> 
> [Med] I confirm this is what we are using. 
> 
> >   data could not yet easily be validated given the current
> > linters/compilers.
> > 
> > General comments on the draft/modules:
> > ----------------------------------------
> > The modules defined in this draft are an update replacement to those
> > defined in RFC8782 with only 1 being updated from the prior
> > publication.
> >   - [e-mail] (updated)
> >   - [e-mail]
> > 
> > - First, I share some of the same comments as the previous review in
> > that it
> >   takes some additional thought on modeling structures for alternate
> > protocol
> >   communications with the YANG language.  Overall, I think this is
> > likely fine
> >   but also question where the draft/RFC specification outlines rules
> > that are
> >   not conveyed within the data-model.  Some examples include
> > specifying
> >   mandatory attributes but not making use of the YANG 'mandatory'
> > statement.
> 
> [Med] We are using "mandatory" statements when this makes sense (e.g.,
> alt-server, hb-status). It is not used for some attributes that may
> appear in both directions, but are not mandatory in one direction. For
> example, "lifetime" is mandatory in a mitigation request, but it is
> when the server replies with a conflict message. 
> 
> FWIW, a constraint we had is to reuse as much as possible the same
> CBOR key/attribute names. This is not possible if we overload the
> module with "choice"s:
> 
> ==
>    Each "case" may
>    contain multiple nodes, but each node may appear in only one "case"
>    under a "choice".
> ==
>
> This is why what is normative for an implementation is the core of the
> specification not the YANG module itself.
>

Ack - however just like other verbiage put within the YANG description
statements to indicate valid/invalid values (some of which is a direct
copy from the document detail), I might expect at a bare minimum the
description for the node carry when a node is mandatory within the
payload or not if it cannot be expressed via a language statement as you
indicate - e.g. This node is mandatory for request and info retrieval
operations.

Also keep in mind, YD reviews likely have not dealt with many such cases
yet so a best practice for these types of use-cases does not necessarily
exist yet.

> >   Other cases are stating the intent of reserved or invalid integers
> > but not
> >   putting the same restrictions on the types under a given data
> > node. 
> 
> [Med] Can you please indicate which data node are you referring to? Thanks.
>

A common example to point out...

An unbounded int32 with the description yielding the rules around valid
values.  From a pure type perspective, all values that fall within the
int32 range are valid and data validation is only then left to the
running implementation and conveying invalid boundaries by way of
protocol error messaging.

105       leaf lifetime {
106         type int32;
107         units "seconds";
108         default "3600";
109         description
110           "Indicates the lifetime of the mitigation request.
111
112            A lifetime of '0' in a mitigation request is an
113            invalid value.
114
115            A lifetime of negative one (-1) indicates indefinite
116            lifetime for the mitigation request.";
117       }

Now to be fair, this also exists in datastore related modules where the
instance data can be valid from a data-model conformance standpoint but
still not meet the requirements of the implementation - all such cases
make it harder for the model consumer to fully validate instance data
outside of the complete implementation.

The tradeoff of leaving this outside the schema is likely fine here.

>  Overall
> >   this means that you cannot validate the instance/payload data
> > according to
> >   the data-model alone.
> 
> [Med] Agree. The data model alone is not sufficient for an
> implementer, it should rely on the detailed spec. 
> 
> > 
> > - Previous review suggested the main edits required switching the
> > top-level
> >   container 'dots-signal'.  This has now been fulfilled by that of
> > an
> >   sx:structure in this revision.
> > 
> 
> [Med] Thanks. 
> 
> > - A recent discussion among YANG doctors suggests the use of
> > normative
> >   language (RFC2119) in YANG description statements. For example
> > 'should' =>
> >   'SHOULD', 'must' => 'MUST', etc... A benefit to this approach is
> > RFC
> >   comprehension even when the module is stripped from the source
> > document it
> >   is contained under.
> 
> [Med] Thank you for sharing this. I understand the benefit for the
> "classic" use of YANG, but in our case I'm afraid thus this will be
> redundant with the core specification itself.
>

YANG modules can become detached from the IETF document containing them
(and is the intention for the normal uses as you mention) and for this
purpose, the module should be as descriptive as possible to live on its
own.

Now for this use, you are defining solely message structures which can
likely still be used for library code generation on its own without
further context.  The implementation however will need to take into
account the specification to build upon and this is fine.

And yes, for all other cases I mention the intention is to be redundant
carrying the verbiage from the specification detail into the YANG
description statements for consistency - especially here where a
protocol specification will already carry this in the detail.

> > 
> > - (draft) L887 nit: "As a reminder, the prefix length must be less
> > than or
> >   equal to 32 (for IPv4) for 128 (for IPv6)"
> 
> [Med] Fixed. Changed to:
> 
> "As a reminder, the prefix length must be less than or equal to 32 for IPv4 and 128 for IPv6."
> 
> > 
> > Module ietf-dots-signal-channel:
> > ----------------------------------------
> > - IMO, the module prefix 'signal' is too generic for what is
> > specific to
> >   message payload over a protocol that uses CoAP.  Looking at all
> > the
> >   published RFCs for dots, we have the same use of generic prefixes.
> > Since
> >   the prefix is of local scope, it is not an entirely large issue
> > but in
> >   hindsight should have had some better consistency especially when
> > the
> >   recommendation to module importers is to utilize the defined
> > prefix of said
> >   module.
> 
> [Med] Fair enough. Changed to "ietf-dots-signal".
> 
> > 
> > - This module imports and references 'ietf-dots-data-channel' as
> > prefix
> >   'ietf-data'.  Not only is this too generic as I mentioned above,
> > it is not
> >   using the suggested prefix of 'data-channel' (which is also too
> > generic) as
> >   defined within that module (RFC7950 Section 7.1.4)
> > 
> >    "When used inside the "module" statement, the "prefix" statement
> >     defines the prefix suggested to be used when this module is
> > imported."
> 
> [Med] Fixed. Updated to use 'data-channel'.
>
> > 
> > - Minor nit: It appears all author email addresses have been updated
> > since the
> >   RFC8782 variant replacing '@' with '&'
> 
> [Med] Fixed. 
> 
> > 
> > Module iana-dots-signal-channel:
> > ----------------------------------------
> > - Similar to what I mention above regarding the module prefix, we
> > use a
> >   generic 'iana-signal' prefix here which is inconsistent and not
> > descriptive
> >   of the overall intent of the module.
> 
> [Med] Updated to "iana-signal-channel".
> 
> > 
> > Overall, I'm not sure if the other referenced and published DOTS
> > YANG modules went through a YD review but the comments above really
> > should be addressed in a new revision of those modules.
> 
> [Med] Sure. We will double check those. 
> 
>   Once the
> > above is addressed and possibly some discussion around when type
> > restrictions are not conveyed within the model, it is probably on
> > the right track. 
> 
> [Med] Great, thanks. We can add some clarification as needed, but as
> indicated above, if you can share the attributes you are referring to,
> this will be helpful. Thanks.  
> 
>  Since this is not the usual use of YANG in the
> > context of YD reviews, it is a bit of an on the fly analysis which
> > may require some additional discussion among other YANG doctors.
> > 
> 
> [Med] Sure.
> 
> 
> _________________________________________________________________________________________________________________________
> 
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
> 
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.
>