Re: [I2nsf] [Last-Call] Yangdoctors last call review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-08

[tom petch <[e-mail]>]

On 29/09/2020 17:14, Rafa Marin-Lopez wrote:
> Hi Tom:
>
> Please see our comments inline:
>
>> El 28 sept 2020, a las 11:33, tom petch <[e-mail]> escribió:
>>
>> On 28/09/2020 10:01, Rafa Marin-Lopez wrote:
>>> Hi Rob, Tom:
>>>
>>> Renaming the modules sounds reasonable. With regard to have a different prefix, it is also ok. Perhaps the easiest way is to solve this is the following: instead of using -sdn- we could include -i2nsf- so we would have: ietf-i2nsf-common, ietf-i2nsf-ike, and ietf-i2nsf-ikeless. This would mean that this is in the context of i2nsf wg.
>>>
>>> What do you think?
>>
>> Ok with -ike and -ikeless
>
> Great.
>
>> but I would not use ietf-i2nsf-common the reason being that there is an awful lot in common across the different i2nsf I-D that with hindsight would have made an excellent common i2nsf module and may be will in future if these modules get revised.
>
> We agree.
>> Other WG have created 'common' or 'types' modules, with more of the latter and then abbreviated that to 't' or 'c' so perhaps ietf-i2nsf-iket, or -ikec.
>
> Ok about ietf-i2nsf-ikec.
>
>
>> Prefix ahould be short so something like nsfike, nsfikeless (longer than I would like - nsfiken?) and nsfiket, or nsfikec.
>
> For the prefix to the common module
>
> prefix "nsfikec";
>
>
> For the prefix to the ietf-i2nsf-ike, nsfike seems to refer to IKE case , which is good.
>
> prefix "nsfike”;
>
> For the prefix to the ietf-i2nsf-ikeless, nsfikeless (a shorter one could be nsfikels but we do not know if that is acceptable) has a clear meaning and it is coherent with the rest of the names. I think we can live with a few letters more.
>
> Is this acceptable?

Yes, although I think that a shorter prefix for ikeless might be better. 
  YANG Guidelines does not give a figure but I mentally think going over 
six can be problematic.  There aren't any instances in the module at 
present but it is when you need to refer to an object (e.g. in a 
constraint, notification, augment) and the absolute form of reference is 
needed or recommended and the object is several layers down.  I see six 
layers in ikeless so a ten character prefix would add 60 characters of 
red tape, a six character on 36 characters.  Currently you do not have 
such references.  (I have been looking at CCAMP lately and they go 17 
layers down - fortunately the prefix are mostly two or three character 
but even so, it does make the YANG harder to follow for me; the path for 
a YANG augment or a when can spread over seven lines).

Tom Petch

> Best Regards.
>
>>
>> Tom Petch
>>
>>>> El 23 sept 2020, a las 12:45, tom petch <[e-mail] <mailto:[e-mail]>> escribió:
>>>>
>>>> On 23/09/2020 11:24, Rob Wilton (rwilton) wrote:
>>>>> Hi Rafa,
>>>>>
>>>>> Thanks for replying with the extra background information.
>>>>>
>>>>> It seems like renaming the modules, as you propose for the -09 version, is the pragmatic path forward here.
>>>>
>>>> And the namespace and the YANG prefix IMHO.  Having ike as a prrefix for a module which is not the 'ike' module I see as a source of confusion. If this is nsf specific, then I would suggest nsfike although nsfikeless then seems a bit long.
>>>>
>>>> Tom Petch
>>>>
>>>>> Regards,
>>>>> Rob
>>>>>
>>>>>
>>>>> From: Rafa Marin-Lopez <[e-mail] <mailto:[e-mail]>>
>>>>> Sent: 23 September 2020 10:29
>>>>>
>>>>> Hi Rob, (Chris):
>>>>>
>>>>> Thank you very much for your answers. Please see our comments inline.
>>>>>
>>>>>
>>>>> El 22 sept 2020, a las 17:38, Rob Wilton (rwilton) <rwilton=[e-mail] <mailto:rwilton=[e-mail]><mailto:rwilton=[e-mail] <mailto:rwilton=[e-mail]>>> escribió:
>>>>>
>>>>> Hi Rafa,
>>>>>
>>>>> Thanks for getting back to me.
>>>>>
>>>>> Yes, changing the name of the module is an okay, if not ideal, resolution.  But I appreciate that you also want to be done with this work.
>>>>>
>>>>> Correct, especially at this point of time. We have been discussing this I-D for a long time. There was a consensus in the WG. I2NSF WG chairs can comment about this. In our opinion, the changes that need to be applied are major changes from what it was approved in the WG. However, this is coming in the “last minute” and we feel this is not merely moving a few lines from here to there. We need to understand the impact of this. Therefore, it is not so trivial as it may seem. See below.
>>>>>
>>>>> But I would like to check:  My understanding is that the changes that Chris is proposing are pretty small.  I.e. move the SA structure under ipsec-common, and put it under a YANG feature.  Are you sure that it is impractical to accommodate this change which would allow a single ipsec module to be shared and extended via YANG augmentations?
>>>>>
>>>>> In the context of our I-D, if we move SAD structure to ipsec-common, what we are meaning is that IPsec SA configuration data (setting values to the SAD structure) are common to the IKE case and the IKE-less cases, which are not. It is confusing. Moreover, the usage of feature means that, after all, this “common” is not “common” to both cases IKE case and IKE-less. Again, it seems confusing. In the IKE case, the SDN/I2NSF controller does not configure the SAD at all but the IKE implementation in the NSF. In our opinion, in order to properly add this IPsec SA operational state to the IKE case we should include operational data about the IPsec SAs (config false) to the ietf-ipsec-ike. Alternatively, we have certain operational data (ro) in the SAD structure in the IKE-less case. If only those are moved to the common part should be ok but we think it does not solve the problem.
>>>>>
>>>>> Therefore it is not only an implementation aspect.
>>>>>
>>>>> As you may observe, our discussion is taking into account there is an I2NSF Controller and we have two cases IKE case and IKE-less case, that have been discussed in the WG. The YANG model in the I-D has tried to reflect this discussion. It was never the intention to provide a general YANG model for IPsec.
>>>>>
>>>>> Best Regards.
>>>>>
>>>>> Thanks,
>>>>> Rob
>>>>>
>>>>> From: Rafa Marin-Lopez <[e-mail] <mailto:[e-mail]><mailto:[e-mail] <mailto:[e-mail]>>>
>>>>> Sent: 22 September 2020 14:05
>>>>> Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-i2nsf-sdn-ipsec-flow-protection-08
>>>>>
>>>>> Dear Rob:
>>>>>
>>>>> Apologies for our delayed answer. We are now working in the revision to submit v09 by compiling all the comments.
>>>>>
>>>>> As you mentioned, we want to avoid any further delay. As we mentioned to Chris in the past (i2nsf mailing list), we do not have any problem to include some additional text (e.g. “-sdn-" in the module names). Therefore, Rob, we agree with your point of view about this.
>>>>>
>>>>> In summary, we are working in the next revision v09, and our idea to address Chris’ comments was to include -sdn- to the module names.
>>>>>
>>>>> We hope this is fine.
>>>>>
>>>>> Best regards.
>>>>>
>>>>>
>>>>>
>>>>> El 22 sept 2020, a las 13:56, Rob Wilton (rwilton) <[e-mail] <mailto:[e-mail]><mailto:[e-mail] <mailto:[e-mail]>>> escribió:
>>>>>
>>>>> Hi draft authors, Chris,
>>>>>
>>>>> Can we also please try and close on this issue raised by Chris.
>>>>>
>>>>> Chris, I don’t think that there is any great way to solve this issue using YANG features, but presumably the constraint could be enforced with a must statement, or groupings could be used to copy parts of the ipsec structure into an sdn specific ipsec tree structure.
>>>>>
>>>>> I understand that there isn't any great desire to delay these drafts by trying to generalize the ipsec YANG model contained within it.  However, I think that means that the modules should have "-sdn-" in their names to indicate that they are intended specifically for the SDN use case, and should not be confused with the more generic ipsec YANG modules that have been proposed.
>>>>>
>>>>> Regards,
>>>>> Rob
>>>>>
>>>>> -----Original Message-----
>>>>> From: yang-doctors <[e-mail] <mailto:[e-mail]><mailto:[e-mail] <mailto:[e-mail]>>> On Behalf Of Christian
>>>>> Hopps
>>>>> Sent: 24 August 2020 18:08
>>>>>
>>>>> [adding in ipsec@]
>>>>>
>>>>> Hi,
>>>>>
>>>>> This draft was discussed in ipsecme at the last IETF, and there was a
>>>>> desire to look closer at a couple changes that would make these models
>>>>> usable by ipsec generally rather than only for SDNs. Otherwise we will end
>>>>> up with 2 models that look very similar and duplicate almost all the
>>>>> functionality. This was going to be done during the next yang doctor
>>>>> review, but it looks like that happened in the meantime (ships in the
>>>>> night).
>>>>>
>>>>> At minimum the module names should include "-sdn-" if no other changes are
>>>>> made to indicate that they are only for sdn use; however, this is not the
>>>>> optimal solution.
>>>>>
>>>>> A better solution would be to move the containers currently under ikeless
>>>>> (for SA and Policy databases) under ipsec-common.
>>>>>
>>>>> The feedback I received from the authors was that the SDN controllers
>>>>> didn't care about the actual SAs and policies when using IKE so they
>>>>> didn't want to require someone implementing ike+common modules to have to
>>>>> support them.
>>>>>
>>>>> The YANG question I suppose is, is there an easy way to move these
>>>>> containers from ipsec-ikeless to ipsec-common, but still allow for them to
>>>>> be empty and/or unimplemented for the SDN IKE use case? If they were made
>>>>> features, is there a proper YANG way to indicate that if the ikeless
>>>>> module is present then those features must also be supported thus matching
>>>>> the functionality as defined by the current draft?
>>>>>
>>>>> Thanks,
>>>>> Chris.
>>>>>
>>>>> On Aug 24, 2020, at 10:37 AM, Martin Björklund via Datatracker
>>>>> <[e-mail] <mailto:[e-mail]><mailto:[e-mail] <mailto:[e-mail]>>> wrote:
>>>>>
>>>>> Reviewer: Martin Björklund
>>>>> Review result: Ready with Nits
>>>>>
>>>>> I did an early YANG Doctor's review of this draft.  Most of my
>>>>> comments then have been addressed in this version.
>>>>>
>>>>> Comments:
>>>>>
>>>>> o  As I wrote in my early review, the RFC editor enforces a common
>>>>>   format of YANG modules, so it is better to adhere to this format
>>>>>   before sending the draft to the RFC editor.  Use
>>>>>
>>>>>     pyang -f yang --yang-line-length 69 <FILE>
>>>>>
>>>>>   to get a consistent look-and-feel for your module.
>>>>>
>>>>>   (You will have to manually re-flow description statements after
>>>>>   this.)
>>>>>
>>>>>
>>>>> o  There are some leafs that are optional in the model, but w/o a
>>>>>   default value and w/o an explanation of what happens if that leaf
>>>>>   is not set.  You should find those and either make them mandatory,
>>>>>   add a default value, or explain what it means when it isn't set.
>>>>>   As an example,
>>>>>   /ipsec-ike/pad/pad-entrypeer-authenticatin/pre-shared/secret
>>>>>   is optional.  I suspect that this leaf needs to be mandatory.
>>>>>   Another example is the leaf espencap.
>>>>>
>>>>>
>>>>> /martin
>>>>>
>
> -------------------------------------------------------
> Rafa Marin-Lopez, PhD
> Dept. Information and Communications Engineering (DIIC)
> Faculty of Computer Science-University of Murcia
> 30100 Murcia - Spain
> Telf: [phone] Fax: [phone] e-mail: [e-mail]
> -------------------------------------------------------
>
>
>
>
>