[Gen-art] Genart last call review of draft-ietf-ospf-ospfv3-segment-routing-extensions-18

[Pete Resnick <[e-mail]>]

Reviewer: Pete Resnick
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-ospf-ospfv3-segment-routing-extensions-18
Reviewer: Pete Resnick
Review Date: 2018-11-16
IETF LC End Date: 2018-11-16
IESG Telechat date: Not scheduled for a telechat

Summary: One serious concern, one minor issue, and some nits.

Major issues:

In 3.1:

I get worried when I see this:

      SID/Label: If length is set to 3, then the 20 rightmost bits
      represent a label.

So, the Length is not a length, but rather a flag. This seems like it has the
potential for an interop problem. If a general implementation treats it as a
length, it's going to get the left-most 24 bits when the value is 3. Even if
the implementation chooses the right-most 24 bits, it's only supposed to take
the right-most 20 bits and mask off the extra 4 bits. Or are you going to
specify that implementations must always set the extra 4 bits to 0?

Maybe this sort of thing is the way things have always been done for TLVs, and
if so feel free to ignore me, but from an code implementation perspective, this
seems like an accident waiting to happen. (Known sometimes as a "foot-gun".)

Minor issues:

In 4.4:

   The SRMS Preference TLV MAY only be advertised once in the OSPFv3
   Router Information Opaque LSA and has the following format:

You mean MUST, not MAY there.

In 7.1 and 7.2:

If SID/Index/Label is a label, is it using the low 20 bits of the first 3 bytes
of the field (i.e., bits 4-23)? Is there a requirement that the high 4 bits and
the low 8 bits must be cleared by the implementation? Some clarification would
be useful.

In 8.1:

You talk about setting the IA-flag, but this version of the document doesn't
define the IA-flag anymore. Is it defined elsewhere?

Nits/editorial comments:

In 3.1:

Ignoring the issue stated above, I also found this text a bit confusing:

      Length: Variable, 3 or 4 octets

Obviously you mean that the value of Length is either 3 or 4. At first I read
it as the value of Length was variable, and that it took up 3 or 4 octets in
the Sub-TLV. If this is the way you've always written these things, fine, but
it seems to me it would be clearer to say.

      Length: Either 3 or 4

In 5:

   s/we need a single advertisement/a single advertisement is needed

Just being pedantic. If you like it, use it. If not, don't.