Re: [Netconf] IETF Last Call Gen-ART review of draft-ietf-netconf-restconf-15

[[e-mail] (Dale R. Worley)]

This is a comparison of draft-ietf-netconf-restconf-16 against my
review for draft-ietf-netconf-restconf-15.  I've removed items that
have been (IMO) completely resolved.

    * Technical nits

    - In a few places, returned data can include URIs that are intended to
    be usable by the client to retrieve information from the Restconf
    server.  These places include the <location> element for accessing an
    event stream, the Location header of POST responses, and the URLs for
    retrieving schema resources (section 3.7).  The problem
    is that implementing this assumes that the server knows a host
    identifier (domain name or address) that the client can use to access
    the server.  In general, given the ubiquity of NATs, the server
    doesn't know this.

    It seems to me that the two possible solutions are for the server to
    be able to return a URI *relative reference* or for the server to use
    as the host-part of the URL the value in the Host header of the
    request (which perforce the server knows the client can use to access
    the server).  Perhaps this is a known problem with a known solution
    and so the draft doesn't bother to mention it.  Then again, perhaps in
    practice the clients and the server are in region of the network
    containing no internal NATS, and the problem does not arise.

    But it seems to me that this could be a very general problem for
    implementers and they should be instructed how to deal with it.  I
    suspect that the authors know the intended resolution of this issue,
    but it would help if the resolution was stated explicitly.

There seems to be no discussion of this issue.

    - Does XRD (RFC 6415) admit relative links as values of the href
    attribute of the Link element?  Relative links are not usually
    considered to be "URIs", but rather "URI-references" (RFC 3986 section
    4.1), and RFC 6415 uses "URI" consistently, not "URI-reference".
    Also, all examples in RFC 6415 show complete URIs.  If the href
    attributes have to be full URIs, then the problem with host
    identifiers described above also arises.

This issue doesn't seem to have been addressed at all in the draft,
though I have a vague memory of discussion of using templates as an
alternative.  But all of the XRD examples seem to be incorrect in that
they use relative URIs as values of the href attribute.

    * General issues

    - Might it be useful to consistently use a trailing-backslash
    convention?

This is largely done, but there are some additional places where it is
needed:

In the example in section 6.2, there are several places where
"</locaton>" appears on a line of its own.  It appears that the
preceding line was intended to end with "/".

In the example in section 6.3, this XML appears to be missing "/":

    - There seem to be six statements in the draft that the fragment field
    in RESTCONF resource URIs has no defined purpose, either for some set
    of resources or for all resources.  (sections 3.4, 3.5, 3.6, 3.8, 5.1
    (twice)  But of course, the fragment identifier is never presented to
    the HTTP/HTTPS server for the resource -- see RFC 7230 section 5.1:

       The target URI excludes the reference's fragment component, if any,
       since fragment identifiers are reserved for client-side processing
       ([RFC3986], Section 3.5).

There are still several statements like "The fragment field in the
request URI has no defined purpose if the target resource is a
datastore resource."  These statements remain meaningless, since by
the specification of HTTP, the fragment identifier is *never* included
in a request URI.

The most you can say is, e.g., that the resource returned by fetching
a datastore resource is never of a media type for which fragment
identifiers are defined.  But that requires checking that datastore
resources can never return such types, either now or in the future.

    * Nits

    1.1.5.  Terms

    The term "operation" is used in at least three senses:

    - HTTP request methods (e.g., the title of section 4)
    ...

Perhaps section 4 could be better titled "RESTCONF Methods", since the
subsections are labeled with HTTP method names?

3.5.3.  Encoding Data Resource Identifiers in the Request URI

   Note that double-quote is not a reserved
   characters and does not need to be percent-encoded.

s/characters/character/

    3.5.3.1.  ABNF For Data Resource Identifiers

	   api-path = "/"  |
		      ("/" api-identifier
			0*("/" (api-identifier | list-instance )))

The new ABNF is much better!  However, there is one point that's not
clear to me:

       string = <a quoted or unquoted string>

This suggests to me that in a path component identifying a list
instance, one can write the quoted string ".../node="key-value"/...".
But I'm sure that's not intended, as there is no mention of quoting
elsewhere in the text.  I suspect that omitting this definition and
leaving

       key-value = string  ;; constrained chars are percent-encoded

would have the intended meaning.

    4.8.3.  The "fields" Query Parameter

   The server does not return a set separate sub-resources.

I think that the word "of" has been omitted from this sentence.

    6.3.  Subscribing to Receive Notifications

      <location
        xmlns="urn:ietf:params:xml:ns:yang:ietf-restconf-monitoring">
        https://example.com/streams/NETCONF
      </location>

Shouldn't there be backslashes here?

    8.  RESTCONF module

	     The following data-def-stmt sub-statements have special
	     meaning when used within a yang-data-resource extension
	     statement.
	     - The list-stmt is not required to have a key-stmt defined.
	     - The if-feature-stmt is ignored if present.
	     - The config-stmt is ignored if present.
	     - The available identity values for any 'identityref'
	       leaf or leaf-list nodes is limited to the module
	       containing this extension statement, and the modules
	       imported into that module.

    It seems like poor practice to have the extension be described as
    changing the semantics of Yang.  Better would be to turn these into
    constraints, so that the valid contents of yang-data are a subset of
    Yang, but that subset has the same semantics as Yang prescribes:

	     - The if-feature-stmt must not be present.
	     - If the config-stmt is present, its value must be 'false'.
	     - The available identity values for any 'identityref'
	       leaf or leaf-list nodes is limited to the module
	       containing this extension statement, and the modules
	       imported into that module. [unchanged!]

    The item "The list-stmt is not required to have a key-stmt defined."
    is redundant, since everything inside yang-data is not configuration
    data, and non-configuration lists need not have keys.

None of these (IMO) improvements have been done.

    9.  RESTCONF Monitoring

    The nodes 'description' and 'replay-support' are shown as optional in
    the tree diagram:

       +--ro restconf-state
	  +--ro streams
	     +--ro stream* [name]
		+--ro description?                string
		+--ro replay-support?             boolean
		+--ro replay-log-creation-time?   yang:date-and-time

    but there is no 'mandatory "false";' for leaf "description" in
    section 9.3.  Or should it have a default value?  (There is a
    default value for leaf reply-support, so it is sort-of optional.)

	       leaf description {
		 type string;
		 description "Description of stream content";
		 reference
		   "RFC 5277, Section 3.4, <description> element.";
	       }

11.3.1.  Media Type application/yang-data-xml

      Fragment identifier considerations: The fragment field in the
         request URI has no defined purpose.

This needs to be rephrased.  By the definition of HTTP, the fragment
field is *never* included in a request URI.  What you mean to say is
something like "Fragment identifiers for this type are not defined.".

    D.2.3.  Edit a Datastore Resource

    "album" sub-resource to eachof 2 "artist" resources:

s/eachof/each of/

Dale