Conversation
This commit adds LDAP as a credential backend, with a caching layer in front of it so as not to try to authenticate every single request.
|
|
||
| type key int | ||
|
|
||
| var infoKey key |
There was a problem hiding this comment.
Why does this need to be package global? It's never written?
| } | ||
| } | ||
|
|
||
| type key int |
|
Thanks @HackAttack ! Apologies for the delay on the review. Overall looks great :-) |
| timeout := c.config.CacheTime | ||
| // Don't cache a negative result for a long time; likely wrong password | ||
| if !authed { | ||
| timeout = 5 * time.Second |
There was a problem hiding this comment.
Out of Curiosity: Why cache a negative result at all?
There was a problem hiding this comment.
If you build with invalid auth and large -j value, I guess you don't want to make several hundred LDAP requests before the client notices and stops/cancels the remaining jobs.
| defer ce.Unlock() | ||
| if ce.authed != nil { | ||
| return *ce.authed | ||
| } |
There was a problem hiding this comment.
ce.Lock()
defer ce.Unlock()
if ce.authed != nil {
return *ce.authed
}
Could this be changed to the below?
authed := ce.authed
if authed != nil {
return authed
}
ce.Lock()
defer ce.Unlock()
That way you don't have to acquire a lock in the case where it was cached?
There was a problem hiding this comment.
I also think this should have a nice fast-path, but this suggestion might need some tweaking to avoid multiple identical queries in quick succession: after obtaining the lock, ce.authed should be re-checked.
| if c.LDAP.UsernameAttribute == "" { | ||
| c.LDAP.UsernameAttribute = "uid" | ||
| } | ||
| if c.LDAP.CacheTime == 0 { |
| return &Cache{ | ||
| config: config, | ||
| BasicAuth: &auth.BasicAuth{ | ||
| Realm: "Bazel remote cache", |
There was a problem hiding this comment.
Should this be a configuration option?
|
If anyone is watching this PR, I'd be really interested if this works for you. I am happy to rebase and merge it if this brings value to people. |
|
are there any show stoppers from preventing a merge besides the review comments? |
|
Wow, I completely and utterly forgot about this! This was one or two companies ago so I don’t know that I have a good way to test this now. I assume I verified it to work at the time, but it’s been a while… |
|
No worries. May you could at least rebase your branch to the latest master? In best case some of the review comments could be fixed right away, but this won't be necessary for testing I assume. |
|
If anyone wants to rebase and/or resubmit a newer version of this, please also add some automated tests. |
4 participants
This PR adds LDAP as a credential backend, with a caching layer in front of it so as not to try to authenticate every single request.
Resolves #100