Hi Med and Senthil, I had a look at this draft, and found some issues that I'd like to discuss.
Regards, Tianran 1. You use the uint32 type id as key for many list objects. For example: | +--rw nat-instances | +--rw nat-instance* [id] | +--rw id uint32 | +--rw enable? boolean Is this design from your implementation? I think a string type "name" is more easy to use. For example, when we create a nat instance, we may type a command line like: nat-instance nat-name This ACL module might be another example. https://datatracker.ietf.org/doc/draft-ietf-netmod-acl-model/?include_text=1 2. It seems your IP address pool only support prefix description. Some more IP pool descriptions may be useful. For example, describe a IP address section using the start IP and the length or end IP. | +--rw external-ip-address-pool* [pool-id] | | +--rw pool-id uint32 | | +--rw external-ip-pool? inet:ipv4-prefix 3. Did you consider the NAT server or dest_address NAT Scenario? 4. Is this "port-quota" stands for the number of ports that can be used by a nat instance? But I did not find something to indicate the port range. Or could you please show me? 5. Your port configuration is associated with the whole IP address pool list. Maybe it's more flexible if we can assign each ip pool with a port range. 6. Here you can set connection-limit by subscriber, vrf,... How about also by the session type, e.g. UDP, TCP, ... | +--rw connection-limit | | +--rw limit-per-subscriber? uint32 | | +--rw limit-per-vrf? uint32 | | +--rw limit-per-subnet? inet:ipv4-prefix | | +--rw limit-per-instance uint32 7. I am not clear on how to use this mapping table. My understanding is this is used for static mapping only. | +--rw mapping-table | +--rw mapping-entry* [index] | +--rw index uint32 | +--rw type? enumeration | +--rw internal-src-address inet:ip-address 8. In all, I think it's better to describe how this module is structured and also give some examples on how to use this module. > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Wednesday, April 26, 2017 4:18 PM > To: Tianran Zhou; Senthil Sivakumar (ssenthil); [email protected] > Cc: Ian Farrer > Subject: RE: [Softwires] Comments on draft-sivakumar-yang-nat-05 > > Hi Tianran, > > Yes, we would like to have a consensus on the NAT YANG data model and proceed > with its publication in opsawg. > > Cheers, > Med > > > -----Message d'origine----- > > De : OPSAWG [mailto:[email protected]] De la part de Tianran > > Zhou Envoyé : mercredi 26 avril 2017 09:46 À : Senthil Sivakumar > > (ssenthil); [email protected] Objet : Re: [OPSAWG] [Softwires] Comments > > on draft-sivakumar-yang-nat-05 > > > > I am looking at the YANG tree. The overall structure of this module is > > clear and simple. > > In addition to the text improvement, what do you want to discuss and > > get consensus wrt the YANG data model? > > > > Regards, > > Tianran > > > > > -----Original Message----- > > > From: OPSAWG [mailto:[email protected]] On Behalf Of Senthil > > > Sivakumar (ssenthil) > > > Sent: Monday, April 24, 2017 9:44 PM > > > To: [email protected] > > > Subject: [OPSAWG] FW: [Softwires] Comments on > > > draft-sivakumar-yang-nat- > > 05 > > > > > > 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 > > _______________________________________________ > > OPSAWG mailing list > > [email protected] > > https://www.ietf.org/mailman/listinfo/opsawg _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
