[Freeipa-devel] [PATCHES] 213-224 Use old entry state in LDAP mods

[Petr Viktorin]

On 12/18/2013 12:50 PM, Jan Cholasta wrote:
> On 11.12.2013 18:28, Petr Viktorin wrote:
>> On 12/11/2013 05:05 PM, Petr Viktorin wrote:
>>> On 12/10/2013 02:10 PM, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> the attached patches fix
>>>> <https://fedorahosted.org/freeipa/ticket/3488>.
>>>>
>>>> Honza
>>>
>>> These look great, thanks! Just a couple of questions/nicpicks.
>>>
>>> 213: ACK
>>> 214: ACK
>>> 215: ACK
>>> 216: ACK
>>> 217: ACK
>>> 218: ACK
>>>
>>> 219:
>>> Does the new method guarantee 'attributetypes' are updated before
>>> 'objectclasses'?
>>> I can move the logic to schemaupdate if needed.
>
> That won't be necessary, fixed.
>
>>>
>>> Why is OnlyOneValueAllowed not raised any more? It should be, since the
>>> method assumes get_single_value() gives correct information.
>
> Fixed.
>
>>>
>>> Why is MOD_REPLACE no longer used when no old values are retained? Or
>>> (MOD_DELETE, a, None) when the new set is empty?
>
> Fixed.
>
> However, I am not convinced if this guessing is the right approach. I
> think we can do better, by either not guessing:
>
>      del entry[attr] -> delete attribute
>      entry[attr] = value -> replace attribute
>      entry[attr][index] = value -> delete old value, add new value
>
> or by guessing better, to minimize the size of the modify request.
>
>>> 220: ACK
>>>
>>> 221:
>>> An ancient comment in ldapupdate still has a reference to orig_data in,
>>> can you remove the comment as well?
>
> Sure.
>
>>>
>>> 222: ACK
>>> 223: ACK
>>> 224: ACK
>>
>> I spoke too soon, NACK. This line:
>>
>>  > -from ipaserver.plugins import ldap2
>>
>> is important, please put it back. Without it api.Backend.ldap2 will not
>> be available and upgrades will fail with AttributeError.
>>
>> (Yes, I know.)
>
> Fixed. (But it's retarded.)
>
>>
>>
>>> I noticed that IPASimpleLdapObject.convert_result's docstring is
>>> obsolete, could you update/shorten it?
>
> Sure.
>
> Also fixed some more issues I noticed.

It would be nice to write unit tests for issues as you notice them.

> Updated patches attached.


I now have a failing test in test_permission_rollback. Let's think about 
this case for a moment:

The permission system has "rollback": if an ACI update fails, the entry 
is rolled back. Currently it works (for ipapermlocation changes) like this:

- The old entry is retreived
- A new entry is populated (NB, the framework's mod operation does not 
retrieve the entry it modifies; rather it builds an entirely new entry 
with *only* the data that's changed, and relies on generate_modlist 
doing MOD_REPLACE when orig data is missing).
- update is called on the new entry
- The ACI is updated, and this fails (e.g. with SyntaxError)
- update is called on the *old* entry retreived in the first step. Up to 
now this had restored the entry (since existing state was looked up 
before each mod), but with these patches it raises EmptyModlist since 
the object has not changed relative to its orig data.

Obviously this approach is wrong given how entry is supposed to work 
now, and I'll be happy to change it it. But it's not clear what's the 
right way to do such rollback.

I'd like to discuss in person after the holidays, so we sync our 
expectations of how ipaldap should work.

-- 
Petr³