On Mon, Oct 5, 2020 at 7:30 PM Mr. Jaehoon Paul Jeong < [email protected]> wrote:
> 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. > > 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 > Thanks. > > Best Regards, > Paul > > > On Tue, Oct 6, 2020 at 3:03 AM Andy Bierman via Datatracker < > [email protected]> wrote: > >> 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. >> >> >> >> _______________________________________________ >> I2nsf mailing list >> [email protected] >> 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 protected], [email protected] > Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php > <http://cpslab.skku.edu/people-jaehoon-jeong.php> >
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
