http: add support for skipping any port from host header #14491

ramaraochavali · 2020-12-21T09:41:24Z

Commit Message: add support for skipping any port from host header
Additional Description: Adds a new config to HCM to skip any port in the host header similar to strip_matching_host_port.
Risk Level: Low
Testing: Added
Docs Changes: Updated
Release Notes: Added
Fixes #12647

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only · 2020-12-21T09:41:31Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14491 was opened by ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2020-12-22T04:48:20Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // Without setting this option, incoming requests with host `example:443` will not match against
+  // route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example`. Defaults to `false`. Note that port removal is not part
+  // of `HTTP spec <https://tools.ietf.org/html/rfc3986>`_ and is provided for convenience.
+  bool strip_any_host_port = 42;


Does it make sense to make it oneof with the above field? Was wondering about it but did not because it needs to go through oneof_promotion dance - Not sure if it is worth it here.

Yeah I think it should be a oneof, but can you use the next major version oneof upgrade? I agree it's not worth doing the deprecation dance for this but marking it as such seems worth while.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123

At a high level LGTM. Just a few small comments. Thank you!

/wait

mattklein123 · 2020-12-28T23:47:30Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // Without setting this option, incoming requests with host `example:443` will not match against
+  // route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example`. Defaults to `false`. Note that port removal is not part
+  // of `HTTP spec <https://tools.ietf.org/html/rfc3986>`_ and is provided for convenience.
+  bool strip_any_host_port = 42;


Yeah I think it should be a oneof, but can you use the next major version oneof upgrade? I agree it's not worth doing the deprecation dance for this but marking it as such seems worth while.

mattklein123 · 2020-12-28T23:48:26Z

source/common/http/conn_manager_config.h

@@ -439,10 +439,15 @@ class ConnectionManagerConfig {
  virtual bool shouldMergeSlashes() const PURE;

  /**
-   * @return if the HttpConnectionManager should remove the port from host/authority header
+   * @return if the HttpConnectionManager should remove the matching port from host/authority header
   */
  virtual bool shouldStripMatchingPort() const PURE;


Even though we can't do it at the API level right now, perhaps make this a single function that returns an enum of the strip type? That would eventually work with a oneof and I think always strip is a superset anyway so it should be fine?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-01-01T09:46:15Z

@mattklein123 Made changes. PTAL

mattklein123

Thanks LGTM with small comments.

/wait

mattklein123 · 2021-01-02T23:54:49Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // Determines if the port part should be removed from host/authority header before any processing
+  // of request by HTTP filters or routing. The port would be removed only if request method is not CONNECT.
+  // This affects the upstream host header as well.
+  // Without setting this option, incoming requests with host `example:443` will not match against
+  // route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example`. Defaults to `false`. Note that port removal is not part
+  // of `HTTP spec <https://tools.ietf.org/html/rfc3986>`_ and is provided for convenience.
+  // Only one of `strip_matching_host_port` or `strip_any_host_port` can be set.


doc nit: this comment should be inside the oneof and above the field to render correctly I think.

mattklein123 · 2021-01-02T23:55:06Z

source/common/http/conn_manager_config.h

+/**
+ * Type that indicates how port should be stripped from Host header.
+ */
+enum class StripPortType { MatchingHost, Any, None };


Can you add a brief comment on what each of these does?

mattklein123 · 2021-01-02T23:55:45Z

source/common/http/conn_manager_utility.cc

@@ -427,7 +428,9 @@ bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_head
 void ConnectionManagerUtility::maybeNormalizeHost(RequestHeaderMap& request_headers,
                                                  const ConnectionManagerConfig& config,
                                                  uint32_t port) {
-  if (config.shouldStripMatchingPort()) {
+  if (config.stripPortType() == Http::StripPortType::Any) {
+    HeaderUtility::stripPortFromHost(request_headers, 0);


Instead of passing 0 as a sentinel, can you use absl::optional to make this more clear?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-01-03T13:03:39Z

@mattklein123 addressed the comments. PTAL

mattklein123

Thanks LGTM with 2 small comments.

/wait

mattklein123 · 2021-01-04T03:26:37Z

source/common/http/header_utility.h

+   * @brief Remove the port part from host/authority header if it is equal to provided port or
+   * provided port is 0.


needs updating

Sorry. Missed it. Fixed now.

mattklein123 · 2021-01-04T03:29:44Z

source/extensions/filters/network/http_connection_manager/config.cc

+  if (config.strip_any_host_port()) {
+    strip_port_type_ = Http::StripPortType::Any;
+  } else if (config.strip_matching_host_port()) {


Given that defaults are false, can we check if both are true and throw an error? I think this is safe to do and will approximate the future oneof?

Yes. We can do that. Changed and added a test. PTAL

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123

Thanks!

add skip any host port

2ddd7ae

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested review from alyssawilk and mattklein123 as code owners December 21, 2020 09:41

repokitteh-read-only bot added the api label Dec 21, 2020

repokitteh-read-only bot assigned htuch Dec 21, 2020

ramaraochavali mentioned this pull request Dec 21, 2020

Allow ignoring port in virtualHosts.domain #12647

Closed

ramaraochavali added 3 commits December 21, 2020 15:50

add to adminimpl

1746a3e

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix fuzz test

32d152f

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

one more change

154d751

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 assigned mattklein123 and unassigned htuch Dec 21, 2020

ramaraochavali commented Dec 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/match_any_port

1859247

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 requested changes Dec 28, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 28, 2020

ramaraochavali added 2 commits December 31, 2020 13:46

move to oneof in api and use port type enum

0f146e7

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

format

fcd5854

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Dec 31, 2020

ramaraochavali added 4 commits December 31, 2020 16:56

fix compilation and add additional test

a79fd44

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix format

0874581

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

format again

5130272

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix conn manager test

4d9663f

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 requested changes Jan 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 2, 2021

address review comments

88d826b

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Jan 3, 2021

properly add comments

dd2a67c

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

mattklein123 requested changes Jan 4, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 4, 2021

throw exception when both are set

39a35d1

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Jan 4, 2021

mattklein123 approved these changes Jan 4, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jan 4, 2021

mattklein123 merged commit e80b8ee into envoyproxy:master Jan 4, 2021

ramaraochavali deleted the fix/match_any_port branch January 5, 2021 04:40

howardjohn mentioned this pull request Jan 27, 2021

Path normalization controls #6589

Open

SpecialYang mentioned this pull request Mar 25, 2023

Remove deprecated flag ISTIO_GATEWAY_STRIP_HOST_PORT. istio/istio#44106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add support for skipping any port from host header #14491

http: add support for skipping any port from host header #14491

ramaraochavali commented Dec 21, 2020

repokitteh-read-only bot commented Dec 21, 2020

ramaraochavali Dec 22, 2020

mattklein123 Dec 28, 2020

mattklein123 left a comment

mattklein123 Dec 28, 2020

mattklein123 Dec 28, 2020

ramaraochavali commented Jan 1, 2021

mattklein123 left a comment

mattklein123 Jan 2, 2021

mattklein123 Jan 2, 2021

mattklein123 Jan 2, 2021

ramaraochavali commented Jan 3, 2021

mattklein123 left a comment

mattklein123 Jan 4, 2021

ramaraochavali Jan 4, 2021

mattklein123 Jan 4, 2021

ramaraochavali Jan 4, 2021

mattklein123 left a comment

		* @brief Remove the port part from host/authority header if it is equal to provided port or
		* provided port is 0.

http: add support for skipping any port from host header #14491

http: add support for skipping any port from host header #14491

Conversation

ramaraochavali commented Dec 21, 2020

repokitteh-read-only bot commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ramaraochavali commented Jan 1, 2021

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

ramaraochavali commented Jan 3, 2021

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

mattklein123 left a comment

Choose a reason for hiding this comment