Hi Andy, Thanks for your review on the revision and new minor comments about pyang reporting. I will address them and submit a new revised draft.
Thanks. Best Regards, Paul ------------------------------------------------------------------------------- P.S. The following are the pyang reporting errors. Status: Ready with nits Nits: pyang 2.4.0 --ietf reporting errors: [email protected]:47: warning: RFC 8407: 3.1: The IETF Trust Copyright statement seems to be missing (see pyang --ietf-help for details). [email protected]:1011: error: RFC 8407: 4.14: statement "grouping" must have a "description" substatement [email protected]:1016: error: keyword "description" not in canonical order, expected "type" (see RFC 6020, Section 12) [email protected]:1017: error: keyword "type" not in canonical order (see RFC 6020, Section 12) [email protected]:1018: error: keyword "default" not in canonical order (see RFC 6020, Section 12) [email protected]:1102: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1119: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1135: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1184: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1206: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1245: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1276: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1322: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1386: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1425: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1492: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1526: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1540: error: keyword "if-feature" not in canonical order (see RFC 6020, Section 12) [email protected]:1605: error: keyword "config" not in canonical order (see RFC 6020, Section 12) [email protected]:1610: error: keyword "key" not in canonical order (see RFC 6020, Section 12) [email protected]:1620: error: keyword "key" not in canonical order (see RFC 6020, Section 12) [email protected]:1631: error: keyword "key" not in canonical order (see RFC 6020, Section 12) [email protected]:1643: error: RFC 8407: 4.14: statement "container" must have a "description" substatement [email protected]:1654: error: keyword "key" not in canonical order (see RFC 6020, Section 12) [email protected]:1780: error: keyword "description" not in canonical order, expected "type" (see RFC 6020, Section 12) [email protected]:1781: error: keyword "type" not in canonical order (see RFC 6020, Section 12) [email protected]:1782: error: keyword "units" not in canonical order (see RFC 6020, Section 12) [email protected]:1783: error: keyword "default" not in canonical order (see RFC 6020, Section 12) On Sun, Feb 21, 2021 at 2:10 AM Andy Bierman <[email protected]> wrote: > Hi, > > I have reviewed the module in draft-05. > Thanks for the revision letter. That really helped make the review go fast. > All my draft-04 comments have been addressed. > > I cannot figure out how to enter a new review in the IETF pages (even > logged in), > so it is attached here. pyang is reporting some minor style issues. > > > Status: Ready with nits (see attached file) > > > Andy > > > On Wed, Feb 17, 2021 at 7:15 AM Mr. Jaehoon Paul Jeong < > [email protected]> wrote: > >> Hi Andy, >> Patrick and I have addressed your comments on the following revision: >> >> >> https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/ >> https://tools.ietf.org/html/draft-ietf-i2nsf-nsf-monitoring-data-model-05 >> >> I attach the revision letter to explain how to address your comments on >> the revision. >> >> If you have further comments, please let me know. >> >> If you are satisfied with our revision, please update the YANG doctor >> review status in the following link: >> >> https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/ >> >> 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 Head 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
