[IPsec] AD Review of draft-ietf-ipsecme-mib-iptfs-03

[Roman Danyliw <[e-mail]>]

Hi!

I performed an AD review of draft-ietf-ipsecme-mib-iptfs-03.  Thanks for this companion document to the YANG module for IP-TFS management.  Below is my feedback:

To the idea that this MIB is redrived from the YANG modules:

** Consider if you want to use the same names for field values.  I'm not sure if this divergence was an explicit design choice, or an accident.

-- usePathMTU (MIB) vs. use-path-mtu-discovery (YANG) (i.e., make it "usePathMTUDiscovery" here)

-- lostPktTimerInt (MIB) vs. lost-packet-timer-interval (YANG) (i.e., make it "lostPacketTimerInterval" here)

-- The various statistics fields in the MIB expand "Packet" but in the YANG they are abbreviated to "pkt" (e.g., txPackets in MIB vs. tx-pkts in YANG)

** Consider if the types are right:

-- in outerPacketSize (MIB)/outer-packet-size(YANG), the types are UnsignedShort/ uint16 respectively.  Practically, UnsignedShort is "Unsigned32 (0 .. 65535)".  However, windowSize(MIB)/window-size (YANG) are Unsigned32/ uint16 which don't match up.  Should windowSize then a UnsignedShort too to be symmetric to the YANG definition?  

-- maxAggregationTime and lostPktTimerInt (MIB) are of type NanoSeconds, practically "Counter64".  Their equivalent in YANG are max-aggregation-time and lost-packet-timer-interval of type decimal64.  These aren't equivalent datatypes.  YANG seems to support negative and fractional values.

** Abstract.  Typo. s/the the/the/

** Section 1.

   The objects defined here are the same as
   [I-D.ietf-ipsecme-yang-iptfs] with the exception that only
   operational data is supported.

-- Could this excluded "operational data" be enumerated?

-- I found this terminology of "operational data" confusing because in Section 3 the text says "This document defines configuration and operational parameters ...".  There is a taxonomy of "operational data" and "operational parameters" being constructed.

** Section 3.  What's the difference between:

(a) "This document is based on the concepts and management model defined
   in [I-D.ietf-ipsecme-yang-iptfs]."

(b) "It reuses the management model defined in
   [I-D.ietf-ipsecme-yang-iptfs]."

AND then

(c) This document defines configuration and operational parameters of IP
   traffic flow security (IP-TFS).

(d) This document specifies an extensible operational model for IP-TFS.

Consider if the both the first and third paragraph if this section is needed since it seems like there is a significant repetition.

** Section 4.2.  Surround the MIB module with  '<CODE BEGINS>' and '<CODE ENDS>' lines

** Section 4.2.  Typo. s/refrence/referenced/

** Section 6.  Thanks for calling out the sensitivity of iptfsOuterStatsTable.  Wouldn't the same caution apply to iptfsInnerStatsTable too?

** Section 6.
   Further, deployment of SNMP versions prior to SNMPv3 is NOT
   RECOMMENDED.  Instead, it is RECOMMENDED to deploy SNMPv3 and to
   enable cryptographic security.  

Given the IPTFS is new functionality and isn't likely to be added to legacy codebases or devices constrained to SNMPv1 is possible, could this read that SNMPv3 is required?

Thanks,
Roman