On Mon, Oct 5, 2020 at 9:05 PM Mr. Jaehoon Paul Jeong < [email protected]> wrote:
> Andy, > 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. > > 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 protected]> wrote: > >> >> >> 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> >>> >> > > -- > =========================== > 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
