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

Support Host header in HTTP health checks #2657

Closed

Conversation

pierrecdn
Copy link

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 #1184

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
@pierrecdn
Copy link
Author

pierrecdn commented Jan 17, 2017

Does not fix #1184 actually, but one use case only (Host header)... I can edit my commit message if needed.

@kamaradclimber
Copy link
Contributor

This PR is very similar to #2474 (and I'd be happy if both could be merged)

@123BLiN
Copy link

123BLiN commented Jan 30, 2017

I would say that this commit is useful not only for reverse-proxy.
For example we need to check local service on local consul node that has only one binding - some_service.service.consul. In case we are using consul DNS to resolve such service names we will not be able to check only this particular service instance, because consul DNS will return all healthy nodes IPs in RR manner. But if we will check http://127.0.0.1/some_foo/bar with host header "some_service.service.consul" this should do the trick.

We need some analog of curl's --resolve option. And I hope that this commit is what we need.

@kamaradclimber
Copy link
Contributor

Hello @slackpad,

would you have any feedback regarding this PR?
We've been using this on top of 0.7.3 for a month without any issue

@DI-DaveGoodine
Copy link

@slackpad - this would really help us with the resiliency of our service proxy setup & consul.

@rclayton-the-terrible
Copy link

This is huge for us. We are using Linkerd and the only thing that differentiates our services is the Host header.

@magiconair
Copy link
Contributor

Why can't you just use the correct host name in the check URL?

@pierrecdn
Copy link
Author

@magiconair : imagine the following setup:

  • service S bound to localhost/127.0.0.1 on port P.
  • reverse-proxy (nginx, whatever) using Host to redirect to the good backend and expose multiple services on the port 80 in the network.
  • multiple instances of this service accross the network and/or a prepared query to fallback on another DC when it's down.

So here the goal of the Consul health check is to reflect the LOCAL state and not the global one which will be given to the end-user by Consul itself (DNS API for ex.).

In this case when a request http://S.query.consul/ arrives it's internally proxied to http://localhost:P/.

To check that my local service is OK, but using the exact same endpoint than my end-user I have to pass the Host header to the reverse proxy.
In this case http://localhost/ + Host: S.query.consul.

@rclayton-the-terrible
Copy link

rclayton-the-terrible commented Jun 3, 2017

@magiconair we are using a Linkerd mesh network, which will proxy and route communication to the correct service. Creating DNS entries for every service pointing to the same Linkerd entrypoint would defeat the purpose of using Linkerd. Since, all requests are made to the entrypoint and then routed by either by Host or linkerd-specific HTTP header, if we could specify a header, we could test our Linkerd routing to ensure it's configured properly. Not being able to assign a header means we have to dump a lot of effort into finding other solutions for testing the health of these endpoints through the proxy (like extend the Consul Docker container or create a sidecar container that performs health checks and reports them to Consul).

@magiconair
Copy link
Contributor

I'm not against adding headers to the health checks but I'd like to understand the actual use-case first. Second, if we're going to add this then we should have some more generic header and method configuration and combine this ideally with #2474 in a single PR. Just adding the host header is too narrow in scope IMO since then people will probably want to add headers for all kinds of things, e.g. auth, tracing, logging, ...

Also, since I'm still maintaining https://github.com/fabiolb/fabio I've got some experience with reverse proxy setups. :)

From what I understand your setup is the following:

client -> GET foo.com/bar -> nginx/linkerd -> GET instance:12345/bar (Host: foo.com)

What I'm also reading is that you register instance:12345 in consul but that querying this would require setting the Host: foo.com header since the instance expects that header and not Host: foo.com:12345 for example.

What I'm also reading is that if you register http://foo.com/bar as health check URL you're testing nginx/linkerd and not the instances which doesn't provide you with the things you want.

Is this a correct description of your setup/problem?

Assuming this is correct, I see two things here:

  1. Testing the L7 router. This is like testing www.google.com. Simple URL should work.
  2. Testing the instances directly: Why do they need the Host header? That's what you have the L7 router for which translates the request to a host:port?

What am I missing?

@magiconair magiconair self-assigned this Jun 3, 2017
@pierrecdn
Copy link
Author

The only thing you're missing here is that we register in Consul what's already accessible to the end-user, meaning instance:80 and not instance:12345.

Now you have 2 choices for the health-check:

  • either doing it on http://localhost:12345 but you'll get false positives for sure (reverse proxy issues, reloads etc.).
  • either doing it through the proxy (so you're using the exact same path and port that the end-user). Here you need the Host header.

As stated in my first comment, I completely agree that something more generic could be even better. Host is one use-case, probably the more common, but there are definitely others.

@magiconair
Copy link
Contributor

@pierrecdn I'm sorry if I'm a bit dense but what is instance:80?

  • instance has the request handler, runs on some port (80 or random) and all requests to it are routed via the L7 router. It is not directly accessible by an end-user because it sits behind a router.

  • The L7 router is user-facing (on the internet) and routes based on hostname and path.

In that case only the L7 router needs the host name for routing unless you need to spoof it for the service the L7 router sends the request to (e.g. instance)

@rclayton-the-terrible
Copy link

@magiconair I like being able to generically set headers (no need for specific headers); this will solve our use case.

What we are trying to achieve specifically is a way to make sure the dynamic routing configuration in Linkerd is correct by checking for those services through the network fabric.

These services are not user facing. We have a an external Linkerd interface that routes public traffic to our Gateway microservice. The Gateway microservice calls/forwards/proxies requests to the appropriate downstream microservice through an internal Linkerd interface.

So it looks something like:

Amazon ALB -> Linkerd External Interface -> Gateway -> Linkerd Internal Interface -> Downstream Service

We dynamically create network meshes with our CI/CD system upon a successful integration test run. The network mesh provides "virtual" addresses for services registered in Consul. For instance, we will deploy as service with the name <service>-<version> (e.g. accounts-44, where 44 is the build number); we refer to this as the "actual service". Then we build a network mesh of microservices together and create "virtual" mappings to those services using Linkerd DTab (something like accounts-staging => accounts-44). A service may have more than one "virtual" mapping. We do this to support idempotent deploys of microservices in our infrastructure.

What I want to do is create health checks for the virtual mappings. Specifically, I want to know if a particular network mesh have nodes that are inaccessible through the Linkerd Internal Interface. I will already know if the service is working via a standard Consul health check. I just want to further make sure that my routing infrastructure is also configured correctly.

I hope that better describes our use case.

@pierrecdn
Copy link
Author

@magiconair I wasn't clear enough in my explanation apparently.

So, instance:80 doesn't exists in reality but this is what the end user cares about.

In fact the real service is bound to 127.0.0.1:12345.

The "L7 router" listen on 80 and forwards to it based on the Host header.

Here it supposes that the end-user will uses the DNS (A) API.

So I agree with "only the L7 router needs the host name for routing". And as the health-check is performed through the L7 router, I needed Host header support into Consul health-checks.

@magiconair
Copy link
Contributor

@pierrecdn If you perform the health check through the L7 router you only need to use the correct URL IMO, e.g. http://foo.com/bar instead of http://instance:80/bar\nHost: foo.com Making a request to http://foo.com/bar will automatically set the Host header to foo.com.

I've opened #3106 to track this.

magiconair added a commit that referenced this pull request Jun 4, 2017
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
@pierrecdn
Copy link
Author

@magiconair yes, but what happen if this URL is http://foo.query/service.consul ? or a fqdn that is a RR DNS entry ? Or a VIP (whatever the technology in place) ? The test will always be valid since it won't check the LOCAL state.

@magiconair
Copy link
Contributor

@pierrecdn Thanks for your patience in this discussion :)

I think this argument doesn't hold since adding the Host header to the check is equivalent with using it in the URL unless you need to spoof the header. Since you are testing the end-user facing service this cannot be the case.

In any case, I've taken your PR and augmented it so that you can set any header and any method since I think this a good addition in any case. Feel free to test #3106 since I still need to add some more tests and update the docs.

I will credit both you and the author of #2474 in the commit since both of you have done the original work. I should be able to get this into the 0.8.4 release.

@pierrecdn
Copy link
Author

I need to spoof the header. If not I may not test the local service but one on the network.
Anyway if there is a way to define HTTP headers in Consul health-checks I'll be happy :).
Thanks !

magiconair added a commit that referenced this pull request Jun 4, 2017
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
magiconair added a commit that referenced this pull request Jun 6, 2017
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
@magiconair magiconair closed this in 825f72f Jun 6, 2017
@kamaradclimber
Copy link
Contributor

will use the new feature in 0.8.4 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants