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

From: "McPherson, Danny" <[email-masked]>
Date: Wed, 4 May 2016 14:27:09 +0000

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.

Minor Issues:
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.
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”.
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.
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.
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"
Only one of the “Figures” is labeled / captioned
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.
S 3.4 the 2-byte “Type” value in the diagram should be “TOPOLOGY”, not “DATALEN”.
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.
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.
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?
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:

Abstract & Introduction: s/by-pass/bypass/
S.2: s/Data Label is reachable from /Data Label are reachable/
A reference for the first use of AFN would be useful, perhaps to the IANA registry.
Expressing TBD code points in [ ] brackets might help with readability as well
S 3.2 “if the Length is 0 or 1 or less” — not sure the “or less" is necessary?

---

From: Donald Eastlake <[email-masked]>
Date: Thu, 5 May 2016 12:02:15 -0400

Hi Danny,

Thanks for your comments. See below.

On Wed, May 4, 2016 at 10:27 AM, McPherson, Danny
<[email-masked]> 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.
>
> 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:
>

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   +1-508-333-2270 (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [email-masked]

---

From: "McPherson, Danny" <[email-masked]>
Date: Thu, 5 May 2016 18:41:07 +0000

I’m comfortable with your stated intentions here Donald.

Thanks for the prompt response,

-danny