Subject: [Netconf] js last call comments on draft-ietf-netconf-monitoring-10

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 8 Dec 2009 23:00:32 +0100

Hi,

I have reviewed draft-ietf-netconf-monitoring-10, which had major text
reorganizations from the previous version. Even though I found a few
things worth to report (see below), I am very pleased by the effort
the editors have put into this revision.

I am going sequentially through the notes I have scribbled on my
printout. Some if the issues are really minor editing nits while other
are more substantial since I found text to be unclear or missing.

0) The document provides this definition of the term schema:

     Schema:  A machine readable data model definition.

   I cross checked the YANG document and even though the YANG spec
   defines the terms such as 'schema node' or 'schema tree', it never
   defines the term schema. I would be nice if the YANG document would
   define the term 'schema' and then we would not need a definition of
   this term in the netconf monitoring document.

1) page 5, replace

     The NETCONF monitoring data defined in this document provides

   with

     The NETCONF monitoring data model defined in this document provides

2) page 5, remove duplicated dots

     YANG module provided in this document (see section 5)..

3) page 5, for consistency replace

    statistics (Container)

   with

    statistics

   (or add '(container)' for all the other branching nodes as well).

   I also think 'statistics' is misleading since there are statistics
   in other subtrees as well. Perhaps 'server' because we have
   accumulated per server statistics in this subtree? There is perhaps
   even a better name for this branch which I do not find at the
   moment - perhaps someone has a good suggestion.

4) page 7, improve consistency by replacing

      The data modeling language of the file/module.  Current selection of
      xsd, yang, yin, rng and rnc.

   with

      The data modeling language the schema is written in (currently
      xsd, yang, yin, rng or rnc).

   and

      The XML namespace defined by the data model.

   with

      The XML namespace defined by the schema.

   (talk about schemas and not about a mixture of schemas, files,
    modules, data models).

5) page 7

   I dislike the leaf name "identifier" - why can we not just call
   this the "name" of the schema?

6) page 7

   I would have called leaf named "version" "revision" since that maps
   directly to the revision statement we have in YANG/YIN? But my
   feelings are less strong about this one compared to 5) above.

7) page 8, replace

       in [NETCONF Configuration Protocol].

   with

       in [RFC4741].

8) page 11, misleading text

    Since the same schema may be available in multiple locations
    and/or have multiple versions and/or multiple formats no
    particular attribute is unique.

   This statement is somewhat at odds with the fact that the key for
   the list listing schemas is (identifier, version, format)

9) page 11, suggested rewrite of

     The URL details returned in the
     list SHOULD facilitate retrieval from a network location via a
     means such as ftp or http.

   with

     The URLs returned in the <location> list SHOULD facilitate
     retrieval from a network location via protocols such as ftp or
     http.

   I am also not sure what the SHOULD implies. Does it mean that if I
   use URLs, then I SHOULD choose the ftp or http scheme? Am I doing
   something wrong if I prefer to use https URLs? Am I supposed to
   provide URLs everytime or is a box implementing just the NETCONF
   access just fine? Some clarification would help me understand what
   the SHOULD requests me to implement.

10) page 11, MUST and SHOULD confusion

      Additionally the ability to retrieve a schema via NETCONF SHOULD be
      supported.  When a schema is available on the device and the
      <get-schema> operation is supported by the NETCONF server a
      location value of 'NETCONF' MUST be used to indicate that it can be
      retrieved via NETCONF using the <get-schema> operation described
      in section 3.1.

    By reading the YANG module, I arrived at the conclusion that the
    <get-schema> operation is _not_ an optional feature. Hence, I do
    not see how the SHOULD and MUST go together here. It might be that
    this is a wording issue and the editors had different intentions
    in mind from what the text actually says.

    (would be nice to have a NETCONF URI format so the special case
     would not be needed by I understand that this is out of scope)

11) page 11, terminology and unclarity

      The NETCONF server returns a list of data models available for
      retrieval.

    For consistency, I think the text should say that the server
    returns a list of schemas (not data models). The unclarity comes
    from the question whether a server can provide arbitrary schemas
    (e.g. the superset of all schemas of a certain vendor) or whether
    there is an implicit assumption that only schemas are listed that
    are implemented on the device. I think this needs to be clarified
    by adding description statements to the 'schemas' and 'schema'
    schema nodes.

12) page 12, consistency with YANG

    The filenames shown in the examples for YANG modules should follow
    the YANG guidelines (note that the choice of the separator is
    still an open issue discussed on the NETMOD mailing list but the
    '-' is clearly not what YANG suggests or what is being discussed).

13) page 16, replace

      "RFC XXXX: NETCONF Monitoring Schema";

    with

      "RFC XXXX: YANG Module for NETCONF Monitoring";

14) page 19, rewrite

      "The /netconf-state subtree is the root of the monitoring
       data model.  It acts as the container for the other
       monitored data.";

    with

      "The netconf-state container is the root of the monitoring
       data model.";

15) page 19, rewrite (and request for clarification)

    container capabilities {
      description
        "The list of currently provided NETCONF capabilities
         exchanged during session setup (i.e. hello).";

    with

    container capabilities {
      description
        "The capabilities container provides the list of currently
	 supported NETCONF capabilities as they would be announced
	 during session setup.";

    Note that I am guessing here since the original text is not clear
    to me. What does "currently provided NETCONF capabilities
    exchanged during session setup" mean in case a server dynamically
    picks up new capabilities, in which case "currently provided"
    might be different from "during session setup".

16) page 19, rewrite

      description
        "List of NETCONF configuration datastores (e.g. running,
         startup, candidate) supported on this device and related
         information.";

    with

      description
        "The datastores container provides the list of NETCONF
         configuration datastores (e.g. running, startup, candidate)
         supported on this device and related information.";

17) page 20,21

    I suggest to add 'reference' statements for 'lock-id', 'lock-info'
    and 'locked-nodes' to reference the partial locking document.

18) page 21, inconsistency with partial locking

                   The scope of the partial lock is defined by the list
                   of locked nodes.  This list can change over time.";

    My reading of the partial locking document has been that the set
    expression is only evaluate once and that the set of locked nodes
    does not change over time.

19) page 22, rewrite

        "Includes session specific data for NETCONF management sessions.

    with

        "The sessions container includes session specific data for
         NETCONF management sessions.

    (write full sentences)

20) page 22,23, inconsistent text

        "Includes session specific data for NETCONF management sessions.

        "Unique identifier for the session.  If the session is a
         NETCONF session, this value is the NETCONF session
         identifier.

    If the first statement is true, then the second is not needed
    since we only deal with NETCONF sessions. Likewise, if the second
    statement is needed, then the first one should be rewritten. More
    confusion then comes from this additional text:

         For purposes of NETCONF management all sessions are one of:

          Known session:  any session which can be managed by the
            NETCONF server SHOULD be reported in this table.

          Unknown session:  such sessions are not managed by the
            NETCONF server and map to NETCONF session identifier 0.
               These MUST be excluded from the list as a result.";

    Now we seem to have NETCONF sessions, non-NETCONF sessions, known
    sessions and unknown session. Furthermore, the MUST does not make
    sense since the session-id does not allow this value to be
    represented. I think this text needs to be simplified.

21) Make sure all statements that should have a description statement
    according to the usage guidelines have a description statement.

22) It is not always clear why some leafs are marked mandatory true or
    not. For example, why is 'locked-by-session' not mandatory?  Is
    this to allow implementations to simply not report the id of the
    locking session or is this to deal with locks that are not
    associated with a NETCONF session-id?

23) page 23, my usual rambling on username...

        leaf username  {
          type string;
          description
            "If present, the username contains an identifier which can
             be used to uniquely identify an individual client (human
             or machine).  This is likely to be implementation specific
             and subject to the security requirements of the device
             vendor and/or operators,  e.g., an SSH user, a host RSA
             fingerprint or other identifier deemed acceptable.";
        }

    If we ever do access control, we likely need a concept such as a
    username on whoes behalf on operation / access is performed. Once
    we get there, we will have to make the username predictable for
    the security administrator and the "e.g." list above will not work
    anymore. Perhaps it is better to remove the second sentence in the
    description above and instead stay very clearly. "The algorithm
    how a NETCONF server derives a username from the authenticated
    identity of a NETCONF session (e.g. an SSH user name or an X.509
    certificate) is currently not standardized and may be the subject
    of future standardization work." This text makes it clear that
    this mapping is currently implementation specific and might change
    once we have to standardize this.

24) page 23, my usual rambling on source-host

        leaf source-host {
          type inet:host;
          description
            "Host identifier (IP address or name) of the client.";
        }

    One could or perhaps should add a warning that (a) the IP address
    may be confusing if the client is behind a NAT and (b) that
    multiple sessions originating from the same network interface will
    not be distinguishable. Furthermore, I like to see some guidance
    when an implementation should report the name instead of the IP
    address. Is the idea that a server does a DNS reverse lockup
    (which does not need to be consistent with forward lookups)? If
    yes, is this lookup to be done when the session gets established
    or when I retrieve the object?

25) page 27, security considerations

    I do not know what the guidelines for YANG modules will be but if
    we following SMIv2 MIB guidelines, then more text is needed. The
    YANG usage document currently says:

      In particular, writable module objects that could be especially
      disruptive if abused MUST be explicitly listed by name and the
      associated security risks MUST be spelled out; similarly, readable
      module objects that contain especially sensitive information or that
      raise significant privacy concerns MUST be explicitly listed by name
      and the reasons for the sensitivity/privacy concerns MUST be
      explained.

26) page 15, 29, namespace prefix

    The document suggests the prefix 'ns' (probably from netconf
    state). Since we expect to us 'nc' for the netconf definition
    itself, we could use something like 'ncm' for netconf monitoring
    instead of 'ns' (but note that I do not have a strong opinion
    about this).

27) I think the [YANG] and [Common YANG Data Types] references must be
    normative.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1, 28759 Bremen, Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>