Re: [yang-doctors] [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04
"Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com> Mon, 22 February 2021 16:46 UTC
Return-Path: <jaehoon.paul@gmail.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1D38A3A1DB1; Mon, 22 Feb 2021 08:46:51 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 1.411
X-Spam-Level: *
X-Spam-Status: No, score=1.411 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HK_NAME_FM_MR_MRS=1.499, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_FREEMAIL_DOC_PDF=0.01, URIBL_BLOCKED=0.001, URI_DOTEDU=1.999] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bKjLchvb8foN; Mon, 22 Feb 2021 08:46:47 -0800 (PST)
Received: from mail-lj1-x230.google.com (mail-lj1-x230.google.com [IPv6:2a00:1450:4864:20::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id F245A3A1620; Mon, 22 Feb 2021 08:46:46 -0800 (PST)
Received: by mail-lj1-x230.google.com with SMTP id r23so59757568ljh.1; Mon, 22 Feb 2021 08:46:46 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Okg4YP88rYuW4qwgjnNFHHMslQf91QrrfNCwBhvVgRM=; b=GryaJI11CEIqDkFd65KvnUwSTMOlOXCKRFSRiJ6eLJEGzE+d+eBrYk+EED4xvOKbPz G37yXfOYG7H16n0iwLaPHvJaNJiDviCE4Br5J40eCG0bNbumAZtP7qm2B+8SlUerPVwY gzDiEteq5H6bODkPY23t+BP3XksVdLiWR3VMCX4DYClX232x6PW8pW2R6hkVmgQv7t4r +de3sxuZ9ez3crbZiPlQvfAUdfopBwTD1ZrSyRjhktbcjQSYfeWZdfgG3DXtpmJSzfId i32n3D2e/YhMNl44NXnvwkpWk9E4PlZj/PVNRbP15X9Rmh7YomazST0jmhm4iE6cId8D +wCw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Okg4YP88rYuW4qwgjnNFHHMslQf91QrrfNCwBhvVgRM=; b=qkMpuVFjTIqmlzpdhwebFY7m0It78g/5DiUf5TNZHlFKGXyTet1vB3yHjetJczW5XK niKKzLfeKpG1dtskRjnyM5n7213qPsL56H3umZaZPXL4JM2SUBXxHYCtgVu0vlBKoef2 PrpgWr1D0nTZ+GVBdxDJAhaOYwYYg9n8uqHrRppyHwPilG5dmyeXpbDPARLotGPujGOE E67LAlXo3r6+jxQUxJ39BG/dngUi+jrmvPFZrsh+1sleM/6jviDtqMkSbhH9kHTptKm7 ZjR72i23RRPMjIZYAN3deNrZy1X1K8b7MT/uqkxtgDtfHcmD5pM/l7v10ZGupUS98Wew zTuA==
X-Gm-Message-State: AOAM533KczqM/udemsP2wzmordmsKAjC1gUwJAHjnIBNTjyWHt/UnZL+ rj/i1y1JIRfiD7bHhlTlQwRf8CMYAg8vRy1vmW2djHWs2UQ=
X-Google-Smtp-Source: ABdhPJxfphxpWhjOF97SuTXNaklSIZqkVkNOZfYWuZOgmTJKEeY1FLB2Kk8QgWcjIXjSepZi+XKSt7WC7CC5x1mgLXs=
X-Received: by 2002:a2e:9b55:: with SMTP id o21mr6206520ljj.380.1614012404808; Mon, 22 Feb 2021 08:46:44 -0800 (PST)
MIME-Version: 1.0
References: <160192102291.6633.15935674903085952087@ietfa.amsl.com> <CAPK2DewwLOJnnj1ZEYMSvZB1Hpc-8rY1+-dFvzOkJvrT4X2jqA@mail.gmail.com> <CABCOCHRaSTLDngZnMaQFiq05AZbymJfFFVY+P_fmzuU4e3S7FQ@mail.gmail.com> <CAPK2Dexwi3KrrgpSZeDTy_oLMZJd=V=WAnZG4Doc9PV28Rs9xA@mail.gmail.com>
In-Reply-To: <CAPK2Dexwi3KrrgpSZeDTy_oLMZJd=V=WAnZG4Doc9PV28Rs9xA@mail.gmail.com>
From: "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>
Date: Tue, 23 Feb 2021 01:46:04 +0900
Message-ID: <CAPK2Dewx2FHC9k1Zomh2JTeCt+Kjfz3Ha0E+TU6zsJ2Gind7VA@mail.gmail.com>
To: Andy Bierman <andy@yumaworks.com>
Cc: YANG Doctors <yang-doctors@ietf.org>, "i2nsf@ietf.org" <i2nsf@ietf.org>, Roman Danyliw <rdd@cert.org>, skku-iotlab-members <skku-iotlab-members@googlegroups.com>, JungSoo Park <pjs@etri.re.kr>, Yunchul Choi <cyc79@etri.re.kr>, Patrick Lingga <patricklink888@gmail.com>, "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.com>
Content-Type: multipart/mixed; boundary="0000000000000d668305bbef8cf4"
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/V9qarsqsDG6SjSjahLIAfXEDmNY>
Subject: Re: [yang-doctors] [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 22 Feb 2021 16:46:51 -0000
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 < jaehoon.paul@gmail.com> wrote: > 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: > > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:47: warning: RFC 8407: 3.1: The > IETF Trust Copyright statement seems to be missing (see pyang --ietf-help > for details). > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1011: error: RFC 8407: 4.14: > statement "grouping" must have a "description" substatement > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1016: error: keyword > "description" not in canonical order, expected "type" (see RFC 6020, > Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1017: error: keyword "type" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1018: error: keyword "default" > not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1102: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1119: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1135: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1184: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1206: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1245: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1276: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1322: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1386: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1425: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1492: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1526: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1540: error: keyword > "if-feature" not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1605: error: keyword "config" > not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1610: error: keyword "key" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1620: error: keyword "key" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1631: error: keyword "key" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1643: error: RFC 8407: 4.14: > statement "container" must have a "description" substatement > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1654: error: keyword "key" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1780: error: keyword > "description" not in canonical order, expected "type" (see RFC 6020, > Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1781: error: keyword "type" not > in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1782: error: keyword "units" > not in canonical order (see RFC 6020, Section 12) > ietf-i2nsf-nsf-monitoring@2021-02-17.yang:1783: error: keyword "default" > not in canonical order (see RFC 6020, Section 12) > > > > On Sun, Feb 21, 2021 at 2:10 AM Andy Bierman <andy@yumaworks.com> 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 < >> jaehoon.paul@gmail.com> 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 < >>> noreply@ietf.org> 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 >>>> I2nsf@ietf.org >>>> 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: +82-31-299-4957 > Email: pauljeong@skku.edu, jaehoon.paul@gmail.com > Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php > <http://cpslab.skku.edu/people-jaehoon-jeong.php> >
- [yang-doctors] Yangdoctors early review of draft-… Andy Bierman via Datatracker
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Mr. Jaehoon Paul Jeong
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Reshad Rahman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Reshad Rahman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Linda Dunbar
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Andy Bierman
- Re: [yang-doctors] [I2nsf] Yangdoctors early revi… Linda Dunbar