Re: [yang-doctors] Yangdoctors early review of draft-ietf-opsawg-nat-yang-06

[Juergen Schoenwaelder <[e-mail]>]

One additional nit: You may want to think about the name of the toplevel
node. Here is what is currently published:

    +--rw interfaces            ietf-interfaces         RFC 7223
    +--rw ipfix                 ietf-ipfix-psamp	RFC 6728
    +--rw key-chains            ietf-key-chain		RFC 8177
    +--rw lmap                  ietf-lmap-control       RFC 8194
    +--ro modules-state         ietf-yang-library	RFC 7895
    +--rw nacm                  ietf-netconf-acm        RFC 6536
    +--ro netconf-state         ietf-netconf-monitoring RFC 6022
    +--rw routing               ietf-routing            RFC 8022
    +--rw snmp                  ietf-snmp               RFC 7407
    +--rw system                ietf-system		RFC 7317
    +--ro system-state          ietf-system     	RFC 7317

    +--ro interfaces-state      ietf-interfaces         RFC 7223 (to be deprecated)
    +--ro routing-state         ietf-routing            RFC 8022 (to be obsoleted)

Calling the toplevel node simply 'nat' may be more inline with the
existing names (which are not all perfect but if we ignore the -state
nodes there is a certain level of consistency - a not toplevel node
uses 'module').

/js

On Fri, Oct 27, 2017 at 11:14:15AM -0700, Jürgen Schönwälder wrote:
> Reviewer: Jürgen Schönwälder
> Review result: Not Ready
> 
> 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.
> 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
Fax:   [phone]         <http://www.jacobs-university.de/>