Re: [pim] AD Review of draft-ietf-pim-assert-packing-05

[Alvaro Retana <[e-mail]>]

FYI - A copy of the review is in the archive:
https://mailarchive.ietf.org/arch/msg/pim/Cp4o5glUFge2b84X9CQMwCWZjAk/

Alvaro.

On October 12, 2022 at 4:47:20 PM, Alvaro Retana ([e-mail])
wrote:



Dear authors:

I just finished reading this document.  It needs significant work!  I have
a couple of high-level points to make (backed by specific comments
in-line):

(1) I'm not sure how impactful this extension will be, even the Simple
Type.
    The Aggregation Type requires new processing that is not clearly
specified.

(2) The PIM message type extension in rfc8736 has not been applied
correctly
    (even after Stig's comments [1]).  In general, I believe that a single
    type.subtype should be requested with the formats identified by the
FBs.

    Note that besides requesting a new type.subtype, rfc8736 also opens the
    possibility of using the Flag Bits in the Assert message to
differentiate
    the formats.  Was this option considered?

(3) Even though it is clear that a new message is defined, the text uses
    "assert message" to refer to both the existing (rfc7761) and the new
    message, resulting in confusion for the reader.

(4) What about AssertCancel?  How would like look like with the new
formats?
    Please specify that as well.


I will wait for the comments to be addressed before starting the IETF Last
Call.

Thanks!

Alvaro.

[1] https://mailarchive.ietf.org/arch/msg/pim/u9PplGrV0CbLbJE1bQjzny5T2-Y


[Line numers from idnits.]

...
51 Abstract

53   In PIM-SM shared LAN networks, there is typically more than one
54   upstream router. When duplicate data packets appear on the LAN from
55   different routers, assert packets are sent from these routers to
56   elect a single forwarder. The PIM assert packets are sent
57   periodically to keep the assert state. The PIM assert packet carries
58   information about a single multicast source and group, along with
59   the metric-preference and metric of the route towards the source or
60   RP. This document defines a standard to send and receive information
61   for multiple multicast sources and groups in a single PIM assert
62   message. This can be particularly helpful when there is   traffic
63   for a large number of multicast groups.

[minor] Please expand acronyms in the abstract:

PIM is well-known, but PIM-SM isn't: "PIM - Sparse Mode (PIM-SM)".
Rendezvous Point (RP)


[major] s/assert packets/Assert messages/g
That is the terminology in rfc7761.


[major] s/assert message/Assert message/g


[] There are some details in the Abstract that are not needed at this
point.

Suggestion>
   In PIM - Sparse Mode (PIM-SM) shared LAN networks, there is
   typically more than one upstream router.  PIM Assert messages
   are used to elect a single forwarder for a single multicast
   source and group.  This document defines a mechanism to send
   and receive information for multiple multicast sources and groups
   in a single PIM message.



...
97 1. Introduction
...
108   This document defines a standard to send and receive information for
109   multiple multicast sources and groups in a single PIM assert
110   message. It can efficiently pack multiple PIM assert messages into a
111   single message and reduce the processing pressure of   the PIM
112   routers. This can be particularly helpful when there is   traffic
113   for a large number of multicast groups.

[nit] s/defines a standard/defines a mechanism


[minor] s/multiple multicast sources and groups in a single PIM assert
message/multiple multicast sources and groups in a single PIM message

The new message is not the same as the current Assert message. ;-)


[nit] s/of   the PIM...is   traffic/of the PIM...is traffic
Extra spaces.



...
137 2. Use Cases

[] The specific use cases mentioned are very generic and vague -- they
don't provide any details.  IMO, the only section that provides useful
background is the Summary.  Suggestion: Leave only the Summary section in
the main text of the draft and, if you want to keep them, move the specific
use cases to an Appendix.



139   PIM Assert will happen in many services where multicast is used and
140   not limited to the examples described below.

[nit] s/PIM Assert will happen/PIM Assert messages are used



142 2.1. Enterprise network

144   When an Enterprise network is connected through a layer-2 network,
145   the intra-enterprise runs layer-3 PIM multicast. The different sites
146   of the enterprise are equivalent to the PIM connection through the
147   shared LAN network. Depending upon the locations and amount of
148   groups there could be many asserts on the first hop routers.

[nit] s/first hop/first-hop/g



...
158 2.3. Financial Services

160   Financial services extensively rely on IP Multicast to deliver stock
161   market data and its derivatives, and current multicast solution PIM
162   is usually deployed. As the number of multicast flows grows, there
163   are many stock data with many groups may result in many PIM asserts
164   on a shared LAN network from publisher to the subscribers.

[nit] s/flows grows/flows grow



166 2.4. IPTV broadcast Video

168   PIM DR and BDR deployments are often used in host-side network for
169   IPTV broadcast video services. Host-side access network failure
170   scenario may be benefitted by assert packing when many groups are
171   being used. According to [RFC7761] the DR will be elected to forward
172   multicast traffic in the shared access network. When the DR recovers
173   from a failure, the original DR starts to send traffic, and the
174   current DR is still forwarding traffic. In the situation multicast
175   traffic duplication maybe happen in the shared access network and
176   can trigger the assert progress.

[nit] rfc7761 doesn't define a BDR.



...
185 2.6. Summary
...
193   Moreover as the multicast services become widely deployed, the
194   number of multicast entries increases, and a large number of assert
195   messages may be sent in a very short period when multicast data
196   packets trigger PIM assert processing in the shared LAN networks.
197   The   PIM routers need to process a large number of PIM assert small
198   packets in a very short time. As a result, the device load is very
199   large. The assert packet may not be processed in time or even is
200   discarded, thus extending the time of traffic duplication in the
201   network.

[nit] s/The   PIM/The PIM


[nit] s/or even is discarded/or even discarded



203   Additionally, future backhaul, or fronthaul, networks may want to
204   connect L3 across an L2 underlay supporting Time Sensitive Networks
205   (TSN). The infrastructure may run DetNet over TSN. These transit L2
206   LANs would have multiple upstreams and downstreams. This document is
207   taking a proactive approach to prevention of possible future assert
208   issues in these types of environments.

[] This last paragraph is ok, but not needed.



210 3. Solution

212   The change to the PIM assert includes two elements: the PIM assert
213   packing hello option and the PIM assert packing method.

[minor] You're introducing 3 elements: the Hello Option, the new Assert
Packing message, and the mechanism itself.



215   There is no change required to the PIM assert state machine.
216   Basically a PIM router can now be the assert winner or loser for
217   multiple packed (S, G)'s in a single assert message instead of one
218   (S, G) assert at a time.

[] "There is no change required to the PIM assert state machine."

See the comments in §4.3/§4.4,


[minor] s/state machine/state machine [rfc7761]


[major] "a PIM router can now be the assert winner or loser for multiple
packed (S, G)'s"

To be clear, each Assert Record is evaluated independently, right?  IOW, a
specific PIM router may not be the designated forwarder for all the Assert
Records in the message.

Please clarify the operation.


[major] rfc7761 talks about "(S,G) something" (state, join, etc.).  Here,
you're really referring not just to the (S,G) state, but to the Assert
Record (as introduced later).  Using "(S,G)" (and "(*,G)") without a
descriptor attached is not correct.  Please reword.

My suggestion is to use "Assert Record", for example:

   ...a PIM router can become the designated forwarder for multiple
   Assert Records (Section 4)...


Also, rfc7761 uses "source-specific assert" (or Assert(S,G) or (S,G)
Assert) and "group-specific assert" (or Assert(*,G) or (*,G) Assert).
Please use existing terminology when possible.  Please pick one and use it
consistently in this document.

It would be ideal if the terminology section explicitly pointed out that
the reader is expected to be familiar with the terminology in rfc7761.


[major] "assert message"

The term "assert message" is used throughout to refer to both the existing
Assert message (from rfc7761) and the new one defined here.  Please use a
different name for the new message.  I recommend against "new Assert
message" (which is used 3 times) -- "PIM Assert Packing message" comes to
mind.


[nit] s/(S, G)/(S,G)/g
The more compact notation is what rfc7761 uses.



220 3.1. PIM Assert Packing Hello Option

222   The newly defined Hello Option is used by a router to negotiate the
223   assert message packing capability. Assert packing can only be used
224   when all PIM routers, in the same shared LAN network, support this
225   capability. This document defines two packing methods. One method is
226   a simple merge of the original messages and the other is to extract
227   the common message fields for aggregation.

[minor] s/newly defined Hello Option/PIM Assert Packing Hello Option
(Section 4.1)


[minor] "used by a router to negotiate"

The capability is not negotiated, it is advertised.


[minor] "two packing methods"

Please (in the next sentence) include a forward reference to where the
methods are defined.



229 3.2. PIM Assert Packing Simple Type

231   In this type of packing, as described in [RFC7761], the assert
232   message body is used as a record. The newly defined assert message
233   can carry multiple assert records and identify the number of
234   records.

[major] "packing, as described in [RFC7761]"

Packing is not described in rfc7761.  But I think you meant that the Assert
message is.

Suggestion>
   In this type of packing the Assert message body [rfc7761] is
   used as an Assert Record, as described in Section 4.2.



236   This packing method is simply extended from the original assert
237   packet, but, because the multicast service deployment often uses a
238   small number of sources and RPs, there may be a large number of
239   assert records with the same metric preference or route metric
240   field, which would waste the payload of the transmitted message.

[major] The method is not explained.  There's a hint in the last paragraph
of the last section ("simple merge of the original messages"), but I
wouldn't call that a specification.  You don't need much, just a sentence
or two that is specific on what to do -- a pointer to the format in §4.2
would also help.


[minor] This last paragraph doesn't seem to belong in this section as it is
really a justification for the next one.



242 3.3. PIM Assert Packing Aggregation Type

244   When the source or RP addresses, in the actual deployment of the
245   multicast service, are very few, this type of packing will combine
246   the records related to the source address or RP address in the
247   assert message.

[minor] s/, in the actual deployment of the multicast service,/


[minor] "are very few"

Does this method only apply to a "few" sources/RPs?  What is a few?

Suggestion>
   This type of packing combines the records related to a source
   or RP in the Assert message.

Also, note that the concept of a record is not introduced until later...



249   * A (S, G) assert only can contain one SPT (S, G) entry, so it can
250   be aggregated according to the same source address, and then all SPT
251   (S, G) entries corresponding to the same source address are merged
252   into one assert record.

[major] Related to the name of the new message (mentioned before) -- the
text above is confusing because it seems to contradict itself until one
realizes that you're referring to two different assert messages: "assert
only can contain one SPT (S, G) entry...then all SPT (S, G) entries...are
merged into one assert record".


[minor] "SPT (S, G) entry"

I haven't seen that terminology used anywhere.  It seems to me that "(S,G)
entry" would be enough.



254   * A (*, G) assert may contain a (*, G) entry or a RPT (S, G) entry,
255   and both entry types actually depend on the route to the RP. So it
256   can be aggregated further according to the same RP address, and then
257   all (*, G) and RPT (S, G) entries corresponding to the same RP
258   address are merged into one assert record.

[nit] s/(*, G)/(*,G)/g
The more compact notation is what rfc7761 uses.

[minor] "RPT (S, G) entry"

I haven't seen that terminology used anywhere.  It seems to me that "(S,G)
entry" would be enough.  However, it is also a relatively obscure detail
that the source address can optionally be included if the RPTbit is set.

I see that the terminology is used later on -- if needed, please define the
term in the terminology section.



260   This method can optimize the payload of the transmitted message by
261   merging the same field content, but will add the complexity of the
262   packet encapsulation and parsing.

[major] As in the last section, the method is not explained.  The records
are combined, but how?



264 3.4. PIM Assert Timer

266   As described in section 4.6 in RFC7761, the Assert Timer function of
267   each (S,G) and (*,G) is not changed. After the Assert Message
268   Packing function defined in this document is enabled, when the first
269   AT of a (S,G) or (*,G) is expired, in order to carry much more
270   assert information in this message, some of other (S,G) or (*,G) may
271   be included in the same message, even though their ATs have not been
272   expired. After the assert message packing, the ATs of all the (S,G)
273   or (*,G), that are packed in the same message, are all reset. That
274   is after the packed assert messages is sent, the ATs of the packed
275   (S,G) or (*,G) will be set with the same value.

[minor] This paragraph seems very verbose.

Suggestion>
   The operation of the Asset Timer (AT), as described in Section
   4.6 of [RFC7761], is not changed.


After making this suggestion I found a message from Stig about packing
[1].  In it, he made the point that "one should not delay sending asserts
in order to achieve optimal packing...there is no harm in sending some
asserts a few seconds earlier to achieve better packing."

I don't see Stig's suggestion reflected in the text above.

Suggestion 2>

   The operation of the Asset Timer (AT), as described in Section
   4.6 of [RFC7761], is not changed.  In order to include more
   Assert Records in a single message, the Assert Packing message
   MAY include Assert Records for which the AT hasn't expired.

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



277 3.5. PIM Assert Format Selection

279   An implementation MUST NOT send assert messages using a packing
280   type, unless all routers on the LAN have indicated support for the
281   type. If both packing types are supported, then it is left to the
282   implementation whether to use assert packing and which packing type
283   to use. It is RECOMMENDED to use the supported method that is most
284   efficient. The Aggregation Packing Format is likely to be the most
285   efficient if the assert message is to include multiple records
286   having the same source address or RP address.

[major] Can multiple types be used at the same time (if supported)?

   If both packing types are supported, then it is left to the
   implementation whether to use assert packing and which packing
   type to use. It is RECOMMENDED to use the supported method that
   is most efficient.

If left to the implementation, and assuming the possibility of multiple
implementations on the same LAN, then it looks like multiple types, or even
(old) Assert messages can be used.  Is that correct?  Please be explicit.


[major] "RECOMMENDED to use the supported method that is most efficient"

This normative recommendation sounds more like a judgement call (around
efficiency) than something that is needed for interoperability -- again,
assuming that multiple types can be used on the LAN.

s/RECOMMENDED/recommended



288   The regular RFC7761 assert format is still allowed to be used. For
289   example the assert only needs to be sent for a single (S, G).

291 4. Packet Format

293   This section describes the format of new PIM messages introduced by
294   this document.

[nit] The first one is not a message.  s/messages/extensions



296 4.1. PIM Assert Packing Hello Option

[minor] Please add a note to indicate where the format of the Hello Options
is defined (Section 4.9.2/rfc7761).



298    0                   1                   2                   3
299    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
300   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
301   |      OptionType = TBD         |      OptionLength = 1         |
302   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
303   |  Packing_Type |
304   +-+-+-+-+-+-+-+-+

[minor] Please include figure numbers for all packet formats.



...
310   - Packing_Type: The specific packing mode is determined by the value

312      of this field:

[nit] There's a extra line above.


[nit] The indentation of the paragraphs below needs to be fixed so that all
the lines are left-aligned.

314      When the first bit from the right is set to 1: indicates simple
315   packing type as described in section 2.2

317      When the second bit from the right is set to 1: indicates
318   aggregating packing type as described in section 2.3

[minor] The sections are in §3.*, not §2.*.

Also, §3.3 uses "PIM Assert Packing Aggregation Type".



320      3-8 bits: reserved for future

[major] The Packing_Type field seems to be a bitfield.  Please define the
field as such and the bits correspondingly -- maybe even name them (S-bit,
A-bit, for example).

Indicate what this field means: for example, it indicates which packing
type(s) is supported.  BTW, is it "packing type" or "packing mode" --
"mode" is only used here.

Also, because the sender selects the packing type, setting a bit to 1 means
that the node is willing and able to both send and receive those types.
IOW, if both types are indicated, it is possible for NodeA to select Type 1
and for NodeB to use Type 2 (and or rfc7761 Assert messages).  Please
indicate in the definition what setting a bit implies.


[nit] s/for future/for future use



322     The node may support multiple packing types. The node MUST set the
323   bits indicating which types they support. They MUST set both bits if
324   they support both type 1 and type 2. The format used MUST be a
325   format supported by all routers on the LAN, see section 3.5 for
326   details.

[] The first couple of sentences talk about the setting, while the last one
about selecting a common format.  The first part belongs in the description
of the new Option.  The second part is about the use (so it should be
formatted with a "normal" indentation).

The information about type 1 and 2 sounds more like an example about this
document (which only defines those 2 types).

Note that §3.5 is not explicit enough (see my comments there)...and it
makes a recommendation that seems like a judgement call.



328     Once the packing format type is selected, this type of coding is
329   used for Assert message packing.

[] This sentence sounds as if all the nodes reach a single agreement...but
the advertisement doesn't work that way.



331     If not all nodes support a same packing format, then Assert
332   message format defined in [RFC7761] is used.

[] Again, rfc7761 Assert messages can still be used, right?  That is what
§3.5 says.

Suggestion>
      Multiple packing types may be supported.  The bits corresponding
      to all the supported packing types MUST be set.

   The packing type used on a link MUST be supported by all nodes.



334 4.2. PIM Assert Simple Packing Format

[minor] Before presenting the format and defining the contents, please
indicate where the PIM messages (and their headers) are defined.  In this
case, rfc7791 and rfc8736.



336    0                   1                   2                   3
337     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

[nit] The bit numbers need to be aligned.



...
369    Type

371       The new Assert Type values TBD1.

373    Type.SubType

375       The new Assert Type.SubType value TBD2.

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


[major] It is not clear whether all the "different" formats use a different
type.subtype or the same.

It seems to me that using the same type.subtype and then differentiating
which Assert Record type is used in the FB would be an ideal use of the new
encoding.  For example, 3 out of the 4 bits can be assigned to the packing
formats -- or the FB can be defined as a hex number providing a total of 16
options.

Note that the descriptions already seem to assume that the subtype can be
used as a discriminator...use the FB instead.



377    FB

379      Flag Bits field. This field is not used for now, it may be used
380    in the future.

[major] Related to the FB assignment, from above.  If the Assert Packing
Hello Option is advertised, can it be assumed that both packing types
defined in this document are supported?  Is there a reason to not consider
support for this document that way?



...
387    The format of each record is the same as the PIM assert message body
388    of section 4.9.6 in [RFC7761].

[minor] s/record/Assert Record/g
Please be consistent.



390     0                   1                   2                   3
391     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
392    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
393    |              Group Address (Encoded-Group format)             |
394    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
395    |            Source Address (Encoded-Unicast format)            |
396    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
397    |R|                      Metric Preference                      |
398    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
399    |                             Metric                            |
400    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[major] How is this new message processed?

Because this format is really the concatenation of multiple Assert
messages, it seems to me that there's no change in processing with respect
to what is specified in §4.6/rfc7761, right?  If so, please be explicit.

Suggestion>
   Each Assert Record is independently processed according to the
   specification in §4.6 of [RFC7761].



402 4.3. PIM (S,G) Assert Aggregation Packing Format

404   This method also extends PIM assert messages to carry multiple
405   records. But the records are divided for (S, G) and (*, G).

[major] "extends PIM assert messages"

This is not an extended message, it's a new message!


[major] §3, and the Hello Option talk about "types", and here you're
talking about "formats".  I can see the difference.  However, it would be
important to call out that by signaling support for the aggregated type,
the node is implicitly signaling support for the two formats.



...
437   Most fields of the specific assert message format is the same as
438   section 4.2, except for the subType fields and records. When
439   aggregated (S, G) records is carried in the message, the subType
440   field is set to TBD3.

[major] As mentioned above, the subType is not a different field -- it is
tied to the type value -- rfc8736.


[] To the point above about a single type.subtype, the format of the
message is the same -- only the format of the Assert Record changes.



...
445      0                   1                   2                   3
446      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
447     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
448     |            Source Address (Encoded-Unicast format)            |
449     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
450     |0|                      Metric Preference                      |
451     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
452     |                             Metric                            |
453     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
454     |        Number of Groups (N)   |           Reserved            |
455     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
456     |                 Group Address 1 (Encoded-Group format)        |
457     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
458     |                 Group Address 2 (Encoded-Group format)        |
459     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
460     |                             .                                 |
461     |                             .                                 |
462     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
463     |                 Group Address N (Encoded-Group format)        |
464     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

466   Source Address, Metric Preference, Metric and Reserved

468       Same as [RFC7761] Section 4.9.6, but the source address MUST NOT
469   be set to zero.

[nit] The indenting needs to be fixed in this section too.


[minor] s/Same as [RFC7761] Section 4.9.6/As specified in Section 4.9.6 of
[RFC7761]/g


[major] The "Reserved" field is not described in §4.9.6/rfc7761.



471   Number of Groups

473       The number of group addresses corresponding to the source address
474   field in the (S, G) assert record.

[nit] s/ in the (S, G) assert record./.


[major] The field in the packet format includes an "(N)", but that is not
reflected in the description...and I don't think it's used anywhere.



476   Group Address

478       Same as [RFC7761] Section 4.9.6, but there are multiple group
479   addresses in the (S, G) assert record.

[minor] The important part here is the definition of the field -- it is a
given that multiple addresses are present.

s/.../As specified in Section 4.9.6 of [RFC7761].


[major] Where do the values in this Assert Record come from?

Given that the information is the same as would be present in several
Assert messages with a common Source Address and the same Metric Preference
and Metric, it seems straight forward to specify how this Assert Record is
built.  Please do so.


[major] How is this Assert Record processed?

Even if the contents are similar to a group of Assert messages with a
common Source (etc.), the state machine in §4.6.1/rfc7761 is written per
(S,G) (not per (S,multiple-Gs) case).  Please specify any modification
(pre-processing maybe) to be done by the receiver before using that rfc7761
specifies.



481 4.4. PIM (*,G) Assert Aggregation Packing Format

483    0                   1                   2                   3
484     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

[nit] The formatting needs fixing.



485    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
486    |PIM Ver| Type  |SubType|   FB  |           Checksum            |
487    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
488    |      Count    |                  Reserved                     |
489    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
490    |                                                               |
491    .                                                               .
492    .                        Assert Record [1]                      .
493    .                                                               .
494    |                                                               |
495    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
496    |                                                               |
497    .                                                               .
498    .                        Assert Record [2]                      .
499    .                                                               .
500    |                                                               |
501    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
502    |                               .                               |
503    .                               .                               .
504    |                               .                               |
505    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
506    |                                                               |
507    .                                                               .
508    .                        Assert Record [M]                      .
509    .                                                               .
510    |                                                               |
511    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

513   Most fields of the specific assert message format is the same as
514   section 4.2, except for the subType fields and records. When
515   aggregated (*, G) records is carried in the message, the subType
516   field is set to TBD4.

[major] As mentioned above, the subType is not a different field -- it is
tied to the type value -- rfc8736.


[] To the point above about a single type.subtype, the format of the
message is the same -- only the format of the Assert Record changes.



518   The (*, G) assert records are organized in the same RP address and
519   are divided into two levels of TLVs. The first level is the group
520   record of the same RP address, and the second level is the source
521   record of the same multicast group address, including (*, G) and RPT
522   (S, G), and the specific message format is:

[nit] s/in the same RP/by the same RP


[major] "two levels of TLVs...group record...and the...source record"

The message format below only shows "Group Records".  ??



524     0                   1                   2                   3
525     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
526    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
527    |             RP Address (Encoded-Unicast format)               |
528    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
529    |1|                      Metric Preference                      |
530    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
531    |                             Metric                            |
532    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
533    |   Number of Group Records(O)  |           Reserved            |
534    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
535    |                                                               |
536    .                                                               .

538    .                        Group Record [1]                       .
539    .                                                               .
540    |                                                               |
541    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
542    |                                                               |
543    .                                                               .
544    .                        Group Record [2]                       .
545    .                                                               .
546    |                                                               |
547    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
548    |                               .                               |
549    .                               .                               .
550    |                               .                               |
551    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
552    |                                                               |
553    .                                                               .
554    .                        Group Record [O]                       .
555    .                                                               .
556    |                                                               |
557    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
558   RP Address

560       The address of RP corresponding to all of the contained group
561   records. The format for this address is given in the encoded
562   unicast address in [RFC7761] Section 4.9.1

[minor] s/given in the encoded unicast address in [RFC7761] Section
4.9.1/described in Section 4.9.1 of [RFC7761].


[major] Quoting Stig:

   I don't understand why you need the second format with both RP
   address and source addresses. If you compare with PIM SM RFC,
   there is no RP address included in asserts even for a (*,G) assert.

https://mailarchive.ietf.org/arch/msg/pim/vF3imfjNTh5XBdCkH95Mqk_VSkI


I didn't see a reply to this message -- I also don't understand. :-(



564   Metric Preference, Metric and Reserved

566       Same as [RFC7761] Section 4.9.6

[] Same comment as above.



568   Number of Group Records

570       The number of packed group records. A record consists of a group
571   address and a source address list.

[major] The field in the packet format includes an "(O)", but that is not
reflected in the description...and I don't think it's used anywhere.


[minor] s/.../The number of Group Records.

Please be consistent with the naming.



573   The format of each group record is:

575     0                   1                   2                   3
576     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
577    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
578    |              Group Address (Encoded-Group format)             |
579    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
580    |        Number of Sources (P)  |           Reserved            |
581    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
582    |              Source Address 1 (Encoded-Unicast format)        |
583    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
584    |              Source Address 2 (Encoded-Unicast format)        |
585    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
586    |                             .                                 |
587    |                             .                                 |
588    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
589    |              Source Address P (Encoded-Unicast format)        |
590    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

592   Group Address and Reserved

594       Same as [RFC7761] Section 4.9.6

596   Number of Sources

598       The number of source addresses corresponding to the group
599   address field in the group record.

[major] The field in the packet format includes an "(P)", but that is not
reflected in the description...and I don't think it's used anywhere.


[minor] s/.../The number of Source Addresses.



601   Source Address

603       Same as [RFC7761] Section 4.9.6, but there are multiple source
604   addresses in the group record.

[minor] The important part here is the definition of the field -- it is a
given that multiple addresses are present.

s/.../As specified in Section 4.9.6 of [RFC7761].


[major] Where do the values for the RP and the Group Record come from?

As was mentioned, the RP is a new element that is not present in rfc7761.
For the Group Record, the information is the same as would be present in
several Assert messages with a common Group Address and the same Metric
Preference and Metric, it seems straight forward to specify how it is
built.  Please do so.


[major] How is this format processed?

Even if the contents are similar to a group of Assert messages with a
common Group (etc.), the state machine in §4.6.2/rfc7761 is written for the
general (*,G) case (or per (S,G), but not the (multiple-S,G) case), and it
seems to derive RP(G) locally (not learn it).  Please specify the mechanism
that needs to be followed to process this information.



606 5. IANA Considerations

608   This document requests IANA to assign a registry for PIM assert
609   packing Hello Option in the PIM-Hello Options and new PIM assert
610   packet type and subtype. The assignment is requested permanent for
611   IANA when this document is published as an RFC. The string TBD
612   should be replaced by the assigned values accordingly.

[major] For the new registry:

- request IANA to create one (they are not assigned)
- what is the registration policy (rfc8126)
- please include a table showing structure of the registry and the current
assignments
- where should the registry be located?


[major] Which registry should the type.subtype be assigned from?

Note that §3.* seems to specify multiple values -- see my comments there.


[major] "The assignment is requested permanent for IANA when this document
is published as an RFC."

Not needed, that is what the requests are for.


[major] "string TBD"

- there are multiple "TBD" strings throughout
- the replacement of the string is not an instruction to IANA.  In general,
that will be taken care of through the RFC Editor process.



614 6. Security Considerations

616   As described in section 6.1 in RFC7761, the forged assert message
617   may cause the legitimate designated forwarder to stop forwarding
618   traffic to the LAN. And the packed function defined in this
619   document, may make this situation worse, because it will affect
620   multiple flows with a single packed assert. That is in case one
621   forged packed assert message is accept, all the (S,G) or (*,G)
622   carried in this message may be stopped forwarding from one or more
623   legitimate designated forwarders. The general security function,
624   such as authentication function defined in RFC7761, or the necessary
625   PIM filtering method, will do good help to avoid the forged assert
626   message.

[minor] Please reword to indicate cause and effect.

Suggestion>

   The security considerations of [RFC7761] apply to the extensions
   defined in this document.

   This document packs multiple Assert Records in a single message.
   As described in Section 6.1 of [RFC7761], a forged Assert message
   could cause the legitimate designated forwarder to stop forwarding
   traffic to the LAN.  The effect may be amplified when using the
   Assert Packing message.


[major]
   The general security function, such as authentication function
   defined in RFC7761, or the necessary PIM filtering method, will
   do good help to avoid the forged assert message.

Authentication doesn't address the case of a rogue node: one that has been
taken over by an attacker.

I'm not sure what the "necessary PIM filtering method" is, but I'm guessing
that you may be referring to this from §6.2/rfc7761:

   A PIM router SHOULD provide an option to limit the set of neighbors
   from which it will accept Join/Prune, Assert, and Hello messages.
   Either static configuration of IP addresses or an IPsec security
   association MAY be used.

Is that it?

If so, then the text is not needed because you're pointing at the same risk
as rfc7761.



...
630 7.1. Normative References
...
643   [RFC8736] Venaas, S., Retana, A., "PIM Message Type Space Extension
644             and Reserved Bits", RFC 8736, February 2020.

[major] There is no in-line citation of this reference.


[EoR -05]