[Freeipa-devel] [PATCHES] 0258-0265 Add schema updater based on IPA schema files

[Ana Krivokapic]

On 11/15/2013 05:28 PM, Petr Viktorin wrote:
> On 11/15/2013 02:09 PM, Petr Viktorin wrote:
>> On 11/11/2013 04:18 PM, Ana Krivokapic wrote:
>>> On 11/11/2013 02:53 PM, Ana Krivokapic wrote:
>>>> On 11/11/2013 12:32 PM, Petr Viktorin wrote:
>>>>> On 11/07/2013 02:34 PM, Ana Krivokapic wrote:
>>>>>> On 11/01/2013 03:26 PM, Petr Viktorin wrote:
>>>>>>> On 09/13/2013 06:44 PM, Petr Viktorin wrote:
>>>>>>>> On 08/01/2013 04:52 PM, Petr Viktorin wrote:
>>>>>>>>> Hello,
>>>>>>>>> With these patches, schema updates will be based on the ldif
>>>>>>>>> files we
>>>>>>>>> use for installation.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3454
>>>>>>>>>
>>>>>>>>> This is a RFE, here is the design doc:
>>>>>>>>> http://www.freeipa.org/page/V3/Improved_schema_updater
>>>>>>>>>
>>>>>>>> I found and filed a bug in python-ldap[0]: it sometimes ignores
>>>>>>>> parts of
>>>>>>>> schema LDIFs when parsing them.
>>>>>>>> Patch 0275 works around the bug. Please apply on top of 0258-0265
>>>>>>>> (they
>>>>>>>> still apply cleanly).
>>>>>>>>
>>>>>>>>
>>>>>>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1007820
>>>>>>>>
>>>>>>> The recent ipaldap patches resulted in a small conflict. Attaching
>>>>>>> rebased patches.
>>>>>> I have tested the patches and overall they seem to work fine. Some
>>>>>> questions/comments are below.
>>>>>>
>>>>>>
>>>>>> Patch 258:
>>>>>> You catch `ldap.LOCAL_ERROR` in the `connect()` function, which is
>>>>>> called from `__init__()`, so no need to catch it again in
>>>>>> `__init__()`.
>>>>> I've added a comment instead of the try/catch
>>>>>
>>>>>> Patch 259:
>>>>>> ACK
>>>>>>
>>>>>> Patch 260:
>>>>>>
>>>>>>   >       # Usually the modlist order does not matter.
>>>>>>   >       # However, for schema updates, we want 'attributetypes'
>>>>>> before
>>>>>>   >       # 'objectclasses'.
>>>>>>   >       # A simple sort will ensure this.
>>>>>>   >       modlist.sort()
>>>>>>
>>>>>> Since `modlist` is a list of tuples, it is sorted by the first
>>>>>> elements
>>>>>> in the tuples, then by the seconds elements, etc. Which means the
>>>>>> resulting list will be sorted by the modification type first
>>>>>> (`MOD_ADD`,
>>>>>> `MOD_DELETE`, etc), and by `attributetypes`/`objectclasses` second.
>>>>>> Was
>>>>>> this the desired effect?
>>>>> I've added a sort key; it should be safer now.
>>>> Hmm, the key you added still retains the default sorting behavior -
>>>> it will sort
>>>> by the first elements of the tuples first. Since we need sorting by
>>>> the second
>>>> elements, I think the key here should be: key=lambda m: m[1].lower()
>>
>> Right, I see what you mean.
>> I've made the change and tested it a bit.
>>
>>>>>> Patch 261:
>>>>>> Man page updates look good, but several options in the man page have 3
>>>>>> dashes in the long form instead of 2. I have attached a mini-patch
>>>>>> that
>>>>>> fixes this along with a couple of typos in the man page. Feel free to
>>>>>> squash it to your patch 261.
>>>>> Nice catch! Squashed.
>>>>>
>>>>>> Patch 262:
>>>>>> Whitespace warnings.
>>>>> I didn't see any with my settings; could you be more specific?
>>>> $ git am
>>>> ~/freeipa-pviktori-0262.3-Remove-schema-modifications-from-update-files.patch
>>>>
>>>> Applying: Remove schema modifications from update files
>>>> /home/ana/freeipa/.git/rebase-apply/patch:497: new blank line at EOF.
>>>> +
>>>> /home/ana/freeipa/.git/rebase-apply/patch:693: new blank line at EOF.
>>>> +
>>>> warning: 2 lines add whitespace errors.
>>
>> Thanks! fixed.
>>
>> [...]
>>> I'm also seeing some errors when testing the patches. During
>>> ipa-server-install,
>>> restarting of DS is failing:
>>>
>>>    [22/38]: restarting directory server
>>> ipa         : CRITICAL Failed to restart the directory server (Command
>>> '/bin/systemctl restart dirsrv at IDM-LAB-ENG-BRQ-REDHAT-COM.service'
>>> returned
>>> non-zero exit status 1). See the installation log for details.
>>>
>>>
>>> The dirsrv log indicates that one of the ldif files is not
>>> syntactically valid:
>>>
>>> [11/Nov/2013:16:10:21 +0100] dse_read_one_file - The entry cn=schema
>>> in file
>>> /etc/dirsrv/slapd-IDM-LAB-ENG-BRQ-REDHAT-COM/schema/60basev3.ldif
>>> (lineno: 1) is
>>> invalid, error code 21 (Invalid syntax) - object class
>>> (2.16.840.1.113730.3.8.12.7 NAME 'ipaKrb5DelegationACL' SUP
>>> groupOfPrincipals
>>> STRUCTURAL MAY ( ipaAllowToImpersonate $ ipaAllowedTarget ) EQUALITY
>>> distinguishedNameMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.12 X-ORIGIN
>>> 'IPA v3' ):
>>> Failed to parse objectclass, error(2) at ( distinguishedNameMatch SYNTAX
>>> 1.3.6.1.4.1.1466.115.121.1.12 X-ORIGIN 'IPA v3' ))
>>> [11/Nov/2013:16:10:21 +0100] dse - Please edit the file to correct the
>>> reported
>>> problems and then restart the server.
>>>
>>> Are you seeing this in your setup?
>>
>> No, ipa-server-install works for me. Does this happen every time?
>> I can't find an error in the syntax there.
>
> The bug was only visible on f20 which has a new version of 389.
> Thanks to Nathan Kinder for spotting the mistake (matching rule&syntax on an
> objectClass) for me, and to 389's new schema parser for being nice and strict.
>
>
> I'm only attaching the changed patch.
>

Thanks, it works well now.

ACK for the whole patch set.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.