[Dime] AD review of draft-ietf-dime-diameter-base-protocol-mib-04.txt

["Romascanu, Dan (Dan)" <[e-mail]>]

Please find below the AD review of
draft-ietf-dime-diameter-base-protocol-mib. This document is in pretty
good shape, however, there are a number of issues that need to be
clarified and answered before it is sent for IETF Last Call. Please
consider the comments below and address them or clarify the questions
that I have misunderstood. 

Regards,

Dan


Meta Issue: it looks to me (although this is in no place clearly
articulated) that this MIB module applies to DIAMETER servers only. If I
am wrong, and a subset of the MIB module applies to clients, than this
needs to be reflected in the MODULE-COMPLIANCE clauses. If I am correct
than the fact it's a server MIB should be explicitly said in the
document, and maybe the document and the MIB module renamed to reflect
this. Then, what happens with clients. Will the WG develop another MIB
module for clients ?


Technical Issues: 

T1. dbpProtocolErrorNotifEnabled has no information about persistency on
reboot assigned, and so do a score of other writeable objects. Need to
add this information, on a global or per object basis. 

T2. dbpLocalId - what useful interoperable information provides this
object if the id string is implementation-specific? 

T3. why is dbpLocalIpAddrTable needed instead of using information from
the ipAddressTable in RFC 4293?

T4. dbpLocalRealm is read-only. How is the information acquired by the
agent? 

T5. what is the discontinuity indicator for dbpLocalStatsTotalMessagesIn
and dbpLocalStatsTotalMessagesOut? 

T6. What is the relation between dbpLocalStatsUpTime and
dbpLocalResetTime? Why are not these the same? Does the ResetTime
counter restart only on a reset command operation? 

T7. In the DESCRIPTION of dbpLocalResetTime: 

-- For software that does not
                 have persistence or does not support a 'reset'
                 operation, this value will be zero.

This behavior is not conformant with the semantics of TimeTicks. You
must define a different TruthValue object here. 

T8. The DESCRIPTION clause of dbpLocalCOnfigurationReset speaks about
'when set to reset(2), but there is no such enumerated value. 

T9. dbpLocalApplicationIndex 

           "A number uniquely identifying a
                  supported Diameter application. Upon reload,
                  dbpLocalApplIndex values may be changed."

'reload' means reset? How does this work? If the unique number which is
also an index in the table changes how will a management application
working with this agent know that this is the same application? 

T10. dbpPeerId - is not this information coming from peers? Who ensures
uniqueness? Or is this just a local index in this server for peers,
created by the management application? 

T11. When a new entry is created in the dbpPeerTable - how is the
read-only information acquired? 

T12. dbpPeerPortConnect - you cannot have a value zero(0) if the syntax
is Unsigned32 (1..65535) - this should rather be Unsigned32(0|1..65535)

T13. dbpPeerFirmwareRevision - the SYNTAX of this object should be
consistent with the entPhysicalFirmwareRev object in RFC 4133 which is
an SnmpAdminString. Actually the DESCRIPTION clause should mention that
if the Entity MIB is supported on the peer, than the two objects must
have identical contents. 

T14. I need to understand how you use the StorageType objects. When
written with the value permanent the row cannot be modified, only
deleted? How would a management application know what to write there? 

T15. Why does dbpAppAdvToPeerEntry have a PeerVendorId as index. Can a
given server run DIAMETER applications originated from more than one
vendor? 

T16. dbbAppAdvToPeerIndex - same question as for T9

T17. Is not information about peers acquired? What does it mean for a
manager to create an entry in the dbpPeerVendorTable? What if the peer
does not exist? 

T18. dbpPerPeerStatsTimeoutConnAtmpts - a counter that resets on
disconnection can mislead the management application. What would it make
if the read is 3 and at the next poll 1? Or 3 and 4 but maybe there was
a 0 value in the meantime? You need a correct syntax counter, and
another counter for the number of disconnects. 

T19. I could not figure out what exactly dbpPerPeerStatsDWReqTimer means
- how does the agent acquire this value? 

T20. Security Considerations section - The security problems that can be
caused by intentional or incidental misconfiguration of writeable
objects must be described. For example 'Misconfiguration of
dbpPeerConnectUpNotifyEnable can cause notifications about changes in
peers connectivity not to be transmitted to the management
application.', etc.  


E1. RFC 4181 section 3.2 recommends that the narrative sections of MIB
documents 'include one or more sections to briefly describe the
structure of the MIB modules defined in the specification'. I see no
reason for an exception here. 

E2. Running id-nits results in a comment / question: 

 -- The document has a disclaimer for pre-RFC5378 work, but was first
     submitted on or after 10 November 2008.  Does it really need the
     disclaimer?

E3. I had a hard time associating the MIB objects with the definitions
in RFC 3588. Adding REFERENCE clauses to the MIB objects would be
extremely useful for the readability of the document. 

E4. The DESCRIPTION clause of dbpProtocolErrorNotif and of other
notification include the phrase: 

-- It can be utilized by an NMS to trigger
                 logical/physical entity table maintenance polls.

What logical/physical entity table is referred to here? Is this from the
Entity MIB. If so please add this information, and the Entity MIB as an
informative reference

E5. dbpLocalOriginHost - is this the host name? 

E6. It would be useful to add UNITS clauses for all counters objects

E7. dbpPeerProtocol should better be named dbpPeerTransportProtocol

E8. Should not the SYNTAX of dbpAppAdvToPeerServices be rather a TC? 

E9. The table dbpPerPeerStatsTable includes not only statistics but also
other status and configuration information. It would be good to rename
it in something like dbpPerPeerInfoTable and change the DESCRIPTION
clause accordingly. 

E10. dbpPerPeerStatsStateDuration - does this mean 'time elapsed since
the last change in state? 

E11. dbpPerPeerStatsTimeoutConnAttempts - s/attempts/attempted/

E12. UNITS clauses would be very helpful for all counter objects. 

E13. The DESCRIPTION clauses of the counters objects refer to DIAMETER
PDUs sometimes as 'messages', sometimes as 'packets'. Using a consistent
terminology would be nice. 

E14. DESCRIPTION clause of dbpRealmKnownPeers: 

-- The index of the peer this realm knows about.
                 This is an ordered list, where the ordering
                 signifies the order in which the peers are
                 tried.  Same as the dbpPeerIndex

Two questions: what is the 'This' that is referred to in the second
phrase? What order means - that the peer with lower index value is tried
first? 

E15. dbpRealmKnownPeerChosen - it is recommended that in such enumerated
other get value other(1) so that if new functional values are added
later, other stays first and not in the middle.