Last Call Review of draft-ietf-nfsv4-rfc3530bis-25
review-ietf-nfsv4-rfc3530bis-25-genart-lc-thomson-2013-04-19-00

Request Review of draft-ietf-nfsv4-rfc3530bis
Requested rev. no specific revision (document currently at 35)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2013-05-28
Requested 2013-03-21
Draft last updated 2013-04-19
Completed reviews Genart Last Call review of -25 by Martin Thomson (diff)
Genart Telechat review of -26 by Elwyn Davies (diff)
Genart Telechat review of -26 by Elwyn Davies (diff)
Genart Telechat review of -33 by Elwyn Davies (diff)
Genart Last Call review of -34 by Elwyn Davies (diff)
Secdir Last Call review of -25 by Yoav Nir (diff)
Assignment Reviewer Martin Thomson
State Completed
Review review-ietf-nfsv4-rfc3530bis-25-genart-lc-thomson-2013-04-19
Reviewed rev. 25 (document currently at 35)
Review result Ready
Review completed: 2013-04-19

Review
review-ietf-nfsv4-rfc3530bis-25-genart-lc-thomson-2013-04-19

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<

http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

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

Document: draft-ietf-nfsv4-rfc3530bis-25
Reviewer: Martin Thomson
Review Date: 2013-04
IETF LC End Date: too soon
IESG Telechat date: ditto

Summary: I have some confidence that this document is ready for
publication.  What I have seen is sufficiently well specified that it
could be implemented, mostly.  There are large parts of this document
that I simply haven't read.  I can't justify spending more time.
(Sorry Jari, I tried.)

Reviewing this document is especially difficult because the it relies
heavily on context.  A lot of this context is only gained by reading on
and stumbling upon that context.  Much of this could be addressed
with appropriate forward references.

It's clear that Section 12 is largely new, or at least significantly
changed from RFC 3530.  Section 12 on its own would constitute a
larger than average RFC.

If accessibility is a goal, the document could have been split and it
probably should have been.  However, I suspect that accessibility
isn't that important to those working on this.

The document talks quite frequently about "before", with reference to
NFSv2 or NFSv3.  This is almost always useless to a reader without
context.  I don't expect you to do anything about this, but I'd
request that any future revision consider removing this.  For one, in
reviewing a -bis draft, I personally think of RFC3530 as the "before"
to which is refered.  On the whole, fewer history lessons and more
real context would have made this a lot easier to read.

General: The document uses lowercase "must" and "may" a lot.  It's not
clear whether these are intended to be interpreted as normative
statements or not.  To my eyes, most of these are.  On the other hand,
there are places where 2119 language is used to effectively specify
implementation details that aren't observable at a protocol level.
If the document weren't so large, I'd suggest that the editors go through
and make sure that every 2119 keyword is in uppercase and then to
check each and every instance.

Nits:

1.5.3 - I realize that it might be premature to explain details when the
draft hasn't yet described how to identify locations (that's a truly
unfortunate choice of word).  Maybe a short primer on that could be
included.  At this point, I'm relying on assumed knowledge about
filesystems to understand concepts like: hierarchical ACLs, pseudo
filesystems, attributes.

Even the fact that a filesystem is a tree with terminal nodes that
contain sequences of bytes.  Non-terminal nodes (directories) and
terminal nodes (files) are named (hence the UTF-8) and have
attributes.  Where it gets fuzzy is when it uses "path name".  Is
"path name" an abstraction that encapsulates "directory name" and
"file name"?  Is a "file name" the name of the node, or is it the
entire locator including the names of the directory above it (and some
unspecified separator)?

1.5.3.1
   [...]  The server provides multiple
   filesystems by gluing them together with pseudo filesystems.  These
   pseudo filesystems provide for potential gaps in the path names
   between real filesystems.

I don't know how to interpret this last statement.  I'm using a fair
bit of guesswork to infer how this works as it is, but that
understanding does not result in gaps, just pseudo filesystems (with
no actual files).


1.5.3.3 - The description of namespaces is singularly unhelpful in
conveying any information about the mechanism.  The benefits of this
snake oil are clearly splendrous.

s/alternate/alternative/ - I know that alternate is acceptable US
English, but alternate means something else to other English speakers.

1.5.5 - NLM could probably use a citation.

3.1 - "The registered port 2049 [RFC3232] for the NFS protocol SHOULD
be the default configuration."  This is an implementation requirement,
not a protocol one.

    - "this will allow NFS to transit firewalls" is only true if the
firewall is configured to recognize NTP using port numbers and allow
its passage.

    - "To date, all NFSv4 implementations are TCP based." Is this
really an appropriate statement for an RFC?

    - "UDP by itself is not sufficient as a transport for NFSv4,
neither is UDP in combination with some other mechanism (e.g., DCCP
[RFC4340], NORM [RFC5740])."  This statement is strange, given that
UDP is specifically prohibited.  I understand that something layered
on UDP (SCTP perhaps) might be able to meet the requirements, but this
statement implies that neither congestion control (DCCP), nor
reliability (NORM) is sufficient.  The draft should enumerate what
properties it expects to obtain from its transport before making such
statements. This section is really fuzzy on reliability expectations
(and 3.1.1 doesn't really help with that).

3.2 - "For NFSv4, the RPCSEC_GSS security flavor MUST be used to
enable the mandatory security mechanism."  Which aspect of security is
mandatory?  I find this confusing.  Earlier statements were pretty
clear: you must implement it, but you don't have to use it.

3.3 - s/may/can/

    - The SHOULD here (which is also a SHOULD in S17) could be
stronger.  I can understand why you need to allow for deployments that
don't use TLS (or equivalent integrity protection), but this

3.3.3 - "For Kerberos V5, the following form is RECOMMENDED:
nfs/hostname" - Q: how do you ensure interoperability if you don't
mandate a format?  Comment: This doesn't provide information on why an
implementation might choose a different form.

4.2 - The definitions of "persistent" and "volatile" are not known,
which makes this introduction to filehandle types hard to place in
context.  In many respects the discussion on the history of the
protocol is unnecessary at this point.  This would flow better if the
entire subsection was replaced with: "There are two[*] types of
filehandle: persistent and volatile.  Servers select the type that
they provide clients and signal the choice using the mandatory
"fh_expire_type" attribute (see Section 4.2.3)."  The rationale for
having both belongs in the respective sections (4.2.2 and 4.2.3).

    * There aren't two types of filehandle, there are six different
      persistence guarantees for filehandles.

4.2.1 - "Clients MUST use filehandle comparisons only to improve
performance, not for correct behavior.  All clients need to be
prepared for situations in which it cannot be determined whether two
filehandles denote the same object and in such cases, avoid making
invalid assumptions which might cause incorrect behavior."  This seems
a little confused.  The context establishes the fact that equal
filehandles always refer to the same file, and sometimes two different
filehandles can refer to the same file (though they SHOULDn't).  So,
what value does the 'MUST' provide here?  (I can't see how this would
be testable at the protocol level.)  Given that there is more
discussion in 10.3.4, I'd recommend dropping these two sentences.

5.1 - Use of lowercase "must" and "may" are confusing.  Consider using
alternatives.

5.2 - is that lowercase "must" in the third sentence really a MUST?

    - "A client MAY ask for the set of attributes the server supports
and SHOULD NOT request attributes the server does not support."  How
does the client ask for the supported set?

    - "A server should provide attributes whenever they don't have to
"tell lies" to the client."  This statement is odd.  Even with an
edit: "Rather than manufacturing fake attribute values, a server is
encouraged to avoid sending attributes that it can't support."  This
seems closer implementation magic than protocol specification.

12.1.2 - LATIN SMALL LETTER E is not U+0xxx

       - This section includes basically the same list twice.  The
first instance is on Page 179 (the first two-item list), the second on
the top of 180.

12.4.2 - What do you do with servers that are identified by server
names?  The last paragraph mentions that IP addresses are just ASCII.
Are server names are subject to the IDNA rules?  Do you want to
reference Section 12.6?

12.7.3 - none of the last four points uses normative 2119 language,
though "must" and "should" appear.  The text looks pretty normative to
me.

       - This looks less predictable than it could be.  Is it
reasonable to require that a server use the same normalization every
time it returns a name?  (That stability would only be required for a
given filehandle, of course.)