Subject: [Dots] Yangdoctors early review of draft-ietf-dots-rfc8782-bis-00

From: Ebben Aries via Datatracker <[email-masked]>
Date: Wed, 23 Sep 2020 18:38:04 -0700

Reviewer: Ebben Aries
Review result: On the Right Track

2 modules in this draft:
- [email-masked]
- [email-masked]

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
  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.
  - [email-masked] (updated)
  - [email-masked]

- 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.
  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.  Overall
  this means that you cannot validate the instance/payload data according to
  the data-model alone.

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

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

- (draft) L887 nit: "As a reminder, the prefix length must be less than or
  equal to 32 (for IPv4) for 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.

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

- Minor nit: It appears all author email addresses have been updated since the
  RFC8782 variant replacing '@' with '&'

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.

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

---

From: <[email-masked]>
Date: Thu, 24 Sep 2020 08:36:08 +0000

Hi Ebben, all,

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Ebben Aries via Datatracker [mailto:[email-masked]]
> Envoyé : jeudi 24 septembre 2020 03:38
> À : [email-masked]
> Cc : [email-masked]; [email-masked]
> Objet : Yangdoctors early review of draft-ietf-dots-rfc8782-bis-00
> 
> Reviewer: Ebben Aries

[Med] I confirm this is what we are using. 

> The modules defined in this draft are an update replacement to those
> defined in RFC8782 with only 1 being updated from the prior
> publication.

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

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

 Overall
>   this means that you cannot validate the instance/payload data

[Med] Agree. The data model alone is not sufficient for an implementer, it should rely on the detailed spec. 

>   sx:structure in this revision.

[Med] Thanks. 

>   comprehension even when the module is stripped from the source

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

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

[Med] Fair enough. Changed to "ietf-dots-signal".

>     defines the prefix suggested to be used when this module is

[Med] Fixed. Updated to use 'data-channel'.

>   RFC8782 variant replacing '@' with '&'

[Med] Fixed. 

> Module iana-dots-signal-channel:

[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

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

---

From: Ebben Aries <[email-masked]>
Date: Thu, 24 Sep 2020 18:53:00 -0600

On Sep 24 08:36 AM, [email-masked] wrote:
> [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
> 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.

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

> [Med] Agree. The data model alone is not sufficient for an
> implementer, it should rely on the detailed spec. 
> 
> > 
> [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.

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

---

From: <[email-masked]>
Date: Fri, 25 Sep 2020 11:26:20 +0000

Hi Ebben, 

Thank you. An udpated verison that takes into account your review is available at: 

https://tools.ietf.org/html/draft-ietf-dots-rfc8782-bis-01 
https://datatracker.ietf.org/doc/html/draft-ietf-dots-rfc8782-bis-01 

A diff from the previous version is available at:
https://github.com/boucadair/rfc8782-yang-update/blob/master/Ebben-Review.pdf 

Cheers,
Med

> De : Ebben Aries [mailto:[email-masked]]
> Envoyé : vendredi 25 septembre 2020 02:53
> À : BOUCADAIR Mohamed TGI/OLN <[email-masked]>
> Cc : [email-masked]; [email-masked]; draft-ietf-dots-rfc8782-
> [email-masked]
> Objet : Re: Yangdoctors early review of draft-ietf-dots-rfc8782-bis-
> 00
> 
> Envoyé :
> > > jeudi 24 septembre 2020 03:38 À : [email-masked] Cc :
> > > [email-masked]; [email-masked]
> > > - 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
> >
> > > - 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
> > >   mandatory attributes but not making use of the YANG
> 'mandatory'
> > > statement.
> >
> 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
> An unbounded int32 with the description yielding the rules around
> valid values.  From a pure type perspective, all values that fall
> within the
> 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.
> 
> > > - Previous review suggested the main edits required switching
> the
> > > top-level
> > >   container 'dots-signal'.  This has now been fulfilled by that
> of
> > > an
> > >   'SHOULD', 'must' => 'MUST', etc... A benefit to this approach
> is
> > > RFC
> 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