-
Notifications
You must be signed in to change notification settings - Fork 5.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
Removing auth from /ping route #5316
Removing auth from /ping route #5316
Conversation
It's the healthcheck that doesn't support authentication? |
That's correct. Here's the docs,, https://docs.aws.amazon.com/elasticloadbalancing/latest/application/target-group-health-checks.html 'm currently matching on the 401 response, but this shows up as auth failures in the internal stats, and I want to monitor this separately. |
7b819b8
to
320c994
Compare
How would you feel about using a separate http_listener_v2 as the ping endpoint? This way we don't need to add a new option that InfluxDB itself doesn't have.
|
So from my testing, influx doesn't actually do auth on the ping route:
I've actually found a lot of the diagnostic routes to not require authentication.
Ping doesn't bother me, but all of those list above do. |
Let's just remove the auth from the Add to that list |
Sounds good. Will update the PR tomorrow. |
320c994
to
0f38767
Compare
h.AuthenticateIfSet(func(res http.ResponseWriter, req *http.Request) { | ||
res.WriteHeader(http.StatusNoContent) | ||
}, res, req) | ||
res.WriteHeader(http.StatusNoContent) |
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.
Covered by existing test case:
func TestQueryAndPingHTTP(t *testing.T) { |
Adding ability to disable authentication on ping route. Load balancers like AWS's ALBs don't support basic auth.
Required for all PRs: