Re: [trill] RtgDir review: TRILL: Interface Addresses APPsub-TLV <draft-ietf-trill-ia-appsubtlv-07.txt>

["McPherson, Danny" <[e-mail]>]

I’m comfortable with your stated intentions here Donald.

Thanks for the prompt response,
 

-danny




On 5/5/16, 12:02 PM, "Donald Eastlake" <[e-mail]> wrote:

>Hi Danny,
>
>Thanks for your comments. See below.
>
>On Wed, May 4, 2016 at 10:27 AM, McPherson, Danny
><[e-mail]> wrote:
>>
>> Hello,
>>
>> I have been selected as the Routing Directorate reviewer for this
>> draft. The Routing Directorate seeks to review all routing or
>> routing-related drafts as they pass through IETF last call and IESG
>> review, and sometimes on special request. The purpose of the review
>> is to provide assistance to the Routing ADs. For more information
>> about the Routing Directorate, please see
>> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
>>
>> Although these comments are primarily for the use of the Routing
>> ADs, it would be helpful if you could consider them along with any
>> other IETF Last Call comments that you receive, and strive to
>> resolve them through discussion or by updating the draft.
>>
>> Document: draft-ietf-trill-ia-appsubtlv-07.txt
>> Reviewer: Danny McPherson
>> Review Date: May 4, 2016
>> Intended Status: Proposed Standard
>>
>>
>> Summary:
>>
>>  I have some minor concerns about this document that I think should
>>  be resolved before publication.
>>
>>
>> Comments:
>>
>> I believe the draft is technically sound, however, the quality and
>> readability needs a bit more work, particularly as it relates to
>> introduction of new terms, and consistent application and use of all
>> terms.  There are also some general error handling and encoding
>> issues that need to be given consideration.
>>
>>
>> Major Issues:
>>
>> I have no “Major” issues with this I-D.
>
>Thanks.
>
>> Minor Issues:
>>
>> 1. ERROR HANDLING: There are a number of places in the document
>> where it discusses the receipt of malformed, badly encoded,
>> non-matching, or corrupt messages, and the advice is to either
>> [silently] discard or ignore the messages.  Some general guidance
>> should be given here to enable operational diagnosis of any issues
>> that may result in temporal or persistent problems, where logging
>> and other actions should occur.  Some aspects of this might leverage
>> the OAM Framework efforts, although it appears much of the TRILL
>> work leaves this to the implementer.
>
>In the IETF context "silently discard" means that there is no
>on-the-wire message sent. It says nothing about whether or not
>counters are kept of such condition or errors are logged. A suggestion
>to log such events and/or keep such counters can be added.
>
>> 2. When using “Nickname” it would be useful to define the encoding
>> as an unsigned 16-bit integer, or just reference "as specified in S
>> 3.7 of RFC6325”.
>
>OK. Will add the reference.
>
>> 3. The inclusion of the “TLV” acronym in the "APPsub-TLV” TLV name
>> seems loose and redundant to me, as opposed to “APPsub TLV” or
>> similar.
>
>This comes from RFC 6823, Section 3.2, which says that sub-TLVs that
>go inside the GENAPP TLV "are refrred to as APPsub-TLVs".
>
>> 4. Inconsistent use of “Interface Address APPsub-TLV”, “IA
>> APPSub-TLV”, “Interface Address APP-subTLV”, and “AppsubTLV” makes
>> it seem like you’re talking about different things.
>
>OK - that should be made more consistent, probably standardizing on
>"IA APPsub-TLV".
>
>> 5. The use of “sub-sub-TLV” seems a bit loose and sloppy to me as
>> well, and should be cleaned up.  E.g., S 5.2 “IA Appsub-TLV
>> Sub-Sub-TLVs SubRegistry"
>
>You don't like "sub-sub-TLV"?
>
>Seems like, strictly speaking, you have IS-IS PDUs which contain
>TLVs. Then some TLVs can contain sub-TLVs. (The GENAPP TLV is the only
>one that occurs to me with a special name for its sub-TLVs, namely
>APPsub-TLVs.) and some sub-TLVs can contain a further nested level,
>which it seems to me to be precise and logical to call sub-sub-TLVs.
>(I am not aware of any requirement for any more deeper nesting in a
>use of IS-IS.) So, would you prefer that what are called sub-sub-TLVs
>in this document just be called "sub-TLVs" (which I agree they are)
>resulting in two different levels with the same name? While there
>might be some errors in their use in this draft, the mere use of
>APPsub-TLV and sub-sub-TLV for the two levels does not seem "loose and
>sloppy" to me...
>
>> 6. Only one of the “Figures” is labeled / captioned
>
>OK. All the principal figures should be labeled. (I don't think cases
>where there is a small, indented figure that just expands part of a
>principal figure and appears shortly after the principal figure need
>to be captioned.) So, the initial figures in Sections 3.1, 3.2, 3.3,
>and 3.4 would have Figures numbers and captions added.
>
>> 7. The use of “Address Sets” and “Address Sets Ends” makes it a bit
>> hard to read when used in sentences.  Perhaps an acronym for each,
>> or hyphenating/underscoring them would make it more readable.
>
>OK - I'll see what I can do.
>
>> 8. S 3.4 the 2-byte “Type” value in the diagram should be
>> “TOPOLOGY”, not “DATALEN”.
>
>Thanks for noticing this error.
>
>> 9. I noticed that Radia was a co-author until the last revision, and
>> now she doesn’t even exist in the Acknowledgements section.  While
>> no explanation is required here, I did find this a bit odd.
>
>I think her listing as an author was in error.
>
>> 10. IANA Considerations: Some guidance from the IANA folks on the
>> formatting of this section might be in order.  It’s not as clear as
>> it could be about what their instructions are here.
>
>There are some improvements that could be made. In inverse order,
>Section 5.3 looks fine. In Section 5.2, "Available" should be changed
>to "Unassigned" as that is the preferred IANA term. Section 5.1 is
>talking about assignments that have already happened and looks OK as
>far as the table of values goes; however, the material after the first
>sentence after that table seems inappropriate in an "IANA
>Considerations" section and should, perhpas, be in a new "Processing
>Address Sets" section.
>
>> 11. S 2: It’s unclear to me if the “Confidence” value of 255 “being
>> treated as if it was 254” is inline with RFC6325 S 4.8.1 guidance?
>
>The idea is that local configuration or learning should be able to
>override address reachability received through network messages.  Thus
>such information, when manually configured, defaults to have confidence
>255.
>
>RFC 6325 Section 4.8.1 just says that information learned via ESADI
>will have a confidence of from 0 to 254 but don't actually say what to
>do if it is recreived as 255. This is updated by Section 6.2 RFC 7357,
>1st paragraph, that makes it clear that a received value of 255 is
>just treated as if it was 254. Thus it is consistent with these prior
>RFCs to the IA APPsub-TLV draft to give this rule for handling the
>value 255 in the Confidence field of IP APPsub-TLVs.
>
>> 12. In general, I agree there appear to be no new Security
>> Considerations here.  I do not believe Asymmetry will be an issue
>> with the forged packet discard issue although some consideration of
>> this might be in order (or perhaps simply a reference to SAVI or
>> other work here).  I wonder if some consideration should be given to
>> broader disclosure of reachable layer 2 addresses here, but that
>> seems a bit reaching as well.
>>
>>
>> Nits:
>>
>> 1. Abstract & Introduction: s/by-pass/bypass/
>
>OK.
>
>> 2. S.2: s/Data Label is reachable from /Data Label are reachable/
>
>"... inteface ... is reachable ...", so I think "is" is correct but
>I'll see if I can re-word this sentence.
>
>> 3. A reference for the first use of AFN would be useful, perhaps to
>> the IANA registry.
>
>OK.
>
>> 4. Expressing TBD code points in [ ] brackets might help with
>> readability as well
>
>OK.
>
>> 5. S 3.2 “if the Length is 0 or 1 or less” — not sure the “or less"
>> is necessary?
>
>OK, the "or less" should be removed.
>
>Thanks,
>Donald
>===============================
> Donald E. Eastlake 3rd   [phone] (cell)
> 155 Beaver Street, Milford, MA 01757 USA
> [e-mail]