Re: [netconf] Shepherd Review on draft-ietf-netconf-https-notif-10

[Mahesh Jethanandani <[e-mail]>]

Hi Qiufang,

Thanks for reviewing this document. Please see comments inline with [mj]

> On Jun 20, 2022, at 7:51 PM, maqiufang (A) <[e-mail]> wrote:
> 
> Hi, all
>  
> I'm the document shepherd for this document as it moves beyond the WG for requested publication as an RFC.
>  
> As part of shepherd write-up. I read the latest version of the draft and have got some comments.
> [I reviewed the draft at -09 during WG last call, so my comments here for -10 are basically editorial with only a few small questions.]
>  
> If the authors could produce a new revision, I will start working on the shepherd write-up. Thanks;-)
>  
> Best Regards,
> Qiufang
>  
> Comments:
> Idnits(https://www6.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-netconf-https-notif-10.txt <https://www6.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-netconf-https-notif-10.txt>)  reports some error and warnings; some are innocuous, while others need to be handled:
> ** There are 2 instances of too long lines in the document, the longest one being 3 characters in excess of 72.
> [Sec 8.2] longer than 72.  Excess characters: 's': 'namespace: urn:ietf:params:xml:ns:yang:ietf-subscribed-notif-receivers'
> [Sec 8.3] longer than 72.  Excess characters: 'tif': 'Name: urn:ietf:capability:https-notif-receiver:encoding:sub-notif'

[mj] Will be fixed.

>  
> ·         Abstract
> The second paragraph
> What’s the reason for the authors to highlight it in the abstract? Isn’t it already the case in YANG notification?
> The receiver is never assumed as a NETCONF/RTESTCONF server.

[mj] It is correct that the receiver is never assumed as a NETCONF or a RESTCONF server. However, the draft does not require the use of YANG notification. 

>  
> ·         Section 1.3 Abbreviations
> ·         The expansion of HTTP here should be aligned with RFC2616, which is “Hypertext Transfer Protocol” (rather than “Hyper Text Transport Protocol”).

[mj] Will be fixed.

>  
> ·         Section 1.4 Terminology
> Should some other terms defined in RFC 8639 such as publisher, receiver, configured subscription, event also be mentioned here?

[mj] The terms you refer to, and used in the document happen to be compatible with the terms in RFC 8639 but are not the same.

>  
> ·         Section 1.4.1 Subscribed Notifications
> Why do we need a single section for this term which is defined elsewhere?

[mj] Will change the title to say “Terms imported from RFC 8639”.

>  
> ·         Section 2 Overview of Publisher to Receiver Interaction
> Would it be beneficial to state clearly in this section that the receiver which is a NETCONF or RESTCONF client, though, works as an HTTPS server to present HTTP target resources?

[mj] We do not require the receiver to be a RESTCONF client, as you observed above. The only requirement is that the receiver be an HTTPS server. Being a HTTP based protocol, it cannot be a NETCONF server or client.

>  
> ·         Section 3.4 Example
> ·         What’s the consideration to express the receiver capability "urn:ietf:capability:https-notif-receiver:encoding:sub-notif" with a "encoding"? This capability isn’t really about encoding, right? This seems kind of weird to me since you only define the URN to begin with "urn:ietf:capability:https-notif-receiver" in IANA consideration.

[mj] Good catch. We will remove the :encoding from the URN for the sub-notif capability.

> ·         The second sentence, 
> s/the "Accept" states that the *receiver* wants to/the "Accept" states that the *publisher* wants to
>                      (The HTTP request is sent by the publisher to learn the capabilities of the receiver.)
> ·         The JSON response:
>        {
>           receiver-capabilities {
>             "receiver-capability": [
>               "urn:ietf:capability:https-notif-receiver:encoding:json",
>               "urn:ietf:capability:https-notif-receiver:encoding:xml",
>               "urn:ietf:capability:https-notif-receiver:encoding:sub-notif"
>             ]
>           }
>        }
>           This is an invalid JSON expression, which I think should be:
>          {
>             "receiver-capabilities": {
>               "receiver-capability": [
>                 "urn:ietf:capability:https-notif-receiver:encoding:json",
>                 "urn:ietf:capability:https-notif-receiver:encoding:xml",
>                 "urn:ietf:capability:https-notif-receiver:encoding:sub-notif"
>               ]
>             }
>           }

[mj] Another good catch. We will replace the word “receiver" with the word “publisher”. Also, yes, we are missing the quotes around “receiver-capabilities”.

>  
> ·         Section 4.1 Request
> The second bullet of the third paragraph: Instead of saying that, for JSON-encoding purposes, the module name for the "notification" element is "ietf-restconf, the module name will instead be "ietf-https-notif".
> Unclosed quotation mark for "ietf-restconf".

[mj] Another good catch. Thanks.

>  
> ·         Section 5.2 YANG Module
> ·         The copyright year in the IETF Trust and authors Copyright Line does not match the current year

[mj] Will fix. Will also replace the word “Simplified” with “Revised”.

> ·         RFC XXXX, YANG Data Module for HTTPS Notifications.
> This is inconsistent with your title (which is "An HTTPS-based Transport for YANG Notifications"). The same case in section 6.2 and Appendix A.2.

[mj] Will fix.

>  
> ·         Section 6.1 Data Model Overview
> ·         I noticed that the authors have removed the "keepalives-supported" feature (the "keepalives" node depends on) in the abridged tree diagram (https://www.ietf.org/rfcdiff?url1=draft-ietf-netconf-https-notif-09&url2=draft-ietf-netconf-https-notif-10&difftype=--html <https://www.ietf.org/rfcdiff?url1=draft-ietf-netconf-https-notif-09&url2=draft-ietf-netconf-https-notif-10&difftype=--html>), what's the consideration for that ( I think this feature is needed from my own generated tree) ?

[mj] It turns out that my local cache of dependent modules was out of date. Will refresh all the modules and update the document.

>  
> ·         Section 6.2 YANG module
> ·         The copyright year in the IETF Trust and authors Copyright Line does not match the current year

[mj] Will fix the year, and replace the word “Simplified” with “Revised”.

>  
> ·         Section 7 Security Considerations
> Since two YANG modules are defined in this document, when you say "the YANG module"/"this YANG module" in this section, it’s not very clear which YANG module you are referring to.

[mj] We will update the section to clarify the use of words “the YANG module” and “this YANG module”.

>  
> ·         Section 8.3 The “Capabilities for HTTPS Notification Receivers” Registry
> ·         The "description" field: An arbitrary description of the algorithm.
> What’s the algorithm? Copy/paste error? s/algorithm/capability?

[mj] Again, a good catch. Will replace the word “algorithm" with “capability".

> ·         "The update policy is either "RFC Required".  Updates do not otherwise require an expert review by a Designated Expert."
> How about: The update policy is “RFC Required”(remove ‘either’). Updates do not otherwise require an expert review by a designated expert (lowercase ‘d’ and ‘e’).

[mj] Ok. Will drop the word “either” and “otherwise". For Designated Expert definition please RFC 8126.

> BTW. Is the authors’ intention that the new registry also doesn’t require a designated expert review, right?

[mj] We are telling IANA that an Designated Expert review is not required for creation or future updates.

> ·         The initial assignment for your new registry: s/Name/URN?
> The field name is URN, as defined in your registry.

[mj] Thanks for catching it. Will fix.

>  
> ·         Appendix A. Using Subscribed Notifications (RFC 8639)
> ·         The second paragraph, s/the Publisher/the publisher;

[mj] Will fix.

> ·         "and configures the "path" leaf value to "/some/path/"…", the subject is missing here, the receiver configures the "path" value to "/some/path/", or s/and/which, right?

[mj] How does this sound?

OLD:

   In both examples, the Publisher, acting as an HTTPS client, is
   configured to send notifications to a receiver at address 192.0.2.1,
   port 443, and configures the "path" leaf value to "/some/path", with
   server certificates, and the corresponding trust store that is used
   to authenticate a connection.

NEW:

   In both examples, the publisher, being an HTTPS client, is
   configured to send notifications to a receiver. 


BTW, in looking into this we uncovered two other issues. One that the example in A.1 is missing the context for the <path> statement, and the YANG module description statement for ‘path’ refers to it as a URI, where it is just a string.

>  
> ·         Appendix A.2 Not Using Subscribed Notifications
> ·         The first paragraph, s/would to need/would need to
> ·         The second paragraph, s/Note that the module is "uses"/Note that the module "uses"/ (remove "is”)?

[mj] Will fix.

Thanks.


Mahesh Jethanandani
[e-mail]