Auto host rewrite option for strict_dns and logical_dns clusters #504

rshriram · 2017-02-23T00:03:47Z

Enables envoy to rewrite Host headers for upstream requests, based on the hostname of the upstream instance (only for strict_dns and logical_dns clusters).

This is specifically useful when proxying to multiple cloud foundry instances (each of which has its own hostname, where the GoRouter routes requests to instances based on the hostname).

fixes #204

cc @mattklein123

While the changes are spread over a lot of files, they are pretty minor in 90% of the cases. The key changes are in the strict/logical dns implementations and hostdescription implementation.

mattklein123

overall nice work, a few comments.

mattklein123 · 2017-02-23T00:49:23Z

docs/configuration/http_conn_man/route_config/route.rst

@@ -97,6 +98,15 @@ host_rewrite
  *(optional, string)* Indicates that during forwarding, the host header will be swapped with this
  value.

+.. _config_http_conn_man_route_table_route_auto_host_rewrite:


nit: don't need the anchor if its not referenced anywhere (though you might want to reference from the router arch overview page).

Yes, that is a todo on my end. Need to provide an example as well. Will add it to router arch

mattklein123 · 2017-02-23T00:49:58Z

include/envoy/router/router.h

@@ -198,6 +198,11 @@ class RouteEntry {
   * @return const VirtualHost& the virtual host that owns the route.
   */
  virtual const VirtualHost& virtualHost() const PURE;
+
+  /**
+   * @return bool true if the Host header should be overwritten with the Upstream hostname.


"Host/:authority header"

s/Upstream/upstream

mattklein123 · 2017-02-23T00:51:46Z

source/common/upstream/logical_dns_cluster.cc

@@ -21,8 +21,7 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader
  }

  dns_url_ = hosts_json[0]->getString("url");
-  Network::Utility::hostFromTcpUrl(dns_url_);
-  Network::Utility::portFromTcpUrl(dns_url_);
+  hostname_ = Network::Utility::hostAndPortFromTcpUrl(dns_url_);


Though technically this should work (host:port), it's possible that some webservers might not handle this correctly. I could go either way, but my recommendation for now would be to remove hostAndPortFromTcpUrl() function that you added, and just populate the hostname() with the host, and without the port. Happy to discuss though.

According to the RFC,
A "host" without any trailing port information implies the default port for the service requested (e.g., "80" for an HTTP URL).

I assumed that if its not a default port, then the port must be part of the Host header.
If stuff works without a port, I would be more than happy to reuse the hostFromTcpUrl function.

@kyessenov thoughts? You hit some issues in k8s regarding this right?

yes, that's the RFC, but I'm just explaining that it might not actually work. In fact, Envoy is broken in this regard. We don't correctly parse out port when doing virtual host / domain matching from the host/auth header. For the use case you are adding this for, I strongly suspect that you probably only need host and not port, but you know your use case.

My understanding is that envoy requires explicit port suffixes in hostname strings (e.g. domains in virtual hosts), just like RFC says.
I'd like to have the port number to be a part of the host rewrite string, and to be consistent with the server side config, it probably should be part of the string literal in the client cluster config. So basically, either strip port on both client and server side, or don't, but be consistent.

There are 2 things being conflated here, how Envoy parses "URLs" and what a web/proxy server expects in the Host/:authority header. This change is about auto-populating the Host/:authority header. Technically, all web/proxy servers should support host:port when doing vhost, etc. matching, but that doesn't always work, and as I said above, Envoy itself is broken in this regard currently. I think you will have better luck with this feature if you just populate the DNS host name and skip the port.

@kyessenov is this feature actually required for Istio? @rshriram I thought you were adding this to deal with cloud foundry.

no, we just use host-rewrite (in ingress) and SDS.

mattklein123 · 2017-02-23T00:52:24Z

source/common/upstream/logical_dns_cluster.cc

@@ -21,8 +21,7 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader
  }

  dns_url_ = hosts_json[0]->getString("url");
-  Network::Utility::hostFromTcpUrl(dns_url_);


I would leave those 2 lines, they make sure the URL is well formed, which hostAndPortFromTcpUrl does not. Otherwise we might get an exception later on. Since it's probably not clear why these lines were here, please add a comment to that effect.

mattklein123 · 2017-02-23T00:55:06Z

test/common/router/router_test.cc

+  HttpTestUtility::addDefaultHeaders(headers);
+  headers.Host()->value(req_host);
+  router_.decodeHeaders(headers, true);
+  EXPECT_EQ(req_host, headers.Host()->value().c_str());


The fact that this gets changed is an implementation detail that might not always be true. I would test that the headers you get in encodeHeaders() have the right host value. Same below.

mattklein123 · 2017-02-23T00:58:30Z

Also, you probably saw, but your tests are failing with asserts enabled so you also need to fix that.

rshriram · 2017-02-23T01:47:37Z

Cloud foundry is the primary use case and I think you are right. Requests hit the front end router at port 80, which then routes it to various CF apps. In istio as well, this feature is useful when we want to use Envoy to be the middleman between a kubernetes pod and a set of non-kubernetes services (something like going out to the internet). In both cases, as you point out, it's probably safer to stick to just the dns name and not the port. The use case Kuat talked about (and the reason I asked for his opinion) was kubernetes internal traffic only. But now, i know the reason for why we generate virtual hosts of the form mysvc.cluster.local:3350 -- Envoy doesn't parse out the port in virtual hosts (not sure if it has to either as per rfc).

On Wed, Feb 22, 2017 at 8:33 PM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/common/upstream/logical_dns_cluster.cc <#504 (comment)>: > @@ -21,8 +21,7 @@ LogicalDnsCluster::LogicalDnsCluster(const Json::Object& config, Runtime::Loader } dns_url_ = hosts_json[0]->getString("url"); - Network::Utility::hostFromTcpUrl(dns_url_); - Network::Utility::portFromTcpUrl(dns_url_); + hostname_ = Network::Utility::hostAndPortFromTcpUrl(dns_url_); @kyessenov <https://github.com/kyessenov> is this feature actually required for Istio? @rshriram <https://github.com/rshriram> I thought you were adding this to deal with cloud foundry. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd8e0vT-uiZ57ktsedSnElbV7QKUiks5rfOIFgaJpZM4MJXQg> .

-- ~shriram

mattklein123 · 2017-02-23T06:08:56Z

In both cases, as you point out, it's probably safer to stick to just the
dns name and not the port.

OK, for now, let's just stick with host name for the rewrite. If we need to revisit this later to add port info we can do that.

mattklein123

2 minor things otherwise looks good.

mattklein123 · 2017-02-26T17:45:44Z

docs/intro/arch_overview/http_routing.rst

@@ -22,7 +22,9 @@ request. The router filter supports the following features:
  level.
 * :ref:`Path <config_http_conn_man_route_table_route_path_redirect>`/:ref:`host
  <config_http_conn_man_route_table_route_host_redirect>` redirection at the route level.
-* :ref:`Host rewriting <config_http_conn_man_route_table_route_host_rewrite>`.
+* :ref:`Explicit host rewriting <config_http_conn_man_route_table_route_host_rewrite>`.
+* :ref:`Automatic host rewriting <config_http_conn_man_route_table_route_auto_host_rewrite>` based on DNS name of


nit: "... the DNS name ..."

mattklein123 · 2017-02-26T17:55:09Z

test/common/router/router_test.cc

+      .WillOnce(Invoke([&](const Upstream::HostDescriptionPtr host)
+                           -> void { EXPECT_EQ(host_address_, host->address()); }));
+  EXPECT_CALL(callbacks_.route_->route_entry_, autoHostRewrite()).WillOnce(Return(true));
+  EXPECT_CALL(*cm_.conn_pool_.host_, hostname()).WillRepeatedly(ReturnRef(dns_host));


It would be better if you made hostname_ a variable in the host mock which defaults to "" and then set up an ON_CALL for it. Then your tests just need to change the value of hostname_ inside the mock both here and below. It's a little less error prone and more realistic. Your test below should actually setup a DNS host like this test, but because autoHostRewrite() returns false, it won't be swapped.

mattklein123 · 2017-02-27T16:11:32Z

test/mocks/upstream/host.h

  MOCK_CONST_METHOD0(address, Network::Address::InstancePtr());
  MOCK_CONST_METHOD0(stats, HostStats&());
  MOCK_CONST_METHOD0(zone, const std::string&());

+  std::string hostname_{};


{} not needed

mattklein123 · 2017-02-27T16:11:55Z

test/mocks/upstream/host.h

@@ -56,10 +56,12 @@ class MockHostDescription : public HostDescription {
  MOCK_CONST_METHOD0(canary, bool());
  MOCK_CONST_METHOD0(cluster, const ClusterInfo&());
  MOCK_CONST_METHOD0(outlierDetector, Outlier::DetectorHostSink&());
+  MOCK_CONST_METHOD0(hostname, const std::string&());


add the ON_CALL to the constructor, and just ReturnRef hostname_

mattklein123 · 2017-02-27T16:13:52Z

test/common/router/router_test.cc

+  HttpTestUtility::addDefaultHeaders(incoming_headers);
+  incoming_headers.Host()->value(req_host);
+
+  ON_CALL(*cm_.conn_pool_.host_, hostname()).WillByDefault(ReturnRef(dns_host));


del this line, del dns_host, set host_.hostname_ = "scooby.doo"

mattklein123 · 2017-02-27T16:14:29Z

test/common/router/router_test.cc

+  HttpTestUtility::addDefaultHeaders(outgoing_headers);
+  outgoing_headers.Host()->value(dns_host);
+
+  ON_CALL(*cm_.conn_pool_.host_, hostname()).WillByDefault(ReturnRef(dns_host));


del this line, del dns_host, set host_.hostname_ = "scooby.doo"

Signed-off-by: Pengyuan Bian <[email protected]>

envoyproxy#504) * zh-translation: docs/root/configuration/http/http_filters/original_src_filter.rst * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> * Update docs/root/configuration/http/http_filters/original_src_filter.rst Co-authored-by: majinghe <[email protected]> Co-authored-by: majinghe <[email protected]>

mattklein123 reviewed Feb 23, 2017

View reviewed changes

rshriram force-pushed the auto_host_rewrite branch from 4dced25 to 264d6f3 Compare February 24, 2017 20:45

mattklein123 reviewed Feb 26, 2017

View reviewed changes

Shriram Rajagopalan added 17 commits February 26, 2017 21:43

adding hostname param to HostDescription

38f761b

fix format

db88323

fixing users of HostImpl

dfbc9f8

disable HostImpl testes temporarily

f720204

adding route option for auto host rewrite

9f1a67f

plumbing

6d5f5fe

adding unit tests

fd52595

FIXME and removing stray files

468b39a

fix typo and remove #if

2a8d2ee

tests and bugfixes

c2ea868

docs

61fd39d

remove stray TODO

93c3dc5

addressing PR nits

6987041

check for only hostname and not port number

884152a

fixing route_test for autohostrewrite

1796336

rebase

bc79df3

addressing final nits

534898e

rshriram force-pushed the auto_host_rewrite branch from 264d6f3 to 534898e Compare February 27, 2017 02:43

mattklein123 reviewed Feb 27, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits February 27, 2017 11:18

Merge branch 'master' into auto_host_rewrite

855373b

ON_CALL for hostname

8cffb0f

mattklein123 approved these changes Feb 27, 2017

View reviewed changes

mattklein123 merged commit e909208 into envoyproxy:master Feb 27, 2017

rshriram deleted the auto_host_rewrite branch March 23, 2017 15:29

lizan pushed a commit to lizan/envoy that referenced this pull request Jun 4, 2020

Update proxy-wasm-cpp-host sha (envoyproxy#504)

452ed6d

Signed-off-by: Pengyuan Bian <[email protected]>

freddygv mentioned this pull request Apr 1, 2021

Auto Rewrite Host Headers for Terminating Gateways hashicorp/consul#9042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto host rewrite option for strict_dns and logical_dns clusters #504

Auto host rewrite option for strict_dns and logical_dns clusters #504

rshriram commented Feb 23, 2017

mattklein123 left a comment

mattklein123 Feb 23, 2017

rshriram Feb 23, 2017

mattklein123 Feb 23, 2017

mattklein123 Feb 23, 2017

mattklein123 Feb 23, 2017

rshriram Feb 23, 2017

mattklein123 Feb 23, 2017

kyessenov Feb 23, 2017

mattklein123 Feb 23, 2017

mattklein123 Feb 23, 2017

kyessenov Feb 23, 2017

mattklein123 Feb 23, 2017

mattklein123 Feb 23, 2017

mattklein123 commented Feb 23, 2017 •

edited

Loading

rshriram commented Feb 23, 2017 via email

mattklein123 commented Feb 23, 2017

mattklein123 left a comment

mattklein123 Feb 26, 2017

mattklein123 Feb 26, 2017

mattklein123 Feb 27, 2017

mattklein123 Feb 27, 2017

mattklein123 Feb 27, 2017

mattklein123 Feb 27, 2017

Auto host rewrite option for strict_dns and logical_dns clusters #504

Auto host rewrite option for strict_dns and logical_dns clusters #504

Conversation

rshriram commented Feb 23, 2017

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Feb 23, 2017 • edited Loading

rshriram commented Feb 23, 2017 via email

mattklein123 commented Feb 23, 2017

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Feb 23, 2017 •

edited

Loading