[Gen-art] Gen-art LC review of draft-ietf-ccamp-automesh-02.txt

[Miguel Garcia <[e-mail]>]

Hi.

I have been selected as the General Area Review Team (Gen-ART) reviewer 
for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call comments 
you may receive.

Document: draft-ietf-ccamp-automesh-02.txt
Reviewer: Miguel Garcia
Review Date:  2006-09-25
IETF LC Date: 2006-10-04
IESG Telechat date: 2006-10-04
Summary: This draft is on the right track but has open issues,
described in the review.


Comments: I think the draft is well written, although I have a few
major issues regarding the clarity and intention of the draft, which
should be clarified. The bulk of my comments are mostly editorial, though.

Issue 1:
--------
After carefully reading the document, it is not really clear to me 
whether the intention of the document is:

a) to define two OSPF TE TLVs (one for IPv4, another for IPv6) and two
IS-IS sub-TLVs (one for IPv4, another for IPv6); or

b) to define one OSPF TE TLV, which is further structured depending on
the Type field.

I suspect it is a), which is endorsed by the IANA considerations
section. If this is correct, I wonder why the two OSPF TLVs are going
to share the same name, why not name them TE-MESH-GROUP-IPV4 and
TE-MESH-GROUP-IPV6, respectively. The same should apply to the IS-IS
sub-TLVs.

For example, Section 3.2 now says "This document specifies
a TE-MESH-GROUP TLV that indicate ..." when in fact the document 
specifies not one, but four TLVs.

In any case, I would suggest to add some high-level description around
Section 3.

Issue 2:
--------
IANA actions for OSPF: It is not clear to me which registry IANA has
to modify. I believe the IANA sections should indicate the registry
and subregistry (by name, not by reference) that IANA has to act upon.

So, I guess you are looking at something similar to this (please check
the registry and subregistry names are correct):

This document defines two new types of OSPF TE TLVs. IANA should
register them in the 'Top level Types in TE LSAs' subregistry of the
'Open Shortest Path First (OSPF) Traffic Engineering TLVs per
[RFC3630]' registry with theh following registration data

    Value     Top Level Types                     Reference
-----------  ----------------------              ---------
           x   TE mesh membership (IPv4)           [RFCXXXX]
           y   TE mesh membership (IPv6)           [RFCXXXX]

Note 1: RFCXXXX refers to this RFC
Note 2: suggested x value: 5. Suggested y value: 6. Note that value 4
is already allocated by IANA, and thus, it cannot be re-allocated
again as suggested in the draft.


Issue 3:
--------

IANA actions to IS-IS. Here is even less clear than with OSPF. First
of all, I failed to understand what needs to be registered. The document
says that two new ISIS TLV code-points. So this makes me think IANA
has to act upon the 'TLV Codepoints Registry' of the 'IS-IS TLV
Codepoints per [RFC3563]' registry. But this seems to be the registry of 
top level TLVs for IS-IS. However, this draft defines two sub-TLVs that 
are advertised in the IS-IS CAPABILITY TLV defined in
draft-ietf-isis-caps. This made me doubt and think that this document
is defining two sub-TLVs in the mentioned CAPABILITY TLV, in which
case first the subregistry of the CAPABILITY TLV must be first
created, and the filled up. If this is correct, I think
draft-ietf-isis-caps and not this draft should establish the registry
for sub-TLVs (which it doesn't), and then this draft should populate
this registry with the two initial values.

Please clarify this issue because it is very confusing and will be more
confusing to IANA.

Issue 4:
--------
Is it true that there aren't any security issues? It is hard to
believe these days about the lack of security issues. In most cases, 
absence of security considerations equals a lack of thought of those 
security issues.

Perhaps the draft should 'import' and 'apply' the Security 
Considerations of the mother protocols (OSPF, IS-IS, OSPF and IS-IS 
Traffic Engineering, OSPF and IS-IS capabilities, etcc.). Perhaps the 
draft means that this document does not impose any security concern 
above those express in [list of mother protocols].

Still some thoughts... for example, what happens if a malicious router
does a join operation to become part of a mesh-group it is not
authorized or shouldn't be part of? Is this a potential attack or not?


Editorial comments
-------------------

5) Some times the text uses the term "ISIS" and some times
"IS-IS". Although existing RFCs (e.g., 4444 and 3358) use different
acronym, I would suggest to use a single one throughout the text.

6) Expand IGP, ISIS, and OSPF in the Abstract and add them to the
Terminology section. No need to expand MPLS, TE, and LSR in
the Abstract since it is expanded already in the title.

7) Expand TLV and LSA in Section 2

8) Section 1, under Figure 1, the text reads:

    The TLV is padded to four-octet alignment; padding
    is not included in the length field (so a three octet value would
    have a length of three, but the total size of the TLV would be eight
    octets).

I would expect that the four-octet alignment rule will make a three
octet value encoded in four octets, three for the value and one more
for padding. This differs from the eight octets indicated in the text.

9) Continuation of the previous point: the text then reads about a
32-bit alignment, implying that it is the same as the four-octet
alignment already mentioned. I would suggest to always use the same
unit of measure, either octets or bits, but not both.

10) At the beginning of Sections 4.1. and 4.2 the text first describes
the structure of the TLV and then its purpose. I think it would be
easier to read the other way round, so I would suggest.

In Section 4.1, move the paragraph:

    The TE-MESH-GROUP TLV is used to advertise the desire of an LSR to
    join/leave a given TE mesh-group.  No sub-TLV is currently defined
    for the TE-mesh-group TLV.

above its preceding paragraph.

In Section 4.2, extract from the first paragraph these sentences:

    The
    TE-MESH-GROUP sub-TLV is used to advertise the desire of an LSR to
    join/leave a given TE mesh-group.  No sub-TLV is currently defined
    for the TE-MESH-GROUP sub-TLV.

make them a standalone paragraph of its own, and make it the first
paragraph in Section 4.2.

11) Figures 2, 3, 4 and 5 are missing a line of digits indicating the 
tens in bits. To be precise, replace:

       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

with

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

at the top of Figures 2, 3, 4, and 5.

And similarly, Figure 1 the line indicating the tens is misplaced. So
in this Figure 1 replace:

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

with

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


12) I would suggest to add some text above Figures 2, 3, 4 and 5
introducing the figures, basically saying that the figures further
break down the of the 'value' depicted in Figure 1. For example (for
Figure 2):

"The value of the TE-MESH-GROUP TLV is further structured, but the
actual structure depends on the Type field. There are two types of
TL-MESH-GROUP TLVs depending on whether they are taylored for IPv4 or
IPv6. Figure 2 shows the format of the TE-MESH-GROUP TLV when the Type
field indicates IPv4 addresses while Figure 3 shows the format of the
same TLV when the Type field indicates IPv6 addresses."

The same applies to Section 4.2.

13) More about Figures 2 and 3. The caption indicates that these
figures are TLVs, however, ther Type and Length are not painted in the
figures (I guess they are assumed from Figure 1). So I deduced
(correct me if I am wrong), that these are not TLVs, but a portion of
it. I would suggest to add the Type and Length boxes that you already
have in Figure 1 to the top of Figures 2 and 3 to become real TLVs and
increase readability. A side effect of the latter is that, when I read
"Type: to be assinged by IANA" I don't need to come back to Figure 1
to see where the Type is represented.

The same applies to Section 4.2.

14) In Figures 2 and 3 there is a box of undetermined length at the
bottom of the figures that do not contain any name. I guess these
boxes represent the continuation of the Tail-end name, but it is not
clear at a first glance. Shouldn't the boxes be labeled to understand
which contents are inside?

15) Then I would suggest to add new subheadings (e.g., 4.1.1 and
4.1.2) or some other types of delimiters to separate the two types of
TLVs that correspond to Figures 2 and 3, respectively.

The same applies to Section 4.2.

16) The draft uses the concept of a 'mesh-group entry', but it isn't
explicitly defined. I deduced that a mesh-group entry is any of the
objects represents in either Figures 2, 3, 4, or 5. I suggest to
explicitly define it and perhaps refers to those figures in the
definition. I believe this is an important concept to understand the
join and leave operations.


17) Section 4.1, page 7 defines the Tail-end name as:

    - A Tail-end name: a variable length field used to facilitate the TE
    LSP identification.  The Name length field indicates the length of
    the display string before padding, in bytes.

I would suggest to start the description with what this object is
rather than what is used for. I also suggest to separate the
description of "Name length" into another paragraph, since it is
separate field. Suggested rewording:

    - A Tail-end name: A display string that is allocated to the
    Tail-end. The field is of variable length field and is used to
    facilitate the TE LSP identification.

    - Name length field: An integer, expressed in octets, that
    indicates the length of the Tail-end name before padding.

18) Section 5.1 I would suggest to further indent the two paragraphs
that describe Types 10 and 11, so they are clearly seen as children of
the previous paragraph. So, the text could be formated as this:

    - entire OSPF routing domain (type 11).  In this case, the flooding
    scope is equivalent to the Type 5 LSA flooding scope.  A router may
    generate multiple OSPF Router Information LSAs with different
    flooding scopes.  The TE-MESH-GROUP TLV may be advertised within a
    type 10 or 11 Router Information LSA depending on the MPLS TE mesh
    group profile:

	 * If the MPLS TE mesh-group is contained within a single area
          (all the LSRs of the mesh-group are contained within a single
          area), the TE-MESH-GROUP TLV MUST be generated within a Type
          10 Router Information LSA;

          * If the MPLS TE mesh-group spans multiple OSPF areas, the TE
	 mesh-group TLV MUST be generated within a Type 11 router
	 information LSA.

19) General comment applicable to all the text: I understand that the
text sometimes reads about a TE mesh-group and sometimes about the
TLV, which is named as "TE-MESH-GROUP TLV". According to this, the
term "TE mesh-group TLV" shouldn't appear, because there is not such
TLV. Suggestion: replace "TE mesh-group TLV" with "TE-MESH-GROUP
TLV". At least I found one instance in Section 4.1 and another one in
Section 5.1. Perhaps there are more...

20) I suspect that Section 5.2 goes beyond its scope by specifying
procedures that are already specified elsewhere. For example, the text
in the first paragraph reads:

    An IS-IS router MUST
    originate a new IS-IS LSP whenever the content of the any of the
    advertised sub-TLV changes or whenever required by regular IS-IS
    procedure (LSP refresh).

While not being my field of expertise, I would expect that the IS-IS
mother specification specifies when an IS-IS router originates a new
LSP, independent of this specification. To avoid double specifying
procedures, I would suggest to replace the above text and rewording
along the following lines:

    According to the RFC xxxx [add ref], an IS-IS router
    originates a new IS-IS LSP whenever the content of the any of the
    advertised sub-TLV changes or whenever required by regular IS-IS
    procedure (LSP refresh).

21) Since this document provides IANA actions on two different
registries, it might be a good idea to create Sections 7.1 and 7.2
devoted to OSPF and IS-IS, respectively.

22) Unused Reference: 'RFC3209' is defined on line 463, but not referenced

BR,

        Miguel
-- 
Miguel A. Garcia           tel:[phone]
sip:[e-mail]
Nokia Research Center      Helsinki, Finland

_______________________________________________
Gen-art mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/gen-art