This is my YANG doctor review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-04. The review focuses on YANG-specifics only, since I am not very familiar with the technology that is modelled. o All three YANG modules should follow the common template for IETF YANG modules found in RFC 8407. Specifically, fix the copyright text and template for RFC 2119 langauge. (if you want help with this, download the latest checked in version of pyang from github, and run pyang --ietf --max-line-length 69) o The YANG modules are difficult to read b/c of the formatting. I had to run: pyang -f yang --yang-line-length 69 Even after this, you need to manually break long lines so that they fit into the I-D / RFC format. Note that the RFC editor now enforce this format, so that all IETF modules have a consistent look and feel. I suggest you run this command, fix the long lines manually, and put the result in the draft. o In general, it would help if the definitions had "reference" statements to the relevant sections in the underlying RFCs. Also use references rather than comments or descriptions: /* This is defined by XFRM */ /* RFC 4301 ASN1 notation. Annex C*/ description "RFC 7619"; description "List of local ports. When the upper-layer-protocol is ICMP this 16 bit value respresents code and type as mentioned in RFC 4301"; o In general, almost all nodes need better descriptions. For example leaf initiator-ikesa-spi { type uint64; description "Initiator's IKE SA SPI"; } This description doesn't add much info... o A general comment; in IETF YANG modules, lower-case-with-hyphens are preferred. In your module you sometimes use lowCamelCase, sometimes underscore_as_separator and sometimes UPPERCASEONLY. You can use pyang --lint --lint-ensure-hyphenated-names to find all occurrences. o Top-level structure and naming Your models define this top-level structure: +--rw ikev2 +--rw ietf-ipsec First of all, is the intention that a device implements one of these moules, or can it implement both? I wonder if a better structure would be: +--rw ipsec // note name suggestion +--rw ikev2 The name "ietf-ipsec-ikeless" sounds a bit odd. It seems to imply that if IKEv2 is implemented, this module is not used. But I suspect that is not true? o Use of groupings In the module ietf-ipsec-ike you have a set of groupings that are used only once. I suggest you remove these groupings. I assume that they are not intended to be re-used by other modules. (if they are, they need better descriptions for how they are supposed to be used) o PAD RFC 4301 says that the PAD is a user-ordered database. You use a uint64 as the key into the "pad-entry" list. I suggest that you make this list "ordered-by user", and use a symbolic string as key instead: list pad-entry { key name; ordered-by user; leaf name { type string; ... } ... } o PAD leafs I notice that in your model, all leafs in the pad-entry list are optional, except for "my-identifier", and w/o default values. Is this correct? If it is, I suggest you add description text that explains what the behavior is if these leafs are not configured. For example, what does it mean if "pad-auth-protocol" isn't configured. o Names... Unless "my-identifier" is a well-known term in this domain, I suggest "auth-identifier". Instead of "pad-auth-protocol" I suggest "auth-protocol" - it is already scoped in a pad-entry so we know it is a pad. Some leafs in "ike-conn-entry" are called "ike-..." (e.g. ike-fragmentation). This seems a bit random, e.g., "version" is not called "ike-version". I suggest you remove the "ike-" prefix from these nodes. Some abbreviations should probably be expanded, e.g., "oaddr", "bmp", "dport", "spi", "espencap" etc. o auth-method model Perhaps change: container auth-method { leaf auth-m { ... } ... } to: container peer-authentication { leaf auth-method { ... } ... } In the case of "digitial-signature", are all leafs in that container optional? Is is ok to configure none of them? Some of these leafs refer to file names. How are these files supposed to be handled - specifically, if I configure a file name here, how do I control the file? o NACM Consider using nacm:default-deny-all on the "secret" and "key-data" etc leafs. o pad-auth-protocol This leaf can currently only have one value, "ikev2". But at the same time, it is located under the top-level container "/ikev2". Is the intention that there might be other pad-auth-protocols in the future? If so, is the top-level name "/ikev2" a good name? Perhaps remove this leaf? And/or rename the top-level container. o SPD RFC 4301 says that the SPD is ordered, consistent with the use of Access Control Lists (ACLs) or packet filters in firewalls, routers, etc. You use a uint64 as the key into the "spd-entry" list. I suggest that you make this list "ordered-by user", and use a symbolic string as key instead: list spd-entry { key name; ordered-by user; leaf name { type string; ... } ... } (Compare also with RFC 8519) o anti-replay-window Should this leaf have a default value? o SPD model You have: +--rw names* [name] | +--rw name-type? ipsec-spd-name | +--rw name string This is not easy to guess what it is. The description gives at least a hint, so perhaps: list policy { key name; leaf name { ... } leaf type { ... } } ... but this probably needs a better description as well. o ike-conn-entry/version This leaf (if it really is needed) should have a default statement. o Usage of yang:timestamp There are a couple of nodes that use yang:timestamp, both for configuration and operational state. In both cases it is probably not the right the type to use, but the description of these nodes are not precise enough for me to recommend another type. As a first step, please re-read the definition of yang:timestamp in RFC 6991.