[mpls] AD review of draft-ietf-mpls-gach-adv

["Adrian Farrel" <[e-mail]>]

Hi,

I have conducted my usual AD review of your document as part of the
publication request processing. The aim of my review is to catch any
issues that might show up in IETF last call, directorate reviews, or
IESG evaluation. The intent is to resolve the issues by text changes
or through email discussion to smooth the passage of the document
through the later stages of the process.

I have reviewed this document jointly with draft-ietf-mpls-tp-
ethernet-addressing and since the author team is the same for the two
documents, I recommend that you process the comments for the two 
documents at the same time.

It will become clear to you as you read this review that I am not
overly impressed with the quality of this protocol specification. It is
possible that the issue lies with documentation details and the level of
review that the working group gave. However, I have not often seen a 
better example of why it is valuable to have interworking 
implementations of a protocol specification before requesting
publication.

As usual, all my comments are open for disagreement and discussion. I
think that you will probably want to produce a revision of this 
document to address my comments, so I have put it into "Revised I-D
Needed" state in the data tracker.

Thanks for the work,
Adrian

===

Please update the captions to the figures as "Figure n : blah, blah"

---

Section 1

   It provides an auxiliary logical data channel
   associated with an MPLS Label Switched Path (LSP), a pseudowire, or a
   section (link) over which a variety of protocols may flow.

Marginally ambiguous because of clause ordering. It is not "a section
over which a variety of protocols may flow." Suggest re-wording...

   It provides an auxiliary logical data channel over which a variety of
   protocols may flow. Each such data channel is associated with an MPLS
   Label Switched Path (LSP), a pseudowire, or a section (link).

---

Section 1

   ...Operations, Administration, and Maintenance (OAM)
   capabilities associated with the underlying LSP, pseudowire, or
   section.

I don't think "underlying" is right, is it?
How about:

   ...Operations, Administration, and Maintenance (OAM)
   capabilities for the associated LSP, pseudowire, or section.

---

Section 1

   The main principle guiding the design of the MPLS G-ACh advertisement
   protocol (GAP) is simplicity.

Capitalise, please.

---

Section 1.1

After...
   The G-ACh advertisement protocol presented in this document
   thus allows LSRs to exchange information of a similar sort to that
   supported by LLDP for Ethernet links.
...could you please a simple sentence explaining why it was not enough
to provide a way to encapsulate LLDP in the G-ACh. [Hint: I think the
answer is that you envisage the GAP being used for a larger set of 
MPLS-specific features than is currently defined in LLDP and for which
you do not consider it would be appropriate to extend LLDP.]

This is particularly important given that:
- the only use of the GAP mentioned anywhere is [I-D.ietf-mpls-tp-
  ethernet-addressing]
- you say...
   Where
   it is anticipated that the sole purpose of the GAP will be to provide
   Ethernet MAC address learning, the use of LLDP SHOULD be considered.
>From where I am sitting, that looks like "This protocol is not needed
because LLDP does everything we want."

So, the question you need to answer (for me and in the I-D) is why have
you invented a new protocol when LLDP does everything that is needed?

And, BTW, I would like to understand the meaning of "SHOULD" in a 2119
context in this document. Since it is a protocol spec, and since it
references 2119, the implication is that implementations of 
[I-D.ietf-mpls-tp-ethernet-addressing] should not be made unless 
another I-D using the GAP is published as an RFC *and* implemented by
the same implementation. 

Please don't think that deleting this text will get you off the hook!
You have already made the point (and with WG consensus). Now you need to
explain to me why there is a need for either document.    

I guess you should also say why you consider LMP to be inappropriate.

---

It is not really necessary to include LSR in Section 1.2 as the term
is only used before that section.

---

Section 2

   Although one GAP message can contain data for several applications,
   the receiver maintains the data associated with each application
   separately. This enables the sender to transmit a targeted update
   that refreshes the data for a subset of applications without
   affecting the data of other applications.

How the receiver maintains the data is entirely an implementation    
issues. I suggest you rephrase this as:

   Each GAP message can contain data for several applications. A
   sender may transmit a targeted update that refreshes the data for a
   subset of applications without affecting the data of other
   applications sent on a previous message.
                                                           
---

Section 3

After "XXXX" please note "(TBD by IANA)" so that the RFC editor
spots this, correlates it with the IANA section, and updates it
accordingly.

In Section 9.1, please mark the value to be assigned as XXXX
                                                                               
---

Section 3

s/A Gap message/A GAP message/

---

Section 3

Can two ADB elements for the same application be present in the
same message? If so what processing rules apply? If not, how is the
error handled?

Can two TLVs of the same type be present in the same ADB element? If
so what processing rules apply? If not, how is the error handled?

And the combination of cases, if two ADB elements for the same
application can be present in the same message, can a TLV of the same
type be present in each? If so what processing rules apply? If not,
how is the error handled?

---

Section 3 and 5

Although Section 5.1 tells us how the Message Identifier can be set, it
is ambiguous wrt Section 3 since in Section 3 we have:

      Message Identifier: Unique identifier of this message

And in 5.1 we have:

   The Message Identifier (MI) uniquely identifies this message and is
   set at the sender's discretion.

What does "at the sender's discretion" mean? It seems to say: "If the 
sender doesn't fancy setting it, that's OK."

Additionally, it is not clear what the uniqueness scope of the Message
Identifier is supposed to be. Presumably not globally unique! Is it per
sender or per data-channel per sender?
                                 
The sole purpose of the MI seems to be contained in the final paragraph 
of Section 5.1. If that is the case, you seem to have some confusion 
between rapid retransmission (same message) and lifetime avoiding
retransmission (different message). All this needs to be cleaned up and
explained. Noting that the last sentence of 5.1 belongs in 5.2.

I have to wonder whether you really need the field.

---

Sections 2, 3, and 5

   The Lifetime field specifies how long,
   in seconds, the receiver should retain the data in this message.  If
   the lifetime is zero the data is immediately marked as expired.

Have you considered offering a lifetime value meaning "retain until    
replaced"? This would *considerably* simplify the job of the sender 
which would not need to run multiple timers to ensure that the 
information is updated in a timely manner and doesn't accidentally
expire.

Note also that 2*16 seconds is not a very long time. things like the
MAC address of an interface can safely be considered "stable".

In rather a topsy-turvy way you say in Section 5.1...
   Lifetimes SHOULD be set in such a way that at least three updates
   will be sent prior to Lifetime expiration.  For example, if updates
   are sent at least every 60 seconds, a Lifetime of 185 seconds may be
   used.
...Isn't it the other way around? Shouldn't updates be sent according to
the lifetime value that has been set?

Discarding the information when the timer expires needs to be clarified.
I think you mean "revert to the configuration-based state that was held 
before the information was received." But you might mean "transition to
the complete absence of this information."

In Section 2 should you discuss message loss and the consequences? Any
mitigation you need to add? Is the retransmission to protect against 
lifetime expiry supposed to handle this? If so, it doesn't seem
frequent enough to install the initial state? Do you need a two-speed
timer: rapid on first use and dropping down to slow for state retention?
Or would an Ack be easier? Probably, the final paragraph of Section 5.1
can be adapted to handle this case, and then you can put a forward 
pointer in Section 2.

But note that the final paragraph of 5.1 is a bit of a mess!...
   In some cases additional reliability may be desired for the delivery
   of a GAP message.  When this is the case, the RECOMMENDED procedure
   is to send three instances of the message in succession, separated by
   a delay appropriate to the application.  This procedure SHOULD be
   used, if at all, only for messages that are in some sense
   exceptional; for example when sending a flush instruction following
   device reset.  The MI may be used to detect and discard duplicate
   messages.
...What is a "delay appropriate to the application?" Maybe it means 
"Each specification of a GAP application must state the appropriate
delay to use for such fast retransmission." Which would be fine, but
would require this document to make the statement for Application 0x0000
and would require [] to make a statement for Application 0x0001.
... Also "The procedure SHOULD be used..." reads like the procedure is
intended to be used. But "...if at all" really means "The procedure 
SHOULD NOT be used except..."

I can't understand what is meant by "If the lifetime is zero the data is
immediately marked as expired." Are you trying to say that the data 
should be processed, potentially replacing any stored information, and
then should be "discarded" (noting my previous comment about "discarded"
not being clear)? Or are you trying to say, that the ADB element should
be discarded unprocessed?

Lastly, the recommended multiplier ("at least three updates will be sent
prior to Lifetime expiration") cuts it too fine! You need to allow for
out-queues, transmission, and processing. The normal approach to this 
(probably going back to Van Jacobson) is to add a unit of half. Thus,
in your case, Lifetime = 3.5 * Update.

---

Section 3

Are TLV Value fields padded up to a multiple of 4 octets?

---

Section 3

It is clear that each ADB element must contain at least one TLV. What 
is the error processing for an empty ADB element?

---

Section 4

Since the Application ID 0x0000 ADB contains "metadata and processing
instructions rather than static data that is meant to be retained" 
please explain how the Lifetime field is to be set/interpreted.

---

Does the TLV in Section 4.1 support the identifiers described in 
[I-D.ietf-mpls-tp-itu-t-identifiers]? The chain of references for
"non-IP MPLS-TP networks" appears to lead through RFC 6428 to RFC
6370 which (I think) does not include the non-IP identifiers.

I see that you are adding new values to the Address Family registry in
Section 9.2. It is not clear to me where the definitive reference for
those types live. I would like to see more clarity in the text about
which address (and where it is specified) is intended for each new
code point that you are asking IANA to allocate.

---

Section 2 says that this is a one-way protocol, however, Section 4.2
defines an Application ID 0x0000 TLV that forms part of a two-way
exchange and is actually a direct request for information. 

Additionally, it seems a little odd to hide this in a TLV when what it
really is is a separate message type. By placing it in the TLVs you 
open up the prospect of message thrash as two implementations send
each other updates without clearing out this TLV.

The problem might be amusingly exacerbated by placing 0x0000 in the
list of Application ID for which a refresh is requested.

In any case, please explicitly clarify that setting a length of zero 
intends retransmission of the 0x0000 Application data.

---

Section 4.3

Does the flush apply to the 0x0000 Application Data previously sent?

Why do you not allow selective flushing per Application?

---

Section 4.4

   The request is strictly advisory: the
   receiver SHOULD accept and act on the request, but MAY override it at
   any time.

The "SHOULD" that you have used is stronger than "advisory". What is 
more you don't mean "advisory" you mean "a request".

I suggest you change this to

   ...the receiver MAY accept and act on the request, MAY ignore the 
   request, or MAY resume transmissions at any time according to 
   implementation or configuration choices, and depending on local
   pragmatics.

This section is another example of a TLV that violates the statement
in Section 2 that this is a one-way protocol.

Lastly, you need to say what it means if the Duration leaves the 
information as expired (i.e. the Duration takes us beyond the end of
the Lifetime). Obviously the sender (of the original information) can
decide that that would be a bad thing and retransmit anyway, but it is
not clear what the receiver (of the information) would do if the
transmission is not made.

Frankly, however, it is not clear why you have this TLV. Having gone
through the hoops of deciding that you need to be able to send the 
data, that the data can time out, and that you need to retransmit the
data, I think you need to give better motivation for this TLV.

---

I wish Sections 4.1 through 4.5 would end up containing the actual 
TLV Type codes to be used for each TLV. This could be achieved by
simply including the values in the figures. (This is safe because 
these are the first allocations from the new registry and there is
no question of IANA not allocating them as requested).

---

5.1 I like that operation of GAP SHOULD be configurable per data
channel. But you appear to have written "SHALL".  This precludes 
making an implementation that automatically and always runs GAP. 
(I think it still allows implementations that don't support GAP at
all because they don't implement this spec).

---

Paragraph 3 of section 5.2 talks about retaining objects for 
application data that can be handled. But this is implementation-
specific. I think it is only the information that has to be retained.
Try to talk about function rather than constrain implementation.

---

The last paragraph of 5.2 is a bit confusing.

   The receiver MAY make use of the application data contained in a GAP
   message to perform some level of autoconfiguration, for example if
   the application is an OAM protocol.  The implementation SHOULD,
   however, take care to prevent cases of oscillation resulting from
   each endpoint attempting to adjust its configuration to match the
   other.  Any such autoconfiguration based on GAP information MUST be
   disabled by default.

I *think* you are really trying to place constraints on future
specifications rather than on implementations (there is nothing in this
specification that could be used for auto-conf). But I note that
[I-D.ietf-mpls-tp-ethernet-addressing] looks like auto-conf and is not
disabled by default.

---

In Section 6.1, you should discuss the operation of key exchange 
protocols in the absence of IP protocols in the network. After all, one
of the use-cases (oh, the only use case :-) for GAP is to operate in
networks that don't have IP available. You might also note that GAP is
described as a protocol for exchanging capabilities and configuration:
keys might be described by some as exactly such information.

If your fall-back is that keys have to be manually configured in the
absence of IP, then you desperately need to call this out in 
[I-D.ietf-mpls-tp-ethernet-addressing] because manual configuration of
keys seems a whole lot harder than manual configuration of MAC
addresses.

---

Section 6.1

I believe that RFC 4107 says that you need to discuss mechanisms for
dynamic key update, to make a recommendation as to whether such 
mechanisms are needed, and to state how often keys need to be updated.

---

The problem I see with processing of the Timestamp described in 6.2 is
that it does not allow for a sender correcting its clock. Can you add
text to help clarify how this case is handled.

---

The final paragraph of Section 6.2 is a bit of a throw-away. What you
are saying, I think is that "Full algorithm flexibility is provided by
the protocol's use of the Authentication Key Identifier which acts as 
an indirect reference to the algorithm and key data in use. Implementors
SHOULD consider the use of [I-D.ietf-karp-crypto-key-table] for key 
management."

You might move this text to the end of 6.1 where it will sit with the
discussion of the Key ID.

---

How do you distinguish "device discovery" in Section 7 from talking to
a device whose MAC address you don't know in [I-D.ietf-mpls-tp-ethernet-
addressing] ?

Very unclear which MAC address to use when.

---

Maybe Section 8 should point at Section 6 since Section 6 seems to be
all about Security mechanisms.

---
                                                                              
You have used [I-D.ietf-karp-crypto-key-table] in a normative way. Not 
a big risk as it is currently in KARP WG last call. Please move it to
Section 10.1.

---

Trivial point, but there are precisely three authors, all marked as 
editors. You could safely dispense with the "editor" designation (on
the front page and in the Authors' Addresses section).