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

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

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:

[e-mail]:47: warning: RFC 8407: 3.1: The
IETF Trust Copyright statement seems to be missing (see pyang --ietf-help
for details).
[e-mail]:1011: error: RFC 8407: 4.14:
statement "grouping" must have a "description" substatement
[e-mail]:1016: error: keyword
"description" not in canonical order, expected "type" (see RFC 6020,
Section 12)
[e-mail]:1017: error: keyword "type" not
in canonical order (see RFC 6020, Section 12)
[e-mail]:1018: error: keyword "default"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1102: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1119: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1135: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1184: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1206: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1245: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1276: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1322: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1386: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1425: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1492: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1526: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1540: error: keyword "if-feature"
not in canonical order (see RFC 6020, Section 12)
[e-mail]:1605: error: keyword "config" not
in canonical order (see RFC 6020, Section 12)
[e-mail]:1610: error: keyword "key" not in
canonical order (see RFC 6020, Section 12)
[e-mail]:1620: error: keyword "key" not in
canonical order (see RFC 6020, Section 12)
[e-mail]:1631: error: keyword "key" not in
canonical order (see RFC 6020, Section 12)
[e-mail]:1643: error: RFC 8407: 4.14:
statement "container" must have a "description" substatement
[e-mail]:1654: error: keyword "key" not in
canonical order (see RFC 6020, Section 12)
[e-mail]:1780: error: keyword
"description" not in canonical order, expected "type" (see RFC 6020,
Section 12)
[e-mail]:1781: error: keyword "type" not
in canonical order (see RFC 6020, Section 12)
[e-mail]:1782: error: keyword "units" not
in canonical order (see RFC 6020, Section 12)
[e-mail]:1783: error: keyword "default"
not in canonical order (see RFC 6020, Section 12)



On Sun, Feb 21, 2021 at 2:10 AM Andy Bierman <[e-mail]> 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 <
> [e-mail]> 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 <
>> [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 Head
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>