Last Call Review of draft-ietf-i2nsf-nsf-facing-interface-dm-16
review-ietf-i2nsf-nsf-facing-interface-dm-16-opsdir-lc-clarke-2021-11-21-00

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?