Summary From a YANG modeling point of view, the module is rather straight forward and I did not discover anything that seems fundamentally problematic. That said, there are a number of details where I doubt the model is correct and things where I believe things are incomplete. Given that there are many different NAT functions, I also expected to see usage of YANG features. One fundamental question one could raise is whether this model should have been broken down into a generic NAT core model plus extensions of the core model for the different types of NATs. Well, a single model with YANG features is an inline version of that as this still requires to mark clearly which parts are specific to certain types or NATs. I did not compile the module myself since the IETF tracker says no errors using pyang and yanglint (and I would not have run anything different). (Is it OK for YANG doctors to trust the tracker tools? Well, since this is labelled as an early review and I expect more updates, I assume this is fine.) I can't tell whether the model makes sense from a NAT point of view. I am not an expert in the various types of NATs and surely more reviews of NAT experts would be good (and they would have likely spotted some things I have spotted, so I guess the usual problem of getting enough substantial reviews). Details - We use revision statements only for published modules. Hence, please replace the revision statements with // RFC Ed. update to match the date of the latest edits revision 2017-xx-yy { description "Initial revision."; reference "RFC XXXX: A YANG Data Model for Network Address Translation (NAT) and Network Prefix Translation (NPT)"; } - We usually follow a different template for the contact statement that has more context and just a list of names and email addresses. - We usually write reference "RFC 3022: Traditional IP Network Address Translator (Traditional NAT)"; instead of just reference "RFC 3022."; since not everybody remembers all the numbers equally well. - Is there a reference for dst-nat? - What is the vrf-routing-instance identity good for? Its used only in the leaf external-vrf-instance and the description of that leaf is not telling me how this is going to be used. Who is going to derive identities? I fear you are trying to achieve something where using identities is really the wrong approach. Section 2.10 does not tell how this is supposed to work either. I assume this needs to be checked by the routing area experts to make sure things fit with their modeling of VRFs. - s/start-port-numbert/start-port-number/ - Are comments like // port numbers: single or port-range necessary given that there are description statements? Note that comments may be removed by tools while description statements usually are preserved. So it is important to have anything important covered in description statements. - What is a PSID algorithm? Spell out acronyms on first usage. - The leafs start-port-number and end-port-number are commented out in port-range, probably in favour of the uses port-number. If they are not needed anymore, they should be removed. - I do not understand port-set-algo from the descriptions. What is the psid-offset applied? What is a 'sharing ration for an IPv4 address'? Is this stuff IPv4 specific? Is the psid something that has a certain meaning outside of this YANG module? If so, where do I find more details (missing reference statement?)? - mapping-entry/index: is this an arbitrary identifier? how are these identifiers allocated? Are they created by the NAT or are they created via configuration? Or even both? Well, I guess it is both given the mapping-entry/type leaf. - mapping-entry/type: instead 'manually configured', I would write 'explicitly configured' (configuration does not have to be a manual process). I also find the other enum labels at least a bit confusing. enum "dynamic-explicit" { description "This mapping is created by an outgoing packet."; } enum "dynamic-implicit" { description "This mapping is created by an explicit dynamic message."; } Note the 'implicit' vs 'explicit' clash here. Perhaps this should be aligned with the definition of dynamic and implicit mappings: o Dynamic implicit mapping: is created implicitly as a side effect of traffic such as an outgoing TCP SYN or an outgoing UDP packet. A validity lifetime is associated with this mapping. o Dynamic explicit mapping: is created as a result of an explicit request, e.g., PCP message [RFC6887]. A validity lifetime is associated with this mapping. But then it seems you really messed things up here. I would even write these definitions differently since 'outgoing' is unclear and potentially confusing. o Dynamic implicit mapping: is created implicitly as a side effect of processing a packet (e.g., an initial TCP SYN packet) that requires a new mapping. A validity lifetime is associated with this mapping. o Dynamic explicit mapping: is created as a result of an explicit request, e.g., PCP message [RFC6887]. A validity lifetime is associated with this mapping. Similarly, I would change the description of enum "dynamic-implicit" { description "This mapping is created implicitely as a side effect of processing a packet that requires a new mapping."; } enum "dynamic-explicit" { description "This mapping is created as a result of an explicit request, e.g., a PCP message."; } - We should perhaps have a type for an IP protocol number, ideally in inet-types. (Just a reminder to myself.) That said, I do not understand what this sentence means: No transport protocol is indicated if a mapping applies for any protocol. Perhaps you wanted to say this: If this leaf is not instantiated, then the mapping applies to any protocol. - container internal-src-port: - container external-src-port: - container internal-dst-port: - container external-dst-port: It is used also to carry the internal source ICMP identifier."; I think details are lacking here. What is the ICMP identifier? What does 'carry' mean and how does this relate to the port-number grouping? See above, I do not understand the ICMP identifier statement. - lifetime: Spell out 3WHS. I think there should be a units statement. And is this a ticking lifetime, i.e., this changes on every get request? Is this useful? Have alternatives been considered such as reporting the point in time when the mapping was established? Or is the idea that the lifetime reports the time left until the mapping will be garbage collected? What does "tracks the connection" really mean here? - I suggest to remove empty lines in say leaf definitions. Instead leaf id { type uint32; description "NAT instance identifier."; reference "RFC 7659."; } write leaf id { type uint32; description "NAT instance identifier."; reference "RFC 7659: Definitions of Managed Objects for Network Address Translators (NATs)"; } - It seems you try to be aligned with the NATV2-MIB module but there is no explicit discussion about this in the document. I suggest that you a section (for example before the tree diagram) where you discuss how this YANG module coexists with the NATV2-MIB module. For example, I see that Natv2InstanceIndex in the MIB module excludes 0 as an instance identifier while your id leaf above allows the usage of 0. - container nat-capabilities: Here we find a bunch of "config true" leafs and I wonder how these work. There are things a NAT implementation is capable to do, and there are things that are enabled in a NAT deployment and of course you can't enable something in a deployment that has not been implemented. So are these 'capabilities' more like NAT features enabled (since we have "config true" nodes) or are these more implemented capabilities (but then "config true" may be wrong)? Depending on the answer, did you consider using YANG features? - nat-flavor and nat44-flavor: Does it make sense to have two objects here? Can I put basic-nat into nat-flavor? Are the differences between lets say napt and basic-nat really nat44 specific? Note that you also derive restricted-nat from nat44 but this option is not mentioned in the description of nat44-flavor. I think the description should be more open since in principle I can derive even more identities in the future. - boolean capability flags Do these need references so one can lookup what the exact meaning of these 'capabilities' are? It would surely help me and it will help implementors that need to decide whether a certain vendor feature fits any of these. - nat-pass-through-pref Perhaps spell out nat-pass-through-prefix (pref might also be understood as preference). And perhaps change the wording in the description OLD "The IP address subnets that match should not be translated. According to NEW "IP addresses matching this prefix are not be translated. According to - Sometimes you repeat prefixes in leaf definitions (e.g., nat-pass-through-port) while at other times you do not. - nat-pass-through-port You seem to have copy pasted the description of nat-pass-through-pref. Is this a single port? So I need multiple nat-pass-through entries for multiple ports. Fine. But is there a special meaning if both nat-pass-through-pref and nat-pass-through-port are configured in a single list entry? Does this mean the port is scoped to the prefix? - Some nat type specific parameter lists are flat, for others there is an additional container. Is this by design? - I meanwhile think there really should be features. Certain lists only make sense for implementations that support certain NAT types. Having features defined allows code generators to easily generate stubs that actually match the capabilities of an implementation. And you automatically benefit from feature announcements. - s/attachedto/attached to/ - s/prefixs./prefixes./ - nat64-prefix What is the purpose of //default "64:ff9b::/96"; ?? - stateless-enable Does this enable leaf make sense on a list entry? Perhaps it does but the description is fairly general and hence I am asking. - external-ip-address-pool/pool-id This seems to relate to a Natv2PoolIndex, which again excludes the value 0. I have not checked all such related things, so please go and check yourself. - external-ip-address-pool The description says Both contiguous and non-contiguous pools can be configured for NAT purposes."; but it seems more accurate to say that a pool is a set of prefixes since this is what the model suggests. - supported-transport-protocols I am again not clear whether you are reporting implementation capabilities here or enabled features or something else. Is the configuration of transport-protocol-id and transport-protocol-name mutually exclusive? If so, should this be a choice? If I can configure both, I assume they have to be consistent. - transport-protocol-name Is this restricted to the acronyms that are used in the IANA protocol numbers registry? - s/masck/mask/ - subscriber-mask-v6 The name seems to indicate that this only applies to IPv6 prefixes handed out to CPEs. Perhaps this should be stated explicitely (but yes it is unlike to ever get an IPv4 prefix). But then, the subscriber-match has an IPv4 prefix example. - quota-type Is this a good name for the leaf? This seems to indicate for which transport protocol a port-limit is enforced. And this list is restricted to TCP, UDP, and ICMP while other parts of the model are more flexible in supporting additional transport protocols. So is it consistent to a rather restricted enum here? - port-set-timeout Needs a units statement. - timeouts Can I make the timeouts arbitrarily small, in the extreme case 0 seconds? In some cases I find text like "for at least 6 seconds". Does this mean that the range is really uint32 { range "6..max"; }? Or is it still OK to configure the value 3 and the 'must' is really a 'should'? - port-timeout I doubt this is of type inet:port-number and there likely needs to be a units statement. - alg-name Is there a list of well-known ALG names? IANA? Or is this a random string that one needs to guess form the vendor's documentation, i.e., this is potentially not interoperable out of the box? Is there a way to obtain the number of ALGs supported or is the idea to do trial and error probing to find out (which is even harder if there are no well-known ALG names)? - all-algs-enable This says "Enable/disable all ALGs." but I _assume_ that I set this to false and to enable specific ALGs. Perhaps this interaction needs to be spelled out. - Is there any throttling mechanism needed in case my NAT is constantly operating around the notification thresholds? - Add units to the mapping limit definitions ("subscribers", "mappings", ...) - limit-per-subnet This is type inet:ip-prefix? I guess you wanted something else here. - Why are some limits mandatory, others not? - If you write 'limit per subscriber', how is a subscriber identified? I assume these are global per subscriber limits, i.e., all subscribers receive the same limit. - limit-per-subnet leaf limit-per-subnet { type inet:ip-prefix; description "Rate-limit the number of new mappings and sessions per subnet."; } I do not see how this works. The other limit-per-XXX objects are numbers - presumably defining the limit. This is a prefix? - logging-info Is this information complete? Perhaps it is for plain syslog (without any security) but I doubt the info is complete for the other transports mentioned, at least not for FTP. And surely, if you want to protect the logging information, then you will neeed way more parameters. And what does 'retrieving' logging entries mean? I assume with syslog and ipfix you push log messages. I am less sure about FTP. Perhaps less is more and simply provide support for syslog and leave the other options for extensions. You have a choice in place, so not need to go and deal with all possible complexity here. - For the statistics, we used to use plural form for counters back in SNMP land and I think this was good practice. So go with sent-packets, sent-bytes, rcvd-packets, rcvd-bytes, dropped-packets, dropped-bytes, ... - total-mappings, total-tcp-mappings, total-udp-mappings, total-icmp-mappings: These may use yang:gauge32. - address-allocated, address-free -> addresses-allocated, addresses-free (may also be yang:gauge32) - Some of these gauges might fluctuate fast (maybe consider exponentially smoothed gauges, but this might also be left for an extension). - The security considerations text should follow the new boilerplate that also covers RESTCONF. I think it would help to be more specific about the security aspects of this data model. This text is quite generic. With a NAT, things even have privacy aspects. If I can force a specific NAT mapping, I can track a subscriber. - At least one example has syntax errors. Examples should ideally be validated by tools so that the examples are syntactically correct and valid regarding the data model.