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

[Andy Bierman <[e-mail]>]

On Mon, Aug 14, 2017 at 6:39 PM, Kent Watsen <[e-mail]> wrote:

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

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.



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



>
>
> >>> 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.";
>
>
I would change messages to requests



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

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