Subject: Re: [dhcwg] draft-kinnear-dhc-dhcpv4-bulk-leasequery-01

From: Kim Kinnear <[email-masked]>
Date: Thu, 11 Dec 2008 20:01:54 -0500

Alfred,

Thank you for the review.  I appreciate the input.

Regards -- Kim

_______________________________________________
dhcwg mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dhcwg

---

From: Alfred Hönes <[email-masked]>
Date: Fri, 12 Dec 2008 01:23:43 +0100 (MEZ)

Hello,
after studying the Internet-Draft authored/edited by you,
          draft-kinnear-dhc-dhcpv4-bulk-leasequery-01,
I'd like to submit a few comments, mostly addressing textual
flaws I found in that memo.

Besides these flaws, the basic design seems to be solid, and its
description is clear enough to allow independent, interoperable
implementations 'from scratch'.

The items below are presented in textual order.
To give more context, sometimes I quote larger blocks of text
literally and show the replacement proposed using the shorthand
notation:

   <original draft text>
---
   <modified text>

I use change bars ('|' in column 1) and occasionally
up/down pointing marker lines ('^^^'/'vvv') to emphasize
the location of textual issues and/or proposed corrections.
Modified text has been re-adjusted to match RFC formatting
rules, where appropriate.  I also try to accomodate published
RFC-Ed policy on style and punctuation etc. in my proposals.

(1)  Section 2, bullet 'clock skew'

Even NTP cannot assure that the clock skew is *zero*.
Therefore, I suggest to replace "zero" by "small" or "negligible".

(This issue recurs in Section 8.4.)

(2)  Section 3, bullets

These bullets are a continuated enumeration of properties,
introduced by the headline,

   The existing query types

Consequentially, you start the text in all four bullets in lowercase.
However, the first three bullets end with a period, and the first one
even is separated into two sentences.  However, the last bullet ends
without any ounctuation.

To achieve consistency in grammar/punctuation, I propose to replace
all periods in the first three bullets by semicolons and add a trailing
period to the last bullet.

(3)  Section 4 -- improved wording

The present draft text contains the 'alternative',
       ... also ...   ... or ...

In order to better clarify the logic of that long sentence,
I suggest to supply the somehow missing "either", giving:

                                                        [...].  The
   mechanism should also allow an Access Concentrator to retrieve
|  consolidated IP address binding information for either the entire
                                                  ^^^^^^^^
   access concentrator or for a single connection/circuit.

[The second "for" in the last line could be dropped as well
 in this case.]

(4)  Section 4.2

The term "Negative Caching" usually indicates making note of the
*absence* of specific information (for the duration of a certain
time span).  In Section 4.2, it is not clear what exactly is
'neagtively caches' in this context.  In particular, the sentence ...

   If Leasequeries result in negative caches, ...

... is rather vague and imprecise.

I would like to see a better explanation of what is meant with
that term in this context.

(5)  Section 4.3 -- missing article

I suggest to insert the definite article in front of "fast path":

|  If Antispoofing is not done in fast path, it will become a bottleneck
   and may lead to denial of service of the access concentrator.  The
   Leasequeries should make it possible to do antispoofing in fast path.
---                              vvvvv
|  If Antispoofing is not done in the fast path, it will become a
   bottleneck and may lead to denial of service of the access
   concentrator.  The Leasequeries should make it possible to do
   antispoofing in fast path.

(6)  Section 4.4, 2nd paragraph -- missing article

Similarly as above, I suggest to modify:

|  Bulk Leasequery allows specification of a query-start-time as well as
   a query-end-time.  Use of query-times allows a network element that
   periodically commits information to stable storage to recover just
   what it lost since the last commit.
---                      vvvvv
|  Bulk Leasequery allows the specification of a query-start-time as
   well as a query-end-time.  Use of query-times allows a network
   element that periodically commits information to stable storage to
   recover just what it lost since the last commit.

(7)  Section 5 -- missing important details

In the protocol overview, I miss mention of the 'normal' response
messages -- it takes ubtil section 7.2 until the reader not fluent
with the basic Leasequery protocol details becomes aware of the
fact that DHCPLEASEUNASSIGNED and DHCPLEASEACTIVE are the 'normal'
response messages used.

Perhaps, an explantion to this end should be inserted after the
fourth paragraph, before the paragraph about DHCPLEASEQUERYDONE,
in order to reflect the usual temporal ordering of messages.

(8)  Section 5, 6th paragraph -- nits

In the present 6th paragraph, I suggest to remove the comma
(not needed in two-party enumerations) and insert another "by"
in front of "Client Identifier", for clarity:

|  Bulk Leasequery supports queries by MAC address, and Client
   Identifier in a way similar to [RFC4388].  The Bulk Leasequery
   protocol also adds several new queries.
---                                               v   vvvv
|  Bulk Leasequery supports queries by MAC address and by Client
   Identifier in a way similar to [RFC4388].  The Bulk Leasequery
   protocol also adds several new queries.

(9)  Section 6

Ther oreder of the last two paragraphs is a bit surprising.
Wouldn't it be better to first describe the general bits and
the normal case (presently in the 5th (last) paragraph) and
subsequently the peculiarities (currently in the 4th paragraph?

To this end, I suggest to simply swap these two paragraphs,
and   s/above/below/  in the present 6th paragraph.

(10)  Section 7.2.2 -- language improvements

In the first entry line of the status code table,
I suggest to insert articles for clarity, as follows:

|    Success         000 Success.  Also signaled by absence of
|                        dhcp-message option.
---
|    Success         000 Success.  Also signaled by the absence of
|                        a dhcp-message option.
                         ^^                        ^^^^^

(11)  Section 7.2.8, last paragraph -- spurious word replication

Please    s/the the/the/

(12)  Section 7.3, table layout

To enhance the readability, please align the table heading with
the table columns and span, for instance:

     Parameter             Default  Description
     ------------------------------------------
     BULK_LQ_CONN_TIMEOUT      30 secs  Leasequery connection timeout
     BULK_LQ_QUERY_TIMEOUT     30 secs  Leasequery query timeout
     BULK_LQ_MAX_CONNS         10       Max Leasequery TCP connections
     BULK_LQ_MAX_CONN_RETRY    60 secs  Max Leasequery retry timeout
     BULK_LQ_DATA_TIMEOUT      30 secs  Leasequery data timeout
---
     Parameter                Default   Description
     -----------------------------------------------------------------
     BULK_LQ_CONN_TIMEOUT     30 secs   Leasequery connection timeout
     BULK_LQ_QUERY_TIMEOUT    30 secs   Leasequery query timeout
     BULK_LQ_MAX_CONNS        10        Max Leasequery TCP connections
     BULK_LQ_MAX_CONN_RETRY   60 secs   Max Leasequery retry timeout
     BULK_LQ_DATA_TIMEOUT     30 secs   Leasequery data timeout

[ Above, the middle column has been left-shifted by one character
  position as well in order to better center it! ]

(13)  Section 8.2

(13a)  1st para

For enhanced readability, I suggest to shuffle the wording a bit
in the first sentence (and insert the missing article):

   Bulk Leasequery is designed to create a connection which will
   transfer the state of some subset (or possibly all) of the IP address
|  bindings to the requestor from  DHCPv4 server.  [...]
---         ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
   Bulk Leasequery is designed to create a connection which will
   transfer the state of some subset (or possibly all) of the IP address
|  bindings from the DHCPv4 server to the requestor.  [...]
            ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^

(13b)  3rd paragraph -- clarification

The draft presently says:

   A DHCPBULKLEASEQUERY request is constructed of one of a series of
   primary queries and the optional addition of one or more qualifiers
   to those primary queries.

The term "series" is confusing in this context.
It will inevitably raise the expectation for having *multiple*
queries allowed in a single DHCPBULKLEASEQUERY.  Reading on, over
the next few pages it becomes clear that that doesn't make sense.

To aviod this possible misconception, I suggest to simplify the 3rd
paragraph and slightly improve the wording in the subsequent (4th)
paragraph; taken together these comprise all necessary specification.

   A DHCPBULKLEASEQUERY request is constructed of one of a series of
   primary queries and the optional addition of one or more qualifiers
   to those primary queries.

   The possible primary queries are listed below.  Each
   DHCPBULKLEASEQUERY request MUST consist of only one of these primary
   queries.
---
|  A DHCPBULKLEASEQUERY request is constructed of one primary query and
|  optionally one or more qualifiers for it.

   The possible primary queries are listed below.  Each
|  DHCPBULKLEASEQUERY request MUST contain of only one of these primary
   queries.
                                   ^^^^^^^

(14)  Section 8.3, response message bullets

According to the *usual* temporal order of appearance, I propose
to move the DHCPLEASEQUERYDONE bullet behind the DHCPLEASEACTIVE
and DHCPLEASEUNASSIGNED bullets.

The DHCPLEASEUNKNOWN bullet does not fit under the introductory
sentence,

   The response messages generated by a DHCPBULKLEASEQUERY request are:

Therefore, I suggest to turn that bullet into normal, non-indented
text:

      o DHCPLEASEUNKNOWN

        The DHCPLEASEUNKNOWN message MUST NOT appear in a response to a
        Bulk Leasequery.
---
   The DHCPLEASEUNKNOWN message MUST NOT appear in a response to a Bulk
   Leasequery.

(15)  Section 8.4, 3rd paragraph

That para says at its end:

           [...].  As such, it is an ideal time to save and use as input
   to a DHCPBULKLEASEQUERY in the query-start-time or query-end-time
   options, should the requestor need to ever issue a DHCPBULKLEASEQUERY
|  message using those options as part of the query.
                                          ^^^

That is confusing; which query is meant there?
I strongly suspect you mean future ones, and therefore suggest
to silghly modify the wording to day:

           [...].  As such, it is an ideal time to save and use as input
   to a DHCPBULKLEASEQUERY in the query-start-time or query-end-time
   options, should the requestor need to ever issue a DHCPBULKLEASEQUERY
|  message using those options as part of a later query.
                                          ^^^^^^^

[ You might use "future" in place of "later", if you prefer. ]

(16)  Section 8.7, 2nd paragraph

The text suddenly turns from present tense to future.
I suggest to   s/will not support/does not support/   (2 instances).

(17)  Sections 8.7 and 8.8

I miss the discussion of the TCP TIME_WAIT peculiarity.
With TCP, the peer that actively closes the connection needs to
retain state for a significant time span, in order to avoid
connection clashes of a future connection using the same transport
identifier (quintuple of transport protocol, source & dest addrs &
ports) by delayed packets still lingering around in the network.

Therefore, as recent discussions in TSVWG and TCPM have shown,
it is important to have, by application design, all CLOSEs
ordinarily be performed by the TCP client, thus saving the
server from keeping state for potentially many past clients.
Application designers and implementers -- in this case DHCP
server and client implementers -- should be reminded that a
DHCP server should only actively close a bulk leasequery TCP
connection if it is able and willing to dedicate the protocol
(kernel) resources for the fulle TIME_WAIT period (~4 min, per
RFC 793 and STD 3).  Therefore, besides under severe kernel memory
pressure, it does not make much sense for a DHCP server to actively
close a TCP connection that reasonably might be used for another
bulk leasequery transaction within a time span of similar magnitude.
Under normal operations conditions, it should always be the
DHCP bulk leasequery client that actively colses the TCP connection
and thereby undertakes dedicating TIME_WAIT system resources.

(18)  Section 9.2 -- textual improvements

(18a)  6th paragraph

I suggest to change, by shuffling words:

   A Bulk Leasequery response MUST contain no more than one message for
   each configured IP address in the DHCPv4 server.  [...]
---     ^^^^^^^^^^ ^^^^^^^^^^
   A Bulk Leasequery response MUST contain no more than one message for
   each IP address configured in the DHCPv4 server.  [...]
        ^^^^^^^^^^ ^^^^^^^^^^

(18b)  9th paragraph

Please insert the missing "be" :

        [...].  Note that the descriptions of the primary queries below
|  must constrained by the actions of any of the three qualifiers
   described subsequently as well.
---
        [...].  Note that the descriptions of the primary queries below
|  must be constrained by the actions of any of the three qualifiers
       ^^^^
   described subsequently as well.

(18c)  'primary query' bullets

Following the explanations by the RFC Editor in its IETF tutorials
the many instances of 'which' in these bullets should be changed
to "that"; to avoid 'epidemic invasion' of "that"s, I recommend
to always change "which matches" into "matching".
For instance, in the first bullet:

      o Query by MAC address

|       Every IP address which has a current binding to a DHCPv4 client
|       which matches the chaddr, htype, and hlen in the
        DHCPBULKLEASEQUERY request MUST be returned in a DHCPLEASEACTIVE
        message.
---
      o Query by MAC address

|       Every IP address that has a current binding to a DHCPv4 client
|       matching the chaddr, htype, and hlen in the DHCPBULKLEASEQUERY
        request MUST be returned in a DHCPLEASEACTIVE message.

Similarly, in the second paragraph of the 5th bullet, I recommend to
    s/which restrict/restricting/ .

(18d)   'query option' bullets

Again,  I recommend to   s/which/that/g  .

In the last paragraph of the 3rd bullet, I recommend a small
clarification, yb inserting "a VPN" and a comma:

     In all cases, if the information returned in a DHCPLEASEACTIVE or
|    DHCPLEASEUNASSIGNED message is for other than the default a vpn-id
     option MUST appear in the packet.
---
     In all cases, if the information returned in a DHCPLEASEACTIVE or
|    DHCPLEASEUNASSIGNED message is for a VPN other than the default,
                                       ^^^^^^^                      ^
     a vpn-id option MUST appear in the packet.

(19)  Section 9.3 -- textual improvements

(19a)  second paragraph

I recommend replacing  "the identical for"  by either  "identical for"
or  "the same for" .

(19b)  bullet #1

Please correct, and insert a comma for clarification:

     ... the message being returned in DHCPLEASEACTIVE then ...
---
     ... the message being returned is DHCPLEASEACTIVE, then ...
                                    ^^                ^
(19c)  bullet #4

The draft says:

      4. If the dhcp-lease-time option code appears in the dhcp-
         parameter-request-list, the DHCPv4 server MUST include a dhcp-
         lease-time option for any state that has a time-out value
|        associated with it, and not just appear in a DHCPLEASEACTIVE
                             ???????????????????
         message.  Thus, [...]

The final half-sentence does not parse, and does not make proper
sense to me.  Please look for a clarification.

(20)  Section 11

Following the spirit of BCP 26, RFC 5226, I strongly recommend that
you chose a *unique* placeholder for each code point to be assigned
by IANA, e.g., "TBA1", "TBA2", ... (to be assigned).
Doing so helps avoiding trouble caused by clerical errors that
-- as the past has painfully shown -- otherwise might recur during
the final editorial processing cleanup after IANA assignment.

In bullet #11, please correct the spelling error:

    s/Addtional/Additional/
                   ^

(21)  Sections 13.*

Based on trouble observed in another thread, I recommend that you
indeed follow the 3rd paragraph of the page-1 boilerplate text
and add a literal "work in progress" (perhaps in parentheses) to
the I-D citations you quote.
As has been said: "The IETF should eat its own doggy-food!"

(22)  Appendix -- many textual improvements

(22a)  1st bullet

-   s/when client sends/when the client sends/
                            ^^^^^

(22b)  2nd bullet, 2nd and 3rd paragraph

-  s/because because/because/

-  I suggest to alays replace  "come"  by  "arrive"  (for packets).

-  please insert the missing article in

      ... until the lease query response ...
               ^^^^^

(22c)  2nd bullet, 5th paragraph

   s/packets that uses/packets that use/
                     ^

(22d)  2nd bullet, 6th and 7th paragraph

Please insert the missing articles tagged below:

   In a spoofing type of attack, negative caching information may grow
|  considerably if the attacker varies the source IP address.  For each
                  ^^^^^                            vvvvv
|  such new source IP address, traffic will come to the slow path, a new
|  lease query needs to be initiated, the response will be processed,
                                     ^^^^^
   and negative caching needs to be done.  That will mean using many
   resources for negative caching.

   [RFC4388] suggests that if the DSLAM knows the network portion of the
   IP addresses that are assigned to its clients, then some amount of
|  antispoofing can be done in the fast path and some lease queries may
                              ^^^^^
   be avoided.  But as indicated before, that information may not always
   be available to DSLAMs.

(22e)  2nd bullet, 8th paragraph

The draft says:

|  [RFC4388] says that DHCPv4 server should be protected from being
                                   ^^                vv
|  flooded with too many Leasequery requests and DSLAM also should not
   send too many lease query messages at a time.  This would mean that
   legitimate requestors may be excessively delayed getting their
|  information in the face of antispoofing attacks.
                              ^^^^^^^^^^^^
In the fitst sentence, IMO plural should be used.
In the last sentence did you really mean "antispoofing attack",
not "spoofing attack" ???

Therefore, I recommend to modify that paragraph to say:

|  [RFC4388] says that DHCPv4 servers should be protected from being
                                   ^^^               vvv
|  flooded with too many Leasequery requests and DSLAMs also should not
   send too many lease query messages at a time.  This would mean that
   legitimate requestors may be excessively delayed getting their
|  information in the face of spoofing attacks.
                              ^^^^^^^^

(22f)  3rd bullet, 1st paragraph

I suggest inserting a couple of missing articles, and a comma
for clarity:

                                                     [...]. (Note that
|  the associated-ip option does not appear in responses for a query by
   ^^^^              vvvvv                    vvvvvv        ^^^
|  IP address).  With the associated-ip option, the DSLAM can get
   information not only about the IP address/MAC address that triggered
   the Leasequery but also about other IP addresses that are associated
   with the original MAC address.  That way, when traffic that uses the
|  other IP addresses comes along, the DSLAM is already prepared to deal
   with them.
                                  ^^^^^

(22g)  3rd bullet, 2nd paragraph

The comma after "Although" should be removed -- "is better" belongs
to that leading word and should not be separated from it by that comma.

Near the end of the paragraph,

   ... periodic querying with DHCPv4 server, ...

should better say:

   ... periodic querying the DHCPv4 server, ...
                         ^^^

Ough! -- That's it for this time.

Best regards,
  Alfred Hönes.

-- 

+------------------------+--------------------------------------------+
| TR-Sys Alfred Hoenes   |  Alfred Hoenes   Dipl.-Math., Dipl.-Phys.  |
| Gerlinger Strasse 12   |  Phone: (+49)7156/9635-0, Fax: -18         |
| D-71254  Ditzingen     |  E-Mail:  [email-masked]                     |
+------------------------+--------------------------------------------+