Subject: [Netconf] review of draft-ietf-netconf-restconf-client-server-04

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 4 Aug 2017 10:42:38 +0200

Here is my review of draft-ietf-netconf-restconf-client-server-04.

Several comments may also apply to
draft-ietf-netconf-netconf-client-server-04 but I have not yet read
this netconf version in detail yet.

- what is this editor note?

  EDITOR NOTE: Please ignore this section, it is incomplete.

  Should there not be more meaningful text?

- Is this correct?

  Just the container is displayed below, but there is also a grouping
  that the container is using.

- autors and copyright year

  These seem to be out of sync in the YANG contact statement.

- endpoint-container [client side]

  OLD

           "The IP address or hostname of the endpoint.  If a
            hostname is configured and the DNS resolution results
            in more than one IP address, the RESTCONF client
            will process the IP addresses as if they had been
            explicitly configured in place of the hostname.";

  NEW

           "The IP address or hostname of the endpoint.  If a
            domain name is configured, then the DNS resolution
	    should happen on each usage attempt. If the the
            DNS resolution results in multiple IP addresses,
	    the IP addresses will be tried according to local
	    preference order until a connection has been 
	    established or until all IP addresses have failed.";

- duplication of endpoints-container

  Since the endpoint-container grouping is duplicated and only used
  once where it is defined, why not inline it? I thought about
  factoring it out but see the endpoint discussion further below...

- endpoints-container [client] description

  The description

      "This grouping is used to configure a set of RESTCONF servers
       a RESTCONF client may initiate connections to.";

  seems to say the wrong thing. Anyway, if the grouping is RESTCONF
  specific and only used once, defining the grouping is somewhat
  pointless. Furthermore, using two definitions of endpoints-container
  in a single document is somewhat confusing.

- endpoint lists asymmetry

  For listening endpoints, the following pattern is used (list with
  embedded choice):

           +--rw endpoint* [name]
              +--rw name    string
              +--rw (transport)
                 +--:(ssh) {ssh-listen}?
                 |  +--rw ssh
                 |     +--rw address?            inet:ip-address
                 |     +--rw port?               inet:port-number
		 :     : /* ssh specifics per endpoint */
                 +--:(tls) {tls-listen}?
                    +--rw tls
                       +--rw address?        inet:ip-address
                       +--rw port?           inet:port-number
		       : /* tls specifics per endpoint */

  Note this is a single list of listening endpoints (each list element
  with a unique name) for all transports supported.

  For initiating endpoints, the structure is different (choice with
  embedded lists):

               +--:(ssh) {ssh-initiate}?
               |  +--rw ssh
               |     +--rw endpoints
               |     |  +--rw endpoint* [name]
               |     |     +--rw name       string
               |     |     +--rw address    inet:host
               |     |     +--rw port?      inet:port-number
	       :     : /* ssh specifics for _all_ endpoints */
               +--:(tls) {tls-initiate}?
                  +--rw tls
                     +--rw endpoints
                     |  +--rw endpoint* [name]
                     |     +--rw name       string
                     |     +--rw address    inet:host
                     |     +--rw port?      inet:port-number
		     : /* tls specifics for _all_ endpoints */

  Note that this means different uniqueness requirements for endpoints
  names. Furthermore, the second version essentially boils down to

  case ssh {
      container ssh {
          uses endpoints-container { ... }
          uses ss:ssh-client-grouping { ... }
      }
  }

  This looks nice but then it is not possible to have different ssh
  client parameters for different endpoints, which may be a serious
  limitation.  The first nesting structure allows endpoint specific
  ssh parameters but it seems more complex to use reusable
  groupings.

  As explained in draft-schoenw-netmod-yang-pattern-00, using multiple
  lists in a choice furthermore has the disadvantage that it is not
  possible to generically refer to a listening endpoint, e.g., using a
  simple leafref.

  My conclusion is that we should be really using "list with embedded
  choice" and not "choice with embedded list".

- 3.1 Tree Diagramm - What does this tell me?

     Just the container is displayed below, but there is also a grouping
     that the container is using.

- What about the commented out NACM import?

- s/REETCONF/RESTCONF/

- What is the purpose of the feature client-cert-auth?

- Why is restconf-server defined as a grouping? It is used only once,
  no?

- Is the listening address really optional? Which address is used if
  the leaf is not present? I think the description text is confusing
  ('of the interfaces' and 'all interfaces') and I would rather make
  the address mandatory. How about:

     mandatory true;
     description
       "The IP address of the endpoint to listen on. Please note that
        some addresses have special meanings (e.g., '0.0.0.0' and '::').";

  I think also the port leaf description should be reworded:

     description
       "The port number of this endpoint to listen on.";

- As discussed above, I suggest the structure of the crurrent call
  home lists and end endpoint list should be changed:

  call-home
    - endpoint* [name]
      - name
        :tls
	  - address
	  - port
	  - tls parameters
      - call home logic (connection type, etc.)

   Yes, this requires to duplicate call home logic if multiple clients
   are configured to call home to but it also easily allows them to be
   different.

- fallback connection types?

  We have persistent and periodic call homes. Is there a need to
  support fallback call homes, e.g., adding a case "fallback" (or
  whatever the right term) which is used when some other connections
  failed? The fallback container could have a leafref to the endpoint
  it serves as a fallback for.

  OK, I see something like this was intended with the reconnect
  strategy. But I think things would be simpler if the server always
  processes all call home endpoints according to the connection-type
  and the connection-type defines if some are there only has
  fallbacks.

  It is also unclear how reconnect-strategy/max-attempts interacts
  with keep-alives/max-attempts.

- security considerations

  This needs some work (remove RPC text since there are no RPCs), add
  discussion of the different subtrees.

- references

  I am wondering whether RFC 8040 and RFC 8071 are not normative
  references for this document.

  It seems NACM is not normative since it is not used (only in a
  commented out section) in anything normatively defined here.

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

---

From: Kent Watsen <[email-masked]>
Date: Fri, 11 Aug 2017 23:09:08 +0000

*** I will post updated drafts as soon as the reviews settle
*** down, even if not all the issues have been resolved.  

Hi Juergen,

> draft-ietf-netconf-netconf-client-server-04 but I have not yet
> read the netconf version in detail yet.

understood.  Though, of course, as I'm making fixes here, I'll be
looking to make the same fix in netconf-client-server.

>  Should there not be more meaningful text?

Silly me, I forgot to remove the note after I added the text 
needed to remove the note.  It's removed now.

>  that the container is using.

Yes, just the container is being displayed.  I'm not, for instance, using
pyang's --tree-print-groupings option.

> - authors and copyright year
>

You made a similar statement for the SSH/TLS modules.  I'm not sure
why these lists need to be in-sync.  For instance, your "contribution"
is the "cert-maps" structure from back when you were working on 
rfc5539bis.  It makes sense to add you to the server yang modules that
use it, and maybe promote you to draft-author for it, but it wouldn't
make sense to add you as an author on the client yang modules that
don't use the cert-maps structure.   Or maybe we should add everybody
everywhere?  I don't really care, I'm just trying to understand the
rules for such things.   Perhaps we should remove Authors from YANG
modules altogether, much like we removed WG Chairs before?  [I just
noticed that you weren't actually listed in the server modules - fixed]

>            established or until all IP addresses have failed.";

Fixed (2 locations in the RC draft, and 2 locations in the NC draft)

>  factoring it out but see the endpoint discussion further below...

I see your point.  The solution below, list with embedded choice, 
resolves this by eliminating the endpoints-container grouping 
altogether.

>  in a single document is somewhat confusing.

The description seems accurate to me (in ietf-restconf-client).
Regardless, the solution below, list with embedded choice, resolves
this by eliminating the endpoints-container grouping altogether.

>   choice" and not "choice with embedded list".

Great observation, consistency is good.  It took a while, but this
change is done, in all of the NC/RC x client/server + examples.  Doing
this eliminated the need for the 'endpoints-container' grouping, which
resolved your other issues above.

> - 3.1 Tree Diagram - What does this tell me?
>

I'm trying to say that the tree output is from:
  pyang -f tree <yang-file>
Not:
  pyang -f tree --tree-print-groupings <yang-file>

> - What about the commented out NACM import?

I removed it from my local copy.  I also added a Security Consideration
note for why there are no NACM annotations.

> - s/REETCONF/RESTCONF/

Fixed.

> - What is the purpose of the feature client-cert-auth?

Leftover cruft, it's now removed.

> - Why is restconf-server defined as a grouping? It is used only once,

This was the result of that long thread we had a couple months ago.  Now
all of the NC/RC x client/server modules define both containers (even
though I don't think the client modules are needed) and groupings (so 
that apps can embed any of these constructs within them).  For instance,
a controller application might contain both NC & RC clients, the app's
config can use these groupings.

>        some addresses have special meanings (e.g., '0.0.0.0' and '::').";

When not specified, the default is for the system to listen to all
interfaces, which we should have a way to do.  I don't know if special
addresses are defined anywhere.  I rewrote the description to say that
*some* addresses may have similar meanings on *some* systems:

    "The IP address to listen on for incoming call-home
     connections.  The RESTCONF server will listen on 
     all interfaces if no value is specified.  Please
     note that some addresses may have similar meanings
     on some systems (e.g., '0.0.0.0' and '::').";

Are these special addresses defined anywhere?

>       "The port number of this endpoint to listen on.";

Updated.

>   are configured to call home to but it also easily allows them to be

I fixed this previously.  The issue isn't duplication between call-home
clients, the issue is duplication between the endpoints within a
single call-home client.

>  it serves as a fallback for.

No need, we have a list we round-robin through.  If there were a
'fallback', we'd need it to be a list too, for high-availability
reasons.

>  and the connection-type defines if some are there only has

The reconnect strategy is intended to work with both the persistent
and periodic connection-types, while related, it can be factored out,
which is what's been done here.  I don't know what you mean by
"process all call home endpoints according to the connection-type",
at least my implement forks/execs them all as independent threads.

>  with keep-alives/max-attempts.

These are different max-attempts.  One counts the number of connection
attempts, the other counts the number of [failed] keep-alive attempts.
They don't interact with each other.  One takes affect when there is no
connection, the other takes affect when there is (presumably) to
connection.

>  discussion of the different subtrees.

This is being discussed in the SSH/TLS reviews but, for the most part,
I think that it's good/better as is.  If it's intended to delete parts
of the template the don't apply, then I'd like to see that instruction
put into the template.

>  references for this document.

Interesting question, but I went ahead and moved them to the Normative
section, since I think that it's the safer/more-conservative option.

>  commented out section) in anything normatively defined here.

Agreed, moved to Informative.

> /js

Kent

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sat, 12 Aug 2017 10:22:33 +0200

> noticed that you weren't actually listed in the server modules - fixed]

The core of a YANG data model RFC is the YANG data model and hence by
default I assume RFC author == YANG data model author and I assumed an
editing glitch here like with the tls/ssh drafts. This is why I wrote
the comment. I am fine to be removed from these documents; the fact
that cert-maps are reused does not make me a co-author.

My involvement goes back to draft-kwatsen-netconf-server-00 (January
2014) and I think even this was an offspring from some other work,
call home maybe. But again, I am happy to not be co-author of this
since these documents have been reorganized and rearranged N times
during the years anyway and I have not been deeply involved in the
last iterations.

>   pyang -f tree --tree-print-groupings <yang-file>

OK. Not sure it is necessary to say that since tree diagrams by
default show the groupings expanded I think. I do not know whether the
official tree diagram formats will have ways to show say a container
with used groupings collapsed. This may actually be useful sometimes
but this is not what you are look for here either. I am thinking about

  +--rw container
     +-u-- <grouping>
     +--rw regular-leaf

Clearly, this is for a different threat...

> Are these special addresses defined anywhere?

Are there systems where listening on 0.0.0.0 does not mean all IPv4
addreses and to listening to :: does not mean all IPv6 addresses?  I
rather have this not optional and use 0.0.0.0 and ::. If you write
'all interfaces', does this mean IP interfaces (likely yes), does it
mean both IPv4 and IPv6 interfaces (unclear), and if so, can I expect
a single socket using a magic mapping or can I expect two sockets with
separate address families? I think all this can be avoided by making
this leaf mandatory and using the ANY addresses.

/js

---

From: Kent Watsen <[email-masked]>
Date: Thu, 17 Aug 2017 21:38:40 +0000

Hi Juergen,

> during the years anyway and I have not been deeply involved in the

I don't think that its right, but if you insist, it's your choice.
Are you sure?

> default show the groupings expanded I think.

Only protocol-accessible nodes (containers, rpcs, notifications)
are shown.  Of course, groupings *used* by containers are also
shown.  However, stand-alone groupings are not shown by default,
that requires --tree-print-groupings

> I do not know whether the

It might be helpful to have the ability to output a tree diagram that
has not expanded its groupings.  But, as you say, for another thread.
Back to the original comment, regarding the sentence in section 3.1 
(and similar sections elsewhere), do you have a proposal for how this
can be said better?

> this leaf mandatory and using the ANY addresses.

Okay, how about this?

  leaf address {
    type inet:ip-address;
    mandatory true;
    description
      "The IP address to listen on for incoming connections.   
       The RESTCONF server will listen on all interfaces if
       no value is specified.  Please note that INADDR_ANY
       (0.0.0.0) or INADDR6_ANY (0:0:0:0:0:0:0:0 a.k.a. ::)
       MUST be used when the server is to listen on all
       IPv4 or IPv6 addresses respectively.";
  }
 
Do I need any refs for this?

K.

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 21 Aug 2017 11:36:44 +0200

> I don't think that its right, but if you insist, it's your choice.

I am just saying I do not care that much and I happily leave it to the
chairs to judge. If my contribution is the import of the cert-maps,
well then this is not a reason to be co-author.
 
> (and similar sections elsewhere), do you have a proposal for how this

You are doing the default, right? So perhaps saying nothing is
good. Or if you want to say something, I think I understand you wanted
to say something like this:

  The schema tree is show below, including the expansion of all
  groupings.
 
> Do I need any refs for this?

I do still do not know what 'on all interfaces' boils down to, I think
this needs to be more clearly defined. Does this imply I have to use
v4 mapped addresses of some sort on a dual stack implementation? My
experience is that this is more a pain than a feature, if you want to
handle any IPv4 address and any IPv6 addresses, it is often better to
listen on 0.0.0.0 and in addition on ::.

/js