On Mon, Feb 22, 2021 at 7:31 PM Reshad Rahman <[email protected]> wrote:
> *I cannot figure out how to enter a new review in the IETF pages (even > logged in),* > > I have run into the same issue in the past. You need to go to the actual > review link, > https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/reviewrequest/13767/ > in this case, and click on the “Correct review” button. My recollection is > that this will create a newer version of the review. > > > But I am not modifying my review of the old draft. I am entering my review of the new draft. I think a better procedure would be for the authors to request a new review for the updated draft. Hopefully the same reviewer can be assigned automatically. Regards, > > Reshad. > > > Andy > *From: *yang-doctors <[email protected]> on behalf of Andy > Bierman <[email protected]> > *Date: *Saturday, February 20, 2021 at 12:11 PM > *To: *"Mr. Jaehoon Paul Jeong" <[email protected]> > *Cc: *Roman Danyliw <[email protected]>, "[email protected]" <[email protected]>, > JungSoo Park <[email protected]>, Yunchul Choi <[email protected]>, Patrick > Lingga <[email protected]>, YANG Doctors <[email protected]>, > skku-iotlab-members <[email protected]> > *Subject: *Re: [yang-doctors] [I2nsf] Yangdoctors early review of > draft-ietf-i2nsf-nsf-monitoring-data-model-04 > > > > 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 > > > > _______________________________________________ yang-doctors mailing list > [email protected] https://www.ietf.org/mailman/listinfo/yang-doctors >
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
