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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to