Re: [netconf] WGLC for draft-ietf-netconf-notification-capabilities

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Balazs,

From: Balázs Lengyel <[e-mail]>
Sent: 03 October 2019 15:20
To: Rob Wilton (rwilton) <[e-mail]>; Mahesh Jethanandani <[e-mail]>; Netconf <[e-mail]>
Subject: RE: [netconf] WGLC for draft-ietf-netconf-notification-capabilities



From: Rob Wilton (rwilton) <[e-mail]<mailto:[e-mail]>>
Sent: 2019. szeptember 30., hétfő 16:45
To: Mahesh Jethanandani <[e-mail]<mailto:[e-mail]>>; Netconf <[e-mail]<mailto:[e-mail]>>; Balázs Lengyel <[e-mail]<mailto:[e-mail]>>
Subject: RE: [netconf] WGLC for draft-ietf-netconf-notification-capabilities

Hi,

I have reviewed version -05b of this document.

I am supportive of this document, and its general approach of making subscription capability information available both online and offline.  I believe that this information is valuable to making YANG-Push more usable.

A few comments:

1.       Terminology and Introduction.  I wasn’t sure whether “implementation-time information” is the best description, or whether “offline information” would be a better description.   I believe that the key is that the information is available offline without requiring access to all the network devices.  Although I can see that providing this information at implementation time is useful and should be RECOMMENDED, I don’t think that it should be a MUST/SHALL (e.g. as per section 3).
BALAZS: I would like to keep the term “implementation-time”, as we had a number of debates about the correct term and this is one term people seemed to agree upon.  In practice it is often important to provide the information early, before the network node is fully implemented as NMS planning and design often begins early as well. In practice the capabilities are decided in implementation time.
[RW]
OK.  But I’m not really convinced that all vendors will be able to provide this information at implementation time.  I would normally expect a vendor to delay providing this information until the implementation is complete, they have tested it and verified that they can make the subscription guarantees that are being stated.  I think that a common case would be provide this information when the software is available (which could be ahead of the hardware availability).  But if you want to call it “implementation time” then I can live with that.
In section 3, I think

   The information SHALL be provided in two ways both following the
   ietf-notification-capabilities module:

should be:

   The information SHOULD be provided in two ways both following the
   ietf-notification-capabilities module:



2.       Another benefit of having information available offline is that if there are classes of devices running the same software and hardware then they should all have the same capabilities without having to check each and every one.  To this end, it would be nice if there was some sort of simple checksum mechanism that could be used to ensure that the information stored in the instance-data file exactly matches the equivalent information provided by the server.  This would allows NMS implementations to be coded to the instance-data document, and then just validate that the checksums match those provided by the server without having to fetch and check that a portion of the data tree matches.  As a side note – YANG packages has a similar requirement.
BALAZS: This seems like a requirement to implement an Entity-tag similar to  https://tools.ietf.org/html/rfc8040#section-3.5.2 but with a deterministic algorithm to generate the tag value. While the idea is interesting/worthwhile, IMHO it should be handled in a generic manner, not for each YANG module separately. Also I was instructed to remove the entity tag from draft-ietf-netmod-yang-instance-file-format.
[RW]
I agree that it is better to handle generically, although in the YANG packages case, I don’t think that this would work, because not everything is part of an instance-data document (e.g. a checksum for a module).  The deterministic algorithm that the packages draft is currently using is a SHA-256 hash.


3.       Section 3, by “formal”, do you mean “machine readable”?  Otherwise expanding what is meant by formal might be useful.
BALAZS: OK, changed text

4.       Section 3, instead of “amount of notifications” would “throughput of notification data” be better?
BALAZS: As you wish, changed text.

In terms of the YANG model, I think that it is possible to design this in a slightly more regular (and arguably simpler) way:

-          Rename “datastore-subscription-throughput-capabilities grouping” to “subscription-throughput-capabilities” (since the grouping is also used at the subscription level)
BALAZS: OK, renamed
BALAZS:: IMHO the other changes look more simple, but will actually make the model more complicated.
[RW]
So, the current tree output is this:

module: ietf-notification-capabilities
  +--ro datastore-subscription-capabilities
     +--ro (update-period)?
     |  +--:(minimum-update-period)
     |  |  +--ro minimum-update-period?     uint32
     |  +--:(supported-update-period)
     |     +--ro supported-update-period*   uint32
    +--ro max-objects-per-update?          uint32
     +--ro minimum-dampening-period?        uint32 {yp:on-change}?
     +--ro datastore-capabilities* [datastore]
        +--ro datastore                          union
        +--ro (update-period)?
        |  +--:(minimum-update-period)
        |  |  +--ro minimum-update-period?       uint32
        |  +--:(supported-update-period)
        |     +--ro supported-update-period*     uint32
        +--ro max-objects-per-update?            uint32
        +--ro minimum-dampening-period?          uint32 {yp:on-change}?
        +--ro on-change-supported-for-config?    boolean {yp:on-change}?
        +--ro on-change-supported-for-state?     boolean {yp:on-change}?
        +--ro per-node-capabilities* [node-selector]
           +--ro node-selector             nacm:node-instance-identifier
           +--ro (update-period)?
           |  +--:(minimum-update-period)
           |  |  +--ro minimum-update-period?    uint32
           |  +--:(supported-update-period)
           |     +--ro supported-update-period*  uint32
           +--ro max-objects-per-update?         uint32
           +--ro minimum-dampening-period?       uint32 {yp:on-change}?
           +--ro on-change-supported             boolean {yp:on-change}?

The model that I proposed would look like this:

module: ietf-notification-capabilities
  +--ro datastore-subscription-capabilities
     +--ro (update-period)?
     |  +--:(minimum-update-period)
     |  |  +--ro minimum-update-period?     uint32
     |  +--:(supported-update-period)
     |     +--ro supported-update-period*   uint32
    +--ro max-objects-per-update?          uint32
     +--ro minimum-dampening-period?        uint32 {yp:on-change}?
     +--ro on-change-supported?             Enum of (config|state|both)
     +--ro datastore-capabilities* [datastore]
        +--ro datastore                          union
        +--ro per-node-capabilities* [node-selector]
           +--ro node-selector             nacm:node-instance-identifier
           +--ro (update-period)?
           |  +--:(minimum-update-period)
           |  |  +--ro minimum-update-period?    uint32
           |  +--:(supported-update-period)
           |     +--ro supported-update-period*  uint32
           +--ro max-objects-per-update?         uint32
           +--ro minimum-dampening-period?       uint32 {yp:on-change}?
           +--ro on-change-supported?            Enum of (config|state|both)


The code interacting with this is more generic , with less corner cases.

But I think that you could go a step further and define it like this, allowing (datastore=’all’ + node-selector = ‘/’) to define the default global capabilities.  Any capabilities for a specific datastore is more specific than “all”.  Capabilities for a more specific path win over a less specific path.  Personally, I would expect this to be simpler to implement because there are less special cases to handle.

module: ietf-notification-capabilities
  +--ro datastore-subscription-capabilities
     +--ro datastore-capabilities* [datastore]
        +--ro datastore                          union
        +--ro per-node-capabilities* [node-selector]
           +--ro node-selector             nacm:node-instance-identifier
           +--ro (update-period)?
           |  +--:(minimum-update-period)
           |  |  +--ro minimum-update-period?    uint32
           |  +--:(supported-update-period)
           |     +--ro supported-update-period*  uint32
           +--ro max-objects-per-update?         uint32
           +--ro minimum-dampening-period?       uint32 {yp:on-change}?
           +--ro on-change-supported?            Enum of (config|state|both)






-          Add “on-change-supported” leaf to “subscription-throughput-capabilities” grouping, but change its type to an enum of “all-nodes, config-only, state-only” – this could then work in a fully hierarchical way.  (I.e. the existing on-change-supported-for-config, on-change-supported-for-state , on-change-supported could be removed).
BALAZS: The enum will need much more explanation e.g. what does it mean to have the “config-only” value on a config=false data node? It can be explained, it will just need more text.
[RW]
I’m not convinced that this would take that much text to explain … e.g. possibly something like

“On change notifications can be supported for (i) only ‘config: true’ nodes, or (ii) only ‘config: false’ nodes, or (iii) all nodes regardless of the ‘config’ flag”



-          Instead of having a set of “datastore” level parameters, could these just be expressed as the set of parameters that apply to “/” within a datastore (e.g. as described in NACM YANG module “leaf path“)?  I.e. given that you support hierarchical paths within a datastore, it just means that you need two levels, per device and per datastore.
BALAZS: It looks simpler, but we would have to have longer explanations:

o   The “/” is not well defined or well known, so its use must be described
[RW]
But it is defined/used in this way for NACM.  I.e. your model would be more consistent with the approach defined in NACM rather than using a different approach.  I think that you also already need to handle the “/” case anyway, or explicitly indicate that it is not allowed.


o   The extra case when there is no per-node-capabilities list-entry needs to be explained/specified
[RW]
I don’t follow your comment.  I’m proposing changing from a model that has 3 levels of hierarchy (global, per-datastore, per datastore schema subtree) to one that only has 2 (global, per datastore schema subtree).




-          Ideally, it would be better to have a dependency on RFC6991-bis for node-instance-identify rather than on NACM.  Although I guess that you may not want to delay this RFC.
BALAZS: Agreed, but please no delay.
[RW]
Yes, OK.  The plan was to get RFC6991bis done fast, I’m not sure how close that document can be to WG LC.  Perhaps we could check with Juegen?


-          Currently “leaf on-change-supported” is marked as mandatory true, but I’m not sure why it should be so.
BALAZS: OK. made it mandatory false; There is a use-case where the per leaf dampening period needs to be specified, but the on-change-supported is inherited from the containing parent/datastore.
[RW]
Sure.


-          Is “max-objects-per-update” optional.  What if the server doesn’t have any hard limit - can they just not return a value here?  If so, perhaps assigning a default value of uint32_max might make sense?
BALAZS: It is optional. If there is no hard limit or the limit is not known the leaf must be absent. The module description include the text:
      Any capability value MAY be absent ... if the publisher is not capable of
      providing a value.
[RW]
OK.  Then I think that there should be a bit more text about what values mean if they are not specified.  E.g. perhaps the following (if this is the right behaviour for all properties).

Old:
      Any capability value MAY be absent if a more generic capability
      value is already provided or if the publisher is not capable of
      providing a value.

New:
      Any capability value MAY be absent if a more generic capability
      value is already provided or if the publisher is not capable of
      providing a value.  If not provided then it should be
      interpreted that the property is unconstrained.


In the security section, are these capabilities sensitive information?  E.g. could it be used by an attacker to more effectively DDOS a server (by knowing which paths to target subscriptions towards)?
BALAZS: IMHO the information is not security sensitive. The minimum dampening period and minimum-update-period can be used to find schema sections that might generate more notifications, but this is really a corner-case and we usually don’t describe such details. However the security section now includes:
         The Network Configuration Access Control Model (NACM) [RFC8341]
         provides the means to restrict access for particular NETCONF or
         RESTCONF users to a preconfigured subset of all available NETCONF or
         RESTCONF protocol operations and content.
         If access control is not properly configured, it can expose
         system internals to those who should not have access to this
         information.
[RW]
Thinking about this some more, I wonder whether the security section could/should refer back to YANG-Push, which allows a server to reject subscriptions if it would cause the server to become overloaded.

Thanks,
Rob


Thanks,
Rob


From: netconf <[e-mail]<mailto:[e-mail]>> On Behalf Of Mahesh Jethanandani
Sent: 24 September 2019 18:50
To: Netconf <[e-mail]<mailto:[e-mail]>>
Subject: Re: [netconf] WGLC for draft-ietf-netconf-notification-capabilities

We were supposed to have closed on the WGLC today. However, between the document becoming a WG item and it going into LC, we have not received too many comments on the draft. As such, we are extending the LC by another week. Please review the draft and provide any comments you might have.

Mahesh & Kent (as co-chairs)


On Sep 10, 2019, at 3:39 PM, Mahesh Jethanandani <[e-mail]<mailto:[e-mail]>> wrote:

Authors have published -04<https://tools.ietf.org/html/draft-ietf-netconf-notification-capabilities-04> version of the draft, which addresses comments they received in IETF 105. If you provided comments please check to make sure your comments have been addressed. At this point, the authors believe that the document is ready for WGLC.

This therefore starts a two week LC, ending on September 24th. Please provide any technical comments you might have on the document. If you believe the document is not ready for LC, please state your reasons.

We will issue a IPR poll separately.

Mahesh & Kent (as co-chairs)