Re: [netconf] Adam Roach's Discuss on draft-ietf-netconf-restconf-notif-13: (with DISCUSS and COMMENT)

["Reshad Rahman (rrahman)" <[e-mail]>]

Hi,

Thanks for the review, please see inline.


On 2019-05-15, 3:20 AM, "netconf on behalf of Adam Roach via Datatracker" <[e-mail] on behalf of [e-mail]> wrote:

    Adam Roach has entered the following ballot position for
    draft-ietf-netconf-restconf-notif-13: Discuss
    
    When responding, please keep the subject line intact and reply to all
    email addresses included in the To and CC lines. (Feel free to cut this
    introductory paragraph, however.)
    
    
    Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
    for more information about IESG DISCUSS and COMMENT positions.
    
    
    The document, along with other ballot positions, can be found here:
    https://datatracker.ietf.org/doc/draft-ietf-netconf-restconf-notif/
    
    
    
    ----------------------------------------------------------------------
    DISCUSS:
    ----------------------------------------------------------------------
    
    Thanks for all the work that the authors and other contributors have
    put into this document. I have two comments that need to be addressed
    before publication, but they should both be very easy to fix.
    
    ---------------------------------------------------------------------------
    
    
    §3.3:
    
    >  If a publisher fails to serve the RPC request for one of the reasons
    >  indicated in [I-D.draft-ietf-netconf-subscribed-notifications]
    >  Section 2.4.6 or [I-D.ietf-netconf-yang-push] Appendix A, this will
    >  be indicated by "406" status code transported in the HTTP response.
    
    This really isn't what 406 means. 406 means "you sent one or more of the
    'Accept', 'Accept-Charset', 'Accept-Encoding', or 'Accept-Language' header
    fields, and I can't generate a response that satisfies what you've asked for."
    
    For some of the errors listed in the two cited sections, there is a reasonable
    semantic mapping onto existing HTTP response codes; e.g. the
    "no-such-subscription" errors could all reasonably map on to HTTP 404.  I'll
    note that RFC 8040 section 7 performs exactly this kind of mapping, so the
    approach seems to be consistent with the way that RESTCONF has elected to use
    HTTP response codes. In fact, this document already maps from the cited errors
    to error tags already, and that table maps from error-tag to
    HTTP response codes, so fixing this should be the relatively straightforward
    exercise of updaing the tables in this section to also include the HTTP response
    code that RFC 8040 maps to the indicated error-tag. For example:
    
         error identity         uses error-tag             HTTP Response
         ---------------------- --------------             -------------
         dscp-unavailable       invalid-value              400
         encoding-unsupported   invalid-value              400
         filter-unsupported     invalid-value              400
         insufficient-resources resource-denied            409
         no-such-subscription   invalid-value              404
         replay-unsupported     operation-not-supported    501
    
         error identity              uses error-tag          HTTP Response
         ----------------------      --------------          -------------
         cant-exclude                operation-not-supported 501
         datastore-not-subscribable  invalid-value           400
         no-such-subscription-resync invalid-value           404
         on-change-unsupported       operation-not-supported 501
         on-change-sync-unsupported  operation-not-supported 501
         period-unsupported          invalid-value           400
         update-too-big              too-big                 400
         sync-too-big                too-big                 400
         unchanging-selection        operation-failed        500
    
    However you choose to address this, if the error isn't related to any of the
    four header fields I mention above, then you can't specify the use of a 406.
<RR> Thank you for suggesting status codes for the different errors, much appreciated. I agree that 406 is not appropriate. Regarding 400, my understanding is that it is for invalid syntax, so for errors such as dscp-unavailable or encoding-unsupported, would 409 be more appropriate than 400?
    
    ---------------------------------------------------------------------------
    
    §3.4:
    
    This section is unclear about how Server-Sent Events are to be used (in
    particular, they don't say anything about event type to be used). Based on the
    one example in Appendix A that shows SSE syntax, I'm assuming you probably do
    not intend to use SSE "event type" fields to distinguish between events in any
    way.  This would mean that you need to specify that all SSE messages are sent
    with an event type of "message," which the server may omit (as it is the
    default specified in the Server-Side Events specification).  This means that
    clients will need to accept both:
    
    data: {
    data:   "ietf-restconf:notification" : {
    data:     "eventTime": "2007-09-01T10:00:00Z",
    data:     "ietf-subscribed-notifications:subscription-modified": {
    data:       "id": "39",
    data:       "uri": "https://example.com/restconf/subscriptions/22"
    data:       "stream-xpath-filter": "/example-module:foo",
    data:       "stream": {
    data:          "ietf-netconf-subscribed-notifications" : "NETCONF"
    data:       }
    data:     }
    data:   }
    data: }
    
    ...and...
    
    event: message
    data: {
    data:   "ietf-restconf:notification" : {
    data:     "eventTime": "2007-09-01T10:00:00Z",
    data:     "ietf-subscribed-notifications:subscription-modified": {
    data:       "id": "39",
    data:       "uri": "https://example.com/restconf/subscriptions/22"
    data:       "stream-xpath-filter": "/example-module:foo",
    data:       "stream": {
    data:          "ietf-netconf-subscribed-notifications" : "NETCONF"
    data:       }
    data:     }
    data:   }
    data: }
    
    It may be helpful to incorporate the SSE syntax into all of the notification
    examples in Appendix A (that is, all of the examples in A.2 and A.3). I would
    recommend a mix of examples with and without "event: message".
<RR> Andy Bierman (one of the co-authors) mentioned that RESTCONF does not define any values for "event" and pointed us to this text from RFC8040 P72    
   A RESTCONF
   server SHOULD NOT send the "event" or "id" fields, as there are no
   meaningful values that could be used for them that would not be
   redundant to the contents of the notification itself.    

    ----------------------------------------------------------------------
    COMMENT:
    ----------------------------------------------------------------------
    
    General comment:
    
    It's a bit unclear about what the normative relationship between a Server-Sent
    Events connection and a subscription is intended to be. For example: if I send
    an RPC command to create a subscription, and then make two different SSE
    connections to the resulting URL, will they both receive events associated
    with that subscription? If so, does a TLS heartbeat failure on one of them
    cause the entire subscription to go away? (If not, what happens?)
<RR> The resource created via the establish-subscription is for 1 subscription. There can only be 1 GET on the URI at the same time, this does not seem to be explicitly stated anywhere, so probably a good idea to add text to that effect.
    
    If I have only one connection related to a subscription, and I close the TCP
    connection, does that necessarily make the subscription go away? What if I
    set up a new TCP connection right away after closing the first one? Will
    that work?
<RR> The subscription will not go away when the TCP connection is closed. As mentioned at the end of 3.4, the subscription goes away on receipt of delete-subscription/kill-subscription RPCs or on loss of TLS heartbeat. If a client does a GET after closing the first GET, yes this is supposed to work.
    
    What if I use RPC to set up a new subscription, and then wait a few minutes
    before connecting to the subscription URL?
<RR> That is also supposed to work as long as the subscription wasn't terminated as per section 3.4.
    
    I don't think you need to answer all of these corner cases per se (although it
    would be nice), but minimally a couple of statements that clearly spell out
    the relationship between subscriptions and the connections used to deliver
    related events would help implementors figure out the answers to these
    questions.
    
    ---------------------------------------------------------------------------
    
    §2:
    
    >  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
    >  "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
    >  document are to be interpreted as described in RFC 2119 [RFC2119].
    
    Please use the boilerplate specified by RFC 8174.
<RR> Will do.    
    ---------------------------------------------------------------------------
    
    §3.1:
    
    >  As stated in Section 2.1 of [RFC8040], a subscriber MUST establish
    >  the HTTP session over TLS [RFC5246] in order to secure the content in
    >  transit.
    
    Is the intention here to restrict clients to TLS 1.2? If not, please cite
    RFC 8446 instead of RFC 5246. If so, please add text that explains why.
    
    (This comment also applies to section 9)
<RR> Will update to RFC8446.
    
    ---------------------------------------------------------------------------
    
    §3.1:
    
    >  Loss of the heartbeat MUST result in any subscription related TCP
    
    nit: "...subcription-related..."
<RR> Will change to subscription-related.    

Regards,
Reshad.
    
    _______________________________________________
    netconf mailing list
    [e-mail]
    https://www.ietf.org/mailman/listinfo/netconf