[rabbitmq-discuss] rabbitmq-auth-backend-ldap patch adding caching and subtree searches

[Simon MacMullen]

On 14/02/13 01:35, Gavin M. Roy wrote:
> Hi all,

Hi! Thanks for showing some interest in the LDAP plugin :-)

> Attached to this email is a patch that refactors
> rabbitmq-auth-backend-ldap a bit to allow it to use subtree searches and
> it caches the connection to the LDAP server. Would anyone on the
> RabbitMQ team be so kind as to code-review it and offer suggestions on
> how it can be improved for possible inclusion in Rabbit?

OK. This definitely goes in an interesting direction, but I'm afraid it 
will take a fair bit of work to get it merged:

* First of all, receiving multiple not-very-related changes in a single 
huge patch makes it really hard to review. The scope changes and the 
shared-connection changes don't have much to do with each other - I 
would much rather review one at a time.

* There also seem to be a few small refactorings like cases where 
functions are just moved around within the file. Again these should be 
separated so they can live or die on their own merits, and not confuse 
the issue of anything else.

* I had envisaged LDAP connection sharing happening per user or per 
connection, I hadn't thought of having a single global connection (umm, 
I didn't realise you could re-bind). So that makes connection sharing 
easier to do, but with the cost of rebinding every time. Have you 
measured to see if this is substantially beneficial vs creating a new 
connection every time?

* ... since sharing the LDAP connection presumably adds the possibility 
that some query will break it, and then we need to recover. I expect 
just getting the gen_server to die and be restarted by its supervisor 
would be the right thing to do, but I don't see evidence of that happening?

* I'm not sure the scope option... has the right scope. I would assume 
if you wanted to specify the query scope you would want to be able to do 
that per query. ATM it seems like you can only do it globally, which 
feels a bit odd.

There are a bunch of smaller issues I'd want to go through as well, but 
those are the high level ones.

> My fork is on Github as well
> https://github.com/gmr/rabbitmq-auth-backend-ldap if anyone wants to
> offer feedback or try it out.
>
> I ran the tests against the current tip version and my version and they
> pass and fail at the same spots, which I'm assuming is a good thing.

Umm, all the tests should pass! They do for me (both on default and with 
your patch). You did install OpenLDAP and run ./example/setup.sh?

Cheers, Simon

-- 
Simon MacMullen
RabbitMQ, VMware