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
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?