Subject: Re: [I2nsf] Opsdir last call review of draft-ietf-i2nsf-nsf-facing-interface-dm-16

From: "Mr. Jaehoon Paul Jeong" <[email-masked]>
Date: Sat, 22 Jan 2022 22:53:06 +0900

Hi Joe,
Here is the revision of I2NSF NSF-Facing Interface YANG Data Model Draft:
https://datatracker.ietf.org/doc/html/draft-ietf-i2nsf-nsf-facing-interface-dm-17

I attach a revision letter to explain how Patrick and I have addressed your
comments.

Thanks.

Best Regards,
Paul

On Mon, Nov 22, 2021 at 12:01 AM Joe Clarke via Datatracker <
[email-masked]> wrote:

> Reviewer: Joe Clarke
> Review result: Has Issues
>
> I have been asked to review draft-ietf-i2nsf-nsf-facing-interface-dm on
> behalf
> of the Ops Directorate.  While this draft represents an info model for the
> NSF-facing I2NSF interface, it seemed more practical from a configuration
> standpoint, and I was left wanting more fleshed out element descriptions.
> I
> found the model overall readable but was left wondering what I as an
> operator
> that might be configuring exactly in certain cases.  I also found some
> perhaps
> YANG-ish things that I think should be fixed (e.g., leaf naming in parts).
> Below are some specific instances of these issues I had when reading:
>
> Section 3.1
>
> "The system policy provides for multiple system policies "
>
> This sentence doesn’t make much sense.  Are you saying that the top-level
> system policy provides for multiple sub-policies?
>
> ===
>
> YANG module
>
> identity system-event {
>   description
>     "Identity for system events";
> }
>
> I’m not crazy about descriptions that are just restatements of the type
> and the
> name.  While you have a reference here, can you make these identity
> descriptions more useful without one needing to jump to other documents?
>
> There are many other identities like this where I'd prefer to see more
> descriptive text to help me as a reader/implementer/operator understand
> more
> without having to jump between documents all the time.
>
> ===
>
> YANG module
>
> identity anti-ddos { ... }
>
> This, and other actions seem to be missing descriptive detail about what
> exactly is expected from the NSF if this is configured.  Maybe this is
> left up
> to implementations, but in that case I'd expect some references to
> potential
> DDoS mitigation approaches to take.
>
> ===
>
> YANG module
>
> identity drop {
>        base ingress-action;
>        base egress-action;
>        base default-action;
>        description
>          "Identity for drop";
>        reference
>          "draft-ietf-i2nsf-capability-data-model-21:
>           I2NSF Capability YANG Data Model - Actions and
>           Default Action";
> ...
> }
>
> Just as above, I was expecting more details about these actions actually
> mean
> and exactly the behavior one could expect.  For example, how is a drop to
> be
> done?  Does it matter if it's a silent drop vs. a drop/unreachable?
>
> ===
>
> YANG module
>
> identity day {
>        description
>          "This represents the base for days.";
>      }
>
> Maybe more of a YANG Doctors thing, but why not make days an enumeration
> where
> you can have day values?  I’d think that would be more useful as I can’t
> foresee someone adding new identities of base day.
>
> ===
>
> YANG module
>
> leaf rule-name {
>            type string;
>            description
>              "The name of the rule.";
>          }
>
> I wouldn’t prefix each leaf with “rule” since you’re already in the rules
> list.
>  Moreover, you’re not doing this consistently here or in other lists (e.g.,
> ethernet vs. ipv4).
>
> ===
>
> YANG module
>
> leaf rule-enable {
>            type boolean;
>            description
>              "True is enable.
>               False is not enable.";
>          }
>
> You have a few "enable" leafs in this module, and I would flesh these out
> a bit
> more to add clarity.  Maybe, .  “If true, the rule is enabled and
> enforced.  If
> false, the rule is configured but disabled and not enforced.”
>
> Something like that.
>
> ===
>
> YANG module
>
> leaf-list date {
>                  when
>                    "../../frequency='monthly'";
>                  type int32{
>                    range "1..31";
>                  }
>                  min-elements 1;
>                  description
>                    "This represents the repeated date of every month.
>                     More than one date can be specified.";
>                }
>
> Does this need to be a 32-bit integer?  Given the range, int8 should do.
>
> ===
>
> YANG module
>
> leaf alert-packet-rate {
>                type uint32;
>                units "pps";
>                description
>                  "The alert rate of flood detection for
>                   packets per second (PPS) of an IP address.";
>              }
>
> As I understand it, these are thresholds before an alert will be
> generated?  If
> so, can you make that more explicit in this and other threshold
> descriptions?
>
> ===
>
> YANG module
>
> In your application container, I'm not sure what application object, group,
> label, and category are.  More description text and references would be
> helpful.
>
> ===
>
> YANG module
>
> container geography-location { ... }
>
> "geographic" reads better to me than "geography"
>
> Speaking of which, why not reference
> https://datatracker.ietf.org/doc/draft-ietf-netmod-geo-location/ for
> geo-location?
>
>
>
> _______________________________________________
> I2nsf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/i2nsf
>