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

[Martin Kosek]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-298-3-add-safe-updates-for-objectclasses.patch
Type: text/x-patch
Size: 6758 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120905/59b7744a/attachment.bin>