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

[Andy Bierman <[e-mail]>]

Hi,

I reviewed your note, the diffs and the YANG module.
I think draft-07 is ready (no issues or nits)

There are probably some clarifications needed to adapt YANG Push dampening
to I2NSF. YP refers to the data nodes changing within the dampening period.
In this case, the notes to implementers should be clear about any events
sent at the end
of the dampening period because events were suppressed (if any). There
might be a different
procedure for each event or sub-event.


Andy


On Wed, Mar 31, 2021 at 7:09 PM Mr. Jaehoon Paul Jeong <
[e-mail]> wrote:

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