Subject: RE: AD review of draft-ietf-bfd-mib

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Sun, 13 Apr 2014 23:05:36 +0000

Hi Adrian,

Thank you so much for thorough review of this document!
Tom and I are working on the changes with help from Jeff, almost there.
Please see comments/reply in-line.

> -----Original Message-----
> From: Rtg-bfd [mailto:[email-masked]] On Behalf Of Adrian Farrel
> Sent: Monday, December 30, 2013 8:03 AM
> To: [email-masked]
> Cc: [email-masked]
> Subject: AD review of draft-ietf-bfd-mib
> 
> Hi authors of draft-ietf-bfd-mib,
> 
> I have done my usual AD review after receiving a publication request.
> The purpose of the review is to catch issues that would show up during IETF
> last call or IESG review and so save the reviewers some time and get the
> quality up to a level that is acceptable.
> 
> This document represents a lot of work: thank you. The issues I have found
> are relatively small. Some of them take the form of questions and you can
> answer these with email rather than feeling compelled to make
> documentation changes. Furthermore, all of the issues are open for
> discussion.
> 
> I will put the document into "Revised I-D" state and wait to hear back from
> you.
> 
> Thanks,
> Adrian
> 
> ===
> 
> You'll obviously want to make updates to accommodate changes to draft-
> ietf-bfd-tc-mib

ACK.

> 
> ---
> 
> Tom may want to change his coordinates.

Done.

> 
> ---
> 
> Could you swap sections 1 and 2 so that the document starts with the
> Introduction.

Done.

> ---
> Section 2
> s/an portion/a portion/

Thanks for catching this. Done.

> 
> ---
> 
> Section 4.4
> 
> I don't think the table maps a discriminator to a TC. Maybe it maps the
> discriminator to the session or the session index or something.
> 
> The same thing shows up in some of the Description clauses, for example,
> bfdSessDiscMapTable

Section 4.4 updated.

> ---
> Similarly 4.5

Section 4.5 updated.

> 
> ---
> 
> It is nice if you name the RFCs in comments next to the import clauses.

That's true. Added.

> 
> ---
> 
> If I am creating an entry in bfdSessTable (and it seems that I can since most
> of the objects are read-create) how do I know what value to pick for
> bfdSessIndex?
> 
> A way to handle this is through a "next available index" global object.

Good point. Added bfdSessIndexNext.

> 
> ---
> 
> bfdAdminStatus is ambiguous!
> Does setting bfdSessAdminStatus to "start" mean that the session is up or
> only that the implementation will attempt to bring it up?
> The normal approach is to have adminStatus described as "the desired
> operational status" and a separate object reports the actual operStatus.
> Since you have bfdSessOperMode, I think this is your intention.

Right. Added bfdOperStatus and bfdSessOperStatus.

> 
> ---
> 
> bfdSessState and bfdSessRemoteHeardFlag are read-only. What does it
> mean to have default values?

Good point. Removed DEFVAL from both.

> 
> ---
> 
> Doesn't bfdSessMultipointFlag need a reference to the document that
> defines multipoint BFD? If so, doesn't that create a normative reference
> gate for this MIB module? That means you either take multipoint out or you
> will have to wait until it completes the process.

Multipoint (M) flag is something defined in RFC5880 (BFD base spec), so this is ok. IANAbfdSessTypeTC in the TC MIB, however, referenced the multipoint draft. IANAbfdSessTypeTC used to have MultipointHead and MultipointTail. We have removed MultipointHead and MultipointTail from IANAbfdSessTypeTC now. Multipoint draft should ask to add new session types in the IANAbfdSessTypeTC registry.

Updated drafts no longer have any normative reference to multipoint draft.

> 
> ---
> 
> You appear to have two mechanisms to turn off GTSM
> 
>     bfdSessGTSM OBJECT-TYPE
>         SYNTAX  TruthValue
>         DESCRIPTION
>            "Setting the value of this object to true(1) will enable GTSM
>             protection of the BFD session.
> ... So presumably setting it to false will disable GTSM. But...
> 
> 
>     bfdSessGTSMTTL OBJECT-TYPE
>         SYNTAX Unsigned32 (0..255)
>         DESCRIPTION
>              The value of zero(0) indicates that
>              bfdSessGTSM is disabled."

Good point. Description of bfdSessGTSMTTL updated.

> 
> ---
> 
> bfdSessAuthPresFlag has a default of false indicating that no authentication
> is to be used. And bfdSessAuthenticationType has a default of -1 meaning
> no authentication is in use. Doesn't that mean that the default for
> bfdSessGTSM should be true and the default for bfdSessGTSMTTL should be
> non-zero?

Good catch. Changed default value of bfdSessGTSM to true, updated default value for bfdSessGTSMTTL to 255 and added recommendation updating of this variable for multihop sessions in description for bfdSessGTSMTTL.

> 
> ---
> 
> Why don't objects like bfdSessDesiredMinTxInterval have default values?

It is probably not correct to have any default values for objects for intervals and multiplier, as RFC5880 does not recommend any. These objects are bfdSessDesiredMinTxInterval, bfdSessReqMinRxInterval, bfdSessReqMinEchoRxInterval and bfdSessDetectMult. We believe it is correct for them to not have default values, but let implementations add their own default values.

> 
> ---
> 
> Shouldn't the default for bfdSessAuthenticationType be noAuthentication
> not -1?

Done.

> 
> ---
> 
> I am unsure about how the three objects for authentication work.
> 
> bfdSessAuthenticationType and bfdSessAuthenticationKeyID say that they
> must be -1 if bfdSessAuthPresFlag is false.
> 
> Now, suppose I want to turn on authentication. I want to set
> bfdSessAuthPresFlag to true, but if I do, the settings of
> bfdSessAuthenticationType and bfdSessAuthenticationKeyID are bogus.
> But if I want to set bfdSessAuthenticationType and
> bfdSessAuthenticationKeyID to real values in preparation to setting
> bfdSessAuthPresFlag to true then I will have broken the rule.
> 
> The only option appears to set all three objects at once (which is possible
> only depending on implementation).

Right. Removed following statement from bfdSessAuthenticationKeyID.

          When bfdSessAuthPresFlag is false(2), then the value
          of this object MUST be -1.  

Now implementations can update bfdSessAuthenticationType, bfdSessAuthenticationKeyID and bfdSessAuthenticationKey, and then enable authentication by setting bfdSessAuthPresFlag to true.

> 
> ---
> 
> I'm surprised that entries in bfdSessDiscMapTable are writeable.
> That is to say, that bfdSessDiscMapStorageType and
> bfdSessDiscMapRowStatus exist and are writeable. Surely this table is
> automatic and entries an artifice of entries in bfdSessTable.
> 
> Why would you want an entry in this table to have a different storage type
> or row status from those in bfdSessTable?
> 
> What does it mean that those two objects are creatable? Does it mean that
> you can create an entry in this table without knowing the value of
> bfdSessDiscMapIndex which is automatic?
> 
> Same questions about bfdSessIpMapTable

Good point. bfdSessDiscMapTable and bfdSessIpMapTable are indeed lookup tables. Googled on how to make these tables read-only. I believe correct thing to do remove RowStatus and StorageType objects from the two tables. Is this correct?

> 
> ---
> 
> The Security section says...
> 
>    The bfdSessAuthenticationType, bfdSessAuthenticationKeyID, and
>    bfdSessAuthenticationKey objects hold security methods and associated
>    security keys of BFD sessions.  These objects SHOULD be considered
>    highly sensitive objects.  In order for these sensitive information
>    from being improperly accessed, implementers MAY wish to disallow
>    read and create access to these objects.
> 
> You don't want to prevent write access as well?

Fixed.

> 
> But note that preventing read access is pretty much not having the object.
> So why *do* you have the objects and then suggest not providing any access
> to them?

We believe that this MIB should have these objects defined in case implementations do want to allow access to some of these objects.

-Nobo, on behalf of authors

---

From: "Adrian Farrel" <[email-masked]>
Date: Mon, 14 Apr 2014 08:24:11 +0100

Hi Nobo,

All looks good. Just an answer to one of your points....

> Good point. bfdSessDiscMapTable and bfdSessIpMapTable are indeed lookup
> tables. Googled on how to make these tables read-only. I believe correct thing
to
> do remove RowStatus and StorageType objects from the two tables. Is this
> correct?

Wow! Google as a MIB-authoring tool ;-)

Yes. Remove those objects.

Looking forward to the next revision.

Thanks,
Adrian