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

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