[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Begin WG Last Call: draft-ietf-netconf-prot-04
On Sun, Oct 31, 2004 at 09:25:00AM -0700, Andy Bierman wrote:
> The WG members are strongly urged to review this document as
> soon as possible, and express any concerns, or identify any errors,
> in an email to the NETCONF WG mailing list.
Attached is my review of the NETCONF protocol document. I did not
manage to read the transport mapping documents before the last call
timer has expired.
/js
--
Juergen Schoenwaelder International University Bremen
<http://www.eecs.iu-bremen.de/> P.O. Box 750 561, 28725 Bremen, Germany
I have completed my review of <draft-ietf-netconf-prot-04.txt> today.
Below are my comments as they appear on my printed hard-copy, that is
almost linear document order. So minor editorial nits are intermixed
with more general comments.
a) The introduction makes the promise that "applications can access
both the syntactic and semantic content of the device's native user
interface" via NETCONF. I think this promise is a bit too strong.
b) I never liked the replacement of "transport protocol" with
"application protocol" - I think the later term is even more
confusing as the original term "transport protocol". Note that I
found a few places where the document still refers to the
"transport" (just search for it). (Transport is used in the error
reporting mechanism, see the ErrorType definition. Replacing
transport here with application would clearly cause confusion.)
As said above, I prefer the reintroduction of the term "transport"
or even "NETCONF transport" with an explanation in section 1 that
transport in this document refers to the NETCONF transport, which
is not the same as a transport layer protocol.
c) Section 4.3, error-tag description should refer to appendix A as
follows:
error-tag: String identifying the error condition. See Appendix A
for allowed values.
d) The description of the error-info states that vendor specific
elements may only be added at the end of the sequence. I do not
really see the need for this rule since namespaces should be used
anyway. I would prefer to not introduce such rules, the
specification should instead spell out that any
extensions/additions must have their own namespace.
e) I prefer "implementor" over "vendor", "implementation-specific"
over "vendor-specific" and so on. This affects several places (just
search for vendor).
f) Is there any reason why error tags are ALL_CAPS and use underscores
while the other XML elements all use hyphens and lowercase?
g) I had some trouble to figure out what RPC responses can
contain. Section 4.4 says that an <ok> element is sent in
<rpc-reply> messages if no error occurred. Looking at the examples
shown later on, it looks like <ok> is only present if the RPC
was successful and no other result data was returned. I am not
sure why this <ok> is needed at all.
Things get a bit more irritating if I look at the <rpc-error>
elements. Such an element is used to signal an error or a warning.
This raises the question whether a warning can be returned even
though the RPC succeeds. Will I then get <ok> or <something>
followed by an <rpc-error> signalling a warning? Or will the
warning come first? If the RPC does not abort on error, I would
suggest that I can get multiple errors. But this does not seem to
be the case (according to the NETCONF RelaxNG spec.) The RelaxNG
spec (did not check the xsd) requires the element to be <data>
while section 4.2 shows an example which shows <some-content>.
If the idea is indeed that RPC results always show up in a <data>
element, then why not replace <ok> with an empty <data/> element?
I recall some discussion that errors/warnings may actually be
generated while an operation is processed and that they were even
supposed at some point to interleave the data portion. So I assume
there is something missing here. Perhaps the examples used later in
the document focus too much on the no error cases.
h) page 15: replace "an XSD" with "a schema".
i) page 17: "...containment nodes the element hierarchy..." ->
"...containment nodes of the element hierarchy..."
j) page 18: "...get request..." -> "...get operation..."
k) page 19: "...the filter This..." -> "...the filter. This..."
l) page 22: "...siibling..." -> "...sibling..."
m) The title of section 6.8.8 should be changed to "Elements with
attribute naming" or something similar since NETCONF does not deal
with tables.
n) The first paragraph in section 6.8.8 refers to 'schema/2.0' while
the example uses 'schema/1.2'.
o) Section 7.1 says under "Positive Response" that the reply contains
a <config> element which I think must be a <data> element.
p) The description of test-option in section 7.2 says it can take two
values while the xsd says it can take three values.
q) Section 7.1 on page describes rollback-on-error which is however not
mentioned in the xsd.
r) Section 7.1 IMHO misses text which explain how overlapping operation
attributes are handled. What for example happens if I have:
<... operation="delete">
<... operation="merge"/>
</...>
There might be more interesting combinations. I would prefer to
have well defined rules so that all implementors do the same thing
here. If there are nested operation attribute value combinations
that do not make sense, I think it would be fair to request that
such things are checked before starting the execution of the merge.
s) Example on page 32: I suggest to replace <mask>255.0.0.0</mask>
with <prefix>24</prefix> as this seems to be the more general
concept.
t) After reading the <edit-config> part of the spec, I felt that there
is in general a need to have more elements of the procedure spelled
out. I think it would help implementors tremendously if it were
spelled out which tests have to be performed, what the error codes
are that can be generated and so on. At the moment, the document
leaves it to the implementor to figure out what needs to be
checked, in which order, before are while processing an edit-config
and so on. I believe it would help interoperability if some more
guidance would be given here.
u) Section 7.3 says: "The source and target parameters cannot identify
the same URL or configuration datastore." Why not say which error
code is being returned in this case and that it when this must be
checked? [This is kind of an example for comment t) shown above.]
v) page 36: "...Expect scripts..." -> "...CLI scripts..."
w) Section 7.5, star bullet two on page 37: Does this only apply if
the commit capability is present?
x) There are several XML lines which exceed the allowed column number
are require some re-formatting.
y) Section 7.7 refers to configuration and state data without saying
which configuration source is meant here. I assume it is the
<running> configuration data set. Perhaps this should be made clear
somewhere.
y) Section 7.9 refers to a 'Bad Value' error, which does not seem to
exist.
z) page 44: "...proprietary vendors." -> "...proprietary extensions."
A) Section 8.3.4.1: I assume it copies to <running> - perhaps this
should be spelled out.
B) I was wondering what the difference is between the #candidate and
just a named file://candidate scratch buffer, e.g.
lock running
lock file://candidate
copy running file://candidate
edit file://candidate ...
copy file://candidate running
unlock file://candidate
unlock running
Perhaps it is the locking? In the example in Appendix D, only the
<running> is locked - does that imply a lock on candidate as well
as it is somehow magically tied to <running>? (And is it magically
tied to <running>?)
Some more explanation might be worthwhile (perhaps in the
appendix). I would find it useful to know whether there are any
real differences here. If there are no real differences, why do we
have two different ways to achieve the same thing?
C) Section 8.4.5.1 talks about "a confirming commit" without really
saying what that is. I think the example should be extended to show
a confirming commit. (Right now, one has to look into the appendix
where you can find an example. It looks like the confirming commit
is a plain commit without the <commit> element. This raises the
question if it is possible to extend a confirmed commit by just
sending another confirmed commit before the timer goes off.
D) Section 8.6.4.1 uses the <candidate> datastore to illustrate the
<validate> operation. Since <candidate> is an optional feature,
would it not make more sense to choose a data store that is always
present? Or does #validate not make sense if there is no #startup,
no #candidate and no #url capability?
E) Section 8.8.3 says shows the following urn format:
urn:ietf:params:xml:ns:netconf:base:1.0#url?protocol={protocol-name,...}
It is unclear what are meta characters here. I guess '{' and '}' are
meta characters if I look at the subsequent examples. Anyway, the
more general comment here is that you identity URI schemes and not
protocols. So perhaps the example should be:
urn:ietf:params:xml:ns:netconf:base:1.0#url?schemes=http,ftp,file
Perhaps life would be even easier if the schemes were announce as
separate capabilities:
urn:ietf:params:xml:ns:netconf:base:1.0#url?scheme=http
urn:ietf:params:xml:ns:netconf:base:1.0#url?scheme=ftp
urn:ietf:params:xml:ns:netconf:base:1.0#url?scheme=file
Perhaps a general discussion how to deal with parameters in
capabilties would be useful. The later version has the advantage
that I do not have to parse the full scheme to check whether the
'file' scheme is supported - a simle string match is good enough.
(But parsing this on the other hand does not seem overly complex
either).
F) page 56: "...SHOULD also provide..." -> "...SHOULD provide..."
G) page 56: "...incompatable." -> "...incompatible."
H) page 57: "...person in the middle..." -> "...eavesdropping..."
(I don't think you have to be in the middle of the communication
to simply listed to the conversation.)
I) Section 10 needs to spell out the registration rules for the urn
namespace that netconf is using I think.
J) Reference [4] is not cited so it can't be normative.
K) References [9], [10] and [11] are not cited.
L) Appendix D assumes that either #candidate or #url?scheme=file is
present. Does it meant without these capabilities, you are
seriously restricted in the sense that you can't have a robust
scheme to configure multiple devices? If yes, this probably needs
to be spelled out somewhere clearly so that implementors understand
which capabilities are important and for what reason.
M) page 72: incomplete sentence "...unrelated changes while."
N) page 77: "...the first class of allows..." -> "...the first class
allows..."
/js