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

[Don Fedyk <[e-mail]>]

Hi Roman

Sorry for the delay I wanted to address your points and align with the latest Iptfs-yang module, but I got delayed.  Here is a pass at addressing the issues you raised. I have posted an update with most of your suggestions. 

Thanks Don & Eric

Comments Inline below [don].



-----Original Message-----
From: IPsec <[e-mail]> On Behalf Of Roman Danyliw
Sent: Thursday, July 21, 2022 2:27 PM
To: [e-mail] WG <[e-mail]>
Subject: [IPsec] AD Review of draft-ietf-ipsecme-mib-iptfs-03

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)
[don] Done

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

-- 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)
[don] The YANG use of pkt is common in YANG and the word pkt is common in MIBs so I have aligned the MIB

** 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?  
[Don] Changed windowSize(MIB)  to UnssignedShort 

-- 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.
[Don] My understanding is the textual convention should handle this. I it now like this.
NanoSeconds ::= TEXTUAL-CONVENTION
    DISPLAY-HINT "d-6"
    STATUS  current         
    DESCRIPTION                     
      "Represents time unit value in nanoseconds."
    SYNTAX      Integer32


** Abstract.  Typo. s/the the/the/
[don] done. 

** 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?
[don] Referenced the tree diagram in section 4.5 

-- 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.
[don] Clarified. 

** 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.

[don] Adjusted. 

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
[don] are we doing this anymore? The practice seems to have disappeared with YANG.  I don't see this in any MIBs I can add but I don't see an example.  

** Section 4.2.  Typo. s/refrence/referenced/
[don] done.
** Section 6.  Thanks for calling out the sensitivity of iptfsOuterStatsTable.  Wouldn't the same caution apply to iptfsInnerStatsTable too?
[don] Aligned with YANG doc - bot outer and inner are called out. 

** 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?
[don] We used the suggested text that was supplied from WG AD review.  I think this is kind of boiler plate.

Thanks,
Roman

_______________________________________________
IPsec mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/ipsec