Re: [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04

[Andy Bierman <[e-mail]>]

On Mon, Oct 5, 2020 at 7:30 PM Mr. Jaehoon Paul Jeong <
[e-mail]> 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 <
> [e-mail]> 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
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/i2nsf
>>
>
>
> --
> ===========================
> Mr. Jaehoon (Paul) Jeong, Ph.D.
> Associate Professor
> Department of Computer Science and Engineering
> Sungkyunkwan University
> Office: [phone]
> Email: [e-mail], [e-mail]
> Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php
> <http://cpslab.skku.edu/people-jaehoon-jeong.php>
>