Thanks for the detailed review - you caught some good stuff. Most of this makes
essence but we should probably talk about usage of 2119 language.
On Aug 9, 2011, at 16:05 , Mary Barnes wrote:
simple
=======================================================================
Document: draft-ietf-p2psip-base-17
Reviewer: Mary Barnes
Review Date: 9 August 2011
IETF LC End Date: 22 July 2011
Summary: Not Ready.
Comments:
----------------
The document is very a dense (with detailed technical information) and long
(150 page) specification for a Peer-to-peer signaling protocol. While the
overall technical functionality seems fairly correct and thoroughly
specified, the primary issue is a tremendous lack of normative language in
the main body of the document. Non-inclusive details of such are included
below.
The 2119 MUST/SHOULD/MAY terms are simply abbreviations for some words defined
in 2119, and different WGs have different styles about how extensively they
should be used. P2PSIP has obviously been on the more sparing side of that
spectrum. This isn't to say that there aren't any places where it would be
useful to add such language, but rather that our policy has been to add it
principally where there is likely ambiguity, rather than everywhere where
behavior is defined. I'll work thought these and see where they might help
reduce the chance of a an implementers doing the wrong thing but in generally
when we define a structure in something like ASN.1 or ABNF, if the structure
always has a field X, we just use the language like ASN.1 or ABNF to indicate
it always has that. We don't also say it MUST be there.
Major Issues:
-------------------
General:
--There is a fair number of cases where normative language should be used. I
have detailed some (but certainly not all) instances where it is lacking
with proposed rewording. I have also noted cases where it is used (lower
case) where it could be confused with needing normative language and where it
isn't necessary and there are some cases where it's just not clear. I
consider these major since it can impact interoperability and could lead
developers to not implement key functionality.
--There is also little or no normative language for the majority of the
elements in the structures - i.e., the reader has to assume which elements
are mandatory and which are optional and it's not always clear. For
example, rather than saying "Field x contains blah", it should say "Field x
MUST contain blah" and rather than "The contents of the message will be x, y
and z", it should say something like "The message MUST (or SHOULD) contain x,
y and z". I have not detailed the places where this is necessary in most
cases. The authors should review the detailed definition for each of the
messages.
However, the syntax actually tells you whether a field is mandatory or not.
I.e.,
there is a "select/case" syntax for indicating optional fields (and some fields
are
variable length). So I don't think it's really ambiguous in the general case
whether
a field need be present.
With that said, this seems like something that would be better discussed real
time - I may be missing the point on some of these. When would be a good time
to have a call?
--The message names are used inconsistently. The IANA registry has the
message codes all lower case. There are places in the examples and the body
of the document where the _req part is missing, there is no "_", the first
letters are Upper case, etc. The messages are also used in a general sense -
e.g., "MUST perform an Attach", which should really be stated as "MUST send
an attach_req message)
Well, it's the Attach is much more than sending a attach_req and we found that
trying to add more just make the document even longer and harder to understand
than it currently is. We been pretty careful about the case as some systems
auto generate code out the the structures in the draft. I'm glad to walk
through these and figure them out with you. That said, these have been changed
various times and it's hard to find them all so there could certainly be a
mistakes in this.
Major - Detailed:
- Sections 32.1, 3.2.2 and 3.3: lots of changes needed to reword using
normative language.
- Section 3.4, last paragraph: Needs to be reworded normatively and more
precisely. It's really not clear to me what the required behavior is as it
first says it needs to (which I'm not sure is a SHOULD or is REQUIRED to)
maintain connections to all the peers near and enough others.., but how much
is "enough". Also, what is meant by the "specified link set" - is that
referring to all the nearby peers or the peers+enough others? Or is this
more clearly (and normatively) specified elsewhere, such that a reference
could be added
All of section 3 is overview and not meant to have the normative descriptions
which happen in the later sections.
- Section 4.2: needs normative language - i.e.:
--"Each Usage needs to …" -> "Each Usage MUST…"
This gets into the whole 2119 usage of words in requirements. Right here we are
writing requirements for what future RFC needs to include.
- Section 5.1.1: Bullet items need normative language
-- 1st bullet: It's also not clear what the its are in "it passes it up". I
think it should be "the node passes the message up to the upper layers".
However, this probably needs to be normative language - i.e., "the node MUST
pass the message to the upper layers". The same applies to the third bullet.
--bullet 2: "the message is destined for this node and is passed up…" ->
"the message is destined for this node and MUST be passed….,
members of the WG objected to MUST level mandating of how the actually server
software was architected. Note this passing up a level is not about anything to
do with what happens on the wire. See section 6 of 2119 which pretty much says
not to use MUST this way.
- Section 5.1.2:
-- Paragraphs 5, 6, 7 and 8 need to be reworded to have normative statements.
Let's talk on the phone - I'm not seeing the issue here.
-- Last paragraph (9) mixes normative text with in an example. The normative
text needs to be separated from the example
seriously? I'm just not getting how this will help make it more interoperable
but again, glad to change if we can figure out a real improvement.
- e.g., split some of the sentences into the normative behavior and then the
example.
-Section 5.2.1 is totally inconsistent with the use of normative language
(i.e., lower case occurrences and lack of KEY words) - shouldn't the
following be normative statements something like the following:
-- First paragraph:
---First sentence: "a node constructs" -> "a node MUST construct".
--- 3rd sentence: "resulting message will use the normal overlay routing" ->
"resulting message MUST use the normal overlay routing"
--- 4th sentence: "The node can also" -> "The node MAY also"
-- Third paragraph:
--- 3rd sentence: "a single request can also" -> "a single request MAY also"
-- Fourth paragraph:
--- 1st sentence: this could be interpreted as normative: "Because messages
may" -> "Because messages can"
2119 is very clear it refers to MAY and not may. I really don't know what word
is best here but ignoring 2119, which word looks better "may" or "can" for this
sentence?
--- 3rd sentence: "the request is retransmitted" -> "the request MUST be
retransmitted"
This all looks like internal software design to help
-Section 5.3.2:
-- General: some of the contents of the structure have normative language and
others don't. There is some normative behavior defined for these fields
later, so perhaps adding section references as you do for some fields would
be helpful.
For example:
--"overlay": shouldn't this be normative: "the overlay name is hashed with
SHA-1" -> "the overlay name MUST be hashed with SHA-1"
--"destination_list": seems like some of this should be normative?
-Section 5.3.2.1:
--1st paragraph: shouldn't this be normative?
"the configuration document has a sequence number" -> "the configuration
document MUST contain a sequence number"
-- 1st paragraph, last sentence: "may" -> "can"
-Section 5.3.2.2:
--Paragraph after the 1st structure: Shouldn't this be stated normatively
since it is described how the structure MUST be handled depending upon the
contents?
--Paragraph after second structure, suggest the following changes:
---"the generation counters should" -> "the generation counters SHOULD"
---"An implementation could use.." -> "An implementation MAY use…"
---"the node confirms that the generation counter matches" -> "the node MUST
confirm that the generation counter matches"
---"If it does not match, it is silently dropped." -> "If it does not match,
it MUST be silently dropped."
-Section 5.3.3.1:
--1st para: "a request returns" -> "a request MUST return"
--1st para: "then the message code is the response code that matches the
request" ->
"then the message code MUST be set to the response code that matches the
request"
--2nd para: "message code is set" -> "message code MUST be set"
-Section 5.3.4 - needs normative language:
--1st paragraph after GenericCertificate structure:
---"All signatures are formatted" -> "Alls signatures MUST be formatted"
---"The input structure…varies.." -> "The input structure … MAY vary…"
-Section 5.4.1: "need to be described" -> "MUST be described"
-Section 5.4.2-Section 7. These sections need to be carefully reviewed and
updated appropriately with normative text. [ I have marks up for pages
55-102 where normative language is needed if someone wants those scanned and
forwarded - I'm just too lazy to type them all out.]
- Section 8: There is only one actual normative word used in this section!
There is a lc "should" that ought to be "SHOULD". I don't know if the
authors assumed that the normative language in TURN was sufficient, but I do
not believe that is the case as it leaves the normative behavior to be
inferred by the reader/implementor. Rather than rewrite the whole section
myself, the authors need to review carefully. For example, there are "needs
to", which likely are really and lots of present tense statements (e.g., "The
address of the peer is stored…"), which ought to be MUSTs or SHOULDs.
- Section 9.2: seems like this needs normative language. Suggest the
following:
OLD:
For this Chord based topology plugin, the size of the Resource-ID is
128 bits. The hash of a Resource-ID is computed using SHA-1
[RFC3174]then truncating the SHA-1 result to the most significant 128
bits.
NEW:
For Chord-RELOAD, the size of the Resource-ID MUST be
128 bits. The hash of a Resource-ID MUST be computed using SHA-1
[RFC3174]. The SHA-1 result MUST be truncated to the most significant
128 bits.
- Section 9.3: Needs normative language.
OLD:
If a peer is not responsible for a Resource-ID k, but is directly
connected to a node with Node-ID k, then it routes the message to
that node. Otherwise, it routes the request to the peer in the
routing table that has the largest Node-ID that is in the interval
between the peer and k. If no such node is found, it finds the
smallest Node-Id that is greater than k and routes the message to
that node.
NEW:
If a peer is not responsible for a Resource-ID k, but is directly
connected to a node with Node-ID k, then it MUST route the message to
that node. Otherwise, it MUST route the request to the peer in the
routing table that has the largest Node-ID that is in the interval
between the peer and k. If no such node is found, it MUST find the
smallest Node-Id that is greater than k and MUST route the message to
that node.
- Section 9.4: Needs normative language.
- Section 9.5: While this section does have a fair amount of normative
language, it is still lacking in a few cases:
--Item 2.: "should" -> "SHOULD"
--Item 5.: "The AP sends the response…" -> "The AP MUST send the response…"
--1st paragraph after list: "…can directly connect…" -> "MAY directly connect"
--2nd paragraph after list: "it can still populate" -> "it MAY still populate"
--2nd paragraph after list: the description of the use of PING should be
reworded with normative language (I'll leave that exercise to the authors)
--3rd paragraph after the list: "JP simply sends" -> "JP MUST send"
- Section 9.7.1:
--1st para: ".., it then sends…" -> ".., it then MUST send …"
--3rd para: needs normative language
OLD:
In no event does
an attempt to reestablish connectivity with a lost neighbor allow the
peer to remain in the neighbor table.
NEW:
In the case of
an attempt to reestablish connectivity with a lost neighbor, the peer
MUST not remain in the neighbor table.
OR:
In the case of
an attempt to reestablish connectivity with a lost neighbor, the peer
MUST be removed from the neighbor table.
-- 4th para: "should" -> "SHOULD" (2 cases), "use Pings" -> "MUST use Pings"
-Section 9.7.2:
--1st para: "..the failed peer are removed.." -> "…the failed peer MUST be
removed.."
--2nd para: "…the peer initiates…" -> "…the peer MUST initiate.."
-Section 9.7.3:
--1st para after list: "is needed" -> "is REQUIRED"
--3rd para after list: "the peer sends an Update" -> "the peer MUST send an
Update"
-Section 9.7.4.4: Needs a scrub for normative language
-Section 9.9: Needs a scrub for normative language.
-Section 10.1: General: Needs a lot of work to be specified using normative
language. The majority of the configuration elements are not specified using
normative language - for example, terms such as permitted, allowed, valid are
used in statements rather than normative language
or they are indicative of the need for normative language. A specific
example is in the definition for "clients-permitted":
OLD:
This element represents whether clients are
permitted or whether all nodes must be peers. If it is set to
"true" or absent, this indicates that clients are permitted. If
it is set to "false" then nodes are not allowed to remain clients
after the initial join.
NEW:
This element represents whether clients are
permitted or whether all nodes need to be peers. If clients
are permitted, the element MUST be set to "true" or absent.
If the nodes are not allowed to remain clients after the
initial join, the element MUST be set to "false".
--Section 10.1 - 4th para after the element list states that "All of the non
optional values MUST be provided". However, the list of elements in the
previous paragraph are not specifically identified as non optional. I think
the first 3 are mandatory. So, I would suggest to re-word the 3rd paragraph
as follows and delete the 1st sentence in the 4th paragraph noted above:
OLD:
In addition, the kind element contains the following elements:
max-count: the maximum number of values which members of the overlay
must support.
data-model: the data model to be used.
max-size: the maximum size of individual values.
access-control: the access control model to be used.
max-node-multiple: This is optional and only used when the access
control is NODE-MULTIPLE. This indicates the maximum value for
the i counter. This is an integer greater than 0.
NEW:
In addition, the kind element MUST contain the following elements:
max-count: the maximum number of values which members of the overlay
must support.
data-model: the data model to be used.
max-size: the maximum size of individual values.
access-control: the access control model to be used.
The kind element MAY also contain the following element:
max-node-multiple: if the access control is NODE-MULTIPLE,
this element MAY be included. This indicates the maximum value for
the i counter. It MUST be an integer greater than 0.
--Section 10.1, next to last para: There's no normative language, but it
seems there should be since it is specifying specification computations and
encoding that I believe are required.
- Section 10.2: Needs more normative language:
-- 2nd para:
OLD: This value is provided by the user or some other out of band
provisioning mechanism.
NEW: This value MUST be provided by the user or some other out of band
provisioning mechanism."
-- 3rd para:
OLD:
Once an address and URL for the configuration server is determined,
the peer forms an HTTPS connection to that IP address.
NEW:
Once an address and URL for the configuration server is determined,
the peer MUST form an HTTPS connection to that IP address.
-- 4th para: "which replaces" -> "which MUST replace"
-- 5th para: "nodes obtain the" -> "nodes MUST obtain the"
- Section 10.3:
-- 3rd para: "is performed" -> "MUST be performed".
-- 3rd para: Bullets need to be modified to normative.
-- 4th para: "the certificate contains" -> "the certificate MUST contain"
-- para after 2nd set of bullets: needs normative language.
-- 2nd para after 2nd set of bullets: "The node then reads…" -> "The node
MUST then use…"
- Section 10.5:
--Second para: "the joining node first forms" -> "the joining node MUST first
form"
--3rd para: text uses the phrase "it can note the Node-ID in the response
and use this Node-ID to start sending requests". It's not clear whether the
use of the Node-ID is a MAY or a MUST.
- Section 12. I'm assuming that the security directorate has reviewed this in
detail, thus my focus in reviewing this section was on general understanding.
The biggest issues is that there is NO normative language at all in the
security section. As in other sections, terms like "need", "are/is
used/sent, etc.", "ensure", lower case reserved words rather than upper
case, etc. are used when normative language ought to have been used.
- Section 13. IANA Registrations:
- General: there seem to be several registries for which the
usage/requirement is unclear as there's no mention of such in the body of the
document. In general, references to the sections in the document describing
the information included in the registries would be more than helpful.
- Section 13.5: Creates a RELOAD Application Registry, but the use and need
for such isn't described elsewhere in the document
Section 5.5.2.1 defines the usage of this and contains the text
A 16-bit application-id as defined in the Section 13.5. This
number represents the IANA registered application that is going to
send data on this connection.
- Section 13.12. Forwarding Options Registry. SEction 5.3.2.3 seems to define
three flags for the Forwarding options type, so it's not clear to me why this
registry has just "invalid" and "reserved"
The registry is for the ForwardingOptionsType code points and not the flags.
Any new forwarding option could use any of these flags. The flags only indicate
who needs to understand the the forwarding option.
- Section 13.15: Defines/registers a RELOAD URI schema but there is no
mention of this URI anywhere else in the document. In what context is it
used?
It would be used by other specifications that use Reload. It's not used inside
of reload itself. Folks wanted it defined here so we kept it consistent and
other specs could use the URI.
Minor Issues:
---------------------
- Section 1.2.1, 2nd paragraph: I don't understand the example as to why a
single application requires multiple usages - i.e, why voicemail? Isn't the
intent to say that an application might need to use both SIP and XMPP - i.e.,
you wouldn't define a "usage" for an application, would you?
storing voicemail in the DHT would require a different usage than the type of
rendezvous service of XMPP or SIP
- Section 2. Some of the definitions use terms that are not yet defined, so
that's not particularly helpful - i.e,. Attach and Update are used. In some
cases (e.g., Connection Table), you could just delete the statement with the
reference.
I'm not sure how to resolve this. I agree that by the time we get to the
definition, if you have not read the rest of the document, the sentence, " This
includes nodes with which Attach handshakes have
been done but which have not sent any Updates." does not currently mean
something that you can understand. But after reading the document, this sense
helps clear up something implementors were confused about so I definitely don't
want to loose it. I'd like to fix this but not sure what you are suggesting.
Are you suggesting just saying in these terms are defined later with a forward
reference and leaving the sentence there?
- Section 3.3, last paragraph. Add a reference to 5.4.2.4 after "RouteQuery
method"
- Section 5.1: "If any of these are incorrect…" - need to specify what
qualifies as "incorrect" - is it wrong formatting, invalid header field
value, etc.?
- Section 5.1.2:
--1st paragraph: I have no idea what the "other three cases" references.
- Section 5.2: "may be configured" -> "can be configured", "alternative
routing algorithms may be selected" -> "alternative routing algorithms MAY be
selected"
- Section 5.3: The three parts of the messages are listed and the last
sentence says that "The following sections describe the format of each part
of the message", but there are 4 sub-sections in 5.3, with the first section
NOT describing one of the three parts of the messages. I suggest to strike
that last sentence and just put references to the sections in the list items
for each of the three messages.
-Section 5.3.3:
--"critical" description: "Whether this extension must be.." "Whether this
extension needs to be…"
-Section 5.3.4: 1st para after certificate structure: "may contain" -> "MAY
contain"
-Section 5.5: It would be helpful to highlight that foundation, priority and
type are based on ICE, in particular since ICE is not discussed until the
subsequent section - e.g., "the foundation production" -> "the ICE foundation
production".
- Section 5.6:
-- The last three paragraphs are confusing.
---In particular, it's not clear what algorithms are being referenced in this
sentence:
"We will first define each of these algorithms, then
define overlay link protocols that use them."
--- In the note paragraph, I believe "These protocols have been chosen…" is
referring to the three overlay link protocols supported by RELOAD per this
document.
---The Note to implementors paragraph seems out of context
--- Per my next comment, I suggest to move the detailed discussion of future
overlay link protocols to the appendix, so some of this text may be relevant
only to that.
-Section 5.6.1: I suggest that the sub-sections in this section be moved to
an appendix and a reference to such be added.
-Section 5.6.3.1.1, next to the last paragraph:
"when a link may be failing" -> "when a link might be failing"
- Section 9.1: While this is an overview, it seems to me that some of this is
describing normative behavior (that I don't see specified else). Per the
general comment, I would reword the first sentence as follows:
OLD:
The algorithm described here is a modified version of the Chord
algorithm.
NEW:
The algorithm described here, Chord-RELOAD is a modified version
of the Chord algorithm.
- Section 10.3, 1st para: "required" -> "REQUIRED"
- Section 10.3.1, last sentence: "must" -> "MUST"
- Section 10.4, 1st para, last sentence: "should use" -> "SHOULD use"
- Section 11 - examples:
--General - there are no detailed messages shown anywhere for the examples.
There should be at least one, in particular given the comment above about the
RELOAD URI that is registered, but not mentioned anywhere else in the
document.
--first example (see my nit below about the examples not being numbered or
labeled in any way): The third sentence says that "It gets routed to the
admitting peer (AP), yet the flow shows that the message first gets routed to
the PP and then onto AP. It would be helpful if that were clarified.
- Also (general), the text mentions the use of ICE. It's not clear to me
exactly where in the flow that happens - it would be good to annotate such
(i.e., in a similar manner as the TLS is illustrated).
- next to the last example (text on page 131, diagram on page 132): I'm quite
confused as the text states that JP has received an update from AP and thus
now knows APs predecessors, which are also JP's predecessors, so JP Attaches
to them. However, the diagram does not show JP getting an Update from AP and
it shows JP doing Updates to PPP and PP rather than Attaches (as the text
suggests).
- Section 13.10. Overlay link types: These specific string types are not used
elsewhere in the document. Suggest that a reference to 5.6 be added and
these particular strings be included in parenthesis after the items in the
list.
makes sense
- Section 13.16, Security considerations portion:
-- The first sentence doesn't make sense. Did you mean?
"This media type is typically not used to
transport information that needs to be kept confidential,
however, there are cases where the integrity of the information is
important."
yes - that is much better - this text was added late and received little review
-- "then the contents of the file need to be…" -> "then the contents of the
file MUST be …"
Nits:
-----
- General: kind is sometimes "Kind" and sometimes "kind". The former is more
readable and highlights it as a reserved word.
Seems reasonable
- Section 2.
--General: It would be helpful if the terms were alphabetized.
this is easy to do but complicates the forward reference problem. Thoughts on
what is best?
--Node: "We use the term "Node" to refer to …" -> "Refers to …"
- Section 3.2:
-- 2nd para, last sentence: "refer such" -> "refer to such"
that needs to b fixed
-- 3rd para, 1st sentence: "concept" -> "entity"
- Section 4.1.4,
(this is section 4.1.1 for others reading this)
4th paragraph: I'm still not getting the applicability of voicemail as a
RELOAD Kind (per the reference also in section 1.2.1). I guess you are
talking about a voicemail message (in this case) and a server or application
in section 1.2.1? Voicemail just does not seem like an obvious example to
me.
All it is trying to say is the size of a storing the audio for a voicemail in
the overlay is much larger than saving a link. Note that several people in the
WG think it is nuts to try and save a full voice message in the overlay and
several others think the opposite. This argument was avoid by explicitly
mention in this draft the example of voicemail.
- Section 5.1.1, last bullet: "In that case,…" -> "In the latter case, …"
make sense
- Section 5.1.2:
-- 2nd para: "arrange" -> "ensure", "This may be arranged" -> "This can be
arranged…"
make sense
-- 3rd para (1st after bullets): It's not clear that A…E is a set of ordered
nodes in the example. I would suggest to reword as:
OLD:
As an example of the first strategy, if node D
NEW:
Consider an example with nodes A, B, C, D and E, respectively. If
node D….
ok
- Section 5.3.1.1: First two sentences can be reworded more succinctly -
you're not really providing definitions here, put rather describing the
the syntax for and providing examples illustrative of the presentation
language.
OLD:
The following definitions are used throughout RELOAD and so are
defined here.
we really are defining opaque and true and false here
They also provide a convenient introduction to how to
read the presentation language.
NEW:
This section provides an introduction to the presentation language
used throughout RELOAD.
- Section 5.3.3.1: Error_Data_Too_Old is duplicated.
that's embarrassing :-)
- Section 5.5.1.6: Add references for SCTP and DCCP.
- Section 6.4.1.3: The reference to "this Version" is superfluous. Propose
the following change:
OLD:
This version of RELOAD (unlike previous versions) does not have an
explicit Remove operation.
NEW:
RELOAD does not have an explicit Remove operation.
make sense
- Section 9: General. The first sentence states that the algorithm defined
here is referred to as "chord-reload", however, subsequent sections don't
seem to use that term.
uh, that should have been upper case in this section.
It would be more clear if that term was used. And, really, it would be more
consistent to use the term Chord-RELOAD. However, this should also be
consistent with the IANA-Registration which has CHORD-RELOAD.
- Section 9.7.4.3, 3rd para:
-- Remove "Note to implementers". Text already highlights that the text is
specific to implementations.
-- "may be found" -> "can be found"
- Section 9.8: "For this topology plugin" -> "For Chord-RELOAD"
- Section 10.2, 2nd para: "determines" -> "determining"
needs to be fixed
- Section 11:
- General: there are no labels for any of the call flows, so it's not always
clear what the text is referring to. In general, I think the text precedes
the flow.
- "abbreviation" -> "abbreviations"
needs to be fixed
- Section 13.16:
--Interoperability considerations: "knows" -> "known"
needs to be fixed
- Section 14: "provied" -> "provided"
needs to be fixed
- Appendix C: Delete now - it's totally useless as is.
make sense
_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf
_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf