[Freeipa-devel] [PATCH] 298 Add safe updates for objectClasses

[Rob Crittenden]

Martin Kosek wrote:
> On 09/05/2012 09:22 AM, Martin Kosek wrote:
>> On 09/05/2012 03:47 AM, Rob Crittenden wrote:
>>> Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> On 08/30/2012 02:53 PM, Rob Crittenden wrote:
>>>>>> Martin Kosek wrote:
>>>>>>> Current objectclass updates in a form of "replace" update instruction
>>>>>>> dependent on exact match of the old object class specification in the
>>>>>>> update instruction and the real value in LDAP. However, this
>>>>>>> approach is
>>>>>>> very error prone as object class definition can easily differ as for
>>>>>>> example because of unexpected X-ORIGIN value. Such objectclass update
>>>>>>> failures may lead to serious malfunctions later.
>>>>>>>
>>>>>>> Add new update instruction type "replaceoc" with the following format:
>>>>>>> replaceoc:OID:new
>>>>>>> This update instruction will always replace an objectclass with
>>>>>>> specified OID with the new definition.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/2440
>>>>>>
>>>>>> This works ok. Martin and I had a conversation in IRC about it.
>>>>>>
>>>>>> This moves from replacing a specific bit of schema with a new one, in
>>>>>> all
>>>>>> cases. I wonder if we should be more conservative and know what we're
>>>>>> replacing
>>>>>> in advance.
>>>>>>
>>>>>> rob
>>>>>>
>>>>>
>>>>> You are right, I was too harsh when replacing the objectclasses. This
>>>>> would
>>>>> cause issues when LDAP update would be run on a replica with lower
>>>>> version and
>>>>> older objectclass definitions.
>>>>>
>>>>> I came up with an alternative solution and instead of always replacing
>>>>> the
>>>>> objectclass I rather reverted to old-OC:new-OC style which should be
>>>>> safer.
>>>>> Now, the LDAP updater always normalizes an objectclass before
>>>>> comparing it
>>>>> using python-ldap objectclass model. With this approach, objectclasses
>>>>> differing only in X-ORIGIN or white spaces should match and be updated.
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> NACK
>>>>
>>>> I think this:
>>>>
>>>> +                            for value in replaced_values:
>>>> +                                entry_values.remove(old)
>>>>
>>>> Should be:
>>>>
>>>> +                                entry_values.remove(value)
>>
>> Right. Thanks for the fix. It only worked in my testing because I had no
>> objectclass update which would differ in X-ORIGIN or whitespaces or case. I
>> tried to mangle an update file and with this fix it did the update even if
>> X-ORIGIN and whitespaces differed.
>>
>>>>
>>>> I'm still doing other testing but this is what I've found so far.
>>>>
>>>> rob
>>>
>>> I did some more testing and it looks like this will do the trick.
>>>
>>> I also found a place where the schema was left as unicode and causing it to
>>> blow up inside python-ldap. Here is the diff on my working instance:
>>>
>>> diff -u  ipaserver/install/ldapupdate.py
>>> /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py
>>> --- ipaserver/install/ldapupdate.py     2012-09-04 16:59:33.210688723 -0400
>>> +++ /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py 2012-09-04
>>> 21:47:01.583574375 -0400
>>> @@ -643,7 +643,7 @@
>>>                                   self.debug('replace: no match for replaced
>>> ObjectClass "%s"', old)
>>>                                   continue
>>>                               for value in replaced_values:
>>> -                                entry_values.remove(old)
>>> +                                entry_values.remove(value)
>>>                           else:
>>>                               entry_values.remove(old)
>>>                           entry_values.append(new)
>>> @@ -772,7 +772,11 @@
>>>                   updated = False
>>>                   changes = self.conn.generateModList(entry.origDataDict(),
>>> entry.toDict())
>>>                   if (entry.dn == DN(('cn', 'schema'))):
>>> -                    updated = self.is_schema_updated(entry.toDict())
>>> +                    d = dict()
>>> +                    e = entry.toDict()
>>> +                    for k,v in e.items():
>>> +                        d[k] = [str(x) for x in v]
>>> +                    updated = self.is_schema_updated(d)
>>>                   else:
>>>                       if len(changes) >= 1:
>>>                           updated = True
>>>
>>> rob
>>>
>>
>> I did not hit this error during testing, but at least it won't harm. Sending an
>> updated patch.
>>
>
> Sending a slightly updated patch which now makes sure that we always pass
> objectclass of str (and not unicode) type to ObjectClass model. This should
> make it more bullet-proof.
>
> Martin
>


ACK, pushed to master and ipa-3-0

rob