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

[Kent Watsen <[e-mail]>]

Hi Andy,

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

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


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

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


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

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



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

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.



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

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

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.


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

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


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

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?




> Andy

Kent