I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair. This is a requested early review.
Please treat these comments just like any other comments.
For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Document: draft-ietf-netconf-restconf-09
Reviewer: Robert Sparks
Review Date: 20Jan2015
IETF LC End Date: (not yet in IETF last call)
IESG Telechat date: (not yet scheduled)
Summary: This document has a few issues that should be addressed before
requesting publication.
Please make sure the semantics you're giving to POST vs PUT sit well
with HTTP experts. Similarly, please make sure you have someone steeped
in character sets and encoding look closely at section 5.3. I'll leave
commenting on those things to people more qualified than me. Other
reviewers have asked for more information about the interaction of this
use of HTTP with things like pipelining.
Major issues:
The document should explicitly discuss how the restconf protocol will be
extended. It mentions that other documents might do that in several
places (specifically with "Additional functionality can be defined in
external documents, outside the scope of this document").
Section 3.4 claims a datastore resource can only be written directly
with the optional to implement PATCH method. That does not seem
consistent with the rest of the document.
Section 2.2 says "RESTCONF requires that the transport-layer protocol
provides both data integrity and confidentiality, such as are provided
by the TLS protocol [RFC5246]." and "RESTCONF implementations MUST
support the "https" URI scheme", but I don't see any text that prohibits
the use of RESTCONF over HTTP without TLS. If the intent was to prohibit
that, please make it more explicit. If the intent is to allow the use of
RESTCONF without requiring the use of TLS, the security considerations
section (at least) needs more discussion about when doing so is a bad idea.
Section 2.3 (Certificate Validation) seems to be restating things that
it could simply point to instead. Why not replace this section with
something like "the RESTCONF peer will follow the procedures and
recommendations in RFC6125."? If it's because you want this section to
talk only about validating the client certificate, that should be
clarified (and there is still probably something that already exists
that you can point to rather than restate things here). The current text
claims RFC5246 requires the peer to terminate the connection if the
certificate path validation fails - where in RFC5246 are you seeing that?
Should section 3.1 talk about what a client should do if more than one
restconf link relation is returned when querying host-meta?
Section 4.3 speaks to returning different content for the same resource
depending on what authentication is provided. Specifically "the
unauthorized content is omitted from the response message-body, and the
authorized content is returned to the client.". Some discussion of how
this interacts with caching (or even the ETag mechanisms) is warranted
here. What happens if the authorization policies for a given user
changes - will they be able to fetch the new things they have been given
permission to see, or will the cache, etag, or if-changed-since
mechanics interfere?
Minor issues:
There are some requirements stated with 2119 keywords buried in the
Introduction sections. Consider moving them to the protocol definition
sections.
Section 2.5 cuts off the possibility of using session-based
authentication. If that's something you want to be able to add later,
make sure you're leaving the right extension points. (I know the typical
restconf client won't be a browser, but for those use cases where using
a browser makes sense, remember that browsers make it very hard to
change identities when you are using HTTP Authentication).
In sections 4.2 and 4.3 consider explicitly saying what to return if
something issues a HEAD or GET request against an operation resource.
The first paragraph in section 5 (before 5.1.1) seems confused in its
use of message vs method. Please review the use of the terms and make
sure they align with the words used in the introduction to rfc7231, for
example.
I suggest deleting section 5.2. You already say RESTCONF uses HTTP, and
that this section is not comprehensive. As it is, it restates things
said in the HTTP documentation without adding anything - at best, it's
an opportunity to introduce errors while restating things. Just point to
HTTP instead. Similarly, the first table in section 7 could be replaced
with a pointer to the relevant place in the HTTP specs.
Nits/editorial comments:
The introduction says a user should not need any prior knowledge of
NETCONF to use RESTCONF. It would be good to also call out here that the
implementer _will_ need prior knowledge of NETCONF.
The first sentence of section 2.4 ("The RESTCONF client MUST carefully
examine the certificate presented by the RESTCONF server to determine if
it meets the client's expectations.") adds nothing. Please delete the
sentence or replace it with specific things the client needs to do..
The introduction says "Edits are usually applied". Why is this qualified
with "usually"?
In the examples at the end of 3.5.1 (before 3.5.1.1), consider saying
list1=key1,key2,key3/ instead of list1=key1val,key2val,key3val to be
consistent with the way you refer to the elements in other places.
It would be useful to explicitly discuss what to return if a resource
already exists in Section 4.4.1. Right now, that's specified primarily
by an example in section 7.1.
The "MUST use this exact path" after the last bullet in 5.1 is not a
good use of 2119. The sentence is observing a truth about the defined
system, not placing an interoperability requirement on the client. The
client uses the returned identifier to access the resource. If it uses
anything else, it is not accessing the resource. I suggest replacing the
prase with "uses this exact path".
In section 5.6, I suggest replacing "Instead of using HTTP caching" with
"Instead of relying on HTTP caching".
Consider adding an example that shows a 500 response corresponding to a
"partial-operation" error in NETCONF, demonstrating what information the
client gets to distinguish this 500 from an "operation-failed" error.
There is a phrase missing in the first sentence of section 7.1. Perhaps
"will be returned" just before the left-parenthesis?
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
