Subject: [Isis-wg] RE: Mib Doctor review of: draft-ietf-isis-wg-mib-19.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 15 Apr 2005 14:36:46 +0200

Thanks again Mike for your re-review.

So Jeff, I assume we can expect a quick respind to address 
the remaining (quite containable) remarks/concerns?

Bert

> -----Original Message-----
> From: C. M. Heard [mailto:[email-masked]]
> Sent: Friday, April 15, 2005 11:09
> To: ISIS WG; Jeff Parker
> Cc: Wijnen, Bert (Bert); Juergen Schoenwaelder
> Subject: Mib Doctor review of: draft-ietf-isis-wg-mib-19.txt
> 
> 
> On Sun, 6 Mar 2005, C. M. Heard wrote:
> > I have done a somewhat cursory review of the most recent version
> > which was <draft-ietf-isis-wg-mib-18.txt>.  In general, it appears
> > to be technically sound to the extent that I am able to judge (which
> > is limited because I know little about IS-IS).  However, there are a
> > number of small things that probably should be corrected before the
> > document goes to the IESG. The details follow below (organized
> > according to the checklist in Appendix A of
> > draft-ietf-ops-mib-review-guidelines-04.txt).  The things most
> > likely to elicit DISCUSS comments from the IESG are probably 6,
> > 10(c), and 10(d).  Note:  please address replies to the WG mailing
> > list with copies to Bert Wijnen, Juergen Schoenwaelder, and myself.
> 
> I have looked over the latest update <draft-ietf-isis-wg-mib-19.txt>
> which was posted on 11 April.  Owing to time constraints I have had
> to focus exclusively on how previous review comments were addresses.
> My findings are as follows.
> 
> > 1.) I-D Boilerplate -- OK
> 
> No new issues.
> 
> > 2.) Abstract -- some citations are present, which is not 
> allowed (see
> > http://www.ietf.org/ID-Checklist.html Section 3(1)C).  In addition,
> > since this document is intended for the standards track, 
> please change
> > the phrase "an experimental portion" to "a portion" in the 2nd
> > paragraph.
> 
> Fixed.
> 
> > 3.) MIB Boilerplate -- present and correct, however, it is customary
> > (and in my opinion preferable) to put it before the narrative that
> > describes the MIB module.  So I would suggest reversing the order of
> > Sections 1 and 2 in the draft.
> 
> Fixed.
> 
> > 4.) IPR Notice -- OK
> 
> No new issues.
> 
> > 5.) References -- there are several problems.
> > 
> > (a) the following item
> > 
> >    [1]  "Integrated IS-IS Management Information Base", IETF Draft,
> >         March 1994, Chris Gunner.
> > 
> > should be removed from the list of normative references since it is
> > not essential to read it in order to implement the MIB module.  I
> > suppose it could be moved to the informative references section, but
> > the best thing would probably be to remove it entirely, since it
> > is difficult (at best) to find a copy of draft-ietf-isis-mib-03.txt
> > 
> (http://www.potaroo.net/ietf/old-ids/draft-ietf-isis-mib-02.txt exists
> > but the -03 version does not show up there).  The original author's
> > efforts are explicitly acknowledged, and that is probably 
> sufficient.
> > 
> > (b) for the following
> > 
> >    [2]  ISO 10589, "Intermediate System to Intermediate 
> System Intra-
> >         Domain Routeing Exchange Protocol for use in 
> Conjunction with
> >         the Protocol for Providing the Connectionless-mode Network
> >         Service (ISO 8473)", 1992.
> > 
> > please change the citation tag from the numeric form to a symbolic
> > one (e.g., [ISO10589]) to match the style of all the other 
> references.
> > You will need to add a citation somewhere in the main body of the
> > text (NOT in the abstract) or else the RFC Editor will complain of a
> > dangling reference.  Perhaps the first line of the abstract can be
> > copied (with citations left intact) into the Overview section.
> > 
> > (c) [RFC1195] needs to be cited in the text (NOT in the abstract).
> > The remedy suggested above in (b) (copying the first line of the
> > abstract into the Overview section with citations left intact)
> > will fix this.
> > 
> > (d) the following reference
> > 
> >    [RFC1901] Case, J., McCloghrie, K., Rose, M., and S. Waldbusser,
> >         "Introduction to Community-based SNMPv2", RFC 1901, January
> >         1996.
> > 
> > is obsolete and uncited.  Please remove it.
> > 
> > (e) the following documents
> > 
> >    [RFC2571] Harrington, D., Presuhn, R., and B. Wijnen, "An
> >         Architecture for Describing SNMP Management Frameworks", RFC
> >         2571, April 1999.
> > 
> >    [RFC3291] Daniele, M.,Haberman, B., Routhier, S., and J.
> >         Schoenwaelder, "Textual Conventions for Internet Network
> >         Addresses", RFC 3291.
> > 
> > have been obsoleted by RFC 3411 and RFC 4001 respectively.  Please
> > update these references (and all citations in the text) accordingly.
> > 
> > (f) I see that normative references do exist for all MIB modules
> > from which things are imported and that appropriate citations exist.
> > I also see that the references required by the boilerplate are
> > present and accounted for.  That is good.  However, the REFERENCE
> > clauses in the MIB module point to RFC 2973 and to RFC 3373, neither
> > of which is present in the reference list.  I think that these
> > should be among the normative references and that they need to be
> > mentioned in the discussion in the overview section.  Also, there
> > are numerous REFERENCE clauses that point to GDMO objects, and it
> > would be good to have some text in the Overview saying where those
> > objects are defined, with references to the appropriate documents.
> > In http://www.potaroo.net/ietf/old-ids/draft-ietf-isis-mib-02.txt
> > I see the following text:
> > 
> >       The objects are mainly derived from the GDMO definitions in
> >       ISO 10589 and from the GDMO definitions in ISO 10733 [5].
> > 
> > with this reference:
> > 
> >       [5]"Information Technology - Telocommunications and 
> information
> >           exchange between systems - Elements of Management
> >           Information Related to OSI Network Layer Standards",
> >           International Standard 10733 (ISO submission copy),
> >           September 1992.
> > 
> > and I wonder if something along those lines would be 
> appropriate here.
> 
> A couple of minor errors remain:
> 
> On the third line of Section 2, first paragraph:
> s/ISO 10589 [2]/ISO 10589 [ISO10589]/
> 
> In Section 7, Normative References, [RFC3289] is listed twice. Both
> listings have minor typos.
> 
> > 6.) Security Considerations Section -- I see that the part 
> on writeable
> > objects states that "there are very few writable attributes 
> that do not
> > have negative side effects on network operations."  It 
> might be better
> > to say "all writeable attributes have the potential to 
> disrupt network
> > operations if improperly modified and may require protection against
> > unauthorized write access."
> > 
> > Also, are there readable objects (objects with an access of greater
> > than not-accessible) that could reveal sensitive information if read
> > by unauthorized parties?  If so, that needs to be 
> discussed.  If not,
> > that needs to be explicitly stated.  See Section 3.4 of the 
> MIB review
> > guidelines (draft-ietf-ops-mib-review-guidelines-04.txt) 
> for discussion.
> 
> These issues have been addressed.
> 
> > 7.) IANA Considerations Section -- content is correct, 
> although it is
> > customary for this section to appear after the MIB module.
> 
> Fixed.
> 
> > 8.) Copyright Notices -- OK
> 
> No new issues.
> 
> > 9.)  Other issues (e.g., from http://www.ietf.org/ID-Checklist.html)
> > that are not covered elsewhere:
> > 
> > (a) missing punctuation in Overview section:
> > OLD
> >    The IS-IS MIB defines the following objects
> > NEW
> >    The IS-IS MIB defines the following objects:
> 
> Fixed.
> 
> > 10.) Technical content -- review of actual technical content for
> > compliance with <draft-ietf-ops-mib-review-guidelines-03.txt>:
> > 
> > (a) MIB compilation using smilint --
> > 
> > The program smilint 0.4.3, as of Mon Jan 17 19:25:32 2005
> > has been used to process your request as follows:
> > 
> > smilint -m -s -e -l 6 mailbody
> > 
> > mailbody:59: [1] {object-identifier-unknown} unknown object
> > identifier label `mib-2'
> > mailbody:6: [5] {import-unused} warning: identifier `experimental'
> > imported from module `SNMPv2-SMI' is never used
> > 
> > The problem is that the draft still IMPORTS 'experimental' instead
> > of 'mib-2'.  Proposed remedy:
> > 
> > OLD:
> >         MODULE-IDENTITY, OBJECT-TYPE, Integer32, Unsigned32,
> >             Counter32, experimental, NOTIFICATION-TYPE
> > NEW:
> >         MODULE-IDENTITY, OBJECT-TYPE, NOTIFICATION-TYPE,
> >             Integer32, Unsigned32, Counter32, mib-2
> > 
> > Note: the same results issue from SMICng.
> 
> Fixed.
> 
> > (b) In the MODULE-IDENTITY DESCRIPTION clause I think that
> > jparker at world dot std dot edu needs to be changed to
> > jparker at world dot std dot com.  (I am not sure if the
> > RFC Editor will let it remain in this form, but I guess
> > it's worth a try :)
> 
> Fixed.
> 
> > (c) As noted in Section 4.6.4 of the MIB review guidelines, it is
> > REQUIRED that each read-create table specify what happens to
> > dynamically-created rows after an agent restart, and when a
> > RowStatus column is present, it is necessary to specify whether
> > modifications are allowed when the status column is 'active'.  I
> > don't thin[k] that these requirements have been met.
> 
> These issues do not appear to have been addressed.  That is likely
> to elicit unfavorable AD review comments.  I suggest that this be
> fixed before submission to the IESG.
> 
> > (d) I see that every InetAddress object has a SYNTAX value of
> > InetAddress (SIZE(4|16)) and that every InetAddressPrefixLength
> > object has a SYNTAX value of InetAddressPrefixLength (0..128),
> > but the corresponding InetAddressType objects have a SYNTAX
> > value of InetAddressType with no constraint but with a proviso
> > in the DESCRIPTION clause that "[a]n implementation is only
> > required to support IPv4 and IPv6 addresses."  If the intent is
> > to forever limit support to IPv4 and IPv6, which is the effect
> > that the present SYNTAX refinements on the InetAddress and
> > InetAddressPrefixLength objects have, then then it would
> > be better to use a SYNTAX refinement in the InetAddressType
> > objects to express that, rather than using text in the
> > DESCRIPTION clause that is more appropriate in a compliance
> > statement.  Please note, however, that it is strongly preferred
> > (at least in the MIB Doctor community) to have _no_ constraints
> > in the definitions of objects of this type, and instead to put
> > OBJECT clauses in the relevant compliance statements or, for
> > inaccessible INDEX objects, text in the compliance statements'
> > DESCRIPTION clauses.  For a usage example in a similar application
> > see draft-ietf-ipv6-rfc2096-update-07.txt, which defines the
> > inetCidrRouteTable.  Note that the lack of constraints on INDEX
> > objects of type InetAddress leads to definitions that could, in
> > principle, exceed the 128 sub-ID length on SNMP object names;
> > as recommended in the MIB review guidelines Section 4.6.6,
> > draft-ietf-ipv6-rfc2096-update-07.txt includes text in the
> > inetCidrRouteEntry DESCRIPTION clause that spells out the
> > limitations.
> 
> Fixed.  Results from smilint are now as follows:
> 
> The program smilint 0.4.3, as of Fri Apr 01 10:26:14 2005
> has been used to process your request as follows:
> 
> smilint -m -s -e -l 9 -i namelength-32 ISIS-MIB
> 
> ISIS-MIB:602: [5] {index-exceeds-too-large} warning: index of row
> `isisSummAddrEntry' can exceed OID size limit by 142 subidentifier(s)
> ISIS-MIB:728: [5] {index-exceeds-too-large} warning: index of row
> `isisRedistributeAddrEntry' can exceed OID size limit by 142
> subidentifier(s)
> ISIS-MIB:2644: [5] {index-exceeds-too-large} warning: index of row
> `isisIPRAEntry' can exceed OID size limit by 144 subidentifier(s)
> 
> The DESCRIPTION clauses of isisSummAddrTable, 
> isisRedistributeAddrTable,
> and isisIPRATable take note of these potential issues.
> 
> > Note that the DESCRIPTION clause of every object of type
> > InetAddress or InetAddressPrefixLength is REQUIRED to specify
> > which InetAddressType object that provides the context in which
> > is it interpreted, and none of the definitions in the module
> > follow that rule.  That needs to be corrected even if it is
> > decided not to remove the constraints on the objects.
> 
> Fixed.
> 
> > (e) The DESCRIPTION clauses of isisSummAddressType,
> > isisRedistributeAddrType, isisISAdjIPAddrType, isisIPRADestType,
> > and isisIPRANextHopType include the sentence "[a]n implementation
> > is only required to support IPv4 and IPv6 addresses."  Similarly,
> > the DESCRIPTION clauses of isisSummAddrExistState,
> > isisRedistributeAddrExistState, isisCircExistState,
> > isisRAExistState, and isisIPRAExistState include the sentence
> > "[s]upport for createAndWait and notInService is not required."
> > Since it is describing what an agent is required to do, rather
> > than what makes "protocol sense" for the object to support, that
> > type of language belongs in an OBJECT clause in a compliance
> > statement, not in the object definition itself.
> 
> Removing the IP version-specific syntactic constraints in the
> OBJECT-TYPE definitions was a good thing, since it allows the
> MIB module to support other address types in the future.  However,
> one needs to be aware that in the absence of OBJECT clauses or
> text in the DESCRIPTION clause saying otherwise, this causes the
> compliance statements to require that implementations support
> all InetAddressType values.  If this is not wanted, then at a
> minimum it is necessary to add language in the DESCRIPTION clauses
> of the compliance statements to the effect that an implementation
> is only required to support global IPv4 and IPv6 addresses.  For a
> more elegant alternative, see the compliance statements in RFC 3289.
> 
> I see that the DESCRIPTION clauses of the RowStatus objects mentioned
> above are unchanged.  This is acceptable since RFC 2579 explicitly
> permits an agent not to support createAndWait and notInService.
> 
> > (f) The DESCRIPTION clauses of isisManAreaAddr,
> > isisSummAddressType, isisSummAddress, isisRedistributeAddrType,
> > isisRedistributeAddrAddress, isisCircIndex, and  isisRAIndex
> > contain the sentence "[t]his object follows the index behavior"
> > or an equivalent phrase.  Similarly, the DESCRIPTION clauses of
> > isisManAreaAddrExistState, isisSummAddrExistState,
> > isisRedistributeAddrExistState, and isisCircExistState include
> > the sentence "[t]his object follows the Row Status behavior."
> > Finally, the DESCRIPTION clauses of isisCircAdminState and
> > isisRAAdminState include the sentence "[t]his object follows
> > the AdminState behavior" or an equivalent phrase.  I'm not quite
> > sure what these sentences are supposed to mean.  If the intent is
> > to say (respectively) "this is an INDEX object" or "this is a
> > RowStatus object" or "this is an AdminState object" then there is
> > no value in having them, since the INDEX clause of the enclosing
> > table or the SYNTAX clause of the object already tells me what I
> > need to know.  If the intent is something else then it needs to
> > be defined (as are the behaviors ResettingTimer,
> > ReplaceOnlyWhileDisabled, and ManualOrAutomatic).
> 
> Mostly fixed.  The DESCRIPTION clause for isisRAIndex still contains
> the sentence "This object follows the index behaviors."  That sentence
> should be removed.  The DESCRIPTION clause for isisRAAdminState still
> contains the sentence This object follows the AdminState and
> ManualOrAutomatic behaviors."  That should be changed to read
> "This object follows the ManualOrAutomatic behaviors."
> 
> > (g) I infer from the DESCRIPTION of the following object that
> > it is set by a manager and then decremented each second by the
> > agent until it becomes zero:
> > 
> >     isisSysLevelSetOverloadUntil OBJECT-TYPE
> >         SYNTAX Unsigned32(1..4294967295)
> >         UNITS "Seconds until clearing manually set Overload Bit"
> >         MAX-ACCESS read-write
> >         STATUS current
> >         DESCRIPTION
> >             "If this object is non-zero, the overload bit is set at
> >              this level when the isisSysAdminState variable goes to
> >              state 'on' for this Intermediate System.  The 
> overload bit
> >              remains set for isisSysLevelSetOverloadUntil seconds.
> >              When isisSysLevelSetOverloadUntil seconds have elapsed,
> >              the overload flag remains set if the implementation has
> >              run out of memory, or if it is set manually using the
> >              isisSysLevelSetOverload object."
> >     ::= { isisSysLevelEntry 6 }
> > 
> > If that is the case, the SYNTAX value needs to be changed to
> > include zero (e.g., Unsigned32 with no range constraint).
> 
> Fixed.
> 
> > (h) The DESCRIPTION clause of isisNextCircIndex does not seem
> > to be entirely consistent with the DESCRIPTION clause of the
> > IndexIntegerNextFree TC in RFC 3289.  Here is a suggestion:
> > 
> > OLD:
> >     isisNextCircIndex OBJECT-TYPE
> >         SYNTAX IndexIntegerNextFree
> >         MAX-ACCESS read-only
> >         STATUS current
> >         DESCRIPTION
> >             "This object is used to assign values to
> >              isisCircIndex as described in 'Textual
> >              Conventions for SNMPv2'.  The network manager
> >              reads this object, and then writes the value
> >              back as the isisCircIndex in a SET that creates
> >              a new instance of isisCircEntry.  If the SET
> >              fails with the code 'inconsistentValue', then
> >              the process must be repeated; If the SET succeeds,
> >              then the object is incremented, and the new
> >              isisCircuit is created according to the manager's
> >              directions."
> > NEW:
> >     isisNextCircIndex OBJECT-TYPE
> >         SYNTAX IndexIntegerNextFree
> >         MAX-ACCESS read-only
> >         STATUS current
> >         DESCRIPTION
> >             "This object is used to assist a management
> >              application in creating new rows in the
> >              isisCircTable.  If it is possible to create
> >              a new instance of isisCircEntry, then this
> >              object will contain a non-zero value that
> >              is not in use as the index of any row in the
> >              isisCircTable.  The network manager reads the
> >              value of this object and then (if the value
> >              value read is non-zero) attempts to create
> >              the corresponding instance of isisCircEntry.
> >              If the set request fails ith the code
> >              'inconsistentValue', the the process must be
> >              repeated;  if the set request succeeds, then
> >              the agent will change value of this object
> >              according to an implementation-specific
> >              algorithm."
> 
> Fixed.
> 
> > (i) From the fact that isisCircIfIndex has a SYNTAX value of
> > InterfaceIndex (not InterfaceIndexOrZero) I infer that there
> > there is always an ifTable entry associated with a circuit.
> > That being the case, neither isisCircIndex nor isisCircIfIndex
> > is strictly necessary -- one could use ifIndex as itself as
> > the index, thereby eliminating the need for both of these
> > objects.  However, the existing design is acceptable.
> 
> No change required.
> 
> > (j) It seems to me that for at least some interface types
> > the object isisCircIfSubSelector will be irrelevant.  If
> > that is true, then it would be good to specify that the
> > object gets some distinguished value (e.g., 0 or -1) if
> > it is not used.
> 
> Fixed.
> 
> > (k) The commentary just prior to the definition of isisRATable
> > contains the following line:
> > 
> > -- The Reachable Address Group is optional.
> > 
> > I would recommend that this statement be removed.  As a general
> > rule, any statements on what is optional to implement and what
> > is required should be in the compliance statements alone and
> > should not be included in object descriptions or in the ASN.1
> > comments that surround them.
> 
> Fixed.
> 
> > (l) The DESCRIPTION clauses of isisRAIndex, isisIPRADestType,
> > isisIPRADest, and isisIPRADestPrefixLen say that they follow he
> > ManualOrAutomatic behavior.  However, that behavior says the object
> > is read-write or read-only, and so cannot apply to inaccessible
> > index objects such as these (it can apply to the corresponding
> > RowStatus object, which allows the row to be created or destroyed if
> > appropriate).
> 
> Fixed.
> 
> > (m) The DESCRIPTION clause for the INDEX object isisLSPID reads:
> >             "The 8 byte LSP ID, consisting of the SystemID,
> >              Circuit ID, and Fragment Number."
> > Maybe this is a stupid question, but shouldn't the
> > isisLSPSummaryTable be concerning itself with entire reassembled
> > link state PDUs and not individual fragments thereof?  If so,
> > perhaps the DESCRIPTION clause should be revised.  (If this were
> > IPv4, and I were describing a complete, reassembled IP PDU, I
> > would specify that Fragment Offset=0 and MF=0, and I would also
> > stipulate that the checksum is what would have been computed for
> > the complete datagram prior to fragmentation.  But perhaps those
> > considerations don't apply here.)
> 
> Review comment was based on a misconception.  No change required.
> 
> > (n) In the isisLSPTLVTable, the checksum and sequence number are
> > provided for each TLV, but are properties of the LSP than contains
> > them, or so I infer.  If that is so, it might be good to change
> > the DESCRIPTION clause of isisLSPTLVSeq as from:
> >             "The 16 bit Fletcher Checksum."
> > to:
> >             "The 16 bit Fletcher Checksum for this LSP."
> > Note that isisLSPTLVSeq already has similar language.
> 
> Fixed.
> 
> > (o) It would be less confusing to readers like me if the ASN.1
> > comment before the start of the notification definitions
> > were changed from:
> > -- Type definitions
> > to:
> > -- Notification definitions
> 
> Fixed.
> 
> > (p) I see the following language in several of the notification
> > definitions:
> >              This should be an edge-triggered notification.
> >              We should not send a second notification about
> >              PDUs received from the same circuit.
> > This seems to imply that no more than one such notification per
> > circuit per agent reboot will be generated.  What if the
> > condition is persistent?  Shouldn't there be additional
> > notifications?  One possible alternative that might be worth
> > considering is allowing no more than one notification per circuit
> > per interval of duration T (where T may be a design parameter that
> > is fixed or the value of some read/write object).  This is
> > suggested in Sec. 4.7 of the MIB review guidelines.
> 
> Fixed.
> 
> > (q) In a related vein:  are all of the notifications that lack
> > explicit throttling by their nature generated at a sufficiently
> > low rate that they will not cause an excess of notifications?  A
> > cursory reading suggests that this is so, but the author should
> > verify that this is the case.
> 
> No change required.
> 
> > (r) Be aware that as written the compliance statements
> > isisCompliance and isisAdvancedCompliance do not provide for
> > read-only implementations of the MIB module.  No changes need
> > to be made if that is the intent, but the author should verify
> > that this is the case.
> 
> Fixed, except for one small omission:  isisReadOnlyCompliance
> does not have an OBJECT clause specifying a MIN-ACCESS of
> 'read-only' for the writeable object isisCirc3WayEnabled.
> That will need to be added.
> 
> This concludes my review of <draft-ietf-isis-wg-mib-19.txt>.  On
> the whole, this looks like a very good MIB document, provided
> that items 10(c), 10(e), 10(f), and 10(r) and the minor issues
> with the references are addressed.
> 
> Regards,
> 
> Mike Heard
> 

_______________________________________________
Isis-wg mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/isis-wg