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
> [email protected]
> https://www.ietf.org/mailman/listinfo/yang-doctors
--
Juergen Schoenwaelder Jacobs University Bremen gGmbH
Phone: +49 421 200 3587 Campus Ring 1 | 28759 Bremen | Germany
Fax: +49 421 200 3103 <http://www.jacobs-university.de/>
_______________________________________________
OPSAWG mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/opsawg
