Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-netconf-restconf-client-server-04

From: Andy Bierman <[email-masked]>
Date: Tue, 1 Aug 2017 12:32:04 -0700

On Mon, Jul 31, 2017 at 3:56 PM, Kent Watsen <[email-masked]> wrote:

> Hi Andy,
>
> Thanks for your review.  Below are my responses to your comments.
>
> Thanks,
> Kent
>
>
> --
>
> Reviewer: Andy Bierman
> Review result: Ready with Nits
>
> Review: draft-ietf-netconf-restconf-client-server-04
>
> Modules:
>  (M1) [email-masked]
>  (M2) [email-masked]
>
>
> YANG Usage:
>
>  I did not find anything wrong in either module.
>  pyang and yangdump-pro do not report any errors or warnings.
>
> Comments:
>
> C1:
>
>  (M1) RESTCONF does not have sessions so the term "RESTCONF session"
>  needs to be defined
>
>
> <KENT> good point, in this case, it's actually the underlying transport
> (TLS) that has a session.  Fixed.  But what about the 'max-sessions' leaf,
> under 'listen', in both modules?  - should this also be changed to talk
> about the TLS session, or removed altogether?
>
>
>

I think you can change it to "transport session" or "TLS session"
if the concept of session reconnect is is kept for RESTCONF

>
> C2:
>
>  (M1) feature tls-initiate
>  (M1) feature tls-listen
>  (M2) feature tls-listen
>  (M2) feature tls-call-home
>
>  Are these features really needed since a RESTCONF server MUST support
>  the TLS transport?
>
> <KENT> the features themselves are needed, as we don't know what
> clients/servers may support.
>

OK -- These features are nested within other features (initiate, listen).
You should add a comment somewhere that TLS is optional-to-support,
but mandatory to support if the TLS transport is implemented.
Currently RESTCONF requires TLS be supported, but future
versions could be different.

Also, recent YANG 1.1 discussions concluded (wrongly IMO) that nested
nodes do not inherit the if-feature-stmts of ancestors.  It is not clear
how one would access /foo/bar if /foo was not supported.

 So perhaps you should add if-feature-stmts:

  feature tls-listen {
     if-feature listen;
     ...
  }

  feature tls-listen {
    if-feature listen;
    ...
  }

> <KENT> the "tls" in the name of these features is likely unnecessary but
> 1) maybe, someday, there will be another transport? and, 2) these names are
> consistent with the names in the ssh modules, where there are both the
> "tls" and "ssh" based feature names, and so make more sense there.
>
>
>
> C3:
>
>  (M1)  /restconf-client/initiate/restconf-server/persistent/
>
>  RESTCONF has no session setup; It only has request and
>  response interactions. The purpose of reconnect, idle timeouts etc.
>  are completely up to the client application.
>
>
> <KENT> similar to the above, this is really meant to reflect the TLS
> layer.  It may not make too much sense for /restconf-client/initiate/restconf-server/persistent.
>  That said, I think it's okay, in the sense that a real HTTPS client might
> keep the underlying TLS session open across many HTTP-level interactions as
> a performance optimization.  What do you think?
>
>
Change it to be TLS session

> FWIW, it does *make* sense for /restconf-server/call-home/res
> tconf-client/connection-type/persistent/.  For instance, a client may
> need to keep the call-home connection open in order to send messages to the
> server when it needs to...
>
>
>
OK -- this may be more clear than hiding persistent in max-retries ==
unlimited

>
> C4:
>
>  (M1)  /restconf-client/initiate/restconf-server/persistent/max-attempts
>
>  What should the RESTCONF client do if a "Connection: close"
>  header is received from the server (indicating the server is dropping
>  the TCP connection) Should the reconnects proceed?
>
> <KENT> I don't understand the question.  The 'max-attempts' regards
> testing the aliveness of a server that the client is already connected
> to.  But, assuming the client was configured to try to maintain a
> persisted TLS connection, the client would immediately try to reconnect
> to one of the configured RESTCONF servers.
>

OK -- this is how our server works. It does not make a special case for
Connection: close.

>
> C5:
>
>  (M1)  /restconf-client/initiate/restconf-server/periodic/
>
>
>       "Periodically connect to the RESTCONF server, so that
>        the RESTCONF server may deliver messages pending for
>        the RESTCONF client.  The RESTCONF server must close
>        the connection when it is ready to release it.
>
> This is not how RESTCONF works.
> The RESTCONF server requires a request message in order to
> send content to a client.  I do not understand the use-case
> for periodic RESTCONF sessions.
>
> <KENT> Right, the server might be buffering data to send to
> the client (e.g., logs).  Maybe it's a wording issue?
>
> OLD
>
>   server may deliver messages pending for the RESTCONF client
>
> NEW
>
>   client can collect data (e.g., logs) from the RESTCONF server
>
> What do you think?
>

It should be clear that the client would have to issue a GET or maybe POST
to initiate the sending of stored data like logs

>
>
>
>
> C6:
>
>  (M1)  /restconf-client/initiate/restconf-server/periodic/
>  (M2)  /restconf-server/call-home/restconf-client/connection-type/
> periodic/
>
>  The RESTCONF notifications via SSE should be exempt from timeouts.
>  The RESTCONF client should terminate an SSE request. The server should
>  not timeout an SSE response connection
>
>
> <KENT> Agreed, and the description statement for the 'idle-timeout' leaf
> says "Sessions that have a notification subscription active are never
> dropped.  But here, under 'periodic', the whole point is to shed idle
> connections.  For instance, imagine a client that wants to collect logs
> from a low-powered server once a day.  What do you think?
>
>

Notification replay has a mode where it is terminated (i.e,., end-time
provided)
I think this would have to be used by the client.  This tells the server to
drop
the connection when the replay is complete.

This is 1 exception -- another is close-session.

   POST /restconf/operations/ietf-netconf:close-session

Is this supposed to do anything in RESTCONF?

>
> C7:
>
> Sec 1.2 Tree Diagrams
>
> Old text should be replaced with reference to
> draft-ietf-netmod-yang-tree-diagrams-01
>
> <KENT> same comment as before.
>
>
>

OK -- wait for tree draft to be stable

>
> C8:
>
>  (M1) IETF copyright says 2014; change to 2017
>  (M2) IETF copyright says 2014; change to 2017
>
> <KENT> fixed.
>
>
>
> Nits:
>
> Some descriptions mention SSH. This should be removed from
> this document.
>
> <KENT> done.
>
>
> The descriptions for choices that have only 1 case should
> explain why a choice is being used.
>
> <KENT> done.
>
>
> The tree diagram shows the fully expanded groupings,
> even many objects are in other drafts. I needed all 5 drafts
> open in windows for searching, plus pyang tree output,
> to really follow the data model structure.
>
> <KENT> I know.  Do you have a proposal?
>
>
The external nodes cannot really use a prefix because they
are pulled in from groupings, and technically in the same module namespace.
I don't know if any changes to the tree diagram should be made for
uses external-grouping.

>
>
> The examples do not show any usage of the hello-params.
> This would be useful because the namespace for the
> identityref objects (tls-version, cipher-suite) is not
> the same as the module that uses the grouping.
>
> <KENT> Point taken.  I sent Gary an email on this also.
>
>
>
> Note that the 'map-type' identityref is shown correctly encoded
> in examples on page 19 and 20.
>
> <KENT> not sure which draft you mean, but it's okay, I know how to prefex
> identities.
>
>
>
>

Andy

---

From: Kent Watsen <[email-masked]>
Date: Wed, 9 Aug 2017 20:48:11 +0000

Hi Andy,

>> <KENT> good point, in this case, it's actually the underlying 
>> transport (TLS) that has a session.  Fixed.  But what about the
>> 'max-sessions' leaf, under 'listen', in both modules?  - should
>> this also be changed to talk about the TLS session, or removed
>> altogether?
>

Fixed in my local copy.

> versions could be different.

I added this to the description statement of the 'tls-listen' and
'tls-initiate' features:

      Currently TLS is the only transport supported by RESTCONF,
      this feature helps support possible future transports.";

and I also added a design note into the draft itself.

> how one would access /foo/bar if /foo was not supported.

Wait, seriously? I didn't see that.  I don't understand how else nested
if-feature statements can be interpreted.  Can you point to the thread
where this was discussed?

> So perhaps you should add if-feature-stmts:

I went ahead and made this change, it seems better.

> Change it to be TLS session

Done.

>>> C4:
> 
> OK -- this is how our server works. It does not make a special case for Connection: close.

Correct.

> to initiate the sending of stored data like logs

Okay, how about this?

   "Periodically connect to the RESTCONF server, so that,
    e.g., the RESTCONF client can collect data (logs) 
    from the RESTCONF server.  The RESTCONF client must
    close the connection when it is ready to release it.
    Once the connection is closed, the RESTCONF client
    restarts its timer until the next connection.";

>>> (M2)  /restconf-server/call-home/restconf-client/connection-type/periodic/
>>>
> Notification replay has a mode where it is terminated (i.e,., end-time provided)
> I think this would have to be used by the client.  This tells the server to drop

Sure, but keep in mind that logs are just an example, there could be other
data-collection mechanisms.  Still, note that when end-time provided is
reached, the line would become idle, if it didn't drop.  This seems like a
more generic mechanism.

> Is this supposed to do anything in RESTCONF?

First, I don't view the above as an exception.  With regards to 'close-
session', does it even have a meaning in RESTCONF?  - and is this actually
an issue for RFC 8040?

> OK -- wait for tree draft to be stable

Yep.

> are pulled in from groupings, and technically in the same module
> namespace. I don't know if any changes to the tree diagram 
> should be made for uses external-grouping.

Agreed.

Kent

---

From: Andy Bierman <[email-masked]>
Date: Thu, 10 Aug 2017 09:48:40 -0700

Hi Kent,

>       this feature helps support possible future transports.";

If you are trying to say that TLS might not be the mandatory-to-implement
transport, this sentence is not quite right.

You know I am not a big fan of YANG conformance.
We have nothing in between "always mandatory" and "purely optional,
server developer's choice".  We have nothing like conditionally
mandatory "If the RESTCONF server supports listening on TLS , then
the listen-tls feature MUST be supported."

The when-stmt can be used to provide this conditionally-mandatory
conformance,
but it has to be customized for each data subtree. It is more expensive
than if-feature
on many levels.

> where this was discussed?
I cannot find it now.
Essentially, features are not coupled to objects:

  container foo {
     if-feature A;
     container foo2 {
        if-feature AA;
     }
  }

  container bar {
     if-feature AA;
   }

The designer has to decide:
Does feature AA depend on A, or just container foo2 depends on container
foo?
Does an external subtree (like /bar) depend on both features or can they be
used independently?

In this case I think the intent is to support feature listen if any
transport is supported,
and feature listen-<transport> is a specific transport is supported, so the
if-feature
inside the 2nd feature is correct.

>     restarts its timer until the next connection.";
Not sure this is needed.
RESTCONF does not define any interactions that are not initiated by the
client,
but if that changes, then no reason to constrain this text. I would change
'must' to 'SHOULD' in the 2nd sentence.

> session', does it even have a meaning in RESTCONF?  - and is this actually

RESTCONF does not specify any behavior for specific operation resources.
It is left as an implementation detail.  Our server treats each RESTCONF
request
as a new management session, so a NETCONF operation like <lock> is granted
and then released right afterwards by the server because the session is
terminated.
I think any standard behavior for application layer sessions would need to
be defined
in a new versions of RESTCONF.

Andy

---

From: Kent Watsen <[email-masked]>
Date: Tue, 15 Aug 2017 01:39:47 +0000

Hi Andy,

> transport, this sentence is not quite right.

Fine, now it says "This feature exists as TLS might not be a 
mandatory-to-implement transport in the future."

> the listen-tls feature MUST be supported."

This seems unnecessary.  Already the 'if-feature' statements will
force the server to advertise the features - or else they're not
configurable!

> conformance, but it has to be customized for each data subtree. It
> is more expensive than if-feature on many levels.

No thank-you.  Let's stick with just the feature statements.

> Does feature AA depend on A, or just container foo2 depends
> on container foo?  Does an external subtree (like /bar) depend
> on both features or can they be used independently?

I don't think the designer has any choice in this.  The features
themselves have no inherent dependency (in your example, we can't
see if the feature definitions actually define a dependency), but
the containers certainly do (e.g., foo2 depends on foo).  External
sub-trees (e.g., bar) can have just if-feature AA, without also
having an if-feature A (assuming that dependency doesn't exist).

> In this case I think the intent is to support feature listen if
> any transport is supported, and feature listen-<transport> is
> a specific transport is supported, so the if-feature inside the
> 2nd feature is correct.

Yes.

> I went ahead and made this change, it seems better.

Nothing to add here, but I'm leaving it for now since it adds
to the above discussion.

>>>> C5:
>>>
>>> It should be clear that the client would have to issue a GET
>>> or maybe POST to initiate the sending of stored data like logs
>>
>> Okay, how about this?
>>
> RESTCONF does not define any interactions that are not initiated
> by the client, but if that changes, then no reason to constrain
> this text. I would change 'must' to 'SHOULD' in the 2nd sentence.

Oh, now I see your concern, how about this?  (I removed the sentence
altogether):

     "Periodically connect to the RESTCONF client, so that
      the RESTCONF client may deliver messages pending for
      the RESTCONF server.  Once the connection has been
      closed, for whatever reason, the server will restart
      its timer until the next connection.";

...and by "for whatever reason", I mean, either due to the
client closing the connection, or due to reasons such as
an idle or keep-alive timeout.

>> Notification replay has a mode where it is terminated (i.e,., 
>> end-time >provided) I think this would have to be used by the
>> client.  This tells the server to drop the connection when the
>> replay is complete.
>
> Sure, but keep in mind that logs are just an example, there could
> be other data-collection mechanisms.  Still, note that when end-time
> provided is reached, the line would become idle, if it didn't drop.
> This seems like a more generic mechanism.

I have nothing to add here, but I'm keeping it here since it's 
connected to the below comment.

>> First, I don't view the above as an exception.  With regards to
>> 'close-session', does it even have a meaning in RESTCONF?  - and
>> is this actually an issue for RFC 8040?
>
> RESTCONF does not specify any behavior for specific operation
> resources.  It is left as an implementation detail.  Our server
> treats each RESTCONF request as a new management session, so a
> NETCONF operation like <lock> is granted and then released right
> afterwards by the server because the session is terminated.  I
> think any standard behavior for application layer sessions would
> need to be defined in a new versions of RESTCONF.

Okay, but can you connect this to the draft?  Are you asking for
a change, or are you actually thinking about a clarification 
needed for RFC 8040?

Kent

---

From: Andy Bierman <[email-masked]>
Date: Mon, 14 Aug 2017 19:02:53 -0700

> force the server to advertise the features - or else they're not

Nothing forces the server to developer to implement a particular object,
because features are purely optional. The description-stmt in the feature
can make this conditional requirement.  But not a big deal
because the IETF is too server-centric to care about the client POV
of YANG conformance.

> No thank-you.  Let's stick with just the feature statements.
agreed

>       its timer until the next connection.";
I would change messages to requests

> a change, or are you actually thinking about a clarification

not asking for a change.
It is currently up to the implementation.
For example CGI will call the server once for each request but Fast-CGI
will call the thin-client once for each HTTP session and call the server
over and over for each request on that session.

Andy

---

From: Kent Watsen <[email-masked]>
Date: Mon, 21 Aug 2017 16:22:51 +0000

Hi Andy,

Trimming down to just the still-open discussion points.

Thanks,
Kent

> because the IETF is too server-centric to care about the client POV

I don't see this as a big deal.  Regardless, it's not for this draft
to resolve, right? 

Done.

> For example CGI will call the server once for each request but
> Fast-CGI will call the thin-client once for each HTTP session
> and call the server over and over for each request on that
> session.

Okay, I'll consider this issue closed then.

Kent

---

From: Andy Bierman <[email-masked]>
Date: Mon, 21 Aug 2017 09:26:15 -0700

> I don't see this as a big deal.  Regardless, it's not for this draft
right