Subject: [yang-doctors] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04

From: Andy Bierman via Datatracker <[email-masked]>
Date: Mon, 05 Oct 2020 11:03:42 -0700

Reviewer: Andy Bierman
Review result: Almost Ready

Major Issues:

 - None

Moderate Issues:

 - top-level 'counters' container does not follow naming conventions.
   Should start with 'i2nsf', probably 'i2nsf-state'

 - There do not seem to be any writable objects in the /counters
   subtree so this container should have a 'config false' statement

 - top-level typedef and grouping description-stmts are self-referential
   and not useful. Need to rewrite description-stmts and/or add
   reference-stmts as needed.

 - grouping common-monitoring-data/time-stamp
   Is this a different time stamp than the one in the NETCONF notification?
   The 'message generation time' sounds like the standard timestamp.
   Does this object represent the event detection time?

 - grouping i2nsf-system-alarm-type-content/usage
 - grouping i2nsf-system-alarm-type-content/threshold
   These are uint8 leafs with unclear descriptions.
   Not sure why uint8 is the appropriate type.
   Needs 1 or more of (reference, units, better description)

 - grouping traffic-rates
   Add a units statement to each leaf. Not sure what units to use
   but it should be consistent. (e.g, pps, bps used in descriptions
   should also be in a units-stmt)

 - grouping i2nsf-system-counter-type-content
   These counters should use the yang:counter32 type instead of uint32

 - container counters/system-interface
 - container counters/nsf-firewall
 - container counters/nsf-policy-hits
  The descriptions are too terse and confusing, and need a rewrite.

 -  container counters/nsf-firewall
 -  container counters/nsf-policy-hits
    - uses i2nsf-nsf-counters-type-content;
    Many of the fields expanded from this grouping all say
    they refer to 'the packet'. Why are they in this global
    container of counters? E.g. (src-ip, dst-ip, src-port, dst-port)
    Not clear at all how the server is supposed to apply this
    grouping to these containers.

 - many leafs use "uint32" type for a rate.
   Should add a units-stmt

 - leaf counters/nsf-policy-hits/hit-times
   The purpose and type are confusing and generic.
   If this is a counter then use counter32

 - cut-and-paste for notification-stmt content should be replaced
   with grouping/uses instead. Applies to the nsf-detection-*
   and the various logging notifications. Even a grouping that
   has 1 object in it is better than cut-and-paste 5+ times

Minor Issues:

 - top-level identifiers are too generic
   should have 'i2nsf-' prefix to be more reusable outside this module

 - quite a lot of identities that an implementation is required to support.
   If this set of identities might change a lot faster than the
   notifications and counter objects, then consider putting them
   in a separate module

 - leaf with same type named differently; both intrusion-attack-type
   - nsf-detection-intrusion/sub-attack-type
   - nsf-log-intrusion/attack-type

 - quite a lot of notification event types for a server to implement
   and a user to manage. All are mandatory (no if-feature statements).
   Some such as nsf-detection-* subset are very similar.
   A section or table would be useful that showed the YANG notification
   names and their purpose -- maybe a reference to another RFC
   with more details

 - there seems to be notifications for intrusion events and then
   again for the logging of those events.  This seems excessive
   but

 - grouping common-monitoring-data/time-stamp
   Is this a different time stamp than the one in the NETCONF notification?
   The 'message generation time' sounds like the standard timestamp.
   Is this event detection time?

 - grouping common-monitoring-data/module-name
   Is this a YANG module or some other type of module?

 - there is no way to configure which notifications should be generated
   or maybe how often.  YANG Push has its own dampening-period.
   Since these are event stream subscriptions, not datastore subscriptions,
   YANG-Push does not apply to this document at all.

   If there are a lot of notifications then a server implementation
   might drop some

 - grouping i2nsf-nsf-event-type-content-extend/src-zone
 - grouping i2nsf-nsf-event-type-content-extend/src-zone
   These use type 'string'. Consider using a typedef that constrains
   the string.  General comment where unconstrained string is used:
   The corner-case values such as empty string are often not allowed
   in implementations.

 - grouping i2nsf-nsf-event-type-content-extend/rule-id
 - grouping i2nsf-nsf-event-type-content-extend/rule-name
 - grouping i2nsf-nsf-event-type-content-extend/profile
   These objects seem to reference objects in another YANG module.
   If so, then leafref types might be more appropriate.

 - grouping i2nsf-nsf-event-type-content/rule-id
 - grouping i2nsf-nsf-event-type-content/rule-name
 - grouping i2nsf-nsf-event-type-content/profile
 - grouping i2nsf-nsf-event-type-content/raw-info
   These objects are cut-and-paste duplicates from
   grouping i2nsf-nsf-event-type-content. They should
   be in a separate grouping used by both. Also applies
   to some other sets of objects

 - limits issues (e.g. current-session, maximum-session
   The type is uint8. This is only OK it is impossible for any
   implementation to ever have or want more than 255 of them.
   If some other RFC really does limit the values where uint8
   is used, then that is OK. If so, a reference-stmt would help.

---

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Tue, 6 Oct 2020 11:30:07 +0900

Hi Andy,
Thanks for your valuable comments on our NSF Monitoring YANG Data Model
Draft.
We authors will reflect your comments on the revision and come back to you
with
the revised draft and revision letter.

Thanks.

Best Regards,
Paul

On Tue, Oct 6, 2020 at 3:03 AM Andy Bierman via Datatracker <
[email-masked]> wrote:

> _______________________________________________
> I2nsf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/i2nsf
>

-- 
===========================
Mr. Jaehoon (Paul) Jeong, Ph.D.
Associate Professor
Department of Computer Science and Engineering
Sungkyunkwan University
Office: +82-31-299-4957
Email: [email-masked], [email-masked]
Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php
<http://cpslab.skku.edu/people-jaehoon-jeong.php>

---

From: Andy Bierman <[email-masked]>
Date: Mon, 5 Oct 2020 20:30:45 -0700

> the revised draft and revision letter.
I forgot 1 comment:

 - the draft is not that explicit about the event stream to use for these
notifications
   Perhaps a new standard event stream (I2NSF) would be better than the
standard (NETCONF)
   The draft should be more clear wrt/ the NETCONF stream.

Andy

---

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Tue, 6 Oct 2020 13:05:07 +0900

Andy,
What do you mean by the event stream?

Actually, I am thinking that the NETCONF can deliver the monitoring data of
a system or an NSF in an XML file.

Do you think that we need another way to convey the monitoring data?

Thanks.

Best Regards,
Paul

On Tue, Oct 6, 2020 at 12:30 PM Andy Bierman <[email-masked]> wrote:

>> We authors will reflect your comments on the revision and come back to
>> you with

---

From: Andy Bierman <[email-masked]>
Date: Mon, 5 Oct 2020 21:50:55 -0700

> What do you mean by the event stream?
https://tools.ietf.org/html/rfc8639#section-2.1

> Actually, I am thinking that the NETCONF can deliver the monitoring data
> of a system or an NSF in an XML file.
>

---

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Wed, 7 Oct 2020 12:04:15 +0900

Hi Andy,
I will investigate the event stream with information you sent me below.

Thanks for your help and advice.

Best Regards,
Paul

On Tue, Oct 6, 2020 at 1:51 PM Andy Bierman <[email-masked]> wrote:

>>>  - the draft is not that explicit about the event stream to use for
>>> these notifications

---

From: Andy Bierman <[email-masked]>
Date: Tue, 6 Oct 2020 20:24:18 -0700

> I will investigate the event stream with information you sent me below.
The notification RFCs are a bit new.
An event stream is shared by all sessions that subscribe to that event
stream.
There is a default value that must be supported : "NETCONF"

You do not have to create a new event stream (e.g. "I2NSF")
You can use "NETCONF" if you want.
It is really up to the device vendor and operator to use proprietary
mechanisms
to configure the event streams.

Andy

>>>>> Thanks for your valuable comments on our NSF Monitoring YANG Data
>>>>> Model Draft.