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

[Martin Kosek]

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?

I thought the right way for this branch would be to just update selected
attributes of the LDAPEntry read from LDAP (entries[0] in your case) and then
call ldap.update_entry(). This way, the entry gets just partially updated, for
example nsslapd-pluginVersion stays the same.

BTW, I do not think that
+     'nsslapd-pluginId'              : 'none',
+     'nsslapd-pluginVersion'         : 'none',
+     'nsslapd-pluginVendor'          : 'none',
+     'nsslapd-pluginDescription'     : 'none',
should be left to be none even in the first branch.

Given this is the last patch we are waiting for before 4.1 release, I still
think we should reconsider moving it to 4.1.1, given the issues.

Martin