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

Do not follow redirects for the health check URLs #5147

Merged

Conversation

coder-hugo
Copy link
Contributor

@coder-hugo coder-hugo commented Jul 23, 2019

What does this PR do?

This PR avoids that the health checker follows HTTP redirects.

Motivation

The default behavior of the go HTTP client is to follow redirects and stop after 10 consecutive requests (see). I wouldn't expect this from a health checker. A redirect could point to a completely different host which is broken. The result of this would be that traefik removes a healthy backend.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Copy link
Collaborator

@SantoDE SantoDE left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

I agree with you :)

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

Design LGTM.

@ldez ldez requested review from ldez and juliens August 6, 2019 19:25
@ldez ldez added this to the 2.0 milestone Aug 6, 2019
@ldez ldez added this to To review in v2 via automation Aug 6, 2019
@ldez ldez force-pushed the feature/no-redirect-on-healthcheck branch from f92aae0 to 2b77869 Compare August 26, 2019 07:50
@coder-hugo coder-hugo requested review from a team as code owners August 26, 2019 07:50
@ldez ldez changed the base branch from master to v2.0 August 26, 2019 07:50
@ldez ldez removed request for a team August 26, 2019 07:50
@ldez ldez removed the bot/no-merge label Aug 26, 2019
@ldez ldez removed this from the 2.0 milestone Nov 14, 2019
@jbdoumenjou
Copy link
Member

Hello @coder-hugo,

We think that not following the redirect for the health check makes sense but we can't integrate it like that without risking breaking existing users relying on the current behavior.

WDYT about integrating it as an health check option?

@ldez ldez force-pushed the feature/no-redirect-on-healthcheck branch from 3bfe71b to 94ff137 Compare February 26, 2020 10:02
@ldez ldez changed the base branch from v2.0 to master February 26, 2020 10:03
@ldez ldez added this to the next milestone Feb 26, 2020
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@traefiker traefiker force-pushed the feature/no-redirect-on-healthcheck branch from 934c907 to e565e2f Compare February 26, 2020 15:52
@jbdoumenjou jbdoumenjou removed their assignment Feb 26, 2020
@traefiker traefiker merged commit 18d90ec into traefik:master Feb 26, 2020
v2 automation moved this from To review to Done Feb 26, 2020
@coder-hugo coder-hugo deleted the feature/no-redirect-on-healthcheck branch February 27, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants