[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin

[Martin Kosek]

On 10/16/2014 11:53 PM, Nathaniel McCallum wrote:
> On Thu, 2014-10-16 at 21:02 +0200, Martin Kosek wrote:
>> On 10/15/2014 09:22 AM, Martin Kosek wrote:
>>> On 10/14/2014 09:01 PM, Nathaniel McCallum wrote:
>>>> On Thu, 2014-10-09 at 18:48 +0200, thierry bordaz wrote:
>>>>> On 10/09/2014 05:51 PM, Nathaniel McCallum wrote:
>>>>>
>>>>>> On Thu, 2014-10-09 at 11:44 +0200, thierry bordaz wrote:
>>>>>>> On 10/09/2014 12:15 AM, Nathaniel McCallum wrote:
>>>>>>>
>>>>>>>> On Wed, 2014-10-08 at 17:19 -0400, Simo Sorce wrote:
>>>>>>>>> On Wed, 08 Oct 2014 15:53:39 -0400
>>>>>>>>> Nathaniel McCallum <npmccallum at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> As I understand my code, all servers will have csnD. Some servers will
>>>>>>>>>> have valueB and others will have valueD, but valueB == valueD.
>>>>>>>>>>
>>>>>>>>>> We *never* discard a CSN. We only discard the counter/watermark mods
>>>>>>>>>> in the replication operation.
>>>>>>>>> What Thierry is saying is that the individual attributes in the entry
>>>>>>>>> have associate the last CSN that modified them. Because you remove the
>>>>>>>>> mods when ValueD == ValueB the counter attribute will not have the
>>>>>>>>> associated CSN changed. But it doesn't really matter because the plugin
>>>>>>>>> will always keep things consistent.
>>>>>>>> Attached is a new version. It removes this optimization. If server X has
>>>>>>>> valueB/csnB and receives valueD/csnD and valueB == valueD, the
>>>>>>>> replication will be applied without any modification. However, if valueB
>>>>>>>>> valueD and csnD > csnB, the counter mods will still be stripped.
>>>>>>>> It also collapses the error check from betxnpre to bepre now that we
>>>>>>>> have a fix for https://fedorahosted.org/389/ticket/47919 committed. The
>>>>>>>> betxnpre functions are completely removed. Also, a dependency on 389
>>>>>>>> 1.3.3.4 (not yet released) is added.
>>>>>>>>
>>>>>>>> Nathaniel
>>>>>>> Hello Nathaniel,
>>>>>>>
>>>>>>>          For me the code is fine. Ack.
>>>>>> New attached patch.
>>>>>>
>>>>>>>          I have two minor comments:
>>>>>>>                * in preop_mod, when a direct update moves the counter
>>>>>>>                  backward you send UNWILLING to perform with a message.
>>>>>>>                  The message is allocated with slapi_ch_smprintf, you
>>>>>>>                  may free it with slapi_ch_free_string (rather than
>>>>>>>                  'free').
>>>>>> Fixed.
>>>>>>
>>>>>>>                * About this message, for example when you have these
>>>>>>>                  MODS (I admit they make no sens):
>>>>>>>
>>>>>>>                  changetype: modify
>>>>>>>                  ipatokenHOTPcounter: MOD_DELETE
>>>>>>>                  -
>>>>>>>                  ipatokenHOTPcounter: MOD_INCREMENT
>>>>>>>
>>>>>>>                  The returned message will be "Will not decrement
>>>>>>>                  ipatokenHOTPcounter", because 'simulate' will return
>>>>>>>                  'COUNTER_UNSET+1'.
>>>>>>>                  Is it the message you expected ?
>>>>>> I changed the logic in simulate(). Please review it.
>>>>>>
>>>>>> Nathaniel
>>>>>>
>>>>> Hello Nathaniel,
>>>>>
>>>>>
>>>>>          The patch is ok for me. Ack.
>>>>
>>>> Since the ACK, the upstream 389 fix actually landed in 1.3.3.5. This
>>>> patch changes nothing except the dependency version. I have tested it
>>>> against the 1.3.3.5 build.
>>>>
>>>> Nathaniel
>>>
>>> Great! As soon as the new build land in Fedora 21 (and we add it to our Copr),
>>> the patch can be pushed.
>>>
>>> Martin
>>
>> Ok, 1.3.3.5 is in koji and our Copr repo.
> 
> +1
> 
>> I almost pushed the patch, but I just 
>> spotted you forgot to solve the upgrades - so NACK.
> 
> You asked me to do that in another patch, not this one. :)

I know, but that does not change the fact it is still missing :-)

> 
>> "cn=IPA OTP Counter,cn=plugins,cn=config" plugin configuration also needs to be 
>> added in some update file.
> 
> So I'm generally confused then. If we have to add the plugin config to
> an update file, why bother creating
> daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif or
> modifying ipaserver/install/dsinstance.py at all? Should these changes
> be removed?

Good question. Most LDAP update related changes are added to update plugins
only. But plugin registration seemed to be still on 2 places, like 'cn=IPA
Range-Check,cn=plugins,cn=config' registration.

Up to you/additional developer discussion.

Martin