[pim] AD Review of draft-ietf-pim-hierarchicaljoinattr-05

["Alvaro Retana (aretana)" <[e-mail]>]

Hi!

I just finished reading this document.  I think that the description of the general idea and of the operation when attributes are in fact included at different levels should be made a lot clearer than what it is now.  The document only presents an idea of how to include attributes at different levels, but not what to do with them once they're in.  I would have liked to see sections with more details about how to process attributes to be sent and/or received (similar to RFC5384 and 5496..).

Clarifying the text, including the IANA Considerations, and updating the RFC4601 reference are all improvements that I would like to see in the document before starting the IETF Last Call.  Please see my detailed comments below.

Thanks!

Alvaro.


Major:

  1.  Replace the RFC4601 references for draft-ietf-pim-rfc4601bis.
  2.  General Idea.  After reading the text a couple of times, I think I got it! :-)  If the new "Hierarchical Join/Prune Attribute Hello Option" is included in the PIM Hello, then it means that the PIM Attribute from RFC5384 can now also be encoded in the Upstream Neighbor Address and the Group Address.  Right?  [Note that some of the comments below are really minor on their own, but add up to not clearly explaining what is intended, so including a succinct explanation would go a long way.]
     *   "This document provides a hierarchical way of encoding attributes and their values in a Join/Prune message…"  This document doesn't define a new encoding, the encoding from RFC5384 is still used.  This document does define a mechanism to include attributes in a hierarchical may…
     *   "This document extends this by specifying the same encoding type also for Encoded-Unicast and Encoded-Group formats."  "This" what?   This document allows the use of type 1…so that Encoded-…may contain a sequence of attributes…  The last paragraph in the Introduction is somewhat redundant, but it does a much better job at explaining what is going on.
     *   Section 4. (PIM Address Encoding Types)
        *   I'm not sure what the purpose of this section is (not to mention some of the speculative language: "it is possible", "one could have").  It seems to rehash information that is already in RFC5384 and draft-ietf-pim-rfc4601bis (BTW, it wouldn't hurt to put a reference related to type 0).  The only conclusion seems to be the renaming of the registry.  Is that the purpose, to justify the renaming of the registry?
        *
        *   Section 3. (Hierarchical Join/Prune Attribute Definition) "In this document we make use of this to allow Join/Prune attributes in each of these addresses, using the encoding in Section 4."  What is "this"?  I don't see any encoding defined in Section 4 (or anywhere else).
  3.  Operation when multiple attributes are present in the hierarchy.
     *   The hierarchy seems straight forward: attributes in the Upstream Neighbor Address take precedence over ones in the Group Address (maybe use "Multicast Group Address n" to be consistent with the packet), which then take precedence over the source ones (Encoded-Source Address).  That is fine, but this sentence in the last paragraph seems to imply that the attributes in the source are always applied (regardless of the hierarchy): "Note that Join/Prune attributes are still applied to sources as specified in [RFC5384]."
     *   I'm assuming that the rules specified in RFC5384 for the Encoded-Source Address apply to the Upstream and Group addresses in this document.  For example, RFC5384 says that a "type 1 Encoded-Source Address MUST contain at least one Join Attribute.  The way to specify that there are no Join Attributes for a particular tree is to use the type 0 Encoded-Source Address."  Please explicitly indicate whether the procedures in RFC5384 apply here or not...or if some do and others don't…or if some of the rules change.
     *   Should the behavior for higher level attributes affect the lower levels?  One of the reasons I'm asking is because RFC5496 (The RPF Vector TLV) says in Section 3.3.2. (Processing a Received Vector Attribute) that "a received PIM Join that contains a Vector Attribute, a router MUST first check to see if the Vector IP address is one of its own IP addresses.  If so, the Vector Attribute is discarded, and not passed further upstream."  If this attribute is included so that it affects all sources, should all other Vector Attributes be discarded from the lower levels?
        *   Disclaimer: I haven't looked at the other attribute RFCs, but I think that other similar issues may come up.
        *   Related question:  are there cases where it doesn't make sense to include a specific attribute at a specific level?  What happens then?
  4.  IANA Considerations
     *   The Introduction says that this "document defines a new IANA registry for PIM encoding types", but the IANA Considerations Section seems to just be renaming the existing registry.  So it is not a new registry, just a new name.
     *   "the more correct name Join/Prune attributes"  Shouldn't this result in renaming the "PIM Join Attribute Types" registry too?
  5.  Security Considerations.  Can including an attribute at a specific level (maybe where it doesn't make sense — think not just of current attributes) cause an unintended consequence?  Knowing that the attributes can be changed (from RFC5384), what are the risks?  You should at least point to the security considerations in RFC5384, even though that doesn't say much.

Minor:

  1.  The Abstract should have a sentence about the update to RFC5384.  Something simple: "This document updates RFC5384 by…"  Also, similar text should also appear in the Introduction (with maybe some more details).  The 3rd paragraph in the Introduction tries, but it is not crisp enough (see above).
     *   I think that the update is just related to the registries.
     *   The hierarchical construct is just an extension, not an update.
  2.  Section 1. (Introduction):  Please add a reference to draft-ietf-pim-rfc4601bis when mentioning "Encoded-Unicast and Encoded-Group".
  3.  Hello Option.  The use of this term is not consistent.  Sometimes it is referred to as just "Hello Option", but the full name seems to be "Hierarchical Join/Prune Attribute Hello Option".  This is specially important in Section 7. (IANA Considerations), where the registry should be specifically called out too.

Nits:

  1.  Section 3 uses "we" several times.  3rd person would be better.
  2.  I'm a little surprised that only one RFC2119 keyword is used.  I'm not suggesting that you add more, as the text can be made clear without them.  Just an observation..
  3.  I am also surprised at the minimal changes between draft-venaas-pim-hierarchicaljoinattr-00 and the current version [1] and how little discussion happened on the list.  Again, just an observation.

[1] https://www.ietf.org/rfcdiff?url1=draft-venaas-pim-hierarchicaljoinattr-00&url2=draft-ietf-pim-hierarchicaljoinattr-05