Last Call Review of draft-ietf-sfc-proof-of-transit-08
review-ietf-sfc-proof-of-transit-08-yangdoctors-lc-bjorklund-2021-09-16-00
Request | Review of | draft-ietf-sfc-proof-of-transit |
---|---|---|
Requested revision | No specific revision (document currently at 08) | |
Type | Last Call Review | |
Team | YANG Doctors (yangdoctors) | |
Deadline | 2021-09-28 | |
Requested | 2021-09-15 | |
Requested by | Joel M. Halpern | |
Authors | Frank Brockners , Shwetha Bhandari , Tal Mizrahi , Sashank Dara , Stephen Youell | |
I-D last updated | 2021-09-16 | |
Completed reviews |
Yangdoctors Last Call review of -08
by Martin Björklund
Secdir Last Call review of -08 by Christian Huitema Genart Last Call review of -08 by Stewart Bryant Opsdir Last Call review of -08 by Ron Bonica |
|
Comments |
Sorry to have been late requesting this. |
|
Assignment | Reviewer | Martin Björklund |
State | Completed | |
Request | Last Call review on draft-ietf-sfc-proof-of-transit by YANG Doctors Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/yang-doctors/2hBTdSXXa_aA_crjY5CaIMvwXpo | |
Reviewed revision | 08 | |
Result | Ready w/nits | |
Completed | 2021-09-16 |
review-ietf-sfc-proof-of-transit-08-yangdoctors-lc-bjorklund-2021-09-16-00
Hi, This is my YANG doctor review of draft-ietf-sfc-proof-of-transit-08. The review is focused on the YANG module in the draft. In general, the draft and YANG module are well-written. o pyang --ietf complains that the module description doesn't contain the (correct) IETF Trust Copyright statement. The problem is probably due to an copy&paste error; some text is duplicated. I suggest you also keep the normal line breaks to make the text easier to read: description "This module contains a collection of YANG definitions for proof of transit configuration parameters. The model is meant for proof of transit and is targeted for communicating the POT-Profile between a controller and nodes participating in proof of transit. Copyright (c) 2020 IETF Trust and the persons identified as authors of the code. All rights reserved. Redistribution and use in source and binary forms, with or without modification, is permitted pursuant to, and subject to the license terms contained in, the Simplified BSD License set forth in Section 4.c of the IETF Trust's Legal Provisions Relating to IETF Documents (https://trustee.ietf.org/license-info). This version of this YANG module is part of RFC XXXX (https://www.rfc-editor.org/info/rfcXXXX); see the RFC itself for full legal notices. The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED', 'MAY', and 'OPTIONAL' in this document are to be interpreted as described in BCP 14 (RFC 2119) (RFC 8174) when, and only when, they appear in all capitals, as shown here."; o It is not clear to me how the profile sets are supposed to be used. Perhaps the intention is one set per packet flow? If so, how is the set tied to a flow? o The list "pot-profile-list" is defined as ordered-by user. Why? How is the order relevant? o In the profile list there is a leaf "status" of type "boolean". It is not immediately clear what "status = false" means (but the meaning of the values are explained in the text). I suggest that the leaf is renamed to "active", or "enabled". But see below! o The current model allows a list where both entries have "status" true. Perhaps a simpler solution could be to remove the "status" leaf, and instead have a leaf in the profile set that refers to the active entry: leaf active-profile { type leafref { path '../pot-profile-list/pot-profile-index'; } } o It is not clear why you have the two groupings in the model. Do you expect other models to reuse these groupings? If so, you should probably add some descriptions that explain how they are to be reused. If not, perhaps remove the groupings. The model is quite simple anyway. o The choice of prefixes for some of the names seems a bit random. Some are called 'pot-profile-xxx' and some just 'xxx'. In YANG we tend to avoid repeating prefixes unless it is necessary. So perhaps, instead of: module: ietf-pot-profile +--rw pot-profiles +--rw pot-profile-set* [pot-profile-name] +--rw pot-profile-name string +--rw pot-profile-list* [pot-profile-index] +--rw pot-profile-index profile-index-range you could have: module: ietf-pot-profile +--rw pot-profiles +--rw profile-set* [name] +--rw name string +--rw profile-list* [index] +--rw index profile-index-range Also I would probably name the list 'profile-list' just 'profile', to align with other models. o Minor: I suggest you run the module through: pyang -f yang --yang-line-length 68 --yang-canonical MODULE in order to fix some inconsistensies in the formatting of the text. This will make the RFC editor's job easier. Also, I suggest you remove the comments you have; I don't think they add anything. o It would be helpful with an example of the XML or JSON data for the example in section 3.3. Perhaps add that as an appendix? /martin