Early Review of draft-ietf-opsawg-nat-yang-06
review-ietf-opsawg-nat-yang-06-yangdoctors-early-schoenwaelder-2017-10-27-00
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.