Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-nat-yang-06

[<[e-mail]>]

Hi Juergen,

Thank you for the follow-up and for the suggestions. 

I made the following changes to address your comments:

- Revert back to the use of features.
- Leave out the logging container for a future extension.
- Remove the "VRF" part from the module, but maintain an extensible design to easily support VRF in the future.  

FWIW, the updated version of the module which integrates those comments is available at: 
https://github.com/boucadair/draft-ietf-opsawg-nat-yang/blob/master/ietf-nat%402017-11-06.yang 

Unless you have any further comment, I'm planning to submit this version early next week. 

Cheers,
Med

> -----Message d'origine-----
> De : Juergen Schoenwaelder [mailto:[e-mail]]
> Envoyé : mercredi 1 novembre 2017 11:04
> À : BOUCADAIR Mohamed IMT/OLN
> Cc : [e-mail]; [e-mail];
> [e-mail]
> Objet : Re: Yangdoctors early review of draft-ietf-opsawg-nat-yang-06
> 
> On Tue, Oct 31, 2017 at 09:55:46AM +0000, [e-mail]
> wrote:
> > > >  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.
> > > >
> > > > [Med] We used to make use of "features", but there was a comment
> during
> > > the WG call for adoption that requested us to make use of identities,
> > > instead.
> > >
> > > But these are not the same thing.
> >
> > [Med] You are right. Sorry for not being clear.
> >
> > What I meant is that the module used, for example, "if-feature nat64"
> and so on to refer to NAT flavors. We replaced those with "when"
> statements that points to the actual capabilities of an implementation.
> > Another change we made, is that we used to have "Boolean" to indicate
> the support of a given tarnation scheme (e.g., nat64-support, nat44-
> support), but we updated those to make use of identities.
> 
> I do not recall having seen these when expressions. But even if there
> were when expressions, this does not the same as features. A when
> expression puts a constraint on a valid configuration, a feature
> expresses that not all parts of a model may be implemented. This is
> implementation time partitioning vs. configuration time partitioning.
> 
> >  If I am not a nat64, then apparently
> > > several objects to not apply to me.
> >
> > [Med] Yes. We tried to cover this by "when" statements.
> >
> >  This is what features allow you to
> > > express. Perhaps someone wanted identities for something different, I
> > > do not know. I think it is reasonable to assume that not all NAT
> > > implementations will support all types of NATs that the model supports
> > > and hence the model should reflect this.
> > >
> >
> > [Med] The module indicates what a given implementation has to support
> based on its capabilities. This is handled by means of "when" statements.
> 
> This seems to be a mis-understanding of what when expressions do. See
> above for the explanation.
> 
> > > > > - What is the vrf-routing-instance identity good for?
> > > >
> > > > [Med] This is used to bind a NAT function to an external VRF routing
> > > instance. Please check https://www.ietf.org/mail-
> > > archive/web/opsawg/current/msg05117.html
> > > >
> > >
> > > I very much doubt that an identity identifies a VRF instance. I am not
> > > questioning the need to identify a VRF, I am questioning that the
> > > solution put in place achieves this.
> >
> > [Med] Thank you for clarifying. This is actually a good point. The
> options we considered are as follows:
> >
> > (1) Use an Identity to avoid struggling with semantics of how a VRF can
> be defined.
> > (2) Point to draft-ietf-rtgwg-ni-model, likely (?) as a normative
> reference.
> > (3) Remove the VRF thing from the draft; future extensions can be
> defined.
> >
> > Given that "VRFs" are not required for all NATs and some reviewers asked
> to add VRFs in addition to interfaces, we went for option (1). Having a
> dependency on a YANG module under development is not justified for all
> implementations.
> >
> 
> But (1) is broken as far as I can tell. You either work out with the
> routing guys how to properly refer to a VRF or you better leave this
> out.
> 
> > > If leafs are not configurable, then they should be config false.
> >
> > [Med] I will revert config to false as we used to have in previous
> versions. As I said earlier, pyang will cry. I don't know how to fix that.
> >
> 
> Then ask for help including the error message. Marking something that
> is conceptually "config false" as "config true" is pretty misleading.
> 
> > [Med] The current module allows for implementations that support many
> translation flavors. In particular, it allows for a same NAT instance to
> enable many features in the same time.
> >
> > There is no need to indicate explicitly the translation scheme(s) to
> enable because this will be implicitly deduced from the configuration
> parameters. For example:
> > - an implementation supplied with an external IPv4 address pool only,
> will automatically behave in the base NAT mode.
> > - an implementation supplied with an external IPv4 address pool and
> port-related parameters will behave in the NAPT mode.
> > - an implementation supplied with an external IPv4 address pool, port-
> related parameters, and NAT64 prefixes will behave in the statefull NAT64
> mode
> > - an implementation supplied with an external IPv4 address pool, port-
> related parameters, dst-nat-enable=true, and dst-ip-address-pool will
> behave in the NAPT mode + destination NAT
> >
> > and so on.
> 
> As long as this is clear from the specification. One option you may
> consider is to use presence containers that can carry the bit when to
> enable / disable a specific NAT function. (Although personally, I am
> not such a big fan of presence containers, perhaps since I am not a
> fan of side effects in general).
> 
> > > > [Med] Yes.
> > > >
> > > > >
> > > > > - 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.
> > > > >
> > > >
> > > > [Med] It is out of scope of specify the exact logging information.
> Those
> > > considerations are out of scope. We only focus on the protocol and the
> > > server information.
> > >
> > > I think my comment was saying that the information is not sufficient
> to
> > > talk to a server.
> >
> > [Med] It is only about the minimal set of information. The information
> is not complete on purpose. Protocol-specific information is out of scope
> of this document.
> 
> I am not sure it is valuable to have a standards-track specification
> for something that is incomplete up the point that it can't be used if
> implemented without annotations and extensions, i.e., the whole thing
> can't be interoperable unless there is a standards-track extension. So
> I would rather not standardize this at all and wait until someone is
> willing to work out the details.
> 
> /js
> 
> --
> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   [phone]         <http://www.jacobs-university.de/>