-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for lease redirect #5594
Fix for lease redirect #5594
Conversation
Looks good. Can you update the changelog and confirm you have signed the CLA. +1 after that. Thanks! |
@@ -322,25 +339,11 @@ func (h *handler) serveLease(w http.ResponseWriter, r *http.Request) { | |||
scheme = "https://" | |||
} | |||
|
|||
leader = scheme + leader + "/lease" | |||
leader = scheme + leader + "/lease?name=" + name + "&nodeid=" + nodeIDStr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be other context in the URL parameters from the requester that we don't need to validate, or other values added at a later point. So, I think it would be preferable to do:
leader = scheme + leader + "/lease?" + q.Encode()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-dard True, I will update the PR.
cde1766
to
fc7be3f
Compare
@corylanou I believe I have already signed the CLA a few months back for some work on telegraf. I'll update the changelog and fix up the PR based on @e-dard 's comments about futureproofing |
Previously, the lease redirect was invalid causing anything relying on a lease for execution (eg. continuous queries) to cease functioning. The name/nodeid URL param parsing has been moved up to the top of the handler so the options can be forwarded on to the real leader. X-Github-Closes: influxdata#5592
fc7be3f
to
63ff0ca
Compare
LGTM 👍 |
Thanks @corylanou and @e-dard - out of curiosity, when does the cron for nightly build RPMs run? |
@rossmcdonald ^^ ? |
@oldmantaiter Nightlies are run at 8AM UTC (midnight PST). |
@oldmantaiter Although we had a build failure last night, so I'm rerunning them now. If you pull the InfluxDB nightly in the next 5 - 10 minutes then it will have this fix included. |
[0.10.2] Backport #5594 to 0.10.0
Previously, the lease redirect was invalid causing anything relying
on a lease for execution (eg. continuous queries) to cease functioning.
The name/nodeid URL param parsing has been moved up to the top of the
handler so the options can be forwarded on to the real leader.
X-Github-Closes: #5592