[Idr] Early MIB Dr. Review of draft-ietf-idr-bgp4-mibv2-06.txt

Jeff,

This is an early MIB Dr. review of draft-ietf-idr-bgp4-mibv2-06.txt.

First, MIB compiler output is given, followed by "General Comments"
on the document, then "Specific Comments".

Thanks,
   Joan

SMICNG output:
=============
W: f(BGP4-MIB), (30,22) The first revision should
match the last update for MODULE-IDENTITY bgp

E: f(BGP4-MIB), (1834,13) Item "bgpAfPathAttrUnknownFlags"
in sequence "BgpAfPathAttrUnknownEntry" has conflicting syntax specified

E: f(BGP4-MIB), (233,13) Index item "bgpPeerAfInstance" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1041,13) Index item "bgpPeerAfInstance" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1152,13) Index item "bgpNlriIndex" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1153,13) Index item "bgpPeerAfInstance" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1228,5) Item "bgpNlriPrefixType" has invalid
value for MAX-ACCESS

E: f(BGP4-MIB), (1362,13) Index item "bgpPeerAfInstance" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1371,13) Index item "bgpAdjRibsOutIndex" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1445,13) Index item "bgpAfPathAttrIndex" must be
defined with syntax that includes a range

W: f(BGP4-MIB), (1703,5) Sequence "BgpAsPathEntry" and
Row "bgpAsPathTableEntry" should have related names

E: f(BGP4-MIB), (1711,13) Index item "bgpAsPathIndex" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1712,13) Index item "bgpAsPathSegmentIndex" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1713,13) Index item "bgpAsPathElementIndex" must be
defined with syntax that includes a range

E: f(BGP4-MIB), (1825,13) Index item "bgpAfPathAttrUnknownIndex"
must be defined with syntax that includes a range

E: f(BGP4-MIB), (1826,13) Index item "bgpAfPathAttrUnknownCode"
must be defined with syntax that includes a range

W: f(BGP4-MIB), (2689,5) Table "bgpRcvdPathAttrTable" and
Row "bgpPathAttrEntry" should have same name prefix

W: f(BGP4-MIB), (1148,13) Row "bgpNlriEntry" does not have a
consistent indexing scheme - index items from current table
must come after index items from other tables

W: f(BGP4-MIB), (1362,13) Row "bgpAdjRibsOutEntry" does not have
a consistent indexing scheme - cannot specify an index
item from additional "base row" bgpPeerAfEntry, since can have only
one "base row" which is bgpNlriEntry

W: f(BGP4-MIB), (1445,13) Row "bgpAfPathAttrEntry" does not have a
consistent indexing scheme - index item bgpAfPathAttrIndex from base
row bgpNlriEntry is not defined as an index item

W: f(BGP4-MIB), (1711,13) Row "bgpAsPathTableEntry" does not have a
consistent indexing scheme - index item bgpAsPathIndex from base row
bgpAfPathAttrEntry is not defined as an index item

W: f(BGP4-MIB), (1825,13) Row "bgpAfPathAttrUnknownEntry" does not have a
consistent indexing scheme - index item bgpAfPathAttrUnknownIndex from
base row bgpNlriEntry is not defined as an index item

W: f(BGP4-MIB), (3144,19) Variable "bgpPeerRemoteAddr" in notification
"bgpEstablishedNotification" is an index for a table

W: f(BGP4-MIB), (3158,19) Variable "bgpPeerRemoteAddr" in notification
"bgpBackwardTransNotification" is an index for a table

E: f(BGP4-MIB), (1970,15) Group "bgpTimersGroup" is both a MANDATORY
and conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1973,15) Group "bgpCountersGroup" is both a MANDATORY
and conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1976,15) Group "bgpAsPathGroup" is both a MANDATORY
and conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1979,15) Group "bgpBaseGroup" is both a MANDATORY and
conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1982,15) Group "bgpErrorsGroup" is both a MANDATORY and
conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1985,15) Group "bgpPeerAfGroup" is both a MANDATORY and
conditional group for module "BGP4-MIB"

E: f(BGP4-MIB), (1988,15) Group "bgpAfPathAttributesGroup" is both a 
MANDATORY
and conditional group for module "BGP4-MIB"

SIMPLE WEB
----------
Severity level requested: 6

Line  Severity  Problem
30 3 revision date after last update

77 3 revision for last update is missing

2218 5 warning: index element `bgpPeerRemoteAddr' of row `bgpPeerEntry'
                should be not-accessible in SMIv2 MIB

2689 5 warning: row identifier `bgpPathAttrEntry' should have the
                same prefix as table identifier `bgpRcvdPathAttrTable'

        5 warning: index element `bgpPathAttrDestNetwork' of row
                `bgpPathAttrEntry' should be not-accessible in SMIv2 MIB
        5 warning: index element `bgpPathAttrPeer' of row `bgpPathAttrEntry'
                should be not-accessible in SMIv2 MIB

2817 5 warning: index element `bgp4PathAttrIpAddrPrefix' of row
                `bgp4PathAttrEntry' should be not-accessible in SMIv2 MIB
        5 warning: index element `bgp4PathAttrIpAddrPrefixLen' of row
                `bgp4PathAttrEntry' should be not-accessible in SMIv2 MIB
        5 warning: index element `bgp4PathAttrPeer' of row 
`bgp4PathAttrEntry'
                should be not-accessible in SMIv2 MIB

3177 5 warning: notification `bgpEstablished' is not reverse mappable

3191 5 warning: notification `bgpBackwardTransition' is not reverse mappable

ID-NITS
--------
Did NOT run through ID-NITS since it is early review.

Comments:
---------

General Comments:
-----------------

1) With regard to the Textual Conventions, has there
been any thoughts to a separate TC document?

The reason is that, there is a strong indication that more
BGP MIB Modules will be forthcoming, thus, having a
separate TC document would help in 2 possible ways:

A. would help to clarify where the TCs were (i.e in a document
named (for example)  BGP4-TC-MIB ) and

B. may encourage other MIB Module authors to add to the document
with their TCs.

2) There aren't any read-write objects in the current MIB Module
(except deprecated objects).  I expect this will change in future
versions of the document, but it did impact my review, since this
kind of information is good to know.  This will likely also impact
the Conformance Section of the MIB, since a read-only Conformance
might be wanted by the working group.  Not to mention, either
StorageType objects or verbage will need to be added.

3) Many objects were using Integer32 (timers for example) and
should probably be Unsigned32.

Here is relevant text from RFC4181:

    "If the value range is between 0..2147483647 (inclusive), and the
     value of the information being modelled does not increase above the
     maximum value nor decrease below the minimum value, then:

     - Unsigned32 is RECOMMENDED;
     - INTEGER, Integer32, and Gauge32 are acceptable."

4) I have suggested a different structure to this MIB.  Currently,
bgp instance, remote peer and peering session information are in
the same table (bgpPeerAfTable).  Many tables then AUGMENT this table.

The suggestion is to separate bgpPeerAfTable into
3 tables: bgp instance information in one table
remote peer information into another table, and
peering session information into a 3rd table.

This structure would then have a ripple effect throughout the MIB
such that you will probably see some counters which AUGMENT the
table with bgp instance info, some which would AUGMENT the bgp session, etc.

Since other MIB Modules within BGP are to build on this MIB, then having
such a flat structure is going to be cumbersome.

This is only a suggestion, and since it is an early review of the MIB
would like to take the opportunity to put this forth as a discussion point.
(Also, my suggestion may not be the best structure for the MIB, but 
hopefully
that would be worked out through discussion.)

5) InetAddress/InetAddressType  the place for specifying the supported
address types and addresses is in the conformance, not in the MIB Module.
If there is a change in the supported types, then the objects with those
types, would all need to be deprecated, but if it is in the conformance
then only the conformance would need to be deprecated and rewritten.
The exception being if the address is an index (then a SIZE should be
used) see RFC4001 for details. Please see the DIFF-SERV-MIB (rfc3289) as an
example of using the Conformance statements to denote Address Type
and size.

6) Deprecated and Obsoleted Objects

One of the other MIB Doctors (David T. Perkins) suggested to me
the following and I would like to pass this on as a suggestion to you

> RFC 2578 has language, sich as that found in section 10.2:
> (3)  A STATUS clause value of "current" may be revised as "deprecated"
>     or "obsolete".  Similarly, a STATUS clause value of "deprecated"
>     may be revised as "obsolete".  When making such a change, the
>     DESCRIPTION clause should be updated to explain the rationale.
>
 <snip>
> For deprecated items, I start the DESCRIPTION text
> with something like..
> "This item is DEPRECRATED. This is done because..."
> I then leave the original DESCRIPTION text. For obsoleted
> items, I generally remove the original DESCRIPTION cause
> text.

Some of the DESCRIPTION clauses have been updated to various degrees
so you are definitely on the right path, but it did seem some places
where skipped and so the deprecated and obsoleted objects should be
reviewed to ensure that the DESCRIPTIONs have been modified.

Specific Comments
-----------------

Section 4. Overview

   "Multi-protocol extensions [RFC4760] were introduced along which allowed
   advertisement of reachability such as IPv6 [RFC2545], MPLS Labeled
   routes [RFC3107], etc."

*) Awkward (suggest rewording)

s/introduced along which allowed/introduced, along with/

Section 5.2 Tables

*) What does Af mean? (Address Family ?)

*) What does AFI adn SAFI stand for?  Okay, I see
Address Family Identifier and Subsequent Address Family Identifier.
Please spell out entire meaning prior to abbreviation.

Section 5.4
-----------

For the TC could you use:

BgpAddressFamilyIdentifierTC

BgpSubsequentAddressFamilyIdentifierTC

s/BgoPathAttrFlagsTC/BgpPathAttrFlagsTC

5.6 Extensions
-----------------
How do you plan to enforce using bgpExtensions for
other MIB Modules?

WRT bgpAfPathAttrIndex, in the event of a reboot of the router,
or a restart of the BGP subsystem, is all the information in the tables
that reference or use this object/index lost?  In other words,
this object becomes an index for other tables.  In the event of
a restart of BGP or restart of the router, or restart of the
SNMP Agent, etc. this object gets re-initialized, but what happens
to the indexes that use this object?

Section 6.1
---------------
*) Awkward:
   "Note that conducting BGP peering sessions over transport protocols
   other that TCP over IP are out of scope of the current BGP
   specifications."

s/that/than/

Specific Comments on the MIB Module
-----------------------------------

BgpIdentifierTC

*) DESCRIPTION is somewhat awkward.

*) Could InetAddress be used instead of creating a new
TC?  In other words, you seem to be saying this is an
IPv4 Address, but I am not sure what is specific to
BGP wrt to this.

*) Is this the correct Reference?
RFC4271(?) The definition in the Terminology section of RFC4271
was useful.

BgpAfiTC

*) rename BgpAddressFamilyIdentifierTC
*) Please spell out what AFI stand for (both first time in
the document text and first time in the MIB Module.

BgpSafiTC

*) rename BgpSubsequentAddressFamilyIdentifierTC
*) Please spell out what SAFI stand for (both first time in
the document text and first time in the MIB Module.

BgpPathAttrFlagsTC

*) Please rename to BgpPathAttributeFlagsTC

    -- bgp 2 and 3 have been deprecated and are documented
    -- elsewhere in this MIB

*) Are you referring to the protocol versions 2 and 3, or do you
mean the MIB objects that are specific to these versions have
been deprecated?

bgpIdentifier
SYNTAX  IpAddress

*) According to RFC4181 IpAddress should not be used. Can
InetAddress/InetAddressType or even InetAddressIPv4 be used?
(Or is the BgpIdentifierTC is more appropriate?)

INDEX Clause for bgpPeerAfEntry and the order
of the objects in the entry

*) I don't understand the ordering here.
The Indexes need to uniquely identify an entry
in the table and they should be at the beginning of
the table.

*) Additionally, are all the indexes necessary.

*) Looking this over, I'd like to make a suggestion
that this table be made into two  (or more?) tables.

This one table represents the "bgp instances" on this
router AND the remote peers AND peering sessions.

My suggestion would be
to have a table for just the "bgp instances" to include
ConfiguredVersion, AdminStatus, PeerState, Local Info, etc.
and the other table for the Remote Peer Information (and
perhaps, another for peering session information?)

Maybe another way to think of this is when information
would be available.  "bgp Instances" should have information
prior to Peer Information, right?

bgpPeerAfInstance

*) Needs to have a range beginning with 1.

The DESCRIPTION is a little confusing, but I think you
are saying that if there is only one BGP routing process
then it is recommended that this index have the value
of 1.

bgpPeerAfPeerState

*) the name of this object is misleading.  This is
the state of the connection so please rename.

bgpPeerAfAdminStatus
*) why is this read-only and not read-write?

bgpPeerAfConfiguredVersion
*) why is this read-only and not read-write?

bgpPeerAfLastErrorReceived
bpgPeerAfLastErrorSent
*) Why did you choose to represent this as a 2 byte
OCTET STRING as opposed to ErrorCode and ErrorSubCode (2 objects)?

*) General Comment for this table, may want to group all the
received objects together followed by the sent objects.

bpgPeerAfLastErrorReceivedText
bpgPeerAfLastErrorSentText
*) Could a REFERENCE clause be added for these 2 objects?

bpgPeerAfFsmEstablishedTime

*) Do peering sessions always reach the Established State?
If not, what is the value of this object (zero?).  That should
probably be added to the DESCRIPTION

bpgPeerAfConfiguredTimersTable

*) General comment (getting back to the issue of
separating the tables into "bgp instance" vs. "remote peer info"
vs "peering session info", this table would seem to largely
be part of the bpg instance information.  I don't understand
why these objects are read-only?  Is configuration not allowed
via SNMP for these objects?

*) Also the name of this table, might be better to take out
the "Peer" so bgpConfiguredTimersTable?

bgpPeerAfNegotiatedTimersTable

*) Could we consider renaming some of these tables?
This is bgpPeeringSession info, correct?

*) Timers in this and the previous table were Integer32,and
should be Unsigned32.
see rfc4181 section 4.6.1.1

bgpPeerAfCountersTable

*) Is there any Discontinuity Time Object(s) associated with
these counters?

DESCRIPTION:
"...This object should be initialized to zero when the connection
is established"

*) Counters by definition cannot be reset.  Please see rfc
2578.txt.  You could create a TimeStamp object which is set to
the time when the FSM transitioned to the established state.

bpgPeerAfFsmEstablishedTrans
*) Please rename to bgpPeerAfFsmEstablishedTransitions

bpgPrefixCountersTable

*) The name says counters but the table has
Gauge32 objects.

bgpPrefixInPrefixes
bgpPrefixInPrefixesAccepted
bgpPrefixOutPrefixes

*) Could references be added to these objects

*) Counters need to be pluralized.

*) Discontinuity for these?

*) This is "new" to the MIB and doesn't appear in rfc4273.
Is it needed for BGP4's basic deployment?  Was there any
consideration to putting this into a separate MIB Module?

    bgpRib
        OBJECT IDENTIFIER ::= { bgp 11 }

*) Please rename to bpgRibObjects

bpgNlriTable

*) Could REFERENCES be given for this table?

*) I didn't understand the indexing and the indexes
do not match the entry of this table.  Very difficult
to review this one and the bgpAdjRibsOutTable.  Please
fix the indexes to match the table entry.

bgpAfPathAttrCount

*) Why is this object needed?
*) Please keep in mind that Counter type objects need to be
pluralized.

bgpAfPathAttrTable
*) Noncontiguous numbering between bgpAfPathAttrTable (4) and
bgpAsPathTable (6) (Number 5 is missing)
Should this be renumbered?

bgpAfPathAttrNextHop

*) Please rename bgpAfPathAttrNextHopAddr (to go along with
bgpAfPathAttrNextHopAddrType)

bgpAfPathAttrLinkLocalNextHop
*) Please add an AddrType object
prior to this and use the Conformance Statement to specify what is
supported.

bgpAfPathAttrMedPresent

*) Please add to the DESCRIPTION an explanation of what true(1) means?

bgpAfPathAttrLocalPref

*) What does the DESCRIPTION mean when it says: "this object will be
absent"?

bgpAfPathAttrAggregatorAS and bgpAfPathAttrAggregatorAddr

*) What does the DESCRIPTION mean when it says:
"this object will not be present in the conceptual row"?

bgpAsPathIndex

*) In the event of a reboot of the router, or a restart of the
BGP subsystem, is all the information in the tables
that reference or use this object/index lost?  In other words,
this object becomes an index for other tables.  In the event of
a restart of BGP or restart of the router, or restart of the
SNMP Agent, etc. this object gets re-initialized, but what happens
to the indexes that use this object?

bgpAfPathAttrUnknownIndex
*) same question as above.

bgpAfEstablishedNotification

*) Would the FSM ever be in a state that would fluxuate in and out
of the established state?  In other words, would this notification
ever get flooded to the network?

bgpAfBackwardTransNotification

*) Please spell out what "Trans" stands for?

Conformance Section
*) If read-write objects are added to the MIB, then a read-only conformance
may be wanted.

---------------end

_______________________________________________
Idr mailing list
Idr@ietf.org
https://www.ietf.org/mailman/listinfo/idr