Re: [Last-Call] Yangdoctors last call review of draft-ietf-dots-telemetry-14 (First Part)

[[e-mail]]

Hi Jan, 

Focusing now on the first part of the review. 

Will let my co-authors further comment as appropriate. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Jan Lindblad via Datatracker [mailto:[e-mail]]
> Envoyé : jeudi 26 novembre 2020 12:37
> À : [e-mail]
> Cc : [e-mail]; [e-mail]; draft-ietf-dots-
> [e-mail]
> Objet : Yangdoctors last call review of draft-ietf-dots-telemetry-14
> 
> Reviewer: Jan Lindblad
> Review result: Almost Ready
> 
> Dear Dots Authors, NETMOD WG,
> 
> This is my YD LC review of draft-ietf-dots-telemetry-14. This draft
> contains two YANG modules, ietf-dots-telemetry and module ietf-dots-
> mapping. Both modules are unusual from a YANG perspective in that
> they consist of solely typedefs, groupings and sx:structures, and
> that their purpose is to define message bodies for a domain specific
> management protocol.

[Med] This comment does not apply to ietf-dots-mapping. 

> 
> Since my previous review of this module (-09, June 2020) the
> underpinnings of the modules have changed significantly with a new
> RFC in the works, addressing the fundamental issues pointed out at
> that time. Very good & impressive. I still think there are important
> issues to resolve, so I will call the current state of -14 "Almost
> Ready".
> 
> Links to previous relevant reviews:
> https://datatracker.ietf.org/doc/review-ietf-dots-rfc8782-bis-00-
> yangdoctors-early-aries-2020-09-23/
> https://datatracker.ietf.org/doc/review-ietf-dots-telemetry-09-
> yangdoctors-early-lindblad-2020-06-30/
> 
> Because of this hitherto unusual application of YANG, the usual YD
> review procedures are not really applicable. Whoever reads this
> should feel free to comment on which perspectives should be included
> (or not) in an YD review of a YANG module defining a new protocol.
> 
> I think the YD review should not be expected to cover the
> usability/interoperability of the newly defined protocol as a whole.
> E.g. which messages are sent when, what is the relevant/reasonable
> content in each message, are there any races or scalability issues,
> etc. IMO, such an analysis is essential, but too much work for an YD
> review (and outside my area of expertise). I have placed my review
> focus on the clear interpretation of the YANG modules, since that
> will be key to interoperability.
> 
> When YANG is mapped to NETCONF or RESTCONF, there are entire RFCs
> devoted to describing how that mapping is done, from what
> "mandatory" actually means in the protocol and how keys are sent,
> all the way to how the XML or JSON payloads are constructed and
> potential error messages. A lot more of that mapping is now present
> in this document, but I still find many aspects that are
> undefined/unclear.
> 
> #1 mandatory true
> 
> A few elements are marked mandatory true in the model. The meaning
> of this is not defined in the draft. I would expect this means
> clients as well as servers MUST provide a value for the mandatory
> element in each message if the parent element exists. This would be
> like how mandatory elements in YANG actions/rpcs/notifications work
> (and different from how mandatory works with configuration data in
> NETCONF/YANG).
> 
> Section 7.1 states: "DOTS telemetry attributes are optionally
> signaled and therefore MUST NOT be treated as mandatory fields in
> the DOTS signal channel protocol."

[Med] Telemetry attributes are optional from a DOTS standpoint. Hence, the text you quoted.  

"mandatory true" is used to tag attributes that must be present if the parent one was present. For example, "capacity" is a mandatory attribute if "total-pipe-capacity" is present. Still, total-pipe-capacity" is optional. 

              list total-pipe-capacity {
                key "link-id unit";
                description
                  "Total pipe capacity of a DOTS client domain.";
                leaf link-id {
                  ...
                }
                leaf capacity {
                  type uint64;
                  mandatory true;
                  description
                    "Pipe capacity.";
                }
                leaf unit {
                  ...
                }
              }
            }

Will consider adding a note to explain this. 

 This blanket statement makes
> things less clear for me. Can I trust the YANG mandatory and key
> statements to indicate what has to be included, and everything else
> is optional? If so, I would advise to state just that.
> 
> #2 key
> 
> There are many key statements around these modules. The exact
> meaning of the key keyword in this dots-signal protocol is not
> defined. Are key sets always unique in a list?

[Med] Yes. We are not deviating from the usual use of "keys". 

Note that we omit the "key" statement for some lists because the keys of these lists is sent in Uri-Path options (for requests) and may not/must not appear in a response message body. For such lists, there is an ordering requirement of these options. This ordering is needed to uniquely identify a DOTS resource. All this is already covered in the text. 

This behaviour is compliant with RFC8791: "The list-stmt is not required to have a key-stmt defined".

 Are keys mandatory?

[Med] Yes. We are not deviating from the usual use of "keys".  

> Are there any sequencing requirements on keys, e.g. do keys need to
> be sent first (as in NETCONF) in a list element?

[Med] The spec does not add any specific sequencing requirement.

> Such a requirement may not be unreasonable for servers with limited
> resources.
> 
> #3) if-feature
> 
> The ietf-dots-mapping.yang module defines one YANG feature and a few
> instances of model statements conditional on it with an if-feature
> statement. There is a description for how a server advertises
> support for this specific feature (by including a very particular
> leaf in one of the messages). From a YANG mapping perspective, it
> might have been better to have a general mechanism for advertising
> features. With the current specification, no other features can
> (ever) be added in a backwards compatible way.

[Med] Usual tools such as RFC7895 can be used, but we do need more granularity within a feature itself. We call that "capability". 

> 
> #5) augment or sx:augment-structure
> 
> The draft is using augment (but means sx:augment-structure) and
> sx:augment-structure. Are implementors allowed to use augment in
> their implementations? If so, how would a client know if an
> augmented model is in use? Section 6.1.2 states that servers MUST
> reject messages when "attribute values are not acceptable". Does
> that have any implications for augment?

[Med] I'm afraid we are mixing two points here:

* "attribute values are not acceptable" is in reference to a value of a configurable parameter that is not within an acceptable range by a server. That range can be retrieve by the client. This has nothing to do with an augment. 

* We don’t make any assumption/restriction about augments, but from DOTS protocol standpoint, what is important is to have a mapping to a CBOR key. If the key is not known to a peer, the attribute will be ignored. 

> 
> #6) deviations
> 
> Deviations are never mentioned. Are implementors allowed to deviate
> from the protocol in any way, if declared using deviation
> statements? If so, how would a client know if a deviated model is in
> use?

[Med] Some comments here: 
* A client in general does not know what model is used by a peer as what it sees is CBOR.
* A server can use distinct default values (e.g., percentile values), but this can be retrieved by a client during the telemetry-setup. 

Other than that, deviations should be avoided as this will impact message validation. 

> 
> #7) config true|false
> 
> RFC 8791 (which defines sx:structure) expressly states that config
> true and false statements have no defined meaning and will be
> ignored in sx:structure.
> Therefore it's very good that this draft defines behavior for the
> GET operation with respect to config true and config false elements.
> Nothing is said about how the config attribute plays with PUT, POST
> or DELETE however.
> 

[Med] Can you please explicit what additional consideration you have in mind? Thank you.

> #8) PATCH
> 
> Is the PATCH HTTP operation supported in this context? It is never
> mentioned.
> If so, it's is unclear how it would work in relation to the protocol
> HTTP headers, the tsid mechanism and the YANG mandatory concept. If
> not, that might be good to point out somewhere.
> 

[Med] Supported CoAP operations are those listed in https://tools.ietf.org/html/draft-ietf-dots-rfc8782-bis-02#section-4.4. 

> #9) Payload encoding references
> 
> As far as I understand, the message payloads are meant to be encoded
> in JSON or CBOR.

[Med] Not JSON. It is encoded in CBOR only: 

   Telemetry messages exchanged between DOTS agents are serialized using
   Concise Binary Object Representation (CBOR) [RFC7049].  CBOR-encoded
   payloads are used to carry signal channel specific payload messages
   which convey request parameters and response information such as
   errors.

or

   All DOTS telemetry parameters in the payload of the DOTS signal
   channel MUST be mapped to CBOR types as shown in the following table:

 This draft has no references to the relevant
> documents describing these encodings
> (https://tools.ietf.org/html/rfc7951 and
> https://tools.ietf.org/html/draft-ietf-core-yang-cbor-13).

[Med] That’s on purpose because these are listed in draft-ietf-dots-rfc8782-bis that says: 

   Messages exchanged between DOTS agents are serialized using Concise
   Binary Object Representation (CBOR) [RFC7049], a binary encoding
   scheme designed for small code and message size.  CBOR-encoded
   payloads are used to carry signal channel-specific payload messages
   that convey request parameters and response information such as
   errors.  In order to allow the reusing of data models across
   protocols, [RFC7951] specifies the JavaScript Object Notation (JSON)
   encoding of YANG-modeled data.  A similar effort for CBOR is defined
   in [I-D.ietf-core-yang-cbor].

We thought it is redundant if we restate it here as well. 

_________________________________________________________________________________________________________________________

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.