On 3 Dec 2014, at 06:05, Matthew Newton <mcn4@LEICESTER.AC.UK> wrote:
On Tue, Dec 02, 2014 at 08:28:07PM -0500, Arran Cudbard-Bell wrote:
On 2 Dec 2014, at 19:33, Matthew Newton <mcn4@LEICESTER.AC.UK> wrote: I've just done a pull request, but I'm sure there are things that need looking at or fixing even if the idea possibly sane. Let me know.
Very nice! Possible improvement (and I may be completely wrong here) but shouldn't it be possible to fork/exec and create a pipe with the ends mapped to stdin/stdout of the execed process?
Slightly less configuration, and maybe slight performance improvement.
Yes, that should work. I thought about it, but the connection pool stuff seems designed to make connections, rather than spawn off processes. Is it safe for mod_conn_create (callback from fr_connection_pool_module_init) to fork/exec and return a struct with file descriptors?
Yes, it's safe.
Then I guess _mod_conn_free would need to wait on the child. It seemed to start getting more complicated...
It's not that complicated, _mod_conn_free signals the child to exit, and waits on it.
Could just look for --helper-protocol option in the arguments to ntlm_auth and enable the connection pool automagically...
Personally I think it's better to specify what you want to use, rather than for the server to guess - the current way is about OK as the behaviour is dependent on one config option being set, but it starts to get messy when you're doing heuristics to find the method. (Though 'method' might be better called 'backend' or 'auth_backend'; maybe it's OK as-is).
I guess, it's just A DRY thing. It's annoying to have to configure the argument and tweak the knob inside FreeRADIUS. I don't see any reason people would wrap ntlm_auth, or why the arguments would change. I know in some instances trying to be smart breaks things in unexpected ways, but the interface for the ntlm_auth binary is well defined. Do you know when the --helper-protocol flag was added? If it's been there for a significant period we might want to consider just adding it automatically if not present, and switching to the new method for 3.1.
The other possibility I'm thinking about next is another method that uses the wbclient library to call winbind directly, but I've not looked into that too much yet. Of course the library may not be available on all systems, so giving the option to run ntlm_auth should stay (and people may be calling a different binary rather than ntlm_auth in some situations).
That's certainly the cleanest solution.
In this case we could likely avoid exposing the knobs of the connection pool, as there's no benefit to tweaking anything, and just fork/exec max_servers instances of ntlm_auth?
Possible. But I'm not yet sure what the implications of this are, as it looks like there may still be throughput issues going through winbind. It might be better to limit the number of connections, rather than pushing them all to winbind and that dropping or queueing them.
Squid seems to have had a similar feature for a long time, and allows the number of ntlm_auth processes to be set, but I don't know whether there is any benefit to keeping the number down or just running loads of them.
Ok, fair enough, I guess keeping it as a tweakable to manage contention would be useful. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS development team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2