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

From: "C. M. Heard" <[email-masked]>
Date: Sun, 6 Mar 2005 14:31:10 -0800 (PST)

On Thu, 17 Feb 2005, Juergen Schoenwaelder wrote:
> I need a MIB doctor for the IS-IS routing protocol MIB which you can
> find in <draft-ietf-isis-wg-mib-17.txt>. There are some idnits kind
> of things the author can hopefully check himself. What seems more
> important here is to check the general construction of the MIB module
> and of course proper usage of the SMIv2.
>
> If you have some spare cycles to help, please drop me a note and
> indicate when you will be able to work on this MIB module.

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.

1.) I-D Boilerplate -- OK

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.

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.

4.) IPR Notice -- OK

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

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.

7.) IANA Considerations Section -- content is correct, although it is
customary for this section to appear after the MIB module.

8.) Copyright Notices -- OK

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:

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.

(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 :)

(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 thing that these requirements have been met.

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

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.

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

(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).

(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).

(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."

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

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

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

(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).

(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.)

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

(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

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

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

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

This concludes my review of <draft-ietf-isis-wg-mib-18.txt>.

Regards,

Mike Heard

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

---

From: "Parker, Jeff" <[email-masked]>
Date: Sun, 06 Mar 2005 18:54:27 -0500

Mike -
    I accept (almost) all of your comments, and I'm you didn't get to (z) in
your list of points.  One comment on (m).

    IS-IS does not depend on IP, and it does it's own fragmentation at the
packet source.  Think "PDU" when you read "Fragment".
    I could use the 10589 term, "LSP number field", rather than "Fragment
Number", but that term seems more ambiguous to me, as the LSP is full of
numbers.  

    This fragmentation scheme leads to a great deal of confusion when
routers have a different value for the MTU, which gave rise to a number of
Notifications about OriginatingBufferSize and the like.

- jeff

---

From: "C. M. Heard" <[email-masked]>
Date: Sun, 6 Mar 2005 17:47:36 -0800 (PST)

Jeff -

I suspected that the question I raised in 10(m) below might just be
a result of my limited understanding of IS-IS.  If the WG is happy
with the object definition as it stands, then so am I.

Regards,

Heard

On Sun, 6 Mar 2005, Parker, Jeff wrote:
>  Mike -
>     I accept (almost) all of your comments, and I'm you didn't
> get to (z) in your list of points.  One comment on (m).
> 
>     IS-IS does not depend on IP, and it does it's own
> fragmentation at the packet source.  Think "PDU" when you read
> "Fragment".
>     I could use the 10589 term, "LSP number field", rather than
> "Fragment Number", but that term seems more ambiguous to me, as
> the LSP is full of numbers.
> 
>     This fragmentation scheme leads to a great deal of confusion
> when routers have a different value for the MTU, which gave rise
> to a number of Notifications about OriginatingBufferSize and the
> like.
> 
> - jeff 
>

---

From: [email-masked]
Date: Tue, 15 Mar 2005 02:28:34 -0500

Mike,

Thanks for a great review.

There is one object which I would like to see added.
This was discussed in emails to the list in August 2004
timeframe and there seemed to be some favor in adding it,
but I don't see this in the current rev of the MIB.

The object is:

isisCircOperState OBJECT-TYPE
    SYNTAX      INTEGER {
                  operStatusUp(1),       -- active
                  operStatusDown(2),     -- inactive
                  operStatusGoingUp(3),  -- activating
                  operStatusGoingDown(4),-- deactivating
                  operStatusActFailed(5) -- activation failed
                }
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
       "The operational state of this circuit."
    ::= { isisCircEntry 5 }

The text in the isisSysType and isisCircLevel descriptions need to be
clarified.
Also, shouldn't there be some errors returned under
certain circumstances? For example, if the sysType object is Level1, then
an operator should receive an error if the circLevel is set to Level2 or
Level1Level2
(would guess an Inconsistent Value error would be appropriate).

Additional text for the isisCircLevelTable is also needed for example:

- Two entries in the isisCircLevelTable exist for every isisCircTable
entry.  One for level 1 and one for level 2.  For example:

"This table will contain 2 rows (one for level1 and one for level2) per 
circuit.  Both of the rows per circuit will be "in use" if the SysType and 
CircuitType corresponding to this row is Level1Level2. If the
value of SysType is Level1 or Level1Level2 and the value of CircuitType is
Level1, then
only the level1 row will be in use and the level2 row is not in use.  
If the value of SysType is Level1Level2 or Level2 and the value of 
CircuitType is Level2, then only the level2 row will be in use and the 
level1 row is not in use."

thanks, Joan

---

From: Christian Hopps <[email-masked]>
Date: Wed, 16 Mar 2005 10:42:27 -0800

While we haven't mentioned this on the list. We are locking down 
changes on the MIB. As soon as the MIB doctor review is done (and any 
required modification made) we will be doing a WG last call on the MIB. 
During this last call we will only allow changes to fix obvious 
problems with the MIB. If another LC is required we will only allow 
changes to the previous changes, and so forth until we have completed 
last call.

IS-IS has been without a MIB for too long and there has been plenty of 
time to get this right. At some point we have to stop and get a 
standard MIB published. Now is that point.

Thanks,
Chris and Dave.