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

[Ana Krivokapic]

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()
>
>>> 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.
>
>
>>> In `60-trusts.update` there are still some `replace:attributeTypes:`
>>> lines. Can those be removed safely?
>> Yes! I've checked they match the ldif, and removed them.
>>
>>> Patch 263:
>>>
>>> +                if not force_replace:
>>> +                    modlist.append((ldap.MOD_DELETE, key, removes))
>>> +                elif new_values == []: # delete an empty value
>>> +                    modlist.append((ldap.MOD_DELETE, key, removes))
>>>
>>> can be combined into one:
>>>
>>> +                if not force_replace or not new_values:
>>> +                    modlist.append((ldap.MOD_DELETE, key, removes))
>> Done
>>
>>> Patch 264:
>>> ACK
>>>
>>> Patch 265:
>>> ACK
>>>
>>> Patch 275:
>>> ACK
>> Thanks for the review!
>> Updated patches attached.
>>
>>
>

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?

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.