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

[<[e-mail]>]

Hi Jürgen,

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Jürgen Schönwälder [mailto:[e-mail]]
> Envoyé : vendredi 27 octobre 2017 20:14
> À : [e-mail]
> Cc : [e-mail]; [e-mail]
> Objet : Yangdoctors early review of draft-ietf-opsawg-nat-yang-06
> 
> 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.

[Med] OK.

 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.

[Med] We used to make use of "features", but there was a comment during the WG call for adoption that requested us to make use of identities, instead.

> 
> 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).

[Med] FYI, the module was reviewed by experts of each of translation flavors covered in the document. All the reviews were shared on the list. 

> 
> 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)";
>   }
> 

[Med] Fixed. Thanks.

> - We usually follow a different template for the contact statement
>   that has more context and just a list of names and email addresses.
> 

[Med] Fixed.


> - 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.

[Med] Fixed. 

> 
> - Is there a reference for dst-nat?

[Med] I don't have an RFC to cite here. 

> 
> - What is the vrf-routing-instance identity good for?

[Med] This is used to bind a NAT function to an external VRF routing instance. Please check https://www.ietf.org/mail-archive/web/opsawg/current/msg05117.html 

 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/

[Med] Fixed.

> 
> - 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.
> 

[Med] Cleaned all these comments. 

> - What is a PSID algorithm? Spell out acronyms on first usage.

[Med] PSID stands for Port Set Identifier. This is an algorithm to assign non-overlapping port sets to users serviced with the same shared IPv4 address. References are added to the module.

> 
> - 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.

[Med] 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?)?

[Med] I updated the description and added pointers to the authoritative spec. 

> 
> - 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.

[Med] Both can be supported. 

> 
> - mapping-entry/type: instead 'manually configured', I would write
>   'explicitly configured' (configuration does not have to be a manual

[Med] Manually is derived from RFC6887, which says: 

      *  Explicit static mappings are created by manual configuration
         (e.g., via command-line interface or other user interface) and
         persist until the user changes that manual configuration.


>   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. 

[Med] Good catch. Thanks.

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.
> 

[Med] Works for me.

>    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.";
>         }
> 

[Med] Deal!

> - 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.

[Med] You got it. I used your proposed wording. 

> 
> - 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.

[Med] 'ICMP Identifier' usage is the same as in RFC6146. 

I added this NEW text:

         As a reminder, all the ICMP Query messages contain 
         an 'Identifier' field, which is referred to in this 
         document as the 'ICMP Identifier'.";

> 
> - lifetime:
> 
>   Spell out 3WHS.
[Med] Replaced with three-way handshake. 

 I think there should be a units statement.
[Med] Agree. Fixed.

 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?

[Med] The initial value indicates the duration to maintain a mapping. When retrieved in a get operation, it indicates the remaining validity lifetime.  
I updated the text to clarify this.

> 
> - 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)";
>       }
> 
[Med] Fixed.


> - 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.

[Med] I will Senthil (one of the co-author of the NATV2-MIB module) comment on this.  

> 
>   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)?

[Med] These are about what an implementation is able to do. When I set config to "false", pyang is complaining.

>   Depending on the answer, did you consider using YANG features?

[Med] Yes, we considered the 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?

[Med] These are specific to characterize a NAT44. For example:
- NPTv6 is by design acting on prefixes; ports are not touched
- NAT64 can be stateless or statefull
- EAM relies on address rewriting

> 
>   Note that you also derive restricted-nat from nat44 but this option
>   is not mentioned in the description of nat44-flavor. 

[Med] That one is removed, because it is conflicting with the use of "restricted-port-support" that is valid for both state full NAT64 and NAT44. 


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.

[Med] I added references as appropriate. 

> 
> - 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
> 

[Med] Works for me.

> - Sometimes you repeat prefixes in leaf definitions (e.g.,
>   nat-pass-through-port) while at other times you do not.
> 

[Med] Fixed.

> - nat-pass-through-port
> 
>   You seem to have copy pasted the description of
>   nat-pass-through-pref.  Is this a single port?

[Med] Yes, this is a single port that can be supplied with or without a prefix

 So I need multiple
>   nat-pass-through entries for multiple ports.

[Med] Yes.

 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?
> 

[Med] Yes. The text was updated accordingly. 

> - Some nat type specific parameter lists are flat, for others there is
>   an additional container. Is this by design?

[Med] Yes, the design was built with NAT/NAT64 as the core functionality. Other specific-techniques are added as containers for clarity. 

> 
> - 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.
> 

[Med] As I mentioned earlier, we used to make use of features in early versions of the draft. I’m really neutral on this.  

> - s/attachedto/attached to/
> 
[Med] Fixed.

> - s/prefixs./prefixes./
> 
[Med] Fixed.

> - nat64-prefix
> 
>   What is the purpose of //default "64:ff9b::/96"; ??

[Med] This is now removed given that there is no recommendation to use a Well-known Prefix or a Network-Specific Prefix (RFC6052). 

> 
> - 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.

[Med] Both stateless and statefull NAT64s are implementing the same address translation algorithm (and consequently the same configuration data). stateless-enable is an explicit indication to a NAT64 to indicate whether it should consider source IPv6 addresses as IPv4-translatabke IPv6 addresses (RFC6052). 

> 
> - 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.

[Med] I will let Senthil comment on this one. 

> 
> - 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.

[Med] Yes, that's it. I changed the text as suggested. 

> 
> - supported-transport-protocols
> 
>   I am again not clear whether you are reporting implementation
>   capabilities here or enabled features or something else. 

[Med] This one is to configure explicitly a transport protocol that will be handled by the NAT.

Is the
>   configuration of transport-protocol-id and transport-protocol-name
>   mutually exclusive? 

[Med] No. A list of protocols can be enabled.

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?

[Med] Yes.  

> 
> - s/masck/mask/
[Med] Fixed. 

> 
> - 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.

[Med] This is IPv6-specific. The text is now updated. 

> 
> - 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?

[Med] Good point. Updated accordingly. 

> 
> - port-set-timeout
> 
>   Needs a units statement.
[Med] Done.


> 
> - 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'?

[Med] Only recommended values are provided. An administrator is free to set the values it sees appropriate for his/her deployment.  

> 
> - port-timeout
> 
>   I doubt this is of type inet:port-number and there likely needs to
>   be a units statement.

[Med] Good catch. Fixed.

> 
> - 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)?
> 

[Med] There is no "standardized/IANA" list of ALGs. There is a list of widely deployed/implemented ones.
FWIW, we used to have this list: 

    |        +--rw ftp-alg-enable?              boolean
    |        +--rw dns-alg-enable?              boolean
    |        +--rw tftp-alg-enable?             boolean
    |        +--rw msrpc-alg-enable?            boolean
    |        +--rw netbios-alg-enable?          boolean
    |        +--rw rcmd-alg-enable?             boolean
    |        +--rw ldap-alg-enable?             boolean
    |        +--rw sip-alg-enable?              boolean
    |        +--rw rtsp-alg-enable?             boolean
    |        +--rw h323-alg-enable?             boolean
    |        +--rw all-algs-enable?             boolean

We changed the design to this one because it is easily extensible. 

> - 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.

[Med] Updated.

> 
> - Is there any throttling mechanism needed in case my NAT is
>   constantly operating around the notification thresholds?

[Med] This is deployment- and implementation-specific. We don't have a standard document to cie. 

> 
> - Add units to the mapping limit definitions ("subscribers",
>   "mappings", ...)
[Med] Will be checked. 

> 
> - limit-per-subnet
> 
>   This is type inet:ip-prefix? I guess you wanted something else here.

[Med] I confirm. A subscriber is assumed to be identified by a prefix. 

> 
> - Why are some limits mandatory, others not?
[Med] Because those that are required for most of existing NAT implementations. 

> 
> - 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.

[Med] A subscriber can be identified by a subscriber-mask or an explicit prefix. 

> 
> - 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?

[Med] Yes. 

> 
> - 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.
> 

[Med] It is out of scope of specify the exact logging information. Those considerations are out of scope. We only focus on the protocol and the server information. 

> - 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, ...

[Med] Actually, we used both of them. I updated when required. 

> 
> - total-mappings, total-tcp-mappings, total-udp-mappings,
>   total-icmp-mappings: These may use yang:gauge32.
[Med] OK.

> 
> - address-allocated, address-free -> addresses-allocated, addresses-free
>   (may also be yang:gauge32)
> 
[Med] OK.

> - Some of these gauges might fluctuate fast (maybe consider
>   exponentially smoothed gauges, but this might also be left for an
>   extension).

[Med] We can leave those for a future extension. 

> 
> - The security considerations text should follow the new boilerplate
>   that also covers RESTCONF.

[Med] Updated.


 I think it would help to be more specific
>   about the security aspects of this data model. This text is quite
>   generic.

[Med] The text Acks that all information are sensitive. They must be protected and secure means must be used. 

 With a NAT, things even have privacy aspects. If I can
>   force a specific NAT mapping, I can track a subscriber.

[Med] IMHO, this is not specific to the NAT. Mandating secure means to control the NAT is a guard against such attack.  

> 
> - 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.
> 
[Med] Will double check.