Re: [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04
Andy Bierman <andy@yumaworks.com> Sat, 20 February 2021 17:10 UTC
Return-Path: <andy@yumaworks.com>
X-Original-To: i2nsf@ietfa.amsl.com
Delivered-To: i2nsf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 8D39B3A11BF for <i2nsf@ietfa.amsl.com>; Sat, 20 Feb 2021 09:10:52 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.887
X-Spam-Level:
X-Spam-Status: No, score=-1.887 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, T_SPF_PERMERROR=0.01, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=yumaworks-com.20150623.gappssmtp.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 WSt0NcloYJZ7 for <i2nsf@ietfa.amsl.com>; Sat, 20 Feb 2021 09:10:50 -0800 (PST)
Received: from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com [IPv6:2a00:1450:4864:20::22f]) (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 023AC3A11C1 for <i2nsf@ietf.org>; Sat, 20 Feb 2021 09:10:49 -0800 (PST)
Received: by mail-lj1-x22f.google.com with SMTP id a22so42078107ljp.10 for <i2nsf@ietf.org>; Sat, 20 Feb 2021 09:10:49 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yumaworks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LdxXHmmT/LjzIht94aTLtfxvLltLeLox1Dbao9KT5fI=; b=BLJ4c9LC6pTulyIk/sLTJwBTFUBXIZQeQAo+6fqVjXtKukosS7TN0ZmvCk15IkOqfN jYp4MmowLcH9ww1wsTEuEHK6clv1GFeODdaQQfKYtKV7orMV/E6QaQyeAKSrKwlbUJPi oNzAjKL/SL9LHzvTiLLX3g5sRVFqh3fDjleBXrtJ+qMoDkx9b22EYnaCaN8f/i+b4zjl ACoydfm39h7ko9yvPyeeCIgAh0cewMWpl4Qw7hPuw3PpRDDnnXvEtiNB6WMwltWMBpfa R6Jq9o5Utb9mi6nZwAMTnQuWlC/VpnTEpAu5crJXxy9r5ZdJJzT0UlVAR+GGR/bUX8pU 0J6A==
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=LdxXHmmT/LjzIht94aTLtfxvLltLeLox1Dbao9KT5fI=; b=N3l4YVSZilwxmga48rf9ZVj6mRzUjGkLCf9Z5IqVdq2Nd0CptKIUPNWQkgR7oPVXuV +nNmJmRxmAjLRCPkZUa/8Xk6KZRjyNZUrxAJIAyra+cim5hXlbXURzt7TGkyAoBcRBH0 /b6JeSu/U/Bcosc7Oz5RjhQE97dzxTYfHCReTpXAdGbEmKtZPCgzI6RP3LVK7lO7+k6x nWIv6utVKbe4N+AMTIGDxMJhmXW0Kyqh+KhnYRATatGnAuZHa7Op+VJR0iGh5l64giAU I6CIMu+vF3k6nKeFcmtrFsHWkXpQyQ+NJaAsFmw6QupdzpG55IS0pM4lhkkZ4UxN+kvW +xzg==
X-Gm-Message-State: AOAM533mElK00qforB9UeS0VAqbDoKA/zQkV8nr2frR3YpgwagULZKDI UmoD+pui7QNHAIHzGHyziRlOCQgJ7zB3ENtBUA/HHg==
X-Google-Smtp-Source: ABdhPJytPqk/vtuQRpm79vI7K36d1q0ibomQhITBnsLIGF5KqsNwycE9lKRHcS4w6zWgd5nObtwAPN59SnE5daGckMk=
X-Received: by 2002:a05:6512:1195:: with SMTP id g21mr8820017lfr.512.1613841047954; Sat, 20 Feb 2021 09:10:47 -0800 (PST)
MIME-Version: 1.0
References: <160192102291.6633.15935674903085952087@ietfa.amsl.com> <CAPK2DewwLOJnnj1ZEYMSvZB1Hpc-8rY1+-dFvzOkJvrT4X2jqA@mail.gmail.com>
In-Reply-To: <CAPK2DewwLOJnnj1ZEYMSvZB1Hpc-8rY1+-dFvzOkJvrT4X2jqA@mail.gmail.com>
From: Andy Bierman <andy@yumaworks.com>
Date: Sat, 20 Feb 2021 09:10:37 -0800
Message-ID: <CABCOCHRaSTLDngZnMaQFiq05AZbymJfFFVY+P_fmzuU4e3S7FQ@mail.gmail.com>
To: "Mr. Jaehoon Paul Jeong" <jaehoon.paul@gmail.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>
Content-Type: multipart/mixed; boundary="000000000000634cb605bbc7a690"
Archived-At: <https://mailarchive.ietf.org/arch/msg/i2nsf/K-Z_PhrX__i6gjrar-gyFQyRMMw>
Subject: Re: [I2nsf] Yangdoctors early review of draft-ietf-i2nsf-nsf-monitoring-data-model-04
X-BeenThere: i2nsf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "*I2NSF: Interface to Network Security Functions mailing list*" <i2nsf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/i2nsf/>
List-Post: <mailto:i2nsf@ietf.org>
List-Help: <mailto:i2nsf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/i2nsf>, <mailto:i2nsf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 20 Feb 2021 17:10:53 -0000
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 >> > >
- [I2nsf] Yangdoctors early review of draft-ietf-i2… Andy Bierman via Datatracker
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] Yangdoctors early review of draft-iet… Andy Bierman
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] Yangdoctors early review of draft-iet… Andy Bierman
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] Yangdoctors early review of draft-iet… Andy Bierman
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] Yangdoctors early review of draft-iet… Andy Bierman
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] Yangdoctors early review of draft-iet… Mr. Jaehoon Paul Jeong
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Andy Bierman
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Reshad Rahman
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Reshad Rahman
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Linda Dunbar
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Andy Bierman
- Re: [I2nsf] [yang-doctors] Yangdoctors early revi… Linda Dunbar