Re: [RTG-DIR] RtgDir QA review: draft-ietf-ospf-segment-routing-extensions-12

[Hannes Gredler <[e-mail]>]

not sure where the ordering requirements have come in. no need from my POV.
feel free to drop it.
/hannes

> On 3 May 2017, at 17:18, Peter Psenak <[e-mail]> wrote:
> 
> Hi Stig,
> 
> please see inline:
> 
>> On 27/04/17 00:08 , Stig Venaas wrote:
>> Hello,
>> 
>> I have been selected as the Routing Directorate reviewer for this
>> draft. This is just an early QA review.
>> 
>> The draft is in good shape, but I did find some minor issues and nits.
>> It is fairly readable, but it could be improved in a few places.
>> 
>> 
>> Minor issues:
>> 
>> In 3.1:
>> The SR-Algorithm TLV is some places called a Sub-TLV. It might be
>> good to be consistent.
> 
> fixed.
> 
>> 
>> This is not clear in 3.1:
>>    The SR-Algorithm Sub-TLV is optional. It MAY only be advertised once
>>    in the Router Information Opaque LSA.
>> Is this trying to say that it MUST NOT be advertised more than
>> once? With the current wording this is not obviously that strict.
> 
> yes, I changed the text to: "It MUST only be advertised once in the Router Information Opaque LSA". Hope that is clear enough.
> 
> 
>> 
>> I see some text regarding multiple SR-Algorithm sub-TLVs, but it also
>> looks like one can have multiple algorithms in one sub-TLV. At least
>> from the diagram.
> 
> yes, multiple algorithms in the same SR-Algorithm TLV is fine.
> 
>> But I don't see any discussion about this. Is it OK
>> to add multiple? When can it be done, what does it mean? What if
>> routers don't support the exact same set of algorithms?
> 
> router advertise all algorithms it supports. Each router may support different set.
> 
>> 
>> The term "lowest flooding scope" is used a couple of places. I think I
>> know what it means, but it might be good to point it out. Also, I'm
>> used to seeing the term "smallest" rather than "lowest". I'm assuming
>> they mean the same.
> 
> I changed all places to "narrowest flooding scope".
> 
>> 
>> In 3.2 there is this bullet point:
>>    The receiving router must adhere to the order in which the ranges
>>    are advertised when calculating a SID/label from a SID index.
>> 
>> You probably should use MUST here.
> 
> fixed.
> 
>> 
>> Section 4:
>> In section 4 there is a range for advertising a range of prefixes.
>> But it looks like it contains a single prefix length and it says
>> the length is the length of the prefix. While it says range size
>> is the number of prefixes. I don't understand from the text what
>> really prefix length and range size means and how this should be
>> used.
> 
> 10.0.1.0/24 - 10.0.100.0/24 represents a range of 100 /24 prefixes, where:
> 
> (starting) prefix - 10.0.1.0
> length of the prefix is 24
> range size is 100
> 
>> 
>> I understand this is IPv4 only since OSPFv2, but rather than just
>> saying IPv4 is 0, maybe refer to an IANA AF registry? This might
>> be helpful if you want to use the same sub-TLV in OSPFv3 and
>> use the same code for parsing etc. IANA has 1 for IPv4 though.
> 
> this is equivalent to section 2.1 of RFC7684. I have updated the text to match RFC7684.
> 
>> 
>> Section 5:
>> Is it intentional that the flags start in position 1 rather than
>> 0?
> 
> yes. Originally we had N-flag (Node Flag) defined at position 0, but we moved that to the OSPFv2 Extended Prefix TLV (section 2.1 of RFC7684). Due to an existing implementations, we did not shift all other bits after that.
> 
>> 
>> I see that the NP flag should be ignored when M is set. Then I
>> see this text:
>>    As the Mapping Server does not specify the originator of a prefix
>>    advertisement, it is not possible to determine PHP behavior solely
>>    based on the Mapping Server advertisement.  However, PHP behavior may
>>    safely be done in following cases:
>> This seems not very precise. Could you say exactly what the behavior
>> should be, rather than saying "behavior may be done"?
> 
> would changing  "may safely be done" to "SHOULD be done" be sufficient?
> 
>> 
>> Section 6:
>> It might be good to make clear that other flag positions are
>> reserved, set to 0 and ignored... Perhaps also point out that
>> weight is in the range 0-255
> 
> fixed both.
> 
>> 
>> I see this sentence:
>>       If the SID/Label Sub-TLV appears in the SID/Label Binding Sub-TLV
>>       more than once, instances other than the first will be ignored and
>> 
>> Should it say MUST be ignored?
> 
> changed to SHOULD as the previous text says "SHOULD only appear once".
> 
>> 
>> Section 6.2 it says:
>>    All ERO Sub-TLVs must immediately follow the SID/Label Sub-TLV.
>>    All Backup ERO Sub-TLVs must immediately follow the last ERO Sub-TLV.
>> 
>> Should these be normative MUSTs?
> 
> Changed both to MUST.
> I'm also going to clarify this with Hannes, whether that is still required, because I do not see equivalent text in ISIS draft.
> 
>> 
>> In 6.2.1:
>> It would be good for all of these to specify that other flags are
>> reserved.
> 
> done.
> 
> 
>> 
>> 
>> Nits:
>> The intro should perhaps mention LAN adjacency and binding SIDs?
> 
> LAN Adjacency SID is a sub-type of the Adjacency SID. I have added sentence about binding/other SID types.
> 
>> 
>> 2nd paragraph of section 2 is confusing. It sounds like
>> the Opaque LSAs in 7684 were defined for SID in particular,
>> but it is a generic mechanism. Perhaps SID was the
>> motivation though?
> 
> this part is a left-over from the original draft before we split to RFC7684 and this draft.
> 
> I replaced the whole paragraph with:
> 
> "Extended Prefix/Link Opaque LSAs defined in <xref target="RFC7684"/> are used for advertisements of the various SID types."
> 
>> 
>> Section 6.1:
>> It says:
>>    The ERO Metric Sub-TLV advertises the cost of an ERO path.  It is
>>    used to compare the cost of a given source/destination path.  A
>>    router SHOULD advertise the ERO Metric Sub-TLV in an advertised ERO
>>    TLV.
>> 
>> Is the ERO TLV the ERO Sub-TLVs defined in 6.2? It would be good to
>> point that out.
> 
> ERO Metric Sub-TLV as well as all the other ERO sub-TLVs in sectin 6.2 are at the same level - they are all sub-TLVs of the Binding TLV. I added some text to 6.2 to make that clear.
> 
>> 
>> In 8.4.2:
>>    Broadcast, NBMA or or hybrid
>> Extra "or".
> 
> fixed.
> 
>> 
>> Section 9:
>> There are no new registries and most of the TLVs are already
>> allocated? It seems there are a few new ones where it should
>> probably say TBD, or say something about being suggested values.
>> That was done in earlier sections. I see in some places it says
>> "are allocated" here, while it says "suggested" in the definition
>> of the TLV.
> 
> section 9 lists all the updates to the four different registries. Values are explicitly mentioned in section 9 for every single code point.
> 
>> 
>> Section 10:
>> It says there are responses from 2 implementers, but I see 3.
> 
> fixed
> 
>> 
>> Section 11:
>> Are these really all the potential security issues?
> 
> I'm not aware of any others. Feel free to suggest more if required.
> 
>> 
>> I'm on vacation the next 2 weeks, so I may not reply to any
>> emails during that period.
> 
> I will post the new version after closing on the ERO part with Hannes and others.
> 
> thanks,
> Peter
> 
>> 
>> Regards,
>> Stig
>> .
>> 
>