Hi Jan, Thank you very much for the detailed review. This is helpful. Will start with the minor points you raised. The other part will be addressed separately. Please see inline. Cheers, Med > -----Message d'origine----- > De : Jan Lindblad via Datatracker [mailto:noreply@xxxxxxxx] > Envoyé : jeudi 26 novembre 2020 12:37 > À : yang-doctors@xxxxxxxx > Cc : last-call@xxxxxxxx; dots@xxxxxxxx; draft-ietf-dots- > telemetry.all@xxxxxxxx > 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. > > 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". > ... > > And now a few minor comments on particular points or wordings. > > #10) The tree diagram in Fig. 30 is not using the correct format > (see the other tree diagrams). > [Med] The figure is correct. This is not augmenting an sx: module. > #11) Section 6.3.1 states "The overlapped lower numeric 'tsid' MUST > be automatically deleted and no longer be available." Since this > mechanism is a central part of the protocol, I would think > specifying exactly what "overlapped" means would improve > interoperability. [Med] The full text is: The relative order of two PUT requests carrying DOTS client domain pipe attributes from a DOTS client is determined by comparing their respective 'tsid' values. If such two requests have overlapping 'link-id' and 'unit', the PUT request with higher numeric 'tsid' value will override the request with a lower numeric 'tsid' value. The overlapped lower numeric 'tsid' MUST be automatically deleted and no longer be available. "overlapped lower numeric 'tsid'" is in reference to the tsids of overlapping requests. Will add a note. > > #12) Section 6.3.1 states "If no target clause is included in the > request, this is an indication that the baseline information applies > for the DOTS client domain as a whole." Define "target clause" (= > tsid?) [Med] s/clause/attribute. This is in reference to target-* nodes in this tree: | +-- target-prefix* | | inet:ip-prefix | +-- target-port-range* [lower-port] | | +-- lower-port inet:port-number | | +-- upper-port? inet:port-number | +-- target-protocol* uint8 | +-- target-fqdn* | | inet:domain-name | +-- target-uri* | | inet:uri > > #13) Section 4.5 states "The use of "identities" is thus suboptimal > from a message compactness standpoint." Clearly, but they excel in > forward extensibility. There may be cases where the identities would > be better and even more compact than an alternative solution without > them. To rule out all use of identities based on a blanket statement > like this does not seem like a good design practice. I suggest to > remove this sentence. [Med] We fully agree that "identities" are better for extensibility, that is why we considered them but abandon it because of the implication on the message size. We wanted to record the reason of our design choice. We can update as follows: OLD: The DOTS telemetry module (Section 10.1) uses "enumerations" rather than "identities" to define units, samples, and intervals because otherwise the namespace identifier "ietf-dots-telemetry" must be included when a telemetry attribute is included (e.g., in a mitigation efficacy update). The use of "identities" is thus suboptimal from a message compactness standpoint. NEW: The DOTS telemetry module (Section 10.1) uses "enumerations" rather than "identities" to define units, samples, and intervals because otherwise the namespace identifier "ietf-dots-telemetry" must be included when a telemetry attribute is included (e.g., in a mitigation efficacy update). The use of "identities" is thus suboptimal from a message compactness standpoint; one of the key requirements for DOTS messages. > > #14) In the YANG, there is a choice with a single case "case server- > to-client-only". This way of using choice is a little unusual, but > it works. [Med] OK, thanks. > > #15) In the YANG, there is a list telemetry without any key. This is > allowed for config false lists in NETCONF/YANG, and it could be > allowed in this protocol. For a constrained device, however, this > situation may not be ideal. > Without a key, the only way for a client to read this information is > to read it all at once. If that is the only reasonable use case for > this information, you kan keep it as is. If this data may be large, > a different way of modeling may be in order so that data could be > requested a little at a time. [Med] We followed Section 4 of [RFC8791] which specifies that a list does not require to have a key statement defined. That was convenient for us because the keys that are not indicated in the module are those that are not carried in the ** message body ** but as Uri-Path option. The YANG module is supposed to cover the message body. > > #16) Section 6.1.1 states " 'server-originated-telemetry' under > 'max-config-values' to 'true' ('false' is used otherwise). If > 'server-originated-telemetry' is not present in a response, this is > equivalent to receiving a request with 'server-originated-telemetry' > set to 'false'. " In YANG, this is usually expressed by adding > "default false" to the leaf. [Med] Fixed. > > #17) There are many leafs in the models that have no default, no > mandatory and no description which tells what happens if the leaf is > not provided. E.g. what happens if "query-type" is not specified? [Med] Will check. For the query-type, if a the list of supported types is retrieved or exposed, the risk is to send a query with an unsupported type. That one is handled with this text: Requests with invalid query types (e.g., not supported, malformed) by the DOTS server MUST be rejected by DOTS servers with a 4.00 (Bad Request). > > #18) Many lists have "unit" as a key. This can work, but is an > unusual choice. > It opens up for sending the same information several times over with > different units. [Med] That's is prevented because agents must auto-scale: When generating telemetry data to send to a peer, the DOTS agent MUST auto-scale so that appropriate unit(s) are used. and: "The traffic can be measured using unit types: packets per second (PPS), Bits per Second (BPS), and/or bytes per second. DOTS agents auto-scale to the appropriate units (e.g., megabit-ps, kilobit-ps)."; An agent is likely to use "PPS and BPS" or "PPS and bytes per second". That is negotiated with the peer agent. > > #19) Section 7.1.1 states "At least one of the attributes 'target- > prefix', 'target-fqdn', 'target-uri', 'alias-name', or 'mid-list' > MUST be present in the target definition." This may be a good thing > to add in a must-statement, or at least in a description statement > in the module. [Med] Noted. Will update the description. > > #20) Section 7.1.1 mentions a "optional subattribute" Please define > this term, or change to a defined term. [Med] OK. > > #21) leaf-list alias-name {type string; description "An alias name > that points to a resource."; Unclear; what is a resource here? What > is the format of the string? Could this be a leafref? [Med] We are referring to any IP resource that can be subject to a DDoS attack. This can be a router, a host, an IoT object, a server, etc. There is no constraint of the format of alias as this is local to a DOTS client domain. > > #22) There are typedefs called "unit-type" and "unit". I would > suggest changing at least one of those names so that readers have a > chance at understanding how they differ. Also, some of their content > overlaps, so maybe one could incorporate the other? [Med] unit-type are used to negotiate which types can be used when sharing telemetry data. 3 types are supported. "unit" defines the actual unit that can be used as a function of auto-scaling. Will check how to make this better. > > #23) Similarly for grouping percentile-config and grouping > percentile. Names that better reflect their content and usage would > be good. [Med] Noted. > > #24) There are still a few instances of augment in the model. They > should be sx:augment-stucture. [Med] That's normal as these are not augmenting an sx: module. > > #25) I didn't go through the entire table in section 11, but I > looked at a few random leafs from the model. A couple seem to be > missing in section 11, e.g. > last-updated and rate-limit. Maybe good to go through the latest > version to really check that all relevant elements are listed. > [Med] That's normal as those are not exchanged in the signal channel, but in the data channel (relying upon RESTCONF) _________________________________________________________________________________________________________________________ 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. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
