Subject: Re: [Teas] I-D Action: draft-ietf-teas-yang-te-26.txt

From: t petch <[email-masked]>
Date: Tue, 6 Apr 2021 10:34:23 +0100

On 24/02/2021 15:47, Tarek Saad wrote:
> Hi Tom and WG,
>
> The latest version contains the following changes:
> - editorial nits
> - new identity for metric "path-metric-loss"
> - rearranging headend LSP attributes
> - addressing issue in reference groupings from external modules (using absolute leafref path, and prefix'ing module prefix in XPath)
> - added 'controller' container to indicate the controller-id and 'protocol-origin' for externally created tunnels
> - added JSON example of the instance tree when using the YANG model to create and query state for a TE tunnel
> - new co-author joined

Tarek

Thanks for that; I think that -26 could do with a  few editorial tweaks

s.2.2
    o  Suitable defaults are specified for all configurable elements.
I think that this overstates the case; many elements but by no means all

s.5
    The detailed tree structure is provided in Figure 2.
Figure 7 I think

Since you have both RPC and action, I suggest a section in 5.1
summarising what these can do for you, when to use RPC, when to use
'action'

s.5.2
line length is ok but splitting the identifiers mid-word perhaps not;
doubtless there is a pyang option to fix this; -25 did not have this
problem

s.5.3
lots of references - good- but they must appear in I-D References.  I do
not see
RFC5440 -
RFC5441 -
RFC5520 -
RFC5557 -
RFC8306 -
RFC8685 -

   revision 2021-02-20 {
...
     reference
       "RFCXXXX: A YANG Data Model for Traffic Engineering Tunnels
        and Interfaces.";
This is not (quite) the title of the I-D

I see many unexpanded abbreviations; BRPC, PKS, APS, LoP, SF, MS, WTR,
MS, SD

     leaf lsp-protection-role {
       ...
         enum protecting {
           description
             "A secondary LSP is an LSP that has been provisioned
              in the control plane only; e.g. resource allocation
should that be i.e. ??

         leaf protection-group-ingress-node-id {
...
              By default, if neither ingress nor egress node-id
              is set, the action applies to ingress node only.";
this could be in the YANG but is not!

         leaf protection-group-egress-node-id {
ditto

   /* TE Tunnel RPCs/execution Data */
Security Considerations mention the use of NACM but the YANG modules do
not use it.  Common practice is to put a
nacm:default-deny-all
at least on any RPC/action that might do harm

s.6.3

    prefix te-dev;
This is not what IANA Considerations says!

   revision 2021-02-20 {
...
     reference
       "RFCXXXX: A YANG Data Model for Traffic Engineering Tunnels
        and Interfaces.";
Again, this is not (quite) the title of the I-D

            leaf value {
              type uint32 {
                range "0..4294967295";
If this is the maximum value then it can be expressed as "0..max"

      leaf threshold-type {
might benefit from a reference

rpc
again , could use NACM

s.10
"unnels-actions":
perhaps   "tunnels-actions":

13.  Appendix A: Examples
these do not match the YANG modules e.g.
          "p2p-primary-paths": [
disappeared several versions ago

Tom Petch

> Tarek
>
>
> Regards,
> Tarek (for the co-authors)
>
> ?On 2/23/21, 7:27 AM, "Teas on behalf of tom petch" <[email-masked] on behalf of [email-masked]> wrote:
>
>
>
>      ________________________________________
>      From: Teas <[email-masked]> on behalf of [email-masked] <[email-masked]>
>      Sent: 22 February 2021 18:31
>