Subject: [pim] AD Review of draft-ietf-pim-null-register-packing-05

From: Alvaro Retana <[email-masked]>
Date: Fri, 26 Jun 2020 14:00:49 -0700

Dear authors, shepherd, chairs:

I'm not sure exactly where to start...

This document needs a significant amount of work to be ready.  The
language is not consistent with rfc7761, the packet formats are not in
line with the latest definitions, the specification of the new
messages and the operation is incomplete (or non-existent), the IANA
Considerations section doesn't provide clear instructions, there is no
Security Considerations section (!), obsolete and unused references...
<sigh>   Also, checking the mail archive, there wasn't significant
discussion, and probably only two people looked at the content...

Sorry for being so direct, but I don't know how else to describe the
document and my disappointment with it.  I am including my review
notes below.

I am returning this draft to the WG for a rewrite and proper review.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
9	                       PIM Null register packing
10	                draft-ietf-pim-null-register-packing-05

[major] s/Null register/Null-Register/g
That is the terminology defined in rfc7761.

12	Abstract

14	   In PIM-SM networks PIM registers are sent from the first hop router
15	   to the RP (Rendezvous Point) to signal the presence of Multicast
16	   source in the network.  There are periodic PIM Null registers sent
17	   from first hop router to the RP to keep the state alive at the RP as
18	   long as the source is active.  The PIM Null register packet carries
19	   information about a single Multicast source and group.  This document
20	   defines a standard to send multiple Multicast source and group
21	   information in a single pim Null register packet and the
22	   interoperability between the PIM routers which do not understand the
23	   packet format with multiple Multicast source and group details.

[major] s/PIM registers/PIM Register messages/g
Again, rfc7761 terminology!

[nit] s/sent from/sent by

[major] "first hop router" is used everywhere in the document.  While
that is technically not incorrect, the DR is specifically the node
taking the Register-related actions.  Please either s/FHR/DR/g or
clarify the role of the DR when defining FHR.  Personally, I would
prefer the use of DR (to be consistent with rfc7761).

[minor] s/RP (Rendezvous Point)/Rendezvous Point (RP)

[nit] s/of Multicast source/of a Multicast source

[nit] s/Multicast source/multicast source/g

[nit] s/pim/PIM/g

...
76	1.  Introduction

78	   PIM Null registers are sent by First hop routers periodically for
79	   Multicast streams to keep the states active on the RP as long as the
80	   Multicast source is alive.  As the number of multicast sources
81	   increases, the number of PIM Null register packets that are sent
82	   increases at a given time.  This results in more PIM packet
83	   processing at RP and FHR.  The control plane policing (COPP),
84	   monitors the packets that gets processed by the control plane.  Due
85	   to the high rate at which Null registers are received at the RP, this
86	   can lead to COPP drops of Multicast PIM Null register packets.  This
87	   draft proposes a method to efficiently pack multiple PIM Null
88	   registers and register stop into a single message as these packets
89	   anyway don't contain data.  The draft also proposes interoperability
90	   with the routers that do not understand the new packet format.

[nit] s/sent by First hop routers/sent by the DR

[nit] s/on the RP as long/on the RP, as long

[nit] s/are sent increases at a given time/are sent also increases

[nit] s/at RP and FHR/at the RP and the DR

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

[major] s/register stop/Register-Stop/g   rfc7761 terminology!

[minor] s/don't contain data/don't contain encapsulated data

[minor] "The draft also proposes interoperability with the routers
that do not understand the new packet format."  It doesn't "propose"
(that would imply potential changes), it explains the backwards
compatibility.

92	1.1.  Conventions Used in This Document

94	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
95	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
96	   document are to be interpreted as described in RFC 2119 [RFC2119].

[major] Use the rfc8174 template.

98	1.2.  Terminology

100	   RP:  Rendezvous Point

102	   RPF:  Reverse Path Forwarding

104	   SPT:  Shortest Path Tree

106	   FHR:  First Hop Router, directly connected to the source

[major] Please expand the definition to explain/cover the role of the DR.

108	   LHR:  Last Hop Router, directly connected to the receiver

[minor] RFP, SPT and LHR are not used.  RP is expanded elsewhere.  The
only definition left would be FHR; you may want to simply include that
explanation in §1.  I recommend replacing it for DR.

110	2.  PIM Register Stop format with capability option

[minor] Perhaps name this section "Packed-Register Capability",
because that is what is being defined, and it indicates the RP's
ability to receive packed Null-Register messages and send packed
Register-Stop messages.

112	   A router (FHR) can decide to pack multiple Null registers based on
113	   the capability received from the RP as part of Register Stop.  This
114	   ensures compatibility with routers that don't support processing of
115	   the new format.  The capability information can be indicated by the
116	   RP via the PIM register stop message sent to the FHR.  Thus a FHR
117	   will switch to the new format only when it learns RP is capable of
118	   handling the packed Null register messages.  Conversely, a FHR that
119	   doesn't support the new format can continue generating the PIM Null
120	   register the current way.  To exchange the capability information in
121	   the Register Stop message, the "reserved" field can be used to
122	   indicate this capability in those register stop messages.  One bit of
123	   the reserved field is used to indicate the "packing" capability (P
124	   bit).  The rest of the bits in the "Reserved" field will be retained
125	   for 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.

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

127	   Figure 1: PIM Register Stop message with capability option

[minor] Put the figure number/description afterwards.

129	    0                   1                   2                   3
130	    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
131	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
132	   |PIM Ver| Type  |P|  Reserved   |           Checksum            |
133	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
134	   |             Group Address (Encoded-Group format)              |
135	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
136	   |            Source Address (Encoded-Unicast format)            |
137	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[major] The common header was changed by rfc8736.  Please use that format.

139	   PIM Version, Reserved, Type, Checksum, Group Address, Source Address
140	       Same as RFC 7761 (Section 4.9.4)

[minor] s/Version, Reserved, Type/Version, Type

142	   P   Capability bit used to indicate support for Packed Null Register

[major] To be in line with rfc8736:

Suggestion>
   Capability bit (flag bit 7) used to indicate support for the
   Packed-Register Capability

[major] Add a request in the IANA Considerations section.

144	3.  New PIM Null register message

[major] rfc7761 already defined a Null-Register...to avoid confusion,
use "PIM Packed Null-Register Message" instead (or something like
that).  No need to say "new".

146	   New PIM Null register message format includes a count to indicate the
147	   number of Null register records in the message.

149	   Figure 2: New PIM Null Register message format

[minor] Put the figure number/description afterwards.

150	       0                   1                   2                   3
151	       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
152	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
153	      |PIM Ver| Type  |SubType| Rsvd  |           Checksum            |
154	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
155	      |   Count       |              Reserved2                        |
156	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
157	      |     Group Address[1]  (Encoded-Group format)                  |
158	      |     Source Address[1]  (Encoded-Unicast format)               |
159	      .                                                               .
160	      .                                                               .
161	      .                                                               .
162	      .                                                               .
163	      .     Group Address[N]                                          .
164	      |     Source Address[N]                                         |
165	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[major] rfc8736 uses "FB" (flag bits) instead of Rsvd.

[minor] s/Reserved2/Reserved

167	      PIM Version, Reserved, Checksum
168	          Same as RFC 7761 (Section 4.9.3)

[major] rfc7761 says that the only exception to calculating the
checksum over the entire PIM message is the Register where the
multicast data packet is excluded.  This new message doesn't
encapsulate a data packet.  Why is the checksum calculation different
than for other "normal" messages?  [IOW, a better reference for the
Checksum is §4.9.]

170	      Type, SubType
171	          The new packed Null Register Type and SubType values TBD

[major] Use the rfc8736 notation (type.subtype) in the definition and
indicate separate TBD values (TBD1.TBD2).

[major] Add a matching request in the IANA Considerations section.

173	      Count
174	          The count of the number of packed Null register records.
175	          A record consists of Group and Source Address

[minor] Suggestion>
   The number of packed Null-Register records.  A record consists of 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?

177	      Group Address
178	          IP address of the Multicast Group

180	      Source Address
181	          IP Address of the Multicast Source

[minor] Point at the definition in rfc7761/§4.9.4 for these entries
(just like you did in the previous section).

183	4.  New PIM Register Stop message format

[major] rfc7761 already defined a Register-Stop message...to avoid
confusion, use "PIM Packed Register-Stop Message" instead (or
something like that).  No need to say "new".

185	   The new PIM register stop is message includes a count to indicate the
186	   number of records that are present in the message.

188	   Figure 3: New PIM Register Stop message format

[minor] Put the figure number/description afterwards.

189	       0                   1                   2                   3
190	       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
191	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
192	      |PIM Ver| Type  |SubType| Rsvd  |           Checksum            |
193	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
194	      |     Count     |          Reserved2                            |
195	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
196	      |     Group Address[1]  (Encoded-Group format)                  |
197	      |     Source Address[1]  (Encoded-Unicast format)               |
198	      .                                                               .
199	      .                                                               .
200	      .                                                               .
201	      .                                                               .
202	      .     Group Address[N]                                          .
203	      |     Source Address[N]                                         |
204	      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[major] rfc8736 uses "FB" (flag bits) instead of Rsvd.

[minor] s/Reserved2/Reserved

206	      PIM Version, Reserved, Checksum
207	          Same as RFC 7761 (Section 4.9.3)

[major] You should point at §4.9.4 -- not just because that is where
the Register-Stop message is defined, but also because of the
Checksum.

209	      Type
210	          The new Register Stop Type and SubType values TBD

[major] Use the rfc8736 notation (type.subtype) in the definition and
indicate separate TBD values (TBD3.TBD4).

[major] Add a matching request in the IANA Considerations section.

212	      Count
213	          The count of the number of packed register stop records.
214	          A record consists of Group and Source Address

[minor] Suggestion>
   The number of packed Register-Stop records.  A record consists of 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?

216	      Group Address
217	          IP address of the Multicast Group

219	      Source Address
220	          IP Address of the Multicast Source

[minor] Point at the definition in rfc7761/§4.9.4 for these entries.

222	5.  Protocol operation

[major] This section sounds like a description...but what is needed is
a specification.

223	   The following combinations exist -
224	   FHR and RP both support the new PIM Register formats -
225	     a. FHR sends the PIM register towards the RP when a new source is
226	        detected
227	     b. RP sends a modified register stop towards the FHR that includes
228	        capability
229	        information by setting the P bit (Figure 2)
230	     c. Based on the receipt of new Register Stop, FHR will
231	        start packing of Null registers using the new packed register
232	        format (Figure 1)
233	     d. RP processes the new Null register message and can generate new
234	        register Stop messages by packing multiple S,Gs towards the same
235	        FHR (Figure 3)

237	   FHR supports but RP doesn't support new PIM Register formats-
238	     a. FHR sends the PIM register towards the RP
239	     b. RP sends a normal register stop without any capability
240	        information
241	     c. FHR then sends Null registers in the old format

243	   RP supports but FHR doesn't support the new PIM Register formats-
244	     a. FHR sends the PIM register towards the RP
245	     b. RP sends a modified register stop towards the FHR that includes
246	        capability information
247	     c. Since FHR doesn't support the new format, it sends Null
248	        registers in the old format

[] For example, I would expect the specification to be something like this:

   As specified in [rfc7761], the DR sends Register messages to the RP when a
   new source is detected.  An RP supporting this specification SHOULD set
   the P-bit in the corresponding Register-Stop messages.

   When a Register-Stop message with the P-bit set is received, the DR MAY
   send Packed Null-Register messages (Section 3) to the RP instead of
   multiple Register messages with the N-bit set ([rfc7761]).  The RP, after
   receiving a Packed Null-Register message MAY start sending Packed
   Register-Stop messages (Section 4) to the corresponding DR instead of
   individual Register-Stop messages.

   A DR that doesn't support this specification will ignore the new P-bit, if
   received ([rfc7761]).  An RP or DR, which doesn't support this
   specification, will discard any of the new PIM messages defined in this
   document ([rfc7761]).  These two actions result in backwards
   compatibility: either a DR or RP supporting this specification can
   interact with nodes that do not support it, using the mechanisms defined
   in [rfc7761].

[major] I wrote this sample specification from my understanding of the
text, but I do have several questions:

(1) The P-bit indicates that the RP supports this document.  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?

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

(3) Even if the RP supports the new functionality, it doesn't have to set the
    P-bit, right?

(4) If the DR receives the P-bit, can it still not use the new messages?

(5) Once the DR/RP starts using the new messages, does it have to always use
    them?  Can a mixture of packed and unpacked messages be used?

(6) If an RP that supports this specification, but didn't set the P-bit,
    receives packed messages from the DR, what should it do?  Accept them or
    ignore them as if they were unrecognized?

(7) Same question for a DR receiving packed messages from the RP before
    sending packed messages itself, or even receiving the P-bit...

[minor] A diagram of the sample exchanges above would be great.

250	6.  PIM Anycast RP considerations

252	   The new PIM register format should be enabled only if its supported
253	   by all PIM anycast RP members in the RP set for the RP address.

[minor] Please add a pointer to rfc4610.

[major] s/new PIM register format/functionality in this specification/g

[major] s/by all PIM anycast RP members 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?

[minor] I'm assuming this section really is directed to the operator,
right?  Please say so.

[major] "should be enabled"  I know this "should" is not an rfc2119
keyword -- is Normative language needed?   What is the impact of a
partial deployment?

255	7.  PIM RP router version downgrade

257	   Consider a PIM RP router that downgrades to a software version which
258	   does not support the PIM register packing and was previously
259	   supporting the PIM register packing.  The FHR that sends the packed
260	   PIM register message will not get a PIM register stop message back.
261	   In such scenarios the FHR can send an unpacked PIM register and check
262	   the PIM register stop to see if the capability option for packed
263	   register is set or not.  If its not set then the FHR will continue
264	   sending unpacked PIM register messages.

[minor] This section is related to the questions I had above about the
operation.  Consider merging the two sections.  Note that the reason
(downgrade, change in configuration, etc.) doesn't seem to really
matter.

[major] "the DR can send an unpacked PIM register..."  How long should
the DR wait?  Is this optional?

266	8.  Fragmentation consideration

268	   When building a packed pim register message using the new PIM
269	   register format, a router should include as many records as possible
270	   based on the path MTU towards RP if path MTU discovery is done, else
271	   the number of records should be limited to the MTU of the outgoing
272	   interface.

[major] rfc7761 required PMTUD for IPv6 when sending Register
messages.  Should a similar requirement exist here?

274	9.  IANA Considerations

276	   This document requires the assignment of 2 new PIM message types for
277	   the packed pim register and pim register stop.

[major] Besides the 2 type.subtype assignments, a request should be
made for the P-bit.

The request should include the specific names of the registries, and
of the new bits/messages (!).  Please include a table making reference
to TBDx, as mentioned above.

...
286	11.1.  Normative References

288	   [I-D.ietf-pim-reserved-bits]
289	              Venaas, S. and A. Retana, "PIM Message Type Space
290	              Extension and Reserved Bits", draft-ietf-pim-reserved-
291	              bits-04 (work in progress), September 2019.

[major] Outdated reference: draft-ietf-pim-reserved-bits has been
published as RFC8736

[major] This reference is unused.  Please use it!

...
309	11.2.  Informative References

311	   [RFC3973]  Adams, A., Nicholas, J., and W. Siadak, "Protocol
312	              Independent Multicast - Dense Mode (PIM-DM): Protocol
313	              Specification (Revised)", RFC 3973, DOI 10.17487/RFC3973,
314	              January 2005, <https://www.rfc-editor.org/info/rfc3973>.

[minor] Unused reference.