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

Setting headers for http checks #1184

Closed
majormoses opened this issue Aug 18, 2015 · 16 comments
Closed

Setting headers for http checks #1184

majormoses opened this issue Aug 18, 2015 · 16 comments
Labels
theme/health-checks Health Check functionality type/enhancement Proposed improvement or new feature

Comments

@majormoses
Copy link
Contributor

I dont see anything in the docs, is there any way to set a header for an http consul service check? I would imagine it useful for people to set headers such as 'X-Forwarded-Proto: https' on a check

@slackpad slackpad added the type/enhancement Proposed improvement or new feature label Aug 25, 2015
@slackpad
Copy link
Contributor

There currently isn't any support for controlling the headers of the HTTP check, marking as an enhancement.

@scalp42
Copy link
Contributor

scalp42 commented Aug 25, 2015

Tweaking the user agent comes to mind as well.

@ryanbreen
Copy link
Contributor

IMHO, adding this feels like a slippery slope. I like the current balance where simple HTTP calls work fine as http checks and more complicated things can be accomplished through script checks. I would hate for us to turn Consul's http check into curl.

@slackpad slackpad added thinking More time is needed to research by the Consul Contributors and removed type/enhancement Proposed improvement or new feature labels Aug 25, 2015
@slackpad
Copy link
Contributor

That's a good point @ryanbreen - even headers brings to mind complexity around supporting multiple, etc. I'll change this to thinking for now, because we don't want to introduce a bunch of complexity. If there are a few extra options that are highly useful, it might make for a better experience, but we don't want a curl-level of stuff for sure.

@majormoses
Copy link
Contributor Author

I can definitely see how this could get out of hand quickly...I think a few options that would cover 90% of the use case would be great. Otherwise you are going to end up with a lot of people resorting to cmd script just to run a curl just to change something like the protocol to https, etc. I will keep thinking about this...see if I can think of some middle ground

@tdooner
Copy link

tdooner commented Feb 3, 2016

Just found this issue after wanting to do this. My use case: I have an nginx instance with multiple virtual hosts and I'd like to health check their instances separately. RIght now I'm forced to use the command health check, e.g.:

curl --fail --silent -H"Host: foo.example.com" http://localhost

It'd be great to not have to shell out in order to accomplish this relatively-basic functionality. It doesn't really feel like a slippery slope to me to allow arbitrary HTTP headers -- just looping through some object and calling req.Header.Set here.

Being able to replicate cURL's --insecure flag would be great too, but I understand the point about not reinventing the wheel as much as possible.

@truszkowski
Copy link

Is possible to send header "Accept: /" (by default)?
This header is needed by RFC... and go-restful for example (406 response).

@slackpad
Copy link
Contributor

Hi @truszkowski this was added in #1819 which will be going out in today's release of Consul 0.6.4.

@slackpad slackpad added type/enhancement Proposed improvement or new feature and removed thinking More time is needed to research by the Consul Contributors labels Apr 13, 2016
@slackpad
Copy link
Contributor

Headers seem pretty reasonable to support and there have been several requests.

@klynch
Copy link

klynch commented Jul 5, 2016

Setting the Host header would be extremely useful for us as well.

@ashald
Copy link
Contributor

ashald commented Sep 12, 2016

We have the same requirement - we need to set host header. I can contribute the PR adding the option if it will help (my last PR didn't get much attention so checking in advance).
If you OK with a PR then would you like an option headers taking a dictionary with headers?

@slackpad
Copy link
Contributor

Hi @ashald we'd take a PR for this - thank you!

@ihr
Copy link

ihr commented Jan 16, 2017

This feature would be very helpful in use cases of PaaS, such as with Cloud Foundry which allows you to identify unique app instance with the inclusion of a X-Cf-App-Instance header

pierrecdn added a commit to criteo-forks/consul that referenced this issue Jan 17, 2017
This commit introduce support for passing Host HTTP Header in HTTP health
checks.
Such check pattern is very common (for ex. when using a reverse-proxy) and
you usually want to check the user facing port/API rather than the
internal service exposed to 127.0.0.1.

Fix hashicorp#1184
kamaradclimber pushed a commit to criteo-forks/consul that referenced this issue Feb 3, 2017
This commit introduce support for passing Host HTTP Header in HTTP health
checks.
Such check pattern is very common (for ex. when using a reverse-proxy) and
you usually want to check the user facing port/API rather than the
internal service exposed to 127.0.0.1.

Fix hashicorp#1184

Change-Id: Ic130c01558eed312dd3bd7168482dd3908d32912
@tgs
Copy link

tgs commented Mar 7, 2017

Since the Host header isn't set, we have to turn off an important security setting of our apps. For example, for a Django app, we have to set ALLOWED_HOSTS = ['*']. This is a security problem because of DNS rebinding attacks, where an attacker circumvents the Same Origin Policy by making a browser think that your server is part of their origin, for example banking.attacker.com instead of banking.com. This enables cross-site scripting, so malicious javascript can perform any actions made available in the web application. The usual solution to this class of attacks is to have the web application check the Host header of incoming requests. If the browser is accessing the app as banking.attacker.com instead of banking.com, the app notices and ignores the request. However, to make our apps pass health checks, we have to disable this check.

@slackpad slackpad added the theme/health-checks Health Check functionality label May 25, 2017
pierrecdn referenced this issue Jun 4, 2017
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
@slackpad
Copy link
Contributor

slackpad commented Jun 6, 2017

Fixed via #3107.

@slackpad slackpad closed this as completed Jun 6, 2017
@ashald
Copy link
Contributor

ashald commented Jun 7, 2017

@slackpad 👍 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/health-checks Health Check functionality type/enhancement Proposed improvement or new feature
Projects
None yet
Development

No branches or pull requests

10 participants