The MPLS WG Archive

["Martin Dubuc" <m.dubuc@rogers.com>]

This is my response to Bert's email which explains how I have addressed Bert's comments in draft-ietf-mpls-telink-mib-01.txt that was submitted this morining to IETF. My comments are embedded in the original mail (prefixed with [Martin]).

 

Martin

 

* * *

 

[added Alex and WG chairs]

First... as you can see, I have quite a bit of comments.
I would think it would be good if the WG sees all that.
However, I cannot post it since the document has not been
given to the WG yet.... WG chairs, what do you think?

Here is my review:

- compiles/syntax-checking clean. Good!

But.....

- linelegth checker:
   $ /bin/checkpage.awk <home/ietf/drafts/draft-ietf-mpls-telink-mib-01.txt
   Long line at 770 with 77 chars
   Long line at 771 with 76 chars
   Long line at 1268 with 73 chars
   -: 3 lines longer than 72 characters, max 77

[Martin] Fixed.

- Last sentence of abstract talks about "draft". I think you want to use
  "document" or "memo" so that it is still valid when that draft becomes
  an RFC. Maybe this occurs at other places too, pls check

[Martin] Fixed. Checked for other references to draft. Was the only reference
to a draft.

- You seem to be using "MIB" many times where "MIB Module" or "MIB document"
  would be better. Remember there is only one (Single) MIB, which is composed
  of many (multiple) MIB modules. One or more MIB modules may be described
  and specified in a MIB document

[Martin] Fixed.

- I have not yet checked sect 7.

- I do not see any words on the relationship betwen teLinkXxxTables
  and componetXxxxTables. Would be good to write something about that

[Martin] I have added text to explain the relationships at end of
section 6.

- I would suggest that maybe you should replace your 1st figure in sect 8.2
  with the figire from the overview document (revision 4) section 11.2. That
  seems clearer to me.

[Martin] Done. I have used similar format for the second figure.

- I still have the impression that sect 5.1 (summary) and section 6 are
  basically a duplicate of each other. I can live with it... but ???

[Martin] You are correct, section 5.1 is roughly the same as section 6.
I have deleted section 5.1 and consolidated its non-duplicated content
into section 6.

- sect 8.1 under ifType you refernece [IanaFamily].
  I think the reference should be to the ianaiftype.mib

[Martin] You are right. Fixed.

- for my understanding, the figure in sect 8.2, can you explain which layers
  in the stack are bundle(s), telink or component link. In fact it might be
  good to add that explanantion to the text, so that people can read it too.

[Martin] Actually the example is not very clear and your comment has triggered
a flurry of mails between the authors. We have updated the example to
make it easier for readers to understand. opticalTrans are component links.
In the example in version 00, the layer on top of opticalTrans was bundled link. The
component links in this example were actually TE links but this fact was not
shown. These TE links were implicit. You would have to be a psychic to see
this. The layer on top of bundled link was also bundled link. With this, we were
showing that one can bundle bundles. But we feel that this adds complexity that
is not required. We have streamlined the example and explained it better. We

have removed the second layer of bundling because this is an advanced

configuration.

- In the CONTACT-INFO, pls add WG mailing list information

[Martin] Done.

- In DESCRIPTION clause of MODULE-IDENTITY..
  - add year (2003) to copyright statement
  And I think I would change:

        This MIB contains managed object definitions for
        MPLS traffic engineering links as defined in:
        Kompella, K., Rekhter, Y., Berger, L.,
        Link Bundling in MPLS Traffic Engineering
        Internet Draft <draft-ietf-mpls-bundling-04.txt>,
        July 2002."

  Into
        This MIB module contains managed object definitions for
        MPLS traffic engineering links as defined in 'Link Bundling
        in MPLS Traffic Engineering'."

[Martin] Fixed.

- In DESCRIPTION clause of teLinkENtry you still have a (TBD), while
  we already know that it is 200, do we not?

  Also, I do not understand the last sentence of that DESCRIPTION clause
  (I dare to bet there are others who won't understand it either). SO
  please clarify in the text

[Martin] Have replaced TBD to 200. Last sentence was removed since this is
no longer true (there is a separate field for outgoing interface id).

- May I recommend that
     teLinkIpAddrType             InetAddressType,
     teLinkIpAddr                 InetAddress,
     teLinkRemoteIpAddr           InetAddress,
  Be renamed to:
     teLinkAddressType             InetAddressType,
     teLinkLocalAddress            InetAddress,
     teLinkRemoteAddress           InetAddress,
  First of all, there can be an unnumbered addresstype (unknown), so then
  it is not an IpAddress.
  Second, I think that the first address is the local address (that is on
  the device where the instance of the object lives, no?

  Further I wonder.. if it is an unnumbered link, it has an identifier somehow
  does it not. Why would we not put that identifier in the address in that case
  and describe what it looks like. In fact I wonder if instead of  the
  InetAddress.... TCs would it not be better to use TeHopAddres... TCs from the
  MPLS-TC-MIB? It allows for TeHopAddressUnnum as an address

  In any event, I would not write:
    teLinkIpAddrType OBJECT-TYPE
      SYNTAX        InetAddressType
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "For IPv4 and IPv6 numbered links, this object represents the
         IP address type associated with the TE link. For
         unnumbered links, a value of unknown(0) must be used."
  But rather something aka:
    teLinkAddressType OBJECT-TYPE
      SYNTAX        InetAddressType
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "The type of Internet address for the TE link. Only IPv4 and
         IPv6 and unknown (for unnumbered links) need to be supported."
 
  Then forther in the Address itself I would do something aka (not that
  the first sentence is REQUIRED as per RFC3291):
    teLinkLocalAddress OBJECT-TYPE
      SYNTAX        InetAddress
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "The local Internet address for the TE link. The type of this
         address is determined by the value of the teLinkAddressType
         object. For an unnumbered TE link, the address represents an
         unnumbered interface in this format:

              octets   contents               encoding
              1-4      unnumbered interface   network-byte order
        "
  I stole some of the text from MPLS-TC-MIB for now

[Martin] I have changed the name teLinkIpAddrType to teLinkAddressType per
your recommendation. However, I have not changed teLinkIpAddr and
teLinkRemoteIpAddr because these fields are really meant to be IP addresses.
If address type is unnumbered, then these fields should not be used (the
teLinkIncomingIfId and teLinkOutgoingIfId fields should be used instead).
It looks like this is not clear enough in the MIB, so
I have added text to clarify this point.

- I see in a lot of places something like:
       REFERENCE
       "[BUNDLING]"
  That is not the proper way to reference in a REFERENCE clause. When a
  MIB module gets extracted from the RFC (and that happens a lot) then all
  the references to the reference section are lost. So we normally use
  something like:
      REFERENCE "Link Bundling in MPLS Traffic Engineering, RFC aaaa."
      -- RFC Editor to fill in RFC number that will be assigned to [BUNDLING]

  Not that this makes for a normative reference to [BUNDLING]

[Martin] Done. Have updated all other references.

- I see this coming back a few times:
    teLinkMuxCapability OBJECT-TYPE
        SYNTAX   INTEGER {
                     packetSwitch1(1),
                     packetSwitch2(2),
                     packetSwitch3(3),
                     packetSwitch4(4),
                     layer2Switch(51),
                     tdm(100),
                     lambdaSwitch(150),
                     fiberSwitch(200)
                 }
   So that is a good candidate for a TC. That would then also allow you to
   say a bit more what packetSwitch1, packetSwitch2, etc in fact means.
   Any explanation for skipping values? It is recommended (but not required)
   that they are contiguous. So explaining in the description why such is not
   the case is always wise. It possibly is as simple that these values
   are already allocated/specified somehwere else (GMPLS-OSPF doc?) and
   reused here.

[Martin] I have defined a TC for this. The reason for skipping values is that
we are using the same values than what is defined is in the GMPLS-OSPF focument
(values used in the messages). I have stated this fact in the description clause.

- Would it make sense to elaborate a bit in the DESCRIPTION clause of
  teLinkProtectionType and explain what the values exactly mean?
  Or if they are explained in that GMPLS-OSPF doc that you refernce, then
  at least make that statement.

[Martin] The values are specified in GMPLS-OSPF, but are actually described in
GMPLS-ROUTING. I have added this statement (and the new reference).

- I think all the "priotity" objects have a value from 0-7 (or one that maps
  to it). Does that call for a TC, in which you can then also describe a bit
  more extensive what it is?

[Martin] Moved to TC. Added explanations in the DESCRIPTION clause.

- teLinkIncomingIfId and teLinkOutgoingIfId
  I am unclear if these are only for unnumbered links or also for numbered
  ones. Can you make that explicit in the DESCRIPTION clauses?

[Martin] This is strictly for unnumbered links. Have added in DESCRIPTION clause.

- The places where you use Priority (1..8) that maps to (0..7), I expect that
  you did that because they are INDEX objects and INDEX objects should not
  include a zero value. However... that is the RECOMMENDED practice. If there
  is a good reason to include value zero in the index range then such can
  be done, and here it seems that such is justified. (I am assuming here
  that the priority is defined in some other doc as being in the range of
  0-7. Maybe it is even a field in some other protocol data structure.

[Martin] When specifying these indices I thought that it was forbidden to use value 0
(should have been more consistent in other places). It does make more sense
to use value 0 to 7 since this is what is used in OSPF protocol data structure
(OSPF traffic engineering extensions). Have added text to state that 0 is valid and why.

- teLinkResourceClass
  Any reference to a document that explains how that bit-valued field is
  composed/conbtructed/used?

[Martin] Have added reference to [OSPF] where resource class is described.

- In all the places/tables where ifIndex is used as an index, could you
  specify if a specific ifType is expected (or maybe even required) for
  such an interface. I have the impression that for some it is a fixed
  value that is acceprtable, for other multiple values are acceptable,
  and for some maybe even any value is acceptable. If you specified it,
  I think that things would be clearer for me (and hopefully for others
  too).

[Martin] You are right. In some places, ifType is fixed, others multiple
values are acceptable. I have added such description in all tables
that use ifIndex.

- At all those places where you use Bandwith of 1000 bits per second.
  You have it in the DESCRIPTION clause. Better is to put it in a UNITS
  clause, so that it is machine readable/parsable.
  It is used at many places. Candidate for a TC ??

[Martin] Have added UNITS clause. But not TC.

- I see this multiple times:
     teLinkEncodingType OBJECT-TYPE
       SYNTAX    INTEGER {
                     packet(1),
                     ethernet(2),
                     ansiEtsiPdh(3),
                     sdhItuSonetAnsi(5),
                     digitalWrapper(7),
                     lambda(8),
                     fiber(9),
                     fiberChannel(11)
                 }
   Yep... as you guessed: why is it not a TC?

[Martin] It is a TC now.

- I see a few of these:
   componentLinkPreferredProtection OBJECT-TYPE
      SYNTAX        INTEGER {
                       primary(1),
                       secondary(2)
                   }
  Candidate for a TC I'd say

[Martin] Done.




- I still see in (I believe all of) your RowStatus objects somthing like:
    DESCRIPTION
       "This variable is used to create, modify, and/or
        delete a row in this table. All read-create objects
        can only be changed when teLinkRowStatus is active."
        -----------
  As I said before, This really weird. Probably/Possibly You mean:
        All read-create objects can be changed even when xxxRowStatus
        is active."
  If so, pls fix, if not, pls explain.
  You may want to reread first para on page 17 of RFC2579
  In any event, when a row is initially created, it cannot be 'active',
  it can only become active if all columns have proper values, so one
  must be able to change a row when in notReady or notInservice state.
  Or when in not-existing state. That is the default. The default is also
  that one cannot write (SET) any columns while a row is active, so that
  is why a DESCRIPTION clause of a RowStatus object MUST specify which
  columns can or cannot be written/changed when the row is active.
  Seems you want to allow all columns to be written-to/changed when a row
  is active, so my suggested text replacement above caters for that.

[Martin] I meant to say that read-create objects can only be changed when
teLinkRowStatus is not active. I have updated all RowStatus text accordingly.

- WHen I see a table like this:
   teLinkDescriptorTable
   TeLinkDescriptorEntry ::= SEQUENCE {
      teLinkDescriptorId           Unsigned32,
      teLinkEncodingType           INTEGER,
      teLinkDescrPriority          Unsigned32,
      teLinkMinReservableBandwidth Unsigned32,
      teLinkMaxReservableBandwidth Unsigned32,  
      teLinkDescrRowStatus         RowStatus,
      teLinkDescrStorageType       StorageType
  Then I always wonder if we cannot make it easier for people to see which
  objects are indeed in that table. The first object name teLinkDescriptorId
  clealrly is in this table. Then we have one teLinkEncodingType that is not
  so clear. Then we have one that is not as clear as the 1st one, but still
  gives some clue. Then we have 2 that give not clue, then we have 2 more
  that give some clue. How about naming them:
   TeLinkDescriptorEntry ::= SEQUENCE {
      teLinkDescriptorId                Unsigned32,
      teLinkDescrEncodingType           INTEGER,
      teLinkDescrPriority               Unsigned32,
      teLinkDescrMinReservableBandwidth Unsigned32,
      teLinkDescrMaxReservableBandwidth Unsigned32,  
      teLinkDescrRowStatus              RowStatus,
      teLinkDescrStorageType            StorageType
  If you can agree with that, then pls check you other tables. The same
  issue/concern comes back in a few other tables.
  In fact our mib guidelines document does discuss this issue.

[Martin] Although it is not obvious why some entries do not have the same table
prefix, some others have been shortened because of the 32 character limit
warning for identifiers reported by smilint. I have changed the
identifiers so that they have the same prefix but stayed within
limit of 32 characters.

- Your teLinkNotifEnable would be better named: teLinkNotificationsEnabled

[Martin] Changed.

- I worry about the number of notifications that can get generated per minute
  or per second? Can you say something about it? Do we need an abject that
  lets an NMS configure how many notification an agent can send at most per
  minute?

[Martin] is only one type of notification in the TE link MIB module.
This notification can only occur if the provider uses bundled link and will
be generated only when there is a mismatch in a link bundle. Since link bundles
and TE links are provisioned by users and not on a regular basis, and since
mismatches are not likely to be common, the number of notifications should be
extremely small. I do not think there is a need to throttle these notifications.

- linkBundleMismatch
  I do not understand the DESCRIPTION clause at all. Specifically, if an error
  is found and the notification is sent, then the 1 second later, the same
  situation probably still exists. Is another notification sent?
  Should such a problem not be detected when someone creates an antry in
  a table and should create operation not be prevented from succeeding?

[Martin] I expected that the notification would be sent only when the
mismatch was detected. We could prevent a create to succeed if we
detect a mismatch, but it may be easier to just flag the error.
I don't know how others feel about this.

- I do not understand the difference between your ...FullCOmpliance and
  ...MonCompliance (by the way, in other MIB modules we have named the
  latter one ...ReadOnlyCompliance). I would think that the ...FullCOmpliance
  one should NOT allow for read-only at all. How can you otherwise claim that
  it is for monitoring AND configuring.

[Martin] I meant for the first compliance to be used by devices that allowed
provisioning and monitoring and the second compliance for devices
that only allowed monitoring (no provisioning). I do not know why read-only
were allowed in the first case. This is not supposed to be. I have changed this.
I have renamed MonCompliance to ReadOnlyCompliance.

- I also have trouble with:
     OBJECT      teLinkIpAddrType
     SYNTAX      INTEGER { unknown(0), ipv4(1), ipv6(2) }
     MIN-ACCESS  read-only
     DESCRIPTION
         "The dns(16) address type need not be supported.
          The ipv4(1) and ipv6(2) address types need not be
          supported if numbered links are not supported. The
          unknown(0) address type need not be supported if
          unnumbered links are not supported."
   I would rather do something like:
     OBJECT      teLinkIpAddrType
     SYNTAX      INTEGER { unknown(0), ipv4(1), ipv6(2) }
     MIN-ACCESS  read-only
     DESCRIPTION
         "Only ipv4(1) and ipv6(2) address types need to be
          supported for numbered links. For unnumbered links
          unknown (0) needs to be supported."
   In the future other types could be added to InetAddressType TC, and
   so you onbly want to list here what needs to be supported, not
   (in explicit form) what needs not be supported.

[Martin] Fine. Have updated accordingly.

   I wonder if you do not need ipv4z and/or ipv6z.  As long as you have
   evaluated this (with the WG) and decide that you do not need it, then
   fine.

[Martin] The issue of what to do with ipv4z and ipv6z has not been raised
by the WG or by the 3 implementations that we know of the MIB.

- I have trouble with:
      OBJECT      teLinkRowStatus
      SYNTAX      INTEGER { active(1), notInService(2),
                            createAndGo(4), destroy(6) }
      DESCRIPTION
          "The notReady(3) state need not be supported."

  I think what you want to specify is that createAndWait is not needed.
  Since you allow all objects to be changed in 'active' mode, the
  notInService may not be needed either. (I am assuming here that I
  did understand your intent... which I am not sure of). In that case
  something like the following example from RFC3289 would be better:

    OBJECT diffServClfrStatus
    SYNTAX RowStatus { active(1) }
    WRITE-SYNTAX RowStatus { createAndGo(4), destroy(6) }
    DESCRIPTION
       "Support for createAndWait and notInService is not required."

[Martin] The notInService is required since it is one of the state that
allows writes (see explanation a few points above). notReady and createAndWait
are not required. For read-only, only active is required. I have updated text
in this respect.

- I have trouble with:
      OBJECT      teLinkStorageType
      SYNTAX      INTEGER { other(1) }
      DESCRIPTION
          "Only other(1) needs to be supported."

  I think I could live with a volatile only, but the 'other' value
  is completely unspecified and seems to make no sense to me at all.
  What is you intention here?

[Martin] I picked this from the LSR MIB. I don't think there is a good
reason to limit to one single value. I have removed this conformance
statement.

- In security considerations...
  - Do All tables have the same considerations? If so you may want
    to make that explicit.

[Martin] Yes, they all have the same considerations. All info in this
MIB module are used for routing purposes. To me, they have the
same sensitivity. Except for IP addresses, which may actually be used
in some sort of attack if known. This is why I have a separate clause
regarding IP addresses.

  - In general, our security ADs probably want some more specificity.
    A statement like "all tables ....." sounds as if you are opting for
    an easy out.

[Martin] It may sounds like I am opting for an easy out, but the fact of
the matter is, all tables have the same considerations.

  - On th eread-only objects... are there no issues with bandwith info?
    Or priorities? On protection?
    would that not reveal info that people don't want to loose?

[Martin] The problem here is where you draw the line. Every MIB object
is vulnerable and may reveal info that would be better kept secret. I have
read the security guidelines thoroughly and my interpretation is that we
should uncover anything that is sensitive to exposure of end-user
personal information or may lead to potential disruption of service.
There isn't anything in TE link MIB that relates to end-user personal
information. There is potential for disruption of service if someone
changes attributes in any table of the MIB because this affects routing
(this is my first statement) and there may be disruption of service if the
IP address are used in some form of spoofing or attack (this is my second
statement). I don't know that there is much else to say according to the
spirit of the security guidelines.

- Sect 13.1
  Not sure whre [Assigned] is referenced or if it is needed. is it?
  Please note that you must make normative references to all RFCs from
  whihc you IMPORT any objects or TCs or such.
 
[Martin] I have removed [Assigned] reference. I have added RFC3291 as reference
since it is is imported (INET-ADDRESS-MIB).

- Sect 13.2
  You seem to have kept a lot of old MIB boilerplate references that seem
  no longer needed,.

[Martin] Fixed.

Thanks,
Bert