[Freeipa-devel] [PATCH] 1088 Recover DNA ranges when deleting a master

[Rob Crittenden]

Petr Viktorin wrote:
> On 03/06/2013 09:52 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
> [...]
>>> On new installs, the ACI on cn=Posix IDs,cn=Distributed Numeric
>>> Assignment Plugin,cn=plugins,cn=config is added before the entry itself.
>>> I didn't test everything as I didn't get the access.
>>
>> It shouldn't make a difference. What isn't working?
>
> I get a CRITICAL message in the log:
>
> add aci:
>          (targetattr=dnaNextRange || dnaNextValue ||
> dnaMaxValue)(version 3.0;acl "permission:Modify DNA Range";allow (write)
> groupdn = "ldap:///cn=Modify DNA
> Range,cn=permissions,cn=pbac,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com";)
>
> modifying entry "cn=Posix IDs,cn=Distributed Numeric Assignment
> Plugin,cn=plugins,cn=config"
>
> 2013-03-07T11:01:48Z DEBUG stderr=ldap_initialize(
> ldap://vm-081.idm.lab.eng.brq.redhat.com:389/??base )
> ldap_modify: No such object (32)
>
> 2013-03-07T11:01:48Z CRITICAL Failed to load replica-acis.ldif: Command
> '/usr/bin/ldapmodify -v -f /tmp/tmpT55upM -H
> ldap://vm-081.idm.lab.eng.brq.redhat.com:389 -x -D cn=Directory Manager
> -y /tmp/tmplFeere' returned non-zero exit status 32
>

Gotcha. I moved where the replica acis are loaded.

> This particular ACI is added later by the updater. But, after failing
> here the rest of the file is skipped, and the subsequent ACIs aren't
> added in updates. Not being able to run CLEANALLRUV prevents cleanly
> deleting a replica.
>
> [...]
>>> [...]
>>>> +    try:
>>>> +        repl = replication.ReplicationManager(realm, hostname,
>>>> dirman_passwd)
>>>> +    except Exception, e:
>>>> +        sys.exit("Connection failed: %s" %
>>>> ipautil.convert_ldap_error(e))
>>>
>>> ipaldap should convert LDAP errors to IPA ones, there's no need to call
>>> convert_ldap_error. Same in other places.
>>
>> It does in some but it isn't consistent. I removed my calls though.
>>
>> $ ipa-replica-manage dnarange-show -p badpassword
>> Connection failed: {'desc': 'Invalid credentials'}
>
> That's a bug. My patch 0194 should fix this, I'll check after it's
> merged. Anyway it's not a show stopper now.
>
> [...]
>>>> +        failed = 0
>>>> +        for ent in entries:
>>>
>>>
>>> This loops more than necessary and is somewhat hard to follow. Consider
>>> using for-else here:
>>>
>>> for ...:
>>>      ...
>>>      if okay:
>>>          break
>>> else:
>>>      raise error
>>
>> I simplified things a bit but a for/else won't work here as we need to
>> check all ranges all the time. It is perfectly fine to not fit into a
>> range, as long as it fits into SOME range.
>
> Well, that's how for's (not if's) else clause works -- it's executed
> after all the looping's done if you didn't `break` out.
> http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops
>
> Maybe I'm just used to it and it's too esoteric to the average reader,
> though.

Thanks for the vote of confidence. Like I said, I wanted it to check all 
the ranges. A for/else quits on the break, which I guess is really 
probably ok assuming we trust that nothing else is going to stuff bad 
ranges in. I can go ahead and make the change.

>
> [...]
>> Ok, I'll drop this since it doesn't affect things with the new LDAP
>> backend.
>
> Please do, you left it in by mistake.

Yeah, there it is sitting unsquashed in my tree :-(

>> I also added one change related to the LDAP core changes. In the past if
>> you did not have a ticket it would prompt for DM password. This was
>> broken after the updates. I added an additional except in
>> test_connection().
>
> This should also be fixed soon in ipaldap. Thanks for putting up with
> the changes.
>

So should I drop this in my patch then? I don't really like having to 
import ldap.

rob