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

[Andy Bierman <[e-mail]>]

Hi Kent,


On Wed, Aug 9, 2017 at 1:48 PM, Kent Watsen <[e-mail]> wrote:

> Hi Andy,
>
> >>> Modules:
> >>>  (M1) [e-mail]
> >>>  (M2) [e-mail]
> >>>
> >>> 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
>
> Fixed in my local copy.
>
>
> >>> 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.
>
> 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.";
>


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.



> and I also added a design note into the draft itself.
>
>
> > 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.
>
> 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?
>
>
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.



>
> > So perhaps you should add if-feature-stmts:
> >
> >  feature tls-listen {
> >     if-feature listen;
> >     ...
> >  }
> >
> >  feature tls-listen {
> >    if-feature listen;
> >    ...
> >  }
>
> I went ahead and made this change, it seems better.
>
>
>
> >>> C3:
> >
> > 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.
>
>
>
> >>> 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?
>
>    "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.";
>
>
>
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.



>
> >>>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.
>
> 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.
>
>
> > This is 1 exception -- another is close-session.
> >
> >   POST /restconf/operations/ietf-netconf:close-session
> >
> > 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?
>
>

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.








>
>
> >>> 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
>
> Yep.
>
>
> >>> Nits:
> >>>
> >>> 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.
>
> Agreed.
>
>
>
> > Andy
>
> Kent
>
>
>
Andy