Auto Rewrite Host Headers for Terminating Gateways

[lawliet89]

This is my first PR for Consul and I am not too familiar with it.

I am trying to partially address #8707 by configuring the Terminating GW to automatically rewrite the HTTP Host header.

My use case for Terminating GW is to exert some form of control over what external services a cluster can or cannot have access to. Having the Host header be automatically rewritten is a huge QOL improvement.

As described in #8196, there is no way to add L7 configuration options to the proxy options for services connected to Terminating GWs. The only way I could "trigger" this code path was to set protocol = "http" or similar in proxy-defaults.

Please help me out with the PR.

• Not sure how to write tests for this
• Not sure if this "hack" via proxy-defaults is the intended way to implement this.

[freddygv]

Hi @lawliet89, thank you for submitting this. I'd like to help you get this done.

Regarding using proxy-defaults to trigger this code path: did you try setting the protocol in a service-defaults config entry for the service that the terminating gateway routes to? There's an example here.

#8196 should be unrelated to what you're doing here.

Regarding tests, I think this PR would be an expansion of this integration test: https://github.com/hashicorp/consul/tree/master/test/integration/connect/envoy/case-terminating-gateway-hostnames

It may be a matter of:

1. Adding a service-default config entry for s4 to the bootstrapped entries here. This is an example of a different test showing what that would look like.
2. Updating this assertion to assert that a Host header is present and contains localhost:8382 (8382 is the port for s4 in the test). You should make a helper bash function like this one.

Let me know if you have any other questions.

[lawliet89]

Thanks @freddygv!

Regarding using proxy-defaults to trigger this code path: did you try setting the protocol in a service-defaults config entry for the service that the terminating gateway routes to? There's an example here.

You are right -- this works.

I have added some tests as you suggested, but for some reason, the rewritten Host header is missing the port number. I managed to replicate this manually when using fortio too. I'll continue to investigate.

My timezone is UTC +8 so there will be quite some delay between our replies, I reckon.

Edit: Digging into the source code of envoy, from this line and tracing up to HostDescriptionImpl, it seems like Envoy only rewrites the hostname and not the port. Not sure if the port matters. What do you think?

[freddygv]

Hi @lawliet89, I'm sorry about the delayed reply. I lost sight of this PR.

Regarding your question about the port, according to RFC 2616, if the port is excluded it implies that the port in use is one of the default ports: 80 or 443. It seems that if the port is not one of those, then the host header should contain the port.

One way to address this would be to use this flag (instead of auto_host_rewrite): response_headers_to_add. Envoy can be configured to replace existing headers with the append flag of the HeaderValueOption.

That way the Host header could be overwritten with a hostname and port parsed from the service address. This would be a place to also optionally strip the port if it's 80 or 443. Excluding default ports seems to be a convention for some applications like browsers but it doesn't seem strictly necessary.

This struct in the Terminating Gateway snapshot has a map of services to service instances whose IP address is a URL. Here is an example for how the address should be accessed.

By using the snapshot's HostnameServices map in routes.go, the hostname could be parsed from the URL with this function from the stdlib: https://golang.org/pkg/net/url/#URL.Hostname

One thing I'm wondering if whether this should be configurable on a per-service level in the Terminating Gateway's config entry. Though if someone wanted to disable this behavior that would imply that they want the Host header to not match the URL in the service address. Maybe that's not something to worry about, this functionality could probably be enabled with no option to disable it for now.

[lawliet89]

Hey @freddygv I have been looking into implementing what you suggested. I have some questions.

You meant that we have to use RequestHeadersToAdd instead of ResponseHeadersToAdd right?

This is my understand so far. Each Terminating GW service has a corresponding Envoy cluster. Each cluster may have one or more endpoints. The RequestHeadersToAdd option is added to a VirtualHost which is then tied to a single cluster. The question is, how do I choose which endpoint to rewrite the Host request header for?

[freddygv]

After some thought I agree that we would not be able to correctly update the host and port, since there could be multiple endpoints with distinct hostnames attached to a single cluster.

I went digging at the original PR that added the auto-host rewriting for Envoy and came across this conversation about whether the port should be included:
envoyproxy/envoy#504 (comment)

Have you tested the original patch against web servers in your environment? How did it work out?

I think we should be OK to merge this with the original auto host rewrite implementation. You would just need to update the integration test assertion to only verify localhost is present and not localhost:<port>.

[lawliet89]

@freddygv I have made the changes as you suggested.

I did test it out on a dev cluster the first time I made the PR. It worked great. Haven't had time to test it again recently. I'll give it a spin on Wednesday.

EDIT: Still works fine as before.

[freddygv]

Looks good, thank you!

[hashicorp-ci]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/347534.

[hashicorp-ci]

🍒❌ Cherry pick of commit e385e59 onto release/1.9.x failed! Build Log

[hashicorp-ci]

🍒❌ Cherry pick of commit e385e59 onto release/1.8.x failed! Build Log

[freddygv]

Backported this to 1.9, but will not backport to 1.8 because Terminating Gateways were only set up as TCP proxies at the time, and didn't have routes configured.