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

["Mr. Jaehoon Paul Jeong" <[e-mail]>]

Hi Andy,
I will investigate the event stream with information you sent me below.

Thanks for your help and advice.

Best Regards,
Paul

On Tue, Oct 6, 2020 at 1:51 PM Andy Bierman <[e-mail]> wrote:

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

-- 
===========================
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>