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

From: [email-masked] (Dale R. Worley)
Date: Tue, 13 Sep 2016 10:10:13 -0400

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

---

From: Andy Bierman <[email-masked]>
Date: Tue, 13 Sep 2016 08:45:02 -0700

>     but it would help if the resolution was stated explicitly.

We asked the WG and nobody throught this issue was  important.
It is just an implementation detail for the server to know its own hostname.

I will look into fixing remaining nits within 2 weeks.

Andy

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

---

From: Andy Bierman <[email-masked]>
Date: Sat, 24 Sep 2016 09:28:54 -0700

> There seems to be no discussion of this issue.

The WG has not agreed that the server does not know what to
put in the Location header.

no edit made

> they use relative URIs as values of the href attribute.

I asked Kent and Martin to research this issue

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

fixed

> resources can never return such types, either now or in the future.

these sentences have been removed

> subsections are labeled with HTTP method names?

changed title of sec. 4

>    characters and does not need to be percent-encoded.

fixed

>        string = <a quoted or unquoted string>

changed to

  string = <an unquoted string>

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

fixed

> Shouldn't there be backslashes here?

fixed

> None of these (IMO) improvements have been done.

The suggested edits are not accepted.
They prevent groupings from being used within a YANG data template.

>     default value for leaf reply-support, so it is sort-of optional.)
The '?' is correct.
The default for the YANG mandatory-stmt is false.
The "description" and "replay-support" nodes do not exist unless set by the
client.
A YANG default is the value that will be used if no instance exists.

> something like "Fragment identifiers for this type are not defined.".

fixed

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

fixed

Andy

---

From: Martin Bjorklund <[email-masked]>
Date: Sun, 25 Sep 2016 17:33:48 +0200 (CEST)

> I asked Kent and Martin to research this issue

This seems to be a general RFC6415-problem, not specific to RESTCONF.
Anyway, the OASIS spec for XRD says that the "href" attribute is of
XSD type "anyURI", which allows relative links.

/martin

---

From: [email-masked] (Dale R. Worley)
Date: Mon, 26 Sep 2016 16:27:36 -0400

> XSD type "anyURI", which allows relative links.

Yes, it looks like that is correct.  I've filed erratum 4811
(https://www.rfc-editor.org/errata_search.php?eid=4811) to get 6415
updated.

Which takes care of that item.

Dale

---

From: [email-masked] (Dale R. Worley)
Date: Mon, 26 Sep 2016 16:34:56 -0400

> They prevent groupings from being used within a YANG data template.

That bit about groupings is a subtle point I hadn't thought of.  If the
WG is OK with that, I'm OK with that.

Though I think the item "The list-stmt is not required to have a
key-stmt defined." is still redundant -- everything inside yang-data is
not configuration data, and non-configuration lists need not have keys.
Even if the list is inside a grouping, that doesn't change anything, in
that the list has a key-stmt or not depending on how the grouping is
defined, and either case is valid (in regard to the data template)
without a special rule.

Dale