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

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Thu, 18 Feb 2021 00:15:04 +0900

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-masked]> 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-masked]
> https://www.ietf.org/mailman/listinfo/i2nsf
>

---

From: Andy Bierman <[email-masked]>
Date: Sat, 20 Feb 2021 09:10:37 -0800

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-masked]> wrote:

> I attach the revision letter to explain how to address your comments on
> the revision.
>

---

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Sun, 21 Feb 2021 17:19:31 +0900

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:

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

-- 
===========================
Mr. Jaehoon (Paul) Jeong, Ph.D.
Associate Professor
Department Head
Department of Computer Science and Engineering
Sungkyunkwan University
Office: +82-31-299-4957
Email: [email-masked], [email-masked]
Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php
<http://cpslab.skku.edu/people-jaehoon-jeong.php>

---

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Tue, 23 Feb 2021 01:46:04 +0900

Hi Andy,
Patrick and I have addressed nits issues in the following revision.

https://tools.ietf.org/html/draft-ietf-i2nsf-nsf-monitoring-data-model-06
https://datatracker.ietf.org/doc/draft-ietf-i2nsf-nsf-monitoring-data-model/

Here is the revision letter to explain how we have resolved those issues.

Thanks for your dedication as a YANG doctor.

Best Regards,
Paul

On Sun, Feb 21, 2021 at 5:19 PM Mr. Jaehoon Paul Jeong <
[email-masked]> wrote:

> [email-masked]:1102: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1119: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1135: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1184: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1206: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1245: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1276: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1322: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1386: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1425: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1492: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1526: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1540: error: keyword
> "if-feature" not in canonical order (see RFC 6020, Section 12)
> [email-masked]:1605: error: keyword "config"
> [email-masked]:1610: error: keyword "key" not
> [email-masked]:1620: error: keyword "key" not
> [email-masked]:1631: error: keyword "key" not
> [email-masked]:1654: error: keyword "key" not
> [email-masked]:1782: error: keyword "units"

---

From: Andy Bierman <[email-masked]>
Date: Wed, 24 Feb 2021 08:29:01 -0800

On Mon, Feb 22, 2021 at 7:31 PM Reshad Rahman <[email-masked]> 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-masked]> on behalf of Andy
> Bierman <[email-masked]>
> *Date: *Saturday, February 20, 2021 at 12:11 PM
> *To: *"Mr. Jaehoon Paul Jeong" <[email-masked]>
> *Cc: *Roman Danyliw <[email-masked]>, "[email-masked]" <[email-masked]>,
> JungSoo Park <[email-masked]>, Yunchul Choi <[email-masked]>, Patrick
> Lingga <[email-masked]>, YANG Doctors <[email-masked]>,
> skku-iotlab-members <[email-masked]>
> *Subject: *Re: [yang-doctors] [I2nsf] Yangdoctors early review of
> draft-ietf-i2nsf-nsf-monitoring-data-model-04
>
>
>
> Hi,
>
>
>
> _______________________________________________ yang-doctors mailing list
> [email-masked] https://www.ietf.org/mailman/listinfo/yang-doctors
>

---

From: Reshad Rahman <[email-masked]>
Date: Mon, 22 Feb 2021 22:31:19 -0500

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.

Regards,

Reshad.

From: yang-doctors <[email-masked]> on behalf of Andy Bierman <[email-masked]>
Date: Saturday, February 20, 2021 at 12:11 PM
To: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Cc: Roman Danyliw <[email-masked]>, "[email-masked]" <[email-masked]>, JungSoo Park <[email-masked]>, Yunchul Choi <[email-masked]>, Patrick Lingga <[email-masked]>, YANG Doctors <[email-masked]>, skku-iotlab-members <[email-masked]>
Subject: Re: [yang-doctors] [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04

Hi,

I cannot figure out how to enter a new review in the IETF pages (even logged in),

 

 

Andy

On Wed, Feb 17, 2021 at 7:15 AM Mr. Jaehoon Paul Jeong <[email-masked]> wrote:

Hi Andy,

I attach the revision letter to explain how to address your comments on the revision.

If you are satisfied with our revision, please update the YANG doctor review status in the following link:

Thanks.

Best Regards,

Paul

On Tue, Oct 6, 2020 at 3:03 AM Andy Bierman via Datatracker <[email-masked]> wrote:

Reviewer: Andy Bierman

Major Issues:

 - None

Moderate Issues:

 - grouping traffic-rates

Minor Issues:

_______________________________________________ yang-doctors mailing list [email-masked] https://www.ietf.org/mailman/listinfo/yang-doctors

---

From: Reshad Rahman <[email-masked]>
Date: Wed, 24 Feb 2021 11:40:19 -0500

From: Andy Bierman <[email-masked]>
Date: Wednesday, February 24, 2021 at 11:30 AM
To: Reshad Rahman <[email-masked]>
Cc: "Mr. Jaehoon Paul Jeong" <[email-masked]>, Roman Danyliw <[email-masked]>, "[email-masked]" <[email-masked]>, JungSoo Park <[email-masked]>, Yunchul Choi <[email-masked]>, Patrick Lingga <[email-masked]>, YANG Doctors <[email-masked]>, skku-iotlab-members <[email-masked]>
Subject: Re: [yang-doctors] [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04

 

<RR> Version 00 will remain as a review of the old rev. The new version will be a review of the new draft (you can provide a new reviewed revision).

I agree that correct review implies that you can only change the review of the old rev, so the button should probably be “Correct/new review” instead. Anyway I believe that, even though the UI is misleading, you can enter a new review while keeping the old review.

Regards,

Reshad.

I think a better procedure would be for the authors to request a new review for the updated draft.  

 

Regards,

Reshad.

Andy

Hi,

 

 

Andy

Hi Andy,

Thanks.

Best Regards,

Paul

Reviewer: Andy Bierman

Major Issues:

 - None

Moderate Issues:

 - grouping traffic-rates

Minor Issues: