[Freeipa-devel] [PATCH, 4.1] 0166 updater: enable uid uniqueness plugin for posixAccount objects

[Martin Kosek]

On 10/21/2014 01:28 PM, Jan Cholasta wrote:
> Dne 21.10.2014 v 13:10 Martin Kosek napsal(a):
>> On 10/21/2014 12:41 PM, Alexander Bokovoy wrote:
>>> On Tue, 21 Oct 2014, Alexander Bokovoy wrote:
>>>> On Tue, 21 Oct 2014, Martin Kosek wrote:
>>>>> On 10/21/2014 08:41 AM, Alexander Bokovoy wrote:
>>>>>> On Tue, 21 Oct 2014, Martin Kosek wrote:
>>>>>>> On 10/20/2014 08:25 PM, Alexander Bokovoy wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> This patch is for ipa-4-1 branch to enable uniqueness plugin for uid
>>>>>>>> attribute for entries with objectclass posixAccount.
>>>>>>>>
>>>>>>>> We don't have uid uniqueness enforced in FreeIPA < 4.1 yet but for
>>>>>>>> posixAccounts it worked due to our design of a flat tree: as uid
>>>>>>>> attribute is
>>>>>>>> part of the DN, renaming user entries
>>>>>>>> enforces uniqueness as MODRDN will fail if entry with the same uid
>>>>>>>> already exists.
>>>>>>>>
>>>>>>>> However, it is not enough for ID views -- we should be able to allow
>>>>>>>> ID view overrides for the same uid across multiple views and we should
>>>>>>>> be able to protect uid uniqueness more generally too.
>>>>>>>>
>>>>>>>> Implementation is done via update plugin that checks for existing uid
>>>>>>>> uniqueness plugin and if it is missing, it will be added. If plugin
>>>>>>>> exists, its configuration will be updated.
>>>>>>>>
>>>>>>>> I haven't added update specific to git master where staging subtree is
>>>>>>>> added but I'll do that after FreeIPA 4.1 release as in 4.1 we don't yet
>>>>>>>> have the staging subtree. Currently master has broken setup for uid
>>>>>>>> uniqueness plugin that doesn't actually work anyway so it will be easier
>>>>>>>> to add upgrade over properly configured entry.
>>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/4636
>>>>>>>
>>>>>>> Hi Alexander,
>>>>>>>
>>>>>>> Thanks for the patch! However, I am personally not very confident with
>>>>>>> merging
>>>>>>> it right before 4.1 release, I thought it will be a simple update
>>>>>>> definition
>>>>>>> while this is a complex upgrade script which needs to be properly tested.
>>>>>>>
>>>>>>> I would rather wait for 4.1.x, especially given it does not block any 4.1
>>>>>>> major
>>>>>>> feature in any way.
>>>>>> I disagree on it for multiple reasons and one of them is that 'a simple
>>>>>> update definition' is not right here. Attribute uniqueness plugin
>>>>>> supports three different types of setting its own arguments. These types
>>>>>> aren't mixable, you have to do switch from one to another. That's why
>>>>>> update plugin is the correct approach here.
>>>>>>
>>>>>> The update plugin I've wrote is very simple by itself.
>>>>>
>>>>> Ok, reviewing the patch now. I found 2 issues:
>>>>>
>>>>> 1) It is missing in plugins' Makefile.am so it won't be built properly
>>>>>
>>>>> 2) Instead of
>>>>> +        if len(entries) == 0:
>>>>> use
>>>>> +        if entries:
>>>> You mean 'if not entries:'? Will fix.
>>>>
>>>>
>>>>> 3) I think we should force "uid uniqueness" plugin creation in all cases so
>>>>> that we can expect it is there in future and for example do simple updates
>>>>> for it.
>>>> It does enforce it creating already. The only case that is not handled
>>>> right now is the case of already existing multiple uid uniqueness
>>>> plugin instances which I explicitly want to handle in git master and
>>>> then merge back to 4.1 as a backport -- we only have any chance to
>>>> encounter this case with current git master.
>>>>
>>>>> So if existing active UID uniqueness is there, should we just remove uid from
>>>>> it's list of watched attributes, give warning and add our own controlled
>>>>> plugin?
>>>> No. There could be multiple uid uniqueness plugin instances, looking for
>>>> different subtrees, potentially with different top level object classes
>>>> and different object class filtering.
>>>>
>>>>> I am just trying to get rid of clashes with custom user configuration.
>>>> I understand that, I'll handle it in git master version. Right now we
>>>> didn't set up any of uid uniqueness and chances touching any custom
>>>> configuration like that are low, looking into my experience with
>>>> existing deployments.
>>>>
>>>>
>>>>> 4) Because of 3), when I enabled "attribute uniqueness" plugin before
>>>>> upgrade,
>>>>> it did a strange franken-update and now I have an update plugin with DN
>>>>> "cn=attribute uniqueness,cn=plugins,cn=config" but "cn: uid uniqueness:
>>>>> inside.
>>>> Good catch. I'll fix this part.
>>>>
>>>>> (BTW these concerns is where my simplicity expectations came from ;-)
>>>> Note that now you realize the whole deal is not that simple as you
>>>> thought. ;)
>>>>
>>>> New patch attached.
>>> Missed Makefile.am part.
>>>
>>
>> The
>> +        if not entries:
>> path works looks ok.
>>
>> However, I am still concerned about the situation when you detect existing UID
>> plugin.
>>
>> You basically create a new LDAP entry with the same DN and then try to
>> overwrite the original one. I do not think this is how the LDAP framework
>> works. Petr/Honza, is it?
> 
> It will work, but updating the old entry object and calling update_entry on it
> would be nicer.
> 
> The difference is that with the current approach, the generated modlist will
> contain MOD_REPLACE for every attribute in the entry, whereas updating the
> original entry will result in a modlist with MOD_ADD/MOD_DELETE only for the
> attributes which were actually modified.

Ok. I also found out that the attributes I objected about are controlled by DS,
so there is no problem with the none values.

ACK then.

Pushed to:
master: eb4d559f3ba0f22e4ea00311df4b9471b187c6b3
ipa-4-1: 2bc287479e728e342a7a5ca4af2757931b257d02

Martin