[pim] AD Review draft-ietf-pim-null-register-packing-11

[Alvaro Retana <[e-mail]>]

Dear authors:

Thank you for the work on this document.  In general, it looks a lot
better compared to when I read it last (-05) [1].  However, I am
repeating some of the comments I made back then -- see inline.

Thanks!

Alvaro.


[1] https://mailarchive.ietf.org/arch/msg/pim/NdNMqSwjwU9pvN0GGD2oXkpIuNI/



[Line numbers from idnits.]

...
15	Abstract
...
25	   This document defines a standard to send multiple Multicast source
26	   and group information in a single PIM Packed Null-Register message.
27	   We will refer to the new packed formats as the PIM Packed Null-
28	   Register format and PIM Packed Register-Stop format throughout the
29	   document.  This document also discusses interoperability between the
30	   PIM routers which do not understand the PIM Packed Null-Register
31	   format and routers which do understand it.

[minor] The "PIM Packed Null-Register message" is mentioned before it
is described.

s/in a single PIM Packed Null-Register message/in a single PIM message


[style nit] Please write in third person, not frst person.

s/We will refer to/This document refers to


[minor] Is it a "format" or a "message"?  I understand that the
message is formatted differently (it is new!), but the use of "format"
to (apparently) mean "message" is confusing.

s/PIM Packed Null-Register format and PIM Packed Register-Stop
format/PIM Packed Null-Register message and PIM Packed Register-Stop
message



...
85	1.  Introduction

87	   PIM Null-Registers are sent by the DR periodically for Multicast
88	   streams to keep the states active on the RP, as long as the multicast
89	   source is alive.  As the number of multicast sources increases, the
90	   number of PIM Null-Register messages that are sent also increases.
91	   This results in more PIM packet processing at the RP and the DR.

[] Suggestion:

s/
  PIM Null-Registers are sent by the DR periodically for Multicast streams to
  keep the states active on the RP, as long as the multicast source is alive.
/
  The DR periodically sends PIM Null-Registers to keep the state of existing
  multicast sources active on the RP.



93	   The control plane policing (COPP), monitors the packets that are
94	   processed by the control plane.  The high rate at which Null-
95	   Registers are received at the RP can lead to COPP drops of Multicast
96	   PIM Null-Register messages.  This draft proposes a method to
97	   efficiently pack multiple PIM Null-Registers [RFC7761] (Section 4.4)
98	   and Register-Stops [RFC7761](Section 3.2) into a single message as
99	   these packets anyway do not contain encapsulated data.

[minor] COPP:  Is there a reference for that?  A vendor-independent
reference would be ideal, but I don't think that this document needs
to mention COPP to justify the enhancement.


[nit] The section numbers are not needed.


[nit] s/these packets anyway do not contain encapsulated data/these
packets do not contain encapsulated data



...
118	2.  Packed Null-Register Capability

120	   A router (DR) can decide to pack multiple Null-Register messages
121	   based on the capability received from the RP as part of the PIM
122	   Register-Stop.  This ensures compatibility with routers that do not
123	   support processing of the new format.  The capability information can
124	   be indicated by the RP via the PIM Register-Stop message sent to the
125	   DR.  Thus a DR will switch to the new format only when it learns that
126	   the RP is capable of handling the PIM Packed Null-Register messages.

128	   Conversely, a DR that does not support the packed format can continue
129	   generating the PIM Null-Register as defined in [RFC7761]
130	   (Section 4.4).  To exchange the capability information in the
131	   Register-Stop message, the "Reserved" field can be used to indicate
132	   this capability in those Register-Stop messages.  One bit of the
133	   Reserved field is used to indicate the "packing" capability (P bit).
134	   The rest of the bits in the "Reserved" field will be retained for
135	   future use.

[major] In this section, please only specify the new capability
bit...the description of the operation should be in just one place.
That description is in §5.

Because the interoperability with routers that don't support this
extension is mentioned in the Abstract and Introduction, please add a
new section to explicitly talk about that (it can be just a few
sentences long), perhaps as a sub-section of the Operational
Considerations.

Suggestion>
   This section allocates a bit in the PIM Register-Stop message
   Flag Bits field for the RP to indicate its ability to receive
   PIM Packed Null-Register messages (Section 3), and send PIM
   Packed Register-Stop messages (Section 4).


[major] The common PIM header was updated by rfc8736, so the
discussion above about the Reserved field and the figure below are out
of date.

The new common header is this:

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |PIM Ver| Type  |   Flag Bits   |           Checksum            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

...so the discussion should be about the allocation of the Flag Bits.



...
149	   PIM Version, Type, Checksum, Group Address, Source Address:

151	      Same as [RFC7761] (Section 4.9.4)

[major] Suggestion:

   The fields in the PIM Register-Stop message are defined in Section
   4.9.4 of [RFC7761], and the common header in [RFC8736].


153	   P:

155	      Capability bit (flag bit 7) used to indicate support for the
156	      Packed Null-Register Capability

[minor] Suggestion:

   P: When set, it indicates the ability of the RP to receive
      PIM Packed Null-Register messages, and send PIM Packed
      Register-Stop messages.


158	3.  PIM Packed Null-Register message format

160	   PIM Packed Null-Register message format includes a count to indicate
161	   the number of Null-Register records in the message.

[nit] s/count/Count field


163	       0                   1                   2                   3
164	       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
165	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
166	      |PIM Ver| Type  |Subtype|  FB   |           Checksum            |
167	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
168	      |   Count       |              Reserved                         |
169	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
170	      |     Group Address[1]   (Encoded-Group format)                 |
171	      |     Source Address[1]  (Encoded-Unicast format)               |
172	      .                                                               .
173	      .                                                               .
174	      .                                                               .
175	      .                                                               .
176	      .     Group Address[N]                                          .
177	      |     Source Address[N]                                         |
178	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

180	              Figure 2: PIM Packed Null-Register message format

182	   PIM Version, Reserved, Checksum:

184	      Same as [RFC7761] (Section 4.9.3)

[major] Again, the updated common header definitions come from rfc8736.


186	   Type, SubType:

188	      The new packed Null-Register Type and SubType values TBD.
189	      [RFC8736]

[major] Type and SubType are not different values -- rfc8736.


191	   Count:

193	      The number of packed Null-Register records.  A record consists of
194	      a Group Address and Source Address pair.

[major] The length of the message is not explicit in the PIM message,
but it is inferred from the IP layer.  What should the receiver do if
the count indicates a number of records that would result in a message
with a length different from that is expected?



...
200	4.  PIM Packed Register-Stop message format

[] The comments from the last section apply here too.



...
241	5.  Protocol operation

243	   The following combinations exist -

[] The different combinations don't need to be mentioned.  Instead,
the specification for case 1 should be enough to cover all of them.

245	   1.  DR and RP both support the PIM Packed Null-Register and PIM
246	       Packed Register-Stop formats:

248	       *  As specified in [RFC7761], the DR sends PIM Register messages
249	          towards the RP when a new source is detected.

251	       *  An RP supporting this specification MUST set the P-bit in the
252	          corresponding Register-Stop messages.

[major]  Is receiving the  P-bit set in one Register-Stop message enough for the
DR to use the new messages for all future communication...OR...should the P-bit
first be set for specific (S,G)/(*,G) pairs before the DR can use the new
messages for them?  IOW, is the P-bit supposed to indicate per (S,G)/(*,G)
support or per RP?


[major] If setting the P-bit, does it have to be set in all messages
or can it be unset after the DR starts using the new messages?  The
text above talks about "the corresponding Register-Stop messages", so
it is not clear.

The text in §8 seems to indicate that the P-bit should always be set.  ??


254	       *  When a Register-Stop message with the P-bit set is received,
255	          the DR SHOULD send PIM Packed Null-Register messages
256	          (Section 3) to the RP instead of multiple Register messages
257	          with the N-bit set [RFC7761].

[major] When it is ok for the DR to not use PIM Packed Null-Register
messages?  IOW, why is this action recommended and not required?

I assume that even if supported, the DR may decide (by
configuration??) that if doesn't want to use the new messages.  If
this is the justification, then it seems to me that a better Normative
keyword would be "MAY" (instead of SHOULD).


[major] Once the DR starts using the new messages, does it have to
always use them?  Can a mixture of packed and unpacked messages be
used?


259	       *  The RP, after receiving a PIM Packed Null-Register message
260	          SHOULD start sending PIM Packed Register-Stop messages
261	          (Section 4) to the corresponding DR instead of individual
262	          Register-Stop messages.

[major] When it is ok for the RP to not use PIM Packed Register-Stop
messages?  IOW, why is this action recommended and not required?

I assume that even if supported, the RP may decide (by
configuration??) that if doesn't want to use the new messages.  If
this is the justification, then it seems to me that a better Normative
keyword would be "MAY" (instead of SHOULD).


[major] Once the RP starts using the new messages, does it have to
always use them?  Can a mixture of packed and unpacked messages be
used?



...
289	6.  Operational Considerations

291	   In case the network manager disables the packed capability at the RP,
292	   the router should not advertise the capability.  However, an
293	   implementation MAY choose to still parse any packed registers if they
294	   are received.  This may be particularly useful in the transitional
295	   period after the network manager disables it.

[major] The specification in §5 says that "An RP supporting this
specification MUST set the P-bit...".  That is an absolute requirement
that doesn't account for configuration.

Perhaps you want add this exception to the specification above: When
enabled/configured, the RP MUST set the P-bit...  (or something like
that)


297	7.  PIM Anycast RP Considerations

[] This seems to be an operational consideration.  It might be good to
make this a sub-section of §6.


299	   The PIM Packed Null-Register format should be enabled only if it is
300	   supported by all PIM Anycast RP [RFC4610] members in the RP set for
301	   the RP address.  This consideration applies to PIM Anycast RP with
302	   MSDP [RFC3446] as well.

[major] See the comments above about the (unqualified) requirement in
§5 to set the P-bit.


[major] s/by all PIM anycast RP members [RFC4610] in the RP set for
the RP address./by all the routers in the Anycast-RP set ([rfc4610]).


[major] If the DR receives the P-bit from an RP, should it assume that
the condition is met, or is there a way to verify?


[major] "should be enabled"  Is Normative language needed?   What is
the impact of a partial deployment?


304	8.  PIM RP router version downgrade

[] This seems to be an operational consideration.  It might be good to
make this a sub-section of §6.


306	   Consider a PIM RP router that supports PIM Packed Null-Registers and
307	   PIM Packed Register-Stops.  When this router downgrades to a software
308	   version which does not support PIM Packed Null-Registers and PIM
309	   Packed Register-Stops, the DR that sends the PIM Packed Null-Register
310	   message will not get a PIM Register-Stop message back from the RP.
311	   In such scenarios the DR can send an unpacked PIM Null-Register and
312	   check the PIM Register-Stop to see if the capability bit (P-bit) for
313	   PIM Packed Null-Register is set or not.  If it is not set then the DR
314	   will continue sending unpacked PIM Null-Register messages.

[major] "In such scenarios the DR can..."

How does the DR detect this situation?  Is there a timer (rfc7761 ??)
that can alert the DR of the lack of support?



...
324	10.  Security Considerations

[] Most of these considerations are the same as the ones already
covered in rfc7761.  There's no need to repeat them here.  Also,
additional requirements (for example, "standard methods to prevent
spoofing") should not be suggested as they would better belong in the
base specification.

Suggestion>

   The Security Considerations from [RFC7761] apply to this document.  In
   particular, the effect of forging a PIM Packed Null-Register or
   Register-Stop message would be amplified to all the records included and
   not a single source/group pair.



...
345	   There is another case where a spoofed Register-Stop can be sent to
346	   make it appear that is from the RP, and that the RP supports this new
347	   packed capability when it does not.  This can cause Null-Registers to
348	   be sent to an RP that doesnt support this packed format.  But
349	   standard methods to prevent spoofing should take care of this case.
350	   For example, uRPF can be used to filter out packets coming from the
351	   outside from addresses that belong to routers inside.

[] This last paragraph points at a different effect of a forged
packet: indicating support where it doesn't exist.

Suggestion>

   By forging a PIM Register-Stop message and setting the P-bit,
   an attacker can trigger the use of PIM Packed Null-Register
   messages by a DR thus creating unnecessary churn in the network.

Is that the impact?


353	11.  IANA Considerations

[major] This section should contain a specific request to IANA ("IANA
is asked to assign..."), along with the specific names of the
registries, and of the new bits/messages (!).


355	      This document requires the assignment of Capability bit (P-bit),
356	      flag bit 7 in the PIM Register-Stop message.

[major] A value can be suggested, not requested.


358	      This document requires the assignment of 2 new PIM message types
359	      for the PIM Packed Null-Register and PIM Packed Register-Stop.

[major] Please include a short table that can be used my IANA to fill
out the registry information.  Note that both requests come from the
same registry.



...
395	13.2.  Informative References

397	   [RFC3446]  Kim, D., Meyer, D., Kilmer, H., and D. Farinacci, "Anycast
398	              Rendevous Point (RP) mechanism using Protocol Independent
399	              Multicast (PIM) and Multicast Source Discovery Protocol
400	              (MSDP)", RFC 3446, DOI 10.17487/RFC3446, January 2003,
401	              <https://www.rfc-editor.org/info/rfc3446>.

[major] This reference should be Normative.

[EoR -11]