Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
apiserver: allow all controller logins #8120
Conversation
jameinel
approved these changes
Nov 22, 2017
I think I misunderstood slightly. I thought this was not rate limiting controller logins, but looks more like its just allowing controllers to login to each other.
LGTM I believe. With one section that looks like a copy & paste error.
| - result.userLogin = !result.anonymousLogin | ||
| + tag, err := names.ParseTag(req.AuthTag) | ||
| + if err == nil { | ||
| + result.tag = tag |
axw
Nov 22, 2017
Member
Heh, yeah :)
I wrote the one above first, then realised we should rate-limit if the tag is bad -- per the old code -- to prevent faulty clients from hammering on the server. Fixed, thanks.
| + // Either the tag is invalid, or | ||
| + // it's not a user; rate limit it. | ||
| + atomic.AddInt64(&a.srv.loginAttempts, 1) | ||
| + defer atomic.AddInt64(&a.srv.loginAttempts, -1) |
jameinel
Nov 22, 2017
Owner
How does this avoid rate limiting Controller logins. Anything that isn't a User is being limited here.
I suppose the idea is that we still rate limit them, but we just let the Login succeed?
axw
Nov 22, 2017
Member
Yep, this is just to keep the rate of logins steady. The controllers will still be able to login, they'll just be slowed down. Nothing's changed in this regard (maybe it should later, but I'd rather consider that separately).
| + if machine.Tag() != a.srv.tag { | ||
| + // We don't want to run pingers for other | ||
| + // controller machines; they run their own. | ||
| + startPinger = false |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
Build failed: Tests failed |
axw
added some commits
Nov 22, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
axw commentedNov 22, 2017
•
Edited 1 time
-
axw
Nov 22, 2017
Description of change
Do not restrict any controller agents from
logging in during a maintenance event.
API root restriction has been moved to after
rate limiting and authentication.
Incidentally fixes an issue where we inform
login observers that a login to the controller
model is not from a controller agent when it is.
QA steps
(wait)
(juju debug-log -m controller; there should be no login errors)
Documentation changes
None.
Bug reference
Fixes https://bugs.launchpad.net/juju/+bug/1733259