Early Review of draft-ietf-opsawg-nat-yang-06
review-ietf-opsawg-nat-yang-06-yangdoctors-early-schoenwaelder-2017-10-27-00

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.