Skip to content
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 empty basic auth #1601

Merged
merged 1 commit into from
May 15, 2017
Merged

Fix empty basic auth #1601

merged 1 commit into from
May 15, 2017

Conversation

emilevauge
Copy link
Member

Description

This PR fixes traefik crash when basic auth user is empty.

Fixes #1592

server/server.go Outdated
@@ -843,7 +844,7 @@ func (server *Server) buildDefaultHTTPRouter() *mux.Router {
return router
}

func parseHealthCheckOptions(lb healthcheck.LoadBalancer, backend string, hc *types.HealthCheck, hcConfig HealthCheckConfig) *healthcheck.Options {
func parseHealthCheckOptions(lb healthcheck.LoadBalancer, backend string, hc *types.HealthCheck, hcConfig *HealthCheckConfig) *healthcheck.Options {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we guarantee on the call site that hcConfig is always != nil? Without that, we could run into a nil reference issue on line 852.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I was thinking the same and I just saw you too :) Fixed.

@timoreimann
Copy link
Contributor

Hmm, how's the health check configuration related to basic auth?

I must be missing a piece of the puzzle... 🤔

@emilevauge
Copy link
Member Author

emilevauge commented May 15, 2017

@timoreimann You are absolutely right. I just fixed this because it failed while I was writing the new tests and I thought it would be safer to remove the pointer dereferencing calling parseHealthCheckOptions. WDYT?

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

Signed-off-by: Emile Vauge <emile@vauge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants