Subject: Re: [pim] AD Review of draft-ietf-pim-hierarchicaljoinattr-05

From: Stig Venaas <[email-masked]>
Date: Thu, 17 Mar 2016 14:56:26 -0700

Hi

I've updated the draft now, please see my comments inline. It
addresses most of your comments.

On Mon, Feb 29, 2016 at 3:29 PM, Stig Venaas <[email-masked]> wrote:
> Hi
>
> It's about time I try to pick this up.
>
> On Thu, Jan 21, 2016 at 10:00 AM, Alvaro Retana (aretana)
> <[email-masked]> wrote:
>> 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..).

It already says what to do with them, but I added an example now to
try to make it clearer.
As it is just a matter of how the attributes are encoded, the
processing is the same as before, except how you figure out which
attributes to process.

> The meaning and processing of attributes is basically the same as
> before. The only new part is how the attributes are included in the
> J/P. The last to paragraphs of section 3 try to explain what you do
> with the attributes etc.
>
>    The complete set of attributes that apply to a given source is
>    obtained by combining the message wide attributes, the attributes of
>    the group set that the source belongs to, and the source specific
>    attributes.  However, if the same attribute is specified at multiple
>    levels, then the one at the most specific level overrides the other
>    instances of the attribute.
>
>    Note that Join/Prune attributes are still applied to sources as
>    specified in [RFC5384].  This document does not change the meaning of
>    any attributes, it is simply a more compact way of encoding an
>    attribute when the same attribute and value applies to multiple
>    sources.
>
> Join attributes as currently defined are in the encoded source
> address. For say a given (S,G) you would have a set of attributes that
> apply to S. Now when you process, for this (S,G) the set of attributes
> for the source is the union of the set of attributes that apply to the
> entire message, the set of attributes for the group, and the
> attributes of the source. Except that if the same attribute is in
> multiple sets, the one from the more specific set is used. So group
> attribute would override a message wide attribute. And a source
> attribute override message and group wide attributes.
>
> Are you looking for some kind of example here? Maybe I can do some
> kind of pseudo code or an example algorithm.

Please see if the example I added is helpful. Or what else you would
like to see.

> Please see more comments inline.
>
>> 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:
>>
>> Replace the RFC4601 references for draft-ietf-pim-rfc4601bis.

Replaced with the new RFC.

>> 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

Fixed.

> It would be better to write "This document extends the encoding
> defined in RFC5384" I suppose.
>
>> 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.
>
> I can try to rewrite it a bit so there is less repetition.

I couldn't quite figure out how to improve this. I think it is
reasonable the way it is now.

>> 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?
>
> It is not just a renaming. The problem is that there is nothing in
> 4601 or 5384 saying that the meaning of the encoding types for
> unicast, group and source encodings are the same. 4601 just had type 0
> (native encoding) which applied to all. 5384 created a registry for
> source encoding, because that was what was needed for join attributes.
> But it makes sense to have a single encoding type registry for all of
> these. This section tries to justify that.
>
> This sentence is where I try to justify that:
>
>    While it
>    is possible to have the same encoding type value indicate different
>    encodings depending on whether it is a Unicast, Group or Source
>    address, it is simpler to have the same encoding type value indicate
>    the same encoding independent of where it is used.
>
> Perhaps it makes sense to remove the justification. I definitely don't
> want to give people the idea that encoding types are defined
> differently for each of these address encodings.

I removed that sentence.

>
>> 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).

Fixed.

> Perhaps it could say "In this document we make use of these encoded
> address fields to allow..."
>
>> Operation when multiple attributes are present in the hierarchy.
>
> The next to last paragraph of section 3 tries to explain this.
>
>> 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
>
> It's the other way around. Say one for the source overrides the same
> in the group. This means that if say all sources should have the same
> attributes except one, then for the one source you would specify the
> attribute unique to that source.
>
>> 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.

Ouch, I missed this. The rules are as in 5384. I guess I should point
that out. I'll wait for your comments on this version before I try to
address it.

> The rules regarding type 1 and type 0 above are exactly the same as in
> 5384, no change. Attributes apply to sources as before, the semantics
> of the attributes are not changing. When you process the message you
> process each of the sources separately, applying the attributes for
> that source. It's just that instead of requiring all the attributes
> for the source being encoded in the source address, you can put some
> of the attributes for the source in the upstream neighbor field or
> group field. The goal is simply that if you have an attribute that
> applies to all the sources, you don't have to encode it for every
> single source.
>
>> 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?

I tried to explain this by saying that you don't process the types and
validate until after you've formed the set of attributes that apply to
the source.

> The way I think it should be done is that you first build the set of
> attributes that apply to the source (which means taking the union of
> the 3 sets). Once you have the set you process the attributes exactly
> like you do today. This means that say RPF vector is in the upstream
> neighbor field, you will walk through all the sources. For each source
> you process the set. For each source you would do the validation and
> discard the attribute.
>
> Let me try to explain something here by example. Assume you have RPF
> vector TLV A in the upstream neighbor field and then for a source you
> have RPF vector TLV B. For that source you B with override A according
> to the rules specified. Then after you have built the set you do the
> validation and you might find that B should be discarded per 5496
> 3.3.2. This does not mean that TLV A should be used. I think it is
> crucial that you first construct the set of attributes that apply to
> the source, and then you process the set exactly as if the full set
> had been included in the encoded source address.

This is what I tried to explain.

>>
>> Disclaimer: I haven't looked at the other attribute RFCs, but I think that
>> other similar issues may come up.
>
> That is why I try to keep the semantics. That it is just a more
> efficient way of storing the attributes in a J/P message. The
> attributes still apply to the sources exactly as in 5384 and the
> individual attribute RFCs.
>
>> Related question:  are there cases where it doesn't make sense to include a
>> specific attribute at a specific level?  What happens then?
>
> A version of the draft had a mechanism for that, but it makes the
> processing fairly complex and I'm not convinced there is a need. In
> such a case you can always fall back to specifying the attribute for
> each of the sources that need it.
>
>> 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?
>
> In a way it is a different registry, or at least the scope is wider.
> But I agree it is better to say we rename it.

I say it is to be renamed now.

>> 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.
>
> Since the semantics are the same, it is just a more efficient encoding
> scheme, I do not see how there can be any new security concerns. I can
> try to say that and refer to 5384.

I added some text now, it mainly just explains in some detail why
there are no new considerations.

>>
>> Minor:
>>
>> 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).
>
> OK.
>
>> I think that the update is just related to the registries.
>
> That is true. I'll say that.

Done.

>> The hierarchical construct is just an extension, not an update.
>>
>> Section 1. (Introduction):  Please add a reference to
>> draft-ietf-pim-rfc4601bis when mentioning "Encoded-Unicast and
>> Encoded-Group".

Done.

>> 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.
>
> Agree.

Done.

>> Nits:
>>
>> Section 3 uses "we" several times.  3rd person would be better.
>> 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.
>
> OK, I'll change the language a bit and think about whether 2119
> keywords may be appropriate more places.

Done.

Stig

> Stig
>
>> Just an observation..
>> 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

---

From: "Alvaro Retana (aretana)" <[email-masked]>
Date: Mon, 21 Mar 2016 20:32:58 +0000

On 3/17/16, 5:56 PM, "Stig Venaas" <[email-masked]> wrote:

Stig:

Hi!  

...
>>> apply here or not...or if some do and others don'tŠor if some of the
>>>rules
>>> change.
>

I think that's it.

I'll start the IETF Last Call.  It would be great if you can update the
document in the next day or two.

Thanks!

Alvaro.