Re: [OPSAWG] Comments on draft-sivakumar-yang-nat-05

["Senthil Sivakumar (ssenthil)" <[e-mail]>]

Please see inline for [Senthil]

On 6/7/17, 7:33 AM, "[e-mail]" <[e-mail]> wrote:

    Hi Tianran, 
    
    Thank you for the comments. 
    
    Please see inline. 
    
    Cheers,
    Med
    
    > -----Message d'origine-----
    > De : Tianran Zhou [mailto:[e-mail]]
    > Envoyé : jeudi 18 mai 2017 10:11
    > À : BOUCADAIR Mohamed IMT/OLN; Senthil Sivakumar (ssenthil);
    > [e-mail]
    > Cc : Ian Farrer
    > Objet : RE: Comments on draft-sivakumar-yang-nat-05
    > 
    > 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
    > 
    
    [Med] OK with the use of name. 

[Senthil] The instance ID is consistent with RFC 7659, draft-ietf-ipfix-nat-logging and syslog-logging.
Natv2InstanceIndex in RFC 7659 states that :
DESCRIPTION
        "A unique value, greater than zero, for each NAT instance
         in the managed system.  It is RECOMMENDED that values are
         assigned contiguously starting from 1.  The value for each
         NAT instance MUST remain constant at least from one
         update of the entity's natv2InstanceDiscontinuityTime
         object until the next update of that object.  If a NAT
         instance is deleted, its assigned index value MUST NOT
         be assigned to another NAT instance at least until
         reinitialization of the entity's management system."
    SYNTAX Unsigned32 (1..4294967295)

You can also have an alias for the instance ID which is a string
natv2InstanceAlias OBJECT-TYPE
    SYNTAX DisplayString (SIZE (0..64))
    MAX-ACCESS read-only
    STATUS current
    DESCRIPTION
        "This object is an 'alias' name for the NAT instance as
         specified by a network manager and provides a non-volatile
         'handle' for the instance.

The instance ID is a mandatory field but you can use an alias, if that is what you are asking, 
We can add that. Maybe we should add that to keep it consistent with the MIB RFC.

    
    > 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
    > 
    
    [Med] The external IP address pool can be a contiguous or a set of non-contiguous IP prefixes. As such, It covers the scheme you are proposing.  
    
    > 3. Did you consider the NAT server or dest_address NAT Scenario?
    
    [Med] I'm not sure to understand the question. If you meant, whether the model supports rewriting the destination address, then the answer is Yes. This is for example useful for NAT64.  
    
    > 
    > 4. Is this "port-quota" stands for the number of ports that can be used by
    > a nat instance?
    
    [Med] It corresponds to the maximum number of ports that can be used by a given subscriber. See this text excerpt from the draft:
    
    "Configures a port quota to be assigned per subscriber.";
    
     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.
    
    [Med] This may be flexible, but I'm afraid there is no point in customizing the port configuration per address pool. A global port configuration is used by NATs/CGNs.
    
    > 
    > 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
    > 
    
    [Med] I will defer to Senthil on this one. 

[Senthil] Yes, I believe Dan Wing also pointed this out. I will add this to the next rev.

    > 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
    
    [Med] The mapping table is used for dynamic mapping. The draft says the following:
    
       The NAT data model is designed to cover both configuration and state
       retrieval, nevertheless this document covers dynamic (implicit)
       mapping while PCP-related functionality to instruct dynamic explicit
       mapping is defined in [I-D.boucadair-pcp-yang].
    
    > 
    > 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.
    
    [Med] Point taken.  
    
[Senthil] Agree with Med.

Thanks
Senthil
    > 
    > 
    > > -----Original Message-----
    > > From: [e-mail] [mailto:[e-mail]]
    > > Sent: Wednesday, April 26, 2017 4:18 PM
    > > To: Tianran Zhou; Senthil Sivakumar (ssenthil); [e-mail]
    > > 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:[e-mail]] De la part de Tianran
    > > > Zhou Envoyé : mercredi 26 avril 2017 09:46 À : Senthil Sivakumar
    > > > (ssenthil); [e-mail] 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:[e-mail]] On Behalf Of Senthil
    > > > > Sivakumar (ssenthil)
    > > > > Sent: Monday, April 24, 2017 9:44 PM
    > > > > To: [e-mail]
    > > > > 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)"
    > > > <[e-mail]>
    > > > > 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)"
    > > > > <[e-mail] on behalf of [e-mail]> 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 [e-mail] 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
    > > > >         [e-mail]
    > > > >         https://www.ietf.org/mailman/listinfo/softwires
    > > > >
    > > > >
    > > > >
    > > > >
    > > > > _______________________________________________
    > > > > OPSAWG mailing list
    > > > > [e-mail]
    > > > > https://www.ietf.org/mailman/listinfo/opsawg
    > > > _______________________________________________
    > > > OPSAWG mailing list
    > > > [e-mail]
    > > > https://www.ietf.org/mailman/listinfo/opsawg