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

[Kent Watsen <[e-mail]>]

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.";

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?



> 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.";



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



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