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
.
_______________________________________________
OSPF mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/ospf
