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

["maqiufang (A)" <[e-mail]>]

Hi, Mahesh

Please see my reply inline.

From: Mahesh Jethanandani [mailto:[e-mail]]
Sent: Wednesday, June 29, 2022 7:21 AM
To: maqiufang (A) <[e-mail]>
Cc: [e-mail]; [e-mail]; Robert Wilton <[e-mail]>
Subject: Re: [netconf] Shepherd Review on draft-ietf-netconf-https-notif-10

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]<mailto:[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)  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.
This paragraph is just obviously true statement from my perspective; if the authors think it needed, I think I can live with that.


•         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.
Then I am not sure if we should redefine these terms.
·

•         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”.
That would be better, thanks.

·

•         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.
So my only question is, would it be good to state clearly that the receiver is required to be an HTTPS server in Sec.2? Just a suggestion, feel free to accept or reject it.

·

•         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.
Thanks!

•         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”.
Yes. It should be “Revised”. And please also notice your first copyright statement in both of your YANG modules is not really correct: s/ Copyright (c) … identified as the document authors/ Copyright (c) … identified as authors of the code. (see pyang --ietf-help)
•         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), 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.
Thanks.


•         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”.
Yes. And see my further comments about the copyright statement above.

•         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”.
Thanks.

·

•         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.
Sure, thanks.

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

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

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.
Yes, indeed. These should be fixed. And there is a typo in the description of the https-receiver list defined in A.2:”A list of HTTPS nofif receiver”(s/nofif/notif)

Best Regards,
Qiufang

•         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]<mailto:[e-mail]>