Hi Joe, I will address your comments on this draft. Thanks.
Best Regards, Paul On Mon, Nov 22, 2021 at 12:01 AM Joe Clarke via Datatracker < [email protected]> 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 protected] > https://www.ietf.org/mailman/listinfo/i2nsf >
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
