Subject: [yang-doctors] Yangdoctors last call review of draft-ietf-netconf-restconf-notif-11

From: Robert Wilton <[email-masked]>
Date: Mon, 07 Jan 2019 09:43:34 -0800

Reviewer: Robert Wilton
Review result: Ready with Issues

I have reviewed this document as part of the YANG doctors directorate's
ongoing effort to review all IETF documents being processed by the IESG.  These
comments were written with the intent of improving the operational aspects of
the IETF drafts. Comments that are not addressed in last call may be included
in AD reviews during the IESG review.  Document editors and WG chairs should
treat these comments just like any other last call comments.

This document and the associated YANG module looks like it is in good shape to
me.

The two minor issues that I had that I think need to be resolved are:
- The section 3.4 handling subscriptions by admin users wasn't intuitive to me
(relative to what is stated in draft-ietf-netconf-subscribed-notifications). 
Specifically what should an admin user be allowed to do vs the client that
initiated the subscription. - Section 4 on HTTP2 QoS seemed unclear to me.

I think that all my other comments are really just nits:

1) Section 3.2 Discovery, last paragraph.

Would it be better for this to reference draft-ietf-netconf-nmda-restconf,
section 2 and '/yang-libary/datastore' draft-ietf-netconf-rfc7895bis, section 3?

2) RPC table in section 3.3.  The identity for "kill-subscription" should
probably be "delete-subscription-error" since "kill-subscription-error" isn't
defined.

3) Diagram in 3.4.  It wasn't immediately obvious to me that the vertical lines
in the lined up with connections (a) and (b).  I'm not sure whether it is worth
adding a comment to make this more explicit.  I figured it out whilst reading
the text below, perhaps most other readers would also do so.

4) Section 3.4, bottom of page 7, "must be sent within (b)" -> "MUST be sent
within (b)"

5) Section 3.4:
   o  RPCs modify-subscription, resync-subscription and delete-
      subscription can only be done by the same RESTCONF username
      [RFC8040] who did the establish-subscription, or by a RESTCONF
      username with the required administrative permissions.  The latter
      also has access to the kill-subscription RPC.

Just to check, is it true that any RESTCONF username with the required
permissions is allowed to invoke the "delete-subscription" RPC?  Or should this
be restricted to same RESTCONF username?  In fact, I was wondering whether an
administrator should be allowed to invoke modify-subscription or
resync-subscription, or whether they should be restricted to kill-subscription
only?

6) Section 4 is unclear to me:

 - The paragraph starting "take any existing subscription "priority" ..."
 states that the "dscp" field is copied into the HTTP2 stream priority, but I
 couldn't find such a field in section 5.3 of RFC7540.  Hence, I was wondering
 whether this paragraph shouldn't instead be stating the the "weighting" field
 is copied into the HTTP2 stream priority header?

E.g.
OLD:
   o  take any existing subscription "priority", as specified by the
      "dscp" leaf node in
      [I-D.draft-ietf-netconf-subscribed-notifications], and copy it
      into the HTTP2 stream priority, [RFC7540] section 5.3, and

NEW:
   o  take any existing subscription "priority", as specified by the
      "weighting" leaf node in
      [I-D.draft-ietf-netconf-subscribed-notifications], and copy it
      into the HTTP2 stream weight, [RFC7540] section 5.3, and

There is also no mention of how the "E" (Exclusive) bit should be set in the
HTTP2 stream headers.  Is this an omission?

7) Section 6 doesn't contain any YANG Tree output.  Probably time to put that
in, but I would suggest not putting in the entire tree output, just the
relevant snippets.  If you want to include the full tree output then I would
suggest putting in an appendix instead.  The text in this section describing
the YANG model needs to be fixed since the YANG module doesn't actually define
any identities.

8) Section 7, YANG module comments

Just nits:

8.1) Check line length of the file, it looks like it goes over 69 characters.
 - In particular, the namespace statement might need to be split.
 - The module description text indentation looks off on the first line, and
 probably needs to be re-flowed. - Some of the other descriptions look wide.

8.2) The description for "subscription-modified" doesn't quite scan (as it goes
over the line break).

9) One of the filter examples appendix A.3 is clearly over 72 characters,
suggest spliting.

Thanks,
Rob

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Mon, 7 Jan 2019 19:00:50 +0000

Thanks Rob, will take a look and respond soon.

Regards,
Reshad.

On 2019-01-07, 12:43 PM, "Robert Wilton" <[email-masked]> wrote:

    Reviewer: Robert Wilton

    

    

    
    
    5) Section 3.4:

    
    
    E.g.
    OLD:
    
    NEW:

    
    
    Just nits:

    
    
    Thanks,
    Rob

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tue, 8 Jan 2019 23:18:48 +0000

Hi Rob,

Thanks again for the review, inline.

    Reviewer: Robert Wilton

    
<RR> See below for response on the issues above.

    
    Would it be better for this to reference draft-ietf-netconf-nmda-restconf,
    section 2 and '/yang-libary/datastore' draft-ietf-netconf-rfc7895bis, section 3?
<RR> I can add section 2 in the reference to nmda-restconf. I don't think the transport drafts need to reference 7895/bis, please note that YP already references 7895.
    
<RR> Good catch.
    
<RR> I'll try to make it clearer.
    
<RR> Yes.
    
    5) Section 3.4:
    
<RR> I do not see the benefit of restricting what an admin user can do. E.g. the admin may want to decrease the frequency of a periodic subscription. 

    
    E.g.
    OLD:
    
    NEW:
 <RR> Good catch.
   
    There is also no mention of how the "E" (Exclusive) bit should be set in the
    HTTP2 stream headers.  Is this an omission?
<RR> It is an omission, implied that it's not used. I can add text stating that it's set to 0.
    
<RR> I believe the YANG module was changed but not the text. Will fix and add the tree diagram.

    Just nits:

    
<RR> Ack.

Regards,
Reshad.
    
    Thanks,
    Rob

---

From: Robert Wilton <[email-masked]>
Date: Thu, 10 Jan 2019 10:18:20 +0000

Hi Reshad,

Please see inline ...

> <RR> I can add section 2 in the reference to nmda-restconf. I don't think the transport drafts need to reference 7895/bis, please note that YP already references 7895.

OK, sounds reasonable.

Off topic for this thread, but 7895bis should be published soon, and it 
obsoletes 7895, so should YP be updated to reference 7895bis?

> <RR> I do not see the benefit of restricting what an admin user can do. E.g. the admin may want to decrease the frequency of a periodic subscription.

I had a couple of concerns:
  1) Trying to align the RESTCONF handling of these RPCs to the 
behaviour specified in draft-ietf-netconf-subscribed-notifications and 
draft-ietf-netconf-netconf-event-notifications.
2) If NACM is used as the access control mechanism, then any client that 
has access to the <*-subscription> RPCs is allowed to invoke them for 
any subscription, unless additional checking is performed.  However, I 
appreciate that this extra checking could be more difficult/expensive to 
implement on some platforms.

Considering both of these, would the following rewording of the 
paragraph be better?

OLD:

NEW:

o In addition to any required access permissions (e.g., NACM), RPCs 
modify-subscription, resync-subscription and delete-subscription SHOULD 
only be allowed by the same RESTCONF  username [RFC8040] who invoked 
establish-subscription. o The kill-subscription RPC can be invoked by 
any RESTCONF username with the required administrative permissions.

> <RR> It is an omission, implied that it's not used. I can add text stating that it's set to 0.

OK.

Thanks,
Rob

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Thu, 10 Jan 2019 13:44:07 +0000

Hi Rob,

Inline.

From: Robert Wilton <[email-masked]>
Date: Thursday, January 10, 2019 at 5:18 AM
To: "Reshad Rahman (rrahman)" <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: Yangdoctors last call review of draft-ietf-netconf-restconf-notif-11

Hi Reshad,

Please see inline ...

Hi Rob,

On 2019-01-07, 12:43 PM, "Robert Wilton" <[email-masked]><mailto:[email-masked]> wrote:

    Reviewer: Robert Wilton

    me.

OK, sounds reasonable.

Off topic for this thread, but 7895bis should be published soon, and it obsoletes 7895, so should YP be updated to reference 7895bis?

<RR2> I believe so, I’ll forward this to the YP authors.

    defined.

<RR> Good catch.

    within (b)"

<RR> Yes.

    5) Section 3.4:

    only?

I had a couple of concerns:
 1) Trying to align the RESTCONF handling of these RPCs to the behaviour specified in draft-ietf-netconf-subscribed-notifications and draft-ietf-netconf-netconf-event-notifications.
2) If NACM is used as the access control mechanism, then any client that has access to the <*-subscription> RPCs is allowed to invoke them for any subscription, unless additional checking is performed.  However, I appreciate that this extra checking could be more difficult/expensive to implement on some platforms.

<RR2> Having perfect alignment is problematic because there are no transport sessions in RESTCONF, that’s why the username/NACM method was proposed by a few people. But I do agree that your point 2) is an issue and your proposed text below addresses that, so I’ll make the change.

Regards,

Reshad.

Considering both of these, would the following rewording of the paragraph be better?

OLD:

NEW:

   o  In addition to any required access permissions (e.g., NACM),

      RPCs modify-subscription, resync-subscription and

      delete-subscription SHOULD only be allowed by the same RESTCONF

      username [RFC8040] who invoked establish-subscription.

   o  The kill-subscription RPC can be invoked by any RESTCONF

      username with the required administrative permissions.

    E.g.

    OLD:

          "dscp" leaf node in

    NEW:

          "weighting" leaf node in

 <RR> Good catch.

OK.

    any identities.

    Just nits:

    over the line break).

    suggest spliting.

<RR> Ack.

Regards,

Reshad.
Thanks,
Rob

    Thanks,

    Rob