Early Review of draft-ietf-i2nsf-nsf-monitoring-data-model-04
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.