search

netconf-wg / restconf

9 stars 4 forks source link

DW review of restconf-13 #67

Closed abierman closed 8 years ago

abierman commented 8 years ago

This is a bunch of comments on draft-ietf-netconf-restconf-13. Some of these comments might be redundant or due to misunderstanding because I'm coming into this process late. But as I'm likely to be the Gen-ART reviewer, it's best to take care of these now.

I think almost all of these comments are at the nits level. The two comments that seem to have technical content are:

1.1.4

o event stream resource: This resource represents an SSE (Server- Sent Events) event stream. The content consists of text using the media type "text/event-stream", as defined by the HTML5 [W3C.REC-html5-20141028] specification. Each event represents one

message generated by the server. It contains a conceptual system or data-model specific event that is delivered within an event notification stream. Also called a "stream resource". Is this correct? I can't find "event-stream" within http://www.w3.org/TR/2014/REC-html5-20141028. It appears that the latest Server-Sent Events specification is http://www.w3.org/TR/2015/REC-eventsource-20150203/. 3.5.1 If a data node in the path expression is a YANG leaf-list node, then the leaf-list value MUST be encoded according to the following rules: o The instance-identifier for the leaf-list MUST be encoded using one path segment [RFC3986]. o The path segment is constructed by having the leaf-list name, followed by an "=" character, followed by the leaf-list value. (e.g., /restconf/data/top-leaflist=fred). In item 2, does "leaf-list value" mean the encoded instance-identifier of item 1? But the example below does not show a full instance-identifier as one path segment, but only the leaf-list element name and value as the last path segment: ``` /restconf/data/example-top:top/Y=instance-value ``` Perhaps item 1 means "the leaf-list name MUST be encoded ...". o Resource URI values returned in Location headers for data resources MUST identify the module name, even if there are no conflicting local names when the resource is created. This Is this restricted to list nodes, as its position indicates? Or is it generally applicable, and should be pulled out to a top-level paragraph? "identify the module name" is vague. I think this means that each element name must be qualified with its module name. For the above YANG definition, a target resource URI for leaf-list "Y" would be encoded as follows: ``` /restconf/data/example-top:top/Y=instance-value ``` Non-unique values are now allowed for leaf-lists, so this syntax may be ambiguous. (Does this apply to instance-identifiers in modules?) Perhaps the 'insert' and 'point' query parameters are used to disambiguate non-unique leaf-list values? It seems that any netconf data node that is materialized as an XML element can't start with "xml" (case-insensitive). But this is not noted in draft-ietf-netmod-rfc6020 as a restriction. The existence of YIN requires that extension keywords also may not start with "xml" (case-insensitive). 3.6 POST /restconf/operations/module-A:reset HTTP/1.1 Server example.com In two locations, the "Server" header is shown without a colon. If the operation is invoked without errors, and if the "rpc" or "action" statement has an "output" section, then a message-body MAY be sent by the server in the response, otherwise the response message MUST NOT include a message-body in the response message, and MUST send a "204 No Content" status-line instead. It seems odd that if an RPC is performed, then it is optional for the output data to be returned to the client. This means the 'input' element of an RPC body is in the namespace of the module. Similarly for 'output' elements. This is not specified anywhere. (Errors is specified to be in "ietf-restconf" in section 7.1.) 4 Operations are applied to a single data resource instance at once. IMO it would be useful to use the word "atomically", as that has a well-understood meaning. 4.3 If a retrieval request for a data resource representing a YANG leaf- list or list object identifies more than one instance, and XML encoding is used in the response, then an error response containing a "400 Bad Request" status-line MUST be returned by the server. What about zero instances? Presumably it gets a 404 response rather than an empty body, but it would be good to specify that. Note that the way that access control is applied to data resources is completely incompatible with HTTP caching. The Last-Modified and ETag headers maintained for a data resource are not affected by changes to the access control rules for that data resource. It is possible for the representation of a data resource that is visible to a particular client to be changed without detection via the Last- Modified or ETag values. Note that it's possible for the client to detect changes in configuration data that it is not allowed to read by detecting changes in the Last-Modified/ETag values. This probably isn't a security problem, but the authors should be aware of it. What does a GET of the datastore URL return? I would guess an XML document with a top node of "datastore" (in the module namespace), but that doesn't seem to be specified. Perhaps it is a known thing that the datastore contains one instance of every top-level data node in the module definition... but there can be multiple modules defined in the server... Similarly, one can ask how PUT, POST, PATCH, and DELETE are applied to the datastore resource. 4.4.1 The message-body MUST NOT contain more than one instance of the expected data resource. It seems like you want "MUST contain exactly one", since it seems that you don't want to allow zero instances. 4.6 This document defines one patch mechanism (Section 4.6.1). The YANG PATCH mechanism is defined in [I-D.ietf-netconf-yang-patch]. Other patch mechanisms may be defined by future specifications. This sentence isn't clear that "one patch mechanism" is different from "The YANG PATCH mechanism ...". Perhaps: This document defines one patch mechanism (Section 4.6.1). Another patch mechanism, the YANG PATCH mechanism, is defined in [I-D.ietf-netconf-yang-patch]. Other patch mechanisms may be defined by future specifications. 4.6.1 Plain patch can be used to create or update, but not delete, a child resource within the target resource. Please see [I-D.ietf-netconf-yang-patch] for an alternate media-type supporting more granular control. I assume this means "supporting deletion of child resources". 4.8 | point | POST, PUT | Insertion point for ordered-yb | "ordered-yb" should be "ordered-by". o Each parameter can appear at most once in a request URI. Despite being given here as a general rule, it is repeated in 4.8.2, 4.8.3, 4.8.4, 4.8.5, 4.8.6, 4.8.7, 4.8.8, and 4.8.9 (but not 4.8.1). Also, if the repetition is to be removed, the general rule needs to add "If more than one instance is present, then a "400 Bad Request" status-line MUST be returned by the server." 4.8.2 By default, the server will include all sub-resources within a retrieved resource, which have the same resource type as the requested resource. Only one level of sub-resources with a different media type than the target resource will be returned. The exception is the datastore resource. If this resource type is retrieved then by default the datastore and all child data resources are returned. Does the second sentence do anything? The only instance I know of of a node whose media type is different from that of its child nodes is the the datastore resource, and that is specifically exempted from the effect of the 2nd sentence. 4.8.3. I take it that "fields=admin(label;catalogue-number)". is equivalent to "fields=admin/label;admin/catalogue-number" But the lack of a / before the ( is not really standard practice and might trip up users. (Compare with Un*x shell "/a/b/c/{d,e}" and "/a/b/c{d,e}".) 4.8.7 It is not valid to specify start times that are later than the current time. It seems that there needs to be a way for the client to determine the time the _server_ thinks it is. The client can't use its own clock, because there can be clock skew between the client and the server, and also the server may not know what time it is, so its time scale could have an arbitrary offset from real time. 7 Since an operation resource is defined with a YANG "rpc" statement, and an action is defined with a YANG "action" statement, a mapping between the NETCONF value and the HTTP status code is needed. The specific error condition and response code to use are data-model specific and might be contained in the YANG "description" statement for the "action" or "rpc" statement. I assume this means "The specific error-tag value and response code ..."; the word "condition" appears nowhere else in the draft. ``` +-------------------------+-------------+ | | status code | +-------------------------+-------------+ ``` Why is "‑" (‑, NON-BREAKING HYPHEN) used here? 7.1 [...] then the server SHOULD send a response message-body containing the information described by the "errors" container definition within the YANG module Section 8. There's probably a missing word before "Section 8". Specifically, this sentence needs to say that "errors" is defined in the YANG module "ietf-restconf" -- as the sentence stands, it's easy to assume that "errors" is within "the YANG module", i.e., the one defining the operation. 11.2 Looking at RFC 3688 and the registry (http://www.iana.org/assignments/xml-registry/xml-registry.xhtml#ns) it looks like you want to explicitly say that the URIs are being registered as namespaces in the IETF XML registry. 11.4 o the name of the RESTCONF capability. By convention, this name is prefixed with the colon ':' character. I think "begins" rather than "prefixed" is preferable, as it makes clear that the colon is part of the name, rather than being prepended to the name. 13 Who is The Space & Terrestrial Communications Directorate? Maybe it would add to the credit of the S&TCD by prefixing the country that funds it ... which seems to be "United States Army". 8. ``` draft-ietf-netmod-yang-json defines JSON encoding rules for all RESTCONF media types that use the '+json' suffix. ``` There needs to be an RFC Editor note to replace "draft-ietf-netmod-yang-json" with the eventual RFC name. D.1.3 Is it true that there's no NS URN? Section 9.3 suggests it should have namespace "urn:ietf:params:xml:ns:yang:ietf-restconf-monitoring". D.2.1 Note that the "Location" header line is wrapped for display purposes only: You might want to promote this comment to a higher section, as wrapping is done in other places. D.3.4 Location: https://example.com/restconf/data/ example-jukebox:jukebox/playlist=Foo-One/song=1 But the text doesn't say that the Location header is wrapped only for display purposes.
abierman commented 8 years ago

I think almost all of these comments are at the nits level. The two comments that seem to have technical content are:

Leaf-list nodes may now have duplicated values, so identifying leaf-list instances by value may not always work. But perhaps that ambiguity never arises in practice due to restrictions on the CRUD operations.

Any identifier that is materialized as an XML element or attribute can't start with "xml" (with any case). This includes the data nodes that are materialized in XML and extension keywords. But this restriction isn't specified in draft-ietf-netmod-rfc6020.

AB: These are YANG 1.1 issues. I think the leaf-list issue is resolved in the latest revision

abierman commented 8 years ago

Who is The Space & Terrestrial Communications Directorate? Maybe it would add to the credit of the S&TCD by prefixing the country that funds it ... which seems to be "United States Army".

This is text required by the SBIR research that is funding my contribution. I put the exact text they asked me to put.

STDC is interested in RESTCONF over CoAP. They are a division of CERDEC, which is part of RDECOM, US Army

abierman commented 8 years ago

closed in draft 15