Please find below the comments on the draft from Dan Wing, who chaired Behave WG in the past and my response to his comments On the draft-sivakumar-yang-nat. Opsawg chairs had asked us to seek reviews from NAT experts.
Thanks Senthil On 4/21/17, 10:42 AM, "Senthil Sivakumar (ssenthil)" <[email protected]> wrote: Sorry, it took a while to get back to this. Thanks for your comments. Please see below for the responses. Thanks Senthil On 2/8/17, 1:15 PM, "Softwires on behalf of Senthil Sivakumar (ssenthil)" <[email protected] on behalf of [email protected]> wrote: I had reached out to Dan Wing and some others, as suggested by Ian to get reviews on the https://tools.ietf.org/html/draft-sivakumar-yang-nat-05. I am just posting the response from Dan here, I will address his comments in a separate email and copy the alias. Thanks Senthil Use [email protected] as my email address, to ease sorting the email if someone happens to follow up. >> was going to ask you for a favor to review a I-D for me. The recommendation from OPS WG was to get >> Some expert reviews from Behave members, finding any behave members willing to do a review is hard these days. >> >> https://tools.ietf.org/html/draft-sivakumar-yang-nat-05 > > My comments -- do you want them public somewhere? > > 1. Why not also cover NAT66 and NPTv6? It seems the design allows those, but the text doesn't mention that they can also be expressed in this model. NPTv6 is definitely a possibility, I am not sure if that is something that needs to be in this yang model though. NAT66 is a can of worms and I would like to stay away from that. If NAT66 becomes a reality, we could define a yang model for it then. > > 2. " A NAT function can either assign individual port numbers or port > sets. Both features are supported in the YANG data model." -> can you provide citation or definition of "port set"? Ok, I will add the definition of port sets. > > 3. " To accommodate deployments where [RFC6302] is not enabled, the NAT > function can be configured to log the destination port number." -> that logging is not a "NAT function" (whereas the previous paragraph does describe a NAT function). Logging is a function of YANG, right? Perhaps instead how about text like "... this defined Yang model can be configured to ..." Well, semantically, the NAT must provide the destination port to be able to log the destination port. draft-ietf-ipfix-nat-logging provides the templates to log the destination ports, the logging of the destination port can be enabled by the Yang model. The intention of the text here is to enable NAT logging of destination ports through the Yang model. > > 4. "This data model assumes that pools of IPv4 addresses can be > provisioned to NAT function. These pools may be contiguous or non- > contiguous." > > First sentence does not read well, would be improved with "... can be provisioned to *the* NAT function". Also, please provide description or citation to what is meant by "pool". I agree, I rephrase the first sentence as follows: “This data model assumes that a block of IPv4 global addresses can be provisioned to the NAT function.” > > 5. "A NAT device can enabled multiple NAT instances;" > > does not read well. Yes, agreed. Will rephrase as: “A single NAT device can have multiple NAT instances;" > > 6. " o Exclude/include ports (e.g.; system port) from the port assignment > pool." > > Nit: That should be "system portS" (plural). Ok. > > Serious: that is a significant deficiency. The reason I think it was excluded was that, it comes with significant complexity and most of the NAT implementations don’t have explicit configuration to allow this behavior. We will discuss this among the authors and See what is the best way to address this. > > 7. "Deterministic NAT assignment scheme" -> provide description or citation about what that means. > Ok. > 8. "module: ietf-nat > +--rw nat-config > | +--rw nat-instances > | +--rw nat-instance* [id] > | +--rw id uint32 > | +--rw enable? boolean > | +--rw external-ip-address-pool* [pool-id] > | | +--rw pool-id uint32 > | | +--rw external-ip-pool? inet:ipv4-prefix" > > How is IPv6 expressed for NAT64? The address pool is always a IPv4 global address pool whether is NAT64 or NAT44. This is the address block from which the source is translated to. This configuration applies To both NAT64 and NAT44 and hence no distinction is required. > > > 9. " | +--rw subscriber-mask-v6? uint8 > | +--rw subscriber-mask-v4* [sub-mask-id] > | | +--rw sub-mask-id uint32 > | | +--rw sub-mask inet:ipv4-prefix" > > Why are IPv6 masks expressed differently than IPv4 mask? The subscriber-mask-v6 is described in the later part of the document and it was thought of having a single mask value that can be used to apply to determine if the packet need to be translated or not. We thought it would simplify the configuration. But thinking about it again, I think some of the implementations of NAT64 don’t understand an integer as a mask but they need the whole ipv6-prefix. I am going to discuss This with the authors and change this to ipv6-prefix. At the least, we will have either a prefix or an integer to represent the mask length. > > Is "subscriber" the same as "internal"? I mean, this whole Yang model seems to use "subscriber" and "external", rather than "internal" and "external". What if the "internal" side isn't """subscribers""", such as a NAT in front of a datacenter, like a NAT64 in front of an IPv6-only datacenter, or a NAT44 in front of an IPv4-only datacenter; in that case the "subscriber" is now confusingly the server. Subscriber does imply internal. I will add some text around this to clarify what subscribers mean here. If all the authors agree, I will change it to internal and external, rather than subscriber and external. > > > 10. | +--rw port-randomization-enable? boolean > | +--rw port-preservation-enable? boolean > > both of those could be set to TRUE, creating opportunity for configuration and implementation conflict, creating interoperability problems. Can you instead define a trinary value, or does Yang like to have booleans so much, even when they can cause interop problems? I will discuss this with the authors and address this. I don’t know if we can represent this more efficiently. > > 11. | +--rw udp-timeouts? uint32 > > Have you considered per-port timeouts? Some NATs are configurable with short timeouts for certain ports, such as 10 seconds on port 53 (DNS) and NTP (123) and longer timeouts on other ports. This Yang model doesn't allow that. Seems a port list might be better to handle such configurations. Good point. I will add the per-port timeouts with a list. > > 12. I noticed there isn't any reference in the text to RFC7857, which updates and clarifies a lot of things. Is the Yang model compliant with the changes caused by RFC7857? The text should certainly cite it where it makes sense, but more importantly if additional settings are required by RFC7857, they need to be part of the yang model. > I believe it is compliant, I will talk to Med and make sure we are covered here. > 13. | +--rw logging-info > | | +--rw destination-address inet:ipv4-prefix > | | +--rw destination-port inet:port-number > > this doesn't indicate UDP or TCP, and doesn't seem able to log ICMP or other protocols that lack ports, which NATs might NAT (e.g., IPsec ESP protocol 50). Should be highlighted in security considerations. Ok. > > 14. | +--rw connection-limit > | | +--rw limit-per-subscriber? uint32 > | | +--rw limit-per-vrf? uint32 > | | +--rw limit-per-subnet? inet:ipv4-prefix > > Can only list one subnet? > Yes, it is a per-subnet basis and optional. > This doesn't allow different limits per VRF (e.g., VRF 1 is limited to 100 mappings, VRF 2 is limited to 5555 mappings). Seems restrictive. > Ok, we will discuss this again and decide if these should be an array of limits. > 15. | +--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 > > OMG. ALGs, really? Do those need to be per-subscriber or per-VRF or per-subnet? How are new ALGs added to Yang, like if I want an ALG for, um, I dunno, WebRTC or ssh. > The yang models are extensible, you can augment more nodes or fields. But you could make the above argument for anything that changes. Honestly, I haven’t seen a need for a new ALG In quite a few years. > 16. In mapping-table, I see: "rw lifetime uint32". Would this track the lifetime while the TCP connection is becoming fully-formed and hits the various Yang-defined 'timeout' values (tcp-idle-timeout, tcp-trans-open-timeout, and so on)?". I suppose it does. Text should say so. > > 17. | +--ro nat44-support? boolean > | +--ro nat64-support? boolean > > > NAT46? NAT66? NPTv6? XLAT64? CLAT? Well, I will add NPTv6 to it, I don’t want to support non-ietf flavors like 46 and 66. > > 18. stealth-mode-support needs better description than "Indicates whether to respond for unsolicited traffic.", and needs to align with IETF host requirements and IETF router requirements RFCs. > Ok, I will improve the description and add references. > > -d > > > > > Thanks Senthil _______________________________________________ Softwires mailing list [email protected] https://www.ietf.org/mailman/listinfo/softwires _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
