Re: [yang-doctors] Yangdoctors last call review of draft-ietf-i2nsf-nsf-monitoring-data-model-06

["Mr. Jaehoon Paul Jeong" <[e-mail]>]

Hi Andy, Linda, and Yoav,
Patrick and I have addressed all the comments from Andy.
Here is the revised draft -07:
https://tools.ietf.org/html/draft-ietf-i2nsf-nsf-monitoring-data-model-07

I attach a revision letter to explain how we addressed the comments.

There are the major updates in this revision as follows:
---
   o  This version is revised according to the comments of Andy Bierman
      who is a YANG doctor.

   o  This version updates its title as "I2NSF NSF Monitoring Interface
      YANG Data Model".  It clarifies the NSF Monitoring Interface to
      deliver NSF monitoring data to an NSF data collector (e.g.,
      Security Controller and NSF data analyzer).

   o  This version adds an attack destination IP address for DDoS-attack
      event to provide an NSF data collector with more information about the
      destination of DDoS-attack packets.

   o  This version supports a notification for monitoring traffic flows
     to detect the source and destination (as a victim) of security attacks
     such as DDoS attack.
---
If you have questions and comments, please let me know.

Thanks.

Best Regards,
Paul


On Wed, Mar 24, 2021 at 3:34 AM Andy Bierman via Datatracker <
[e-mail]> wrote:

> Reviewer: Andy Bierman
> Review result: Ready with Issues
>
>
> Status: Ready with Issues
>
> Most of the issues raised in the review of draft-04 have been
> addressed.
>
> Major Issues:
>
>  - None
>
> Moderate Issues:
>
> 1) too many YANG features
>
> There are 13 YANG features, one for each of the 13 notification-stmts
> defined.  There should be as few YANG features defined as possible.
> They should only be used if it is an unreasonable burden (compared
> to the feature value) for all servers to support the functionality.
>
> 2) list /i2nsf-monitoring-configuration/system-alarm
>
> This is yet another alarm management system created in the IETF.
> I guess the WG decided that RFC 8632 was not suitable.
>
> It is not clear how this system prevents excessive notifications
> sent to a client.
>
> What happens when the CPU, memory, or disk usage crosses back and
> forth over the threshold? Seems like an alarm is generated for each
> upward crossing of the threshold leaf.
>
> The precise behavior for triggering and then re-arming an alarm
> needs to be specified in the YANG module.
>
> RMON Alarms (RFC 2819) defines one way to prevent bursts of
> SNMP notifications, using an alarm reset threshold.
>
> YANG Push (RFC 8641) uses a dampening-period approach to prevent
> flooding the receiver with notifications.
>
> Also, it is not clear what use-case is served by "threshold = 0".
> The range is 0..100 instead of 1..100.
>
>
> Minor Issues:
>
> 3) too many notifications
>
> This module creates a lot of notifications to manage, and they are
> all optional to implement. This increases complexity in both
> the client implementation and operations.
>
> If you really need all 13 notifications then OK, but
> 13 notification events is a lot for one YANG module,
> especially if this set will get even larger over time.
>
> Here is one way to reduce the number of event definitions.
> The example below has 1 event and 13 sub-event types, but it could
> also apply to N event types each with some sub-event types.
>
> This design template adds one more layer in the notification message,
> but it is probably easier for the client and operator to manage.
> The deployment may require filters and access control rules that become
> more complex for a large number of notifications.
>
>
>     notification i2nsf-event {
>       description
>         "Wrapper for all I2NSF events";
>
>       choice sub-event-type {
>         description
>           "This choice must be augmented with cases for each allowed
>            sub-event.  Only 1 sub-event will be instantiated in each
>            i2nsf-event message. Each case is expected to define one
>            container with all the sub-event fields.";
>
>          // could put sub-events inline
>          case i2nsf-system-detection-alarm {
>            if-feature "i2nsf-system-detection-alarm";
>            container i2nsf-system-detection-alarm {
>              // contents of i2nsf-system-detection-alarm data
>            }
>          }
>
>        }
>      }
>
>
>      // could add sub-events via augments at any time
>      augment "/i2nsf-event/sub-event-type" {
>        case i2nsf-system-detection-event {
>          if-feature "i2nsf-system-detection-event";
>          container i2nsf-system-detection-event {
>            // contents of i2nsf-system-detection-event data
>          }
>        }
>      }
>
>
> Nits:
>
> 4) underscore vs. hyphen
>
> There are many field names in sec. 7 that are incorrect
> because they use an underscore instead of a hyphen char
> (e.g. req_cookies but leaf name is req-cookies)
>
> 5) verbose SNMP-style names
>
> The term -configuration in the object names is unusual.
> Repeating the parent name (like SMIv2) is not usually done in YANG.
> (e.g., i2nsf-system-detection-event-configuration)
>
> 6) identifiers should use well-known abbreviations or spell
> out the word if not too long.  E.g "ave" -> "average"
>
> 7) Is there a reason some identities are ALL-CAPS and others
> are all-lower-case? This should be explained in the YANG module.
>
>
>
>
>
>
>