[Netconf] draft-ietf-netconf-monitoring-09 last call comments from js

[Juergen Schoenwaelder <j.schoenwaelder at jacobs-university.de>]

On Tue, Oct 20, 2009 at 02:36:24PM +0200, Ersue, Mehmet (NSN - DE/Munich) wrote:
 
> With this mail we start a new WGLC for the NETCONF 
> monitoring draft, which is planned to publish as a 
> Proposed Standard RFC. The WGLC will run until 
> November 2, 2009. 

I have reviewed the monitoring document and below are my comments. I
quoted (using the : marker) text from the monitoring document and then
wrote down my comments/questions/suggestions. I did not find any show
stoppers, but I believe more editing work (means at least one more
revision) is to be done to get this document into good shape.

:   [...]  The
:   capabilities of a NETCONF server may change over time.  However, once
:   a NETCONF server has announced its capabilities in the <hello>
:   message, the capabilities for that session MUST NOT change.  A server
:   MUST reply with a 'capabilities-changed' error if the client sends a
:   request which is affected by a modified capability.  A server MAY
:   choose to send 'capabilities-changed' as the response to any request
:   other than <close-session> if its capabilities has changed.

It seems to me that it is not the job of the monitoring document to
establish rules how NETCONF works. I suggest to remove this text and
to include such text in RFC 4741bis instead.

:   Through updated monitoring data NETCONF clients can adjust their
:   capabilities throughout a session.

I wonder how this is supposed to work. Once the server starts to
generate 'capabilities-changed' errors, I need an explicit resync
action - reading the monitoring data can hardly be sufficient to
declare that things are not synchronized again.

:   Schema:  A machine readable data model definition.  The schema is
:      independent of which data modeling language is used for the data
:      model.

I have trouble to understand the second sentence. Every machine
readable data model is written in a specific format. So how can the
schema be format specific and at the same time independent? Perhaps
the second sentence should just be removed.

:   The following data allows a NETCONF client to monitor both the
:   NETCONF server itself and the associated network device operational
:   data.

I wonder what "associated network device operational data" means. I
thought the YANG model only reports NETCONF server operational state
and statistics.

:   To provide clients the ability to manage locked resources lock
:   information is provided for each ConfigurationDataStore instance.

I have no clue what this ConfigurationDataStore instance is.

:    For YANG data models, the format is one of "yang" or "yin".

I am concerned about interoperability here. I prefer that the "yang"
format is required and "yin" can be provided optionally. Otherwise,
you require that all management applications have two parsers instead
of one.

:    A schema entry may be located on a network device (eg: xs:anyURI),
:    a remote file system (eg: xs:string reference to file system for
:    ftp retrieval) or available explicitly via NETCONF (xs:string
:    value 'NETCONF') for NETCONF servers which support the
:    <get-schema> operation.

You probably want to replace the references to XSD types for
consistency.

:    For YANG data models, this is the module's namespace.  If the list
:    entry describes a submodule, this field contains the namespace of
:    the module to which the submodule belongs.

This text seems misplaced since I believe this has nothing to do with
the location.

:   Includes session specific data for NETCONF management sessions.
:   The session list MUST include all currently active NETCONF sessions,
:   and MAY include other sessions as well.

It is unclear what "other sessions" mean. If you mean "inactive"
sessions or "recently closed sessions", simply say so. If you mean non
NETCONF sessions, well that that kind of violates the first sentence.
Please clarify.

:     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 session table as a result.

I dislike the word table - XML is not tables like SMIv2. That said, I
find the text confusing. What you are really saying is: Monitoring
data is only provided for session with a non-zero session-id.

:   transport (identityref, transport)
:     Idenfities type for each session, e.g. "netconf-ssh",
:     "netconf-soap", etc.

Replace with "Identifies that transport for each ..."

:   username (string)
:     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.

I do not want to boil the ocean, but we will likely have to work this
out in more detail later when access control is defined and we need a
deterministic way to derive a username from whatever has been used for
authentication. Those who follow the ISMS work will know why I have to
comment on this. So take this as a warning that in the future, we will
have to define much more precise ways to derive a username if we
associate access control rules with user identities.

:   When the schema is available on the device this operation is
:   used to return it via NETCONF.

This conditional sentence sounds backwards. What about this:

     This operation can be used to retrieve a schema form the NETCONF
     server.

:             module bar {
:               bar version 2008-06-01 yang module
:               contents here ...
:             }

Make this legal yang by using comments:

             module bar {
               // bar version 2008-06-01 yang module
               // contents here ...
             }

:           module bar-types {
:             bar-types version 2008-06-01 yang module
:             contents here ...
:           }

Make this legal yang by using comments:

           module bar-types {
             // bar-types version 2008-06-01 yang module
             // contents here ...
           }

:  identity transport {
:    description
:      "Base identity for session types.";
:  }

This should be "Base identity for NETCONF transports.". Please remove all
mentions of "session types".

:    reference "RFC 4742";

Replace with

    reference "RFC 4742: Using the NETCONF Configuration Protocol 
                         over Secure SHell (SSH)";

:    reference "RFC 4743";

Replace with

    reference "RFC 4743: Using NETCONF over the Simple Object 
		         Access Protocol (SOAP)";

:    reference "RFC 4744";

Replace with

    reference "RFC 4744: Using the NETCONF Protocol over the
                         Blocks Extensible Exchange Protocol (BEEP)";

:    reference "RFC 5539";

Replace with

    reference "RFC 5539: NETCONF over Transport Layer Security (TLS)";

:    reference "W3C REC REC-xmlschema-1-20041028";

Replace with

    reference "W3C REC REC-xmlschema-1-20041028: XML Schema Part 1: Structure
               W3C REC REC-xmlschema-2-20041028: XML Schema Part 2: Datatypes";

:    reference "ISO/IEC 19757-2";

Replace with

    reference "ISO/IEC 19757-2: RELAX NG";

:    reference "ISO/IEC 19757-2";

Replace with

    reference "ISO/IEC 19757-2: RELAX NG";

:            list partial-locks {
:              key lock-id;
:              description
:                "For a partial lock this is the lock id returned
:                  in the <partial-lock> response.";
:              leaf lock-id {
:                type uint32;
:              }

The description like should be a description of the leaf lock-id and
not the partial-locks list itself.

:      list session {
:        key session-id;
:        leaf session-id {
:          type session-id;
:        }
:        leaf transport {
:          mandatory true;
:          type identityref {
:            base transport;
:          }
:        }
:        leaf username  {
:          type string;
:        }
:        leaf source-host {
:          type inet:host;
:        }

I am badly missing description statements here. In general, it seems
that many description statements leave out details that are explained
in section 2. In SMIv2 land, it was considered good practice to have
all normative texts in the data model so that the text is there once
extracted from the RFC. I believe we should follow this practice and
move most of the text from section 2.1 into description clauses. This
also avoids any potential inconsistencies.

        leaf login-time {
          mandatory true;
          type yang:date-and-time;
          description
            "Time at which the session was established.";
        }

I am wondering why some objects are mandatory while others are
not. For example, why is the login-time mandatory while the
netconf-start-time is not mandatory? It is not clear to me as a reader
whether there are any specific requirements on login-time that give it
a special status and that do not apply to netconf-start-time.

Finally, I like to comment that I find the choice lock-type a somewhat
artifical construction.  I understand that a global lock and a partial
lock can't exist together. Still, I would have modeled a simple
global-lock container and a partial-locks list. I find this somewhat
simpler to follow, also because partial locks really are a
feature. (The model does not really distinguish features in general.)
But perhaps this is just a style issue and not really important to
agree on.

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