Re: [IPsec] Yangdoctors early review of draft-ietf-ipsecme-yang-iptfs-01

[Don Fedyk <[e-mail]>]

Hi Jürgen

Thanks for the Review. Most comments are an easy Accept. 
Response on intent are inline Don:

Cheers
Don


-----Original Message-----
From: Jürgen Schönwälder via Datatracker <[e-mail]> 
Sent: Wednesday, October 6, 2021 4:21 PM
To: [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Yangdoctors early review of draft-ietf-ipsecme-yang-iptfs-01

Reviewer: Jürgen Schönwälder
Review result: On the Right Track

I have reviewed draft-ietf-ipsecme-yang-iptfs-01.txt.

- Looking at [1], following the most popular naming scheme would
  suggest the title "A YANG Data Model for IP Traffic Flow Security"
  instead of the current title.
Don:  Accept

- Newer data models usually state in the abstract whether the model
  conforms to the Network Management Datastore Architecture (NMDA).
Don: Accept We support NMDA.

- The document is generally well written and easy to read. Thanks for
  that.

- Not sure what "actively published YANG modules" are. In the YANG
  world, we usually consider YANG modules under development not really
  as published. (See how 'published' is used in RFC 7950.) What you
  are referring to here are modules actively being worked on I think.
Don: Accept

- You import RFC 2119 terms but then I am not sure they are actually
  used anywhere. If you do not need RFC 2119 terms, there is also no
  need to import them.
Don: Will check.

- It seems that [I-D.ietf-i2nsf-sdn-ipsec-flow-protection] has been
  published as RFC 9061, so please update your document.
Don: I think I missed updating a text reference the RFC reference was updated automatically. 


- "IP-TFS YANG augments IPsec YANG model from" - why not use the
  actual YANG module names to avoid any confusion?
Don: Accept. 

- I did not understand what the purpose of this is:

   The data model uses following constructs for configuration and
   management:

   o Configuration

   o Operational State

  The text following this is useful, but the value of the quoted text
  was unclear to me.
Don: We can remove. 

- I did not understand what this text means:

   IP-TFS YANG augments:

   *  Yang catalog entry for [e-mail]

   *  Yang catalog entry for [e-mail]

  What is a Yang catalog entry for xxx and how can you augment it?
Don: I think we should just say Model not catalog here. 

- The YANG module names you are augmenting include the WG name, which
  is a rather weird construction and generally not recommended. I
  wonder how this went through. You seem to follow this and you put
  ipsecme into the name. Ideally, module names would be tied to a
  technology but not to RFCs or WGs (all of which come and go).
Don: Will revisit naming. 

- Copyright needs some updating... and in less than 3 months again.
Don: Accept

- You use type uint64 for counters instead of the yang:counter64
  type. Is there a special reason? How would an overflow be handled?
  Can there be any counter discontinuities during their lifetime?

- I found this:

         // config true; want this so we can refine?
  Can you elaborate what the question is here?
Don: I will check and update I think this a simply saying that we found config true had to be set
So that a refine could be done.

- The congestion control leaf is an enable/disable knob. Is there
  exactly one and only one way to do congestion control or is there /
  will there be a need to be more detailed, i.e., does this have to be
  extensible instead of just a simple boolean?
Don: We don't have a congestion mechanism for IP-TFS but instead a knob that it 
support underlying mechanisms. 

- What happens if I set use-path-mtu-discovery to true and
  outer-packet-size to 1234? Which takes precedence? Is it desirable
  to allow both to be configured?
Don: The description says that use-path-mtu-discovery takes precedence
Over values if the outer packet size is greater that the MTU discovery value. 
I think this works for all cases. 
- Consider adding a units clause to l2-fixed-rate and l3-fixed-rate.
Don: Accept.

- I did not extract the YANG to do syntax checking etc, I assume that
  the authors have a proper build automation in place (or any errors
  will have to be dealt with when the ID has passed WG last call).

- I did not verify the xml and json examples in the appendix. However,
  I suggest to use documentation addresses for prefixes in the
  examples instead of 1.1.1.1/32 and 2.2.2.2/32 (and some may even
  want to see some IPv6 prefixes there as well).
Don. Accept.

- Editorial

  fll out -> fill out

  Congestion Control With the -> With the
Don - Will update.

[1] https://en.wikipedia.org/wiki/YANG#Standards-track_data_models