Subject: Re: [ntpwg] My review of: draft-ietf-ntp-ntpv4-mib-05.txt

From: "Odonoghue, Karen F CIV NSWCDD, W23" <[email-masked]>
Date: Thu, 19 Feb 2009 11:06:14 -0600

Heiko,

I'm driving and need to answer via blackberry. The short answer is we need to make the changesd. Bert is part of the official review process. I finally go to Chris. Email him at [email-masked]. I will give you a more coherent answer when I arrive at my destination.

Karen

----- Original Message -----
From: ntpwg-bounces+karen.odonoghue=[email-masked] <ntpwg-bounces+karen.odonoghue=[email-masked]>
To: Bert Wijnen (IETF) <[email-masked]>
Cc: [email-masked] <[email-masked]>
Sent: Thu Feb 19 02:19:57 2009
Subject: Re: [ntpwg] My review of: draft-ietf-ntp-ntpv4-mib-05.txt

Bert,

thank you very much for your detailed and useful review of our MIB 
module. I will try to implement as many points as possible but I am 
unsure what that does to the standardization process (it is my first RFC 
....). It took us all a long time to get into last call and I am a 
little bit scared that the modifications will restart the whole 
procedure again. I asked my wg chair for advise what to do.

See my additional comments below (inline):

Bert Wijnen (IETF) schrieb:
> Note: I am not subscribed to the ntpwg mailing list and do not plan to
> subscribe either. So if you want me to see any responses, please
> copy me explicitly
>
> I did a review for:
>    http://www.ietf.org/internet-drafts/draft-ietf-ntp-ntpv4-mib-05.txt
>
> Here are my findings:
>
> SMICng results:
>
>  C:\bw\smicng\work>smicng ntpv4.inc
>  E: f(ntpv4.mi2), (272,14) Default value for "ntpEntStatusCurrentModeVal"
>      must be a name and not a number
>
>   *** 1 error and 0 warnings in parsing
>
> This means changing
>    DEFVAL { 99 }
> into
>    DEFVAL { unknown }
> will fix it.
Strange, I used smilint at level 6 and did not get any errors. This is 
an easy fix, though.
> But I also find:
>
> - writable objects for which I see no text in the DESCRIPTION clauses
>  about expected persistency behaviour
That means you want to know whether an object stores the changes on that 
object permanently or not? I am not sure if this is not 
implementation-specific.
> - Counter32 objects with no test in the DESCRIPTION clauses about
>  possible discontinuities and which timer object would indicate such.
>  Specifically, the fact that (I think) the ntpd keeps track of the 
> Counters,
>  it seems that there may be discontinuities at other times than when
>  the snmpd restarts (e.g. at other times than reset of sysUpTime)
Not sure what you mean, can you please provide me with an example?
> - I see a whole list of REVISION clauses. Normally we only have a
>  single revision clause at RFC publication time. Not a list of revisions
>  that took place during I-D revisions.
OK.
> - I am missing the Copyright statement in the DESCRIPTION clause
>  of the MODULE_IDENTITY
I guess this has been overseen but can be added quite easily. Where can 
I find the required copyright statement in its correct form?
> - I am missing that this revision of the document is published as RFC 
> xxxx
>  (xxxx to be filled in by RFC-editor)
OK.
> - I see a lot of objects with a SYNTAX of DisplayString.
>  I would like to point out "note 2" on web page
>       http://www.ops.ietf.org/mib-common-tcs.html
>   Note 2.  DisplayString does not support internationalized text.
>            It MUST NOT be used for objects that are required to
>            hold internationalized text (which is always the case
>            if the object is intended for use by humans [RFC2277]).
>            Designers SHOULD consider using SnmpAdminString,
>            Utf8String, or LongUtf8String for such objects.
The DisplayString objects are mainly used for values which require to be 
floating point and we agreed in the working group discussions that it 
would make sense to use a string instead that can be converted into an 
fp value by a machine.
> - Instead of
>     ntpEntNotifPrefix  OBJECT IDENTIFIER ::= { ntpSnmpMIBObjects 6 }
>     ...
>     ntpEntNotifications OBJECT IDENTIFIER ::= { ntpEntNotifPrefix 0 }
>
>  I propose/suggest to do
>     ntpEntNotifications OBJECT IDENTIFIER ::= { ntpSnmpMIB 0 }
>
>  This makes it more inline with what we often do in MIB module.
>  Also makes the OIDs of notifications shorter
OK, agreed.
> - I strongly recommend to use the MIB structure as per RFC4181,
>  which suggests that you would replace
>       ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 6 }
>  with
>       ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 2 }
>
OK.
> - I see OBJECT-GROUP definitions like
>   ntpEntObjectsGroup1 OBJECT-GROUP
>      OBJECTS {
>              ntpEntSoftwareName,
>              ntpEntSoftwareVersion,
>              ntpEntSoftwareVersionVal,
>              ntpEntSoftwareVendor,
>              ntpEntSystemType,
>              ntpEntStatusEntityUptime,
>              ntpEntStatusDateTime,
>              ntpAssocName,
>              ntpAssocRefId,
>
>              ntpAssocAddressType,
>              ntpAssocAddress
>     }
>     STATUS      current
>     DESCRIPTION
>         "A collection of objects for the NTP MIB that all NTP
>          or SNTP entities should implement."
>     ::= { ntpEntGroups 1 }
>
>  Text about "should implement" or "optional" or such is NOT supposed 
> to be
>  in an OBJECT-GROUP DESCRIPTION clause. The OBJECT-GROUP is
>  only meant to group objects logically together.
>  If such a group is mandatory or optional is something that is expressed
>  via MANDATORY-GROUPS or via the GROUP clauses in the
>  MODULE-COMPLIANCE.
OK, I will remove the text from the description and change the group 
clauses accordingly.
> As another example, you have
>
>     ntpEntObjectsGroup2 OBJECT-GROUP
>     ...
>     DESCRIPTION
>        "A collection of objects for the NTP MIB that are optional
>         for NTP or SNTP entities to implement."
>     ::= { ntpEntGroups 2 }
>
>  So here you say that the group is optional while you have listed this 
> group
>  as a MANDATORY-GROUP here:
>
>    ntpEntNTPCompliance MODULE-COMPLIANCE
>       STATUS      current
>       DESCRIPTION
>          "The compliance statement for SNMP entities which use NTP and
>           implement the NTP MIB"
>      MODULE  -- this module
>          MANDATORY-GROUPS {
>                           ntpEntObjectsGroup1,
>                           ntpEntObjectsGroup2,
>                           ntpEntNotifPrefixGroup
>         }
>         ::= { ntpEntCompliances 1 }
>
>  So that is a conflict in your MIB-Module.
OK.
> - I see a lot of MIB-level comment lines, aka:
>
>  ntpEntSoftwareVersion OBJECT-TYPE
>    SYNTAX      DisplayString
>    MAX-ACCESS  read-only
>    STATUS      current
>    DESCRIPTION
>        "The software version of the installed NTP implementation."
>    -- full version string, e.g. "[email-masked] ..."
>    ::= { ntpEntInfo  2 }
>
>   I think it is much better to include that informatrion in the 
> DESCRIPTION
>   clauses. Otherwise it will often be lost when the MIB module is 
> extracted
>   or used as input for generating help text etc.
Good point, noted.
> - I see references/citations aka:
>
> NtpDateTime ::= TEXTUAL-CONVENTION
>    DISPLAY-HINT "4d:4d:4d.4d"
>    STATUS      current
>    DESCRIPTION
>        "NTP date/time on the device, in 128-bit
>         NTP date format. Ref: draft-ietf-ntp-ntpv4-proto-06,
>         section 6:
>          It includes a 64-bit signed seconds field
>          spanning 584 billion years and a 64-bit fraction
>          field resolving .05 attosecond (i.e. 0.5e-18).
>          For convenience in mapping between formats, the
>          seconds field is divided into a 32-bit era field
>          and a 32-bit timestamp field.
>
>         If time is not syncronized this field shall be a
>         zero-length string.
>
>         This TC is not to be used for objects that are used
>         to set the time of the node querying this object.
>
>         NTP should be used for this--or at least SNTP."
>    SYNTAX      OCTET STRING (SIZE (0 | 16))
>
>  I suggest that the pointer to Ref: draft-ietf-ntp-ntpv4-proto-06
>  is changed to Ref: RFC yyyy (and ask RFC-editor to fill in
>  that yyyy value. I see that Ref: draft-ietf-ntp-ntpv4-proto-11 is
>  also in Last Call, so that would be fine.
>  In genreal though, it is probably better to use the REFERENCE
>  clause. So it would be something like
>
>    REFERENCE   "Network Time Protocol Version 4 Protocol
>                                 And Algorithms Specification, sect 6, 
> RFC yyyy"
>
OK, makes perfect sense.
> - It seems to me that for something like this:
>   ntpEntStatusEntityUptime OBJECT-TYPE
>      SYNTAX      Unsigned32
>      MAX-ACCESS  read-only
>      STATUS      current
>      DESCRIPTION
>          "The uptime of the NTP entity in seconds."
>      -- time since ntpd was (re-)started (not sysUptime!)
>      DEFVAL { 0 }
>      ::= { ntpEntStatus 9 }
>
>  I would add a  UNITS clause.
>  I would also move the comment line inside the DESCRIPTION clause.
>  I am not sure if a DEFVAL of 0 (zero) makes sense.
>  Specifically... what would the value be if the snmp agent has started
>  later that ntpd? What if the snmp agent restarts and the ntpd does not?
>  In fact, I think that a DEFVAL clause makes no sense for this object.
Agreed, I will remove the DEFAL line. Can you provide me with an example 
on how to use the UNITS clause?

>  I in fact wonder if most of the DEFVAL values for the read-only objects
>  make sense.
>
> - For
>  ntpEntStatPktModeTable OBJECT-TYPE
>    SYNTAX           SEQUENCE OF NtpEntStatPktModeEntry
>    MAX-ACCESS       not-accessible
>    STATUS           current
>    DESCRIPTION
>        "The number of packets sent and received by packet mode."
>    ::= { ntpEntStatus 18 }
>
>  ntpEntStatPktModeEntry OBJECT-TYPE
>    SYNTAX      NtpEntStatPktModeEntry
>    MAX-ACCESS  not-accessible
>
>    STATUS      current
>    DESCRIPTION
>        "The number of packets sent and received by packet mode."
>    INDEX       { ntpEntStatPktMode }
>    ::= { ntpEntStatPktModeTable 1 }
>
>  I am surprised by the SAME DESCRIPTION clause for each of these!!??
I will change that.
> - When I see:
> ntpAssocAddressType OBJECT-TYPE
>    SYNTAX      InetAddressType
>    MAX-ACCESS  read-only
>    STATUS      current
>    DESCRIPTION
>        "The type of address of the association."
>    -- contains the type of address for uni/multi/broadcast associations
>    ::= { ntpAssociationEntry 4 }
>
> ntpAssocAddress OBJECT-TYPE
>    SYNTAX      InetAddress
>    MAX-ACCESS  read-only
>    STATUS      current
>    DESCRIPTION
>        "The IP address (IPv4 or IPv6) of the association."
>    -- contains IP address of uni/multi/broadcast associations
>    ::= { ntpAssociationEntry 5 }
>
>  Then I like to point out that RFC4001 states that the use of InetAddress
>  SYNTAX, MUST have a DESCRIPTION clause that explain./states
>  which object of SYNTAX InetAddressType controls the format of
>  the InetAddress object. It is most probably ntpAssocAddressType
>  but it would be good to state so.
>
>  Besides, if you are sure that this ALWAYS only applies to IPv4 and IPv6,
>  then I would do it as follows:
>
>    ntpAssocAddressType OBJECT-TYPE
>       SYNTAX      InetAddressType { ipv4, ipv6 }
>
>   ntpAssocAddress OBJECT-TYPE
>       SYNTAX      InetAddress SIZE(4|16)
>
Thanks, I will do it this way because we currently only have NTP over 
IPv4 and IPv6 - to my knowledge.
> You must check if you also want to use Zoned addressed, because then
>  that must be added too.
Any thoughts on that by anybody?
> If, at the other hand, other values than ipv4/ipv6 are possible (maybe
> in the future?) then it might be better to put the "minimum support for
> ipv4/ipv6 requirement in the MODULE-COMPLIANCE via an OBJECT
> and SYNTAX clause.
Again, since this is my first MIB I would appreciate an example.
>
> - I see some 8 notifications. Is there a risk that too many get generated
>  at any point in time, such that it would flood the network and/or
>  overload a NMS ??
>  It would be good to add some text about the expected max notification
>  rates and/or add a throthle or an enable/disable switch.
OK, I will look into this.
> - It seems that RFC4001 is missing as a normative reference.
OK.
> - I would think that the new draft-ietf-ntp-ntpv4-proto-11 (version 4 
> of NTP)
>  should also be a normative reference
Shouldn't we use the RFC yyyy approach instead (yyyy being the 
to-be-assigned RFC for the protocol RFC)?
> - The security section is not according to the guidelines provided at
>       http://www.ops.ietf.org/mib-security.html
I unfortunately currently seem to be unable to access this website (and 
www.ietf.org as well), but I think it would be helpful to know which 
points are missing / wrong in your oppinion.
>
> - I have not checked the new NTP version 4 spec to see if the 
> knobs/sensors
>  in that protocol have properly been modeled in this MIB module.
>  That is a separate task that may actually need much more time than the
>  review I have done for now.
Since this has been discussed in the working group I would expect that 
everybody is happy with what is in the MIB right now. But I really 
appreciate your efforts and your very detailed review of this document. 
I only wish we would have somebody sending me such a review earlier in 
the process, before everything went into last call. Anyway, you did a 
great job and I think this was a very useful review. I will go back to 
my drawing desk and start to implement the advised changes.

Best Regards,
  Heiko

> Bert Wijnen

_______________________________________________
ntpwg mailing list
[email-masked]
https://lists.ntp.org/mailman/listinfo/ntpwg
_______________________________________________
ntpwg mailing list
[email-masked]
https://lists.ntp.org/mailman/listinfo/ntpwg