Allow HTTP CONNECT proxying to route based on HTTP headers

[nicktrav]

Currently, connect_matcher only supports matching on the HTTP CONNECT method.

I'd like to propose / request enhancing this matcher by allowing it to match on the other fields under a route's match config. Specifically, we'd like to be able to use the HTTP headers to control where the CONNECT is directed.

In terms of the existing code, ConnectRouteEntryImpl only supports checking to see if the request method is CONNECT, here:

envoy/source/common/router/config_impl.cc

Lines 1029 to 1036 in 50ef094

	RouteConstSharedPtr ConnectRouteEntryImpl::matches(const Http::RequestHeaderMap& headers,
	const StreamInfo::StreamInfo&,
	uint64_t random_value) const {
	if (Http::HeaderUtility::isConnect(headers)) {
	return clusterEntry(headers, random_value);
	}
	return nullptr;
	}

The following patch is essentially what we're looking for:

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
index ae92edeab..d5ef311c5 100644
--- a/source/common/router/config_impl.cc
+++ b/source/common/router/config_impl.cc
@@ -1027,9 +1027,10 @@ void ConnectRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers,
 }

 RouteConstSharedPtr ConnectRouteEntryImpl::matches(const Http::RequestHeaderMap& headers,
-                                                   const StreamInfo::StreamInfo&,
+                                                   const StreamInfo::StreamInfo& stream_info,
                                                    uint64_t random_value) const {
-  if (Http::HeaderUtility::isConnect(headers)) {
+  if (Http::HeaderUtility::isConnect(headers) &&
+      RouteEntryImplBase::matchRoute(headers, stream_info, random_value)) {
     return clusterEntry(headers, random_value);
   }
   return nullptr;

Happy to put up this patch along with some tests, unless there's good reason for not allowing more advanced matching for CONNECT?

[htuch]

@alyssawilk WDYT?

[alyssawilk]

No objection on my part. I think the problem was that all the other matchers assume the request has a path, and CONNECT HTTP/1.1 does not which is why @mattklein123 preferred separate matching entirely. Extending the CONNECT matcher to do other header based matching SGTM as long as we're careful to continue supporting nonexistent paths :-)

[mattklein123]

SGTM!