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

From: "C. M. Heard" <[email-masked]>
Date: Fri, 15 Apr 2005 02:08:41 -0700 (PDT)

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

---

From: "Parker, Jeff" <[email-masked]>
Date: Fri, 15 Apr 2005 10:03:00 -0400

Thanks for the gimlet-eyed review.  I will revise, circulate a draft
privately, and resubmit a new ID shortly.

- jeff parker

On 4/15/05 5:08 AM, "C. M. Heard" <[email-masked]> wrote:
 
> ...

- and -

On 4/15/05 8:36 AM, "Wijnen, Bert (Bert)" <[email-masked]> wrote:

> 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