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

[<[e-mail]>]

Re-,

OK, thanks.

Cheers,
Med

> -----Message d'origine-----
> De : Juergen Schoenwaelder [mailto:[e-mail]]
> Envoyé : dimanche 29 octobre 2017 09:37
> À : [e-mail]
> Cc : [e-mail]; [e-mail]
> Objet : Re: [yang-doctors] Yangdoctors early review of draft-ietf-opsawg-
> nat-yang-06
> 
> 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/>