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

[Andy Bierman <[e-mail]>]

On Tue, Sep 13, 2016 at 7:10 AM, Dale R. Worley <[e-mail]> wrote:

> 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.
>


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




>
> 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
>
> _______________________________________________
> Netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf
>