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

[Juergen Schoenwaelder <[e-mail]>]

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/>