Internet Group Management Protocol Version 3 (IGMPv3) and Multicast Listener Discovery Version 2 (MLDv2) Message Extension
draft-ietf-pim-igmp-mld-extension-08

Thank you for the work put into this document. The mechanism is simple and powerful; the document is also easy to read.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

Please also have a look at Tommy Pauly's INT directorate review at <https://datatracker.ietf.org/doc/review-ietf-pim-igmp-mld-extension-05-intdir-telechat-pauly-2022-01-20/>.

Special thanks to Mike McBride for the shepherd's write-up including the section about the WG consensus. 

I hope that this helps to improve the document,

Regards,

-éric

# Section 3

It is unclear to me what to do when more TLV should be sent than allowed by the MTU. May/should/may not different subset of TLVs be sent over multiple packets (à la IPv6 Router Advertisement) ?

## Section 3.1

I was about to raise a DISCUSS on this issue. It appears that the E-bit is specified in this document while it was defined as reserved in MLDv2 RFC 3810, i.e., should this document formally update RFC 3810 (and possibly IGMPv3 as well)? I.e., in the header + abstract + introduction ?

Also, does this mean that once the E-bit is set, then there cannot be any other extensions except TLVs ? This seems a limiting factor.

## Section 6

Indeed, a lot of small TLVs could increase the cost of processing, hence be an element of attack; but I would expect from an I-D to have some mitigation proposals.

Thanks for this work; it's good to see a well-designed extension mechanism
be provided for these longstanding protocols.

Section 3

If there is to be no alignment or padding, having an example of that (or
of an unaligned "tail" with gap in the figure) would probably help drive
that point home.

Section 4

   Future documents defining a new type MUST specify any additional
   processing and validation.  These rules, if any, will be examined
   only after the general validation (above) succeeds.

Just to confirm: this means that if the extensions block is malformed per
the ruls above, all extensions in it (even ones that could be fully
parsed) are ignored?  There are probably security issues if some
implementations do partial processing and others don't, which we might
call out in the security considerations if needed.

Section 5

   IGMP and MLD implementations, particularly implementations on hosts,
   rarely change, and the adoption process of this extension mechanism
   is expected to be slow.  Also as new extensions are defined, it may
   take a long time before they are supported.  Due to this, defining
   extensions should not be taken lightly, and it is crucial to consider
   backwards compatibility.

In the vein of the intdir reviewer's remarks, it seems like it would be
useful for testing implementation correctness if *some* TLV type was
defined now, even if it's just a "padding" extension with semantics of
"value bytes must be zeros".  As written, this sounds like we're asking
people to implement something, wait years for it to be deployed and a
"real" TLV defined, and only then uncover implementation bugs.  Having
something defined now that people can play around with and use for testing
seems like it would be valuable for evaluating implementation quality.

   Implementations that do not support this extension mechanism will
   ignore it, as specified in [RFC3376] and [RFC3810].

The spec clearly says so, but are we sure?  Have we tested it?  Being able
to follow this sentence up with "and confirmed in real-world testing of
major implementations" would be a big confidence boost.

NITS

Section 1

   When this extension mechanism is used, it replaces the Additional
   Data section defined in IGMPv3/MLDv2 for TLVs.

The construction "<A> replaces the <X> defined in <Y> for <Z>" seems to only
be parsable as saying that X is defined for Z, not that A is used for Z.
"replaces ... with" or "repurposes ... for" seem like viable alternatives.

Section 3

   IGMPv3 and MLDv2 messages are defined so that they can fit within the
   network MTU, in order to avoid fragmentation.  When this extension
   mechanism is used, the number of Group Records in each Report message
   SHOULD be kept small enough that the entire message, including any
   extension TLVs can fit within the network MTU.

"Group Record" seems to be a defined term for IGMP but not MLD; do we want
to tweak the wording here to be fully generic?

Also, comma after "extension TLVs".

Section 4

   Unsupported types MUST be ignored.

Is this "the value portion, if any, of extensions with unsupported types
MUST be ignored"?  Or does ignoring even the type as well come into play?

Section 5

   message types.  It MUST also be specified what the behavior should be
   if a message is not used in the defined manner, e.g., if it is
   present in a query message, when it was only expected to be used in
   reports.

The bit after "e.g." provides an example only of the "not used in the
defined manner" part, and not the "what the behavior should be" part.
That's not inherently problematic, but perhaps an example of both is
possible.

[S3; nit]

* Always helpful to specify the byte order of multi-byte integer fields
  (I assume the Extension Length is in network byte order, but up to you
  if you want to be explicit).

Thanks for this document. It’s clear and useful. I have a few comments I hope may be helpful.

1. I’d like to echo Rob Wilton’s first comment, about how the use of the term “extension” to mean slightly different things at different points is not helpful to readability. Concretely:

- Maybe the title could be something like “… Message Extensibility Mechanism” instead of “… Message Extension”?

- As Rob points out the use of “extension” is particularly vexatious in §5. I like his suggestion of using “extension TLVs” although there are many other options, for example you could say “… new extension types”. 

2. In Section 1 you say “Additional Data is defined for…” and then go on. On first glance, I thought maybe you meant that there was existing use of the Additional Data field in the cited documents (which would have necessitated some further discussion, perhaps). This minor confusion would be easily prevented by changing to say “The Additional Data field is defined…” 

3. Apropos the above, I suppose I don’t need to worry about whether there is legacy use of the Additional Data field, because your introduction of the E-bit takes care of that concern?

4. I was surprised you don’t reserve anything at all out of your 2^16 code point space for experimental use or development. Given that you’ve specified a relatively restrictive allocation policy (IETF Review), what’s your expectation for how people will work on under-development extensions? Are they expected to just squat on code points? It seems like a case of training people to behave badly by not giving them any options for behaving well. :-(

5. Speaking of code points, I like the suggestion mentioned in Ben’s review, that it might be a good idea to define a noop TLV in this document.

Thanks to Pete Resnick for their General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/ZaFgT1o9GT23_P4KfhXWmFiTFxI).

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 3.4. , paragraph 3, nit:
> n mechanism is expected to be slow. Also as new extensions are defined, it ma
>                                     ^^^^
A comma may be missing after the conjunctive/linking adverb "Also".

Section 4. , paragraph 5, nit:
> chanism only for IGMPv3 and MLDv2. Hence this mechanism does not apply if ho
>                                    ^^^^^
A comma may be missing after the conjunctive/linking adverb "Hence".

Thanks to Wes Eddy for the TSVART review.

I suggest that, in Section 7, you replace what appears to be a table header with a list of registry field names and brief descriptions for each.

Hi,

Thanks for this document.  I found the use of the term "extension" to perhaps not be as clear as it could be.  In most places the reference is to Extension in the singular, and the attributes carried within the extension as TLVs.  However, section 5 talks of "as new extensions are defined".  I presume that what is meant here are "new extension TLVs" and not a different new Extension to replace the Extension defined in this document.

Related to the ambiguity above, although the document makes it clear how an implementation is expected to behave if it doesn't understand the "extension" format, it doesn't define how an implementation should behave if it receives an extension TLV that it doesn't understand.  I presume, for backwards compatibility, that the default behavior is that receivers MUST ignore extension TLVs that they don't understand and I think that needs to be specified here.

Thanks,
Rob

Thank you for writing this; it's well documented and easy to understand :-)

Thanks to the authors for the effort on this specifications. Thanks to Wesley Eddy for the TSV review.

I have couple of questions. It would be nice to get responses to them - see below :

* Section 3: it is not clear to me from this specification when the Extension length may be zero. Can it be clarified why one would include the extension without values?

* What is the "Group Record" for MLDV2?