StripTrailingHostDot: Expose new configuration option to enable Envoy removal of trailing dot on hostnames

[saley89]

Fixes #6334

This PR adds:

• A new configuration option in the NetworkParameters of Contour.
• The new option is called StripTrailingHostDot.
• This option when set to true will configure all Envoy routing to strip any trailing dot from a hostname before processing the request. This will allow it to be matched by any existing routes for that hostname without a dot.
• Tests to show the configuration option set to to true being added to the underlying listener configuration.

[github-actions]

Hi @saley89! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[saley89]

Any thoughts on this one? This is quite a small, safe change adding a new configuration flag to add the option to the configured listener.

It is based heavily on the XffNumTrustedHops flag and usage in similar areas of the code.

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.70%. Comparing base (39c7cb9) to head (3ef268b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/contour/serve.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6792   +/-   ##
=======================================
  Coverage   80.70%   80.70%           
=======================================
  Files         131      131           
  Lines       19805    19816   +11     
=======================================
+ Hits        15983    15993   +10     
- Misses       3532     3533    +1     
  Partials      290      290           

Files with missing lines	Coverage Δ
cmd/contour/servecontext.go	84.37% <100.00%> (+0.03%)	⬆️
internal/contourconfig/contourconfiguration.go	98.18% <100.00%> (+0.01%)	⬆️
internal/envoy/v3/listener.go	98.50% <100.00%> (+<0.01%)	⬆️
internal/xdscache/v3/listener.go	91.64% <100.00%> (+0.07%)	⬆️
pkg/config/parameters.go	87.69% <100.00%> (+0.03%)	⬆️
cmd/contour/serve.go	21.91% <0.00%> (-0.03%)	⬇️

[saley89]

@tsaarni for this one is there anything else I need to do/add or is it in a state that it can be added to an upcoming release? I ask mainly for our planning purposes as we are maintaining a fork for now with some of our changes and will take the upstream once they become merged.

Thanks.

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[tsaarni]

Hi @saley89 apologies for the extra-delayed review! The change looks great overall! Just a couple of small adjustments are needed. I hope you are still able to make these despite the time that has passed.

There are no conflicts, but some updates on the main branch require a rebase, running make generate, and applying the following small changes:

diff --git a/internal/featuretests/v3/listeners_test.go b/internal/featuretests/v3/listeners_test.go
index a98d41fa..525c7601 100644
--- a/internal/featuretests/v3/listeners_test.go
+++ b/internal/featuretests/v3/listeners_test.go
@@ -1234,6 +1234,9 @@ func TestHTTPProxyStripTrailingHostDot(t *testing.T) {
        })

        defer done()
+       envoyGen := envoy_v3.NewEnvoyGen(envoy_v3.EnvoyGenOpt{
+               XDSClusterName: envoy_v3.DefaultXDSClusterName,
+       })

        rh.OnAdd(fixture.NewService("backend").
                WithPorts(core_v1.ServicePort{Name: "http", Port: 80}))
@@ -1264,7 +1267,7 @@ func TestHTTPProxyStripTrailingHostDot(t *testing.T) {
        // verify that the xff-num-trusted-hops have been set to 1.
        httpListener := defaultHTTPListener()

-       httpListener.FilterChains = envoy_v3.FilterChains(envoy_v3.HTTPConnectionManagerBuilder().
+       httpListener.FilterChains = envoy_v3.FilterChains(envoyGen.HTTPConnectionManagerBuilder().
                RouteConfigName("ingress_http").
                MetricsPrefix("ingress_http").
                AccessLoggers(envoy_v3.FileAccessLogEnvoy("/dev/stdout", "", nil, contour_v1alpha1.LogLevelInfo)).
diff --git a/internal/xdscache/v3/listener_test.go b/internal/xdscache/v3/listener_test.go
index 3e271ac4..18f9d32d 100644
--- a/internal/xdscache/v3/listener_test.go
+++ b/internal/xdscache/v3/listener_test.go
@@ -2148,7 +2148,7 @@ func TestListenerVisit(t *testing.T) {
                                Name:    ENVOY_HTTP_LISTENER,
                                Address: envoy_v3.SocketAddress("0.0.0.0", 8080),
                                FilterChains: envoy_v3.FilterChains(
-                                       envoy_v3.HTTPConnectionManagerBuilder().
+                                       envoyGen.HTTPConnectionManagerBuilder().
                                                RouteConfigName(ENVOY_HTTP_LISTENER).
                                                MetricsPrefix(ENVOY_HTTP_LISTENER).
                                                AccessLoggers(envoy_v3.FileAccessLogEnvoy(DEFAULT_HTTP_ACCESS_LOG, "", nil, contour_v1alpha1.LogLevelInfo)).

Additionally, the config file documentation needs an update:

diff --git a/site/content/docs/main/configuration.md b/site/content/docs/main/configuration.md
index 5277ab79..5fad112b 100644
--- a/site/content/docs/main/configuration.md
+++ b/site/content/docs/main/configuration.md
@@ -184,6 +184,7 @@ The network configuration block can be used to configure various parameters netw
 | ---------------- | ---- | ------- | ----------------------------------------------------------------------------------------------------------------------- |
 | num-trusted-hops | int  | 0       | Configures the number of additional ingress proxy hops from the right side of the x-forwarded-for HTTP header to trust. |
 | admin-port       | int  | 9001    | Configures the Envoy Admin read-only listener on Envoy. Set to `0` to disable.                                          |
+| strip-trailing-host-dot | bool | false | Defines if trailing dot of the host should be removed from host/authority header before any processing of request by HTTP filters or routing. This affects the upstream host header. Without setting this option to true, incoming requests with host example.com. will not match against route with domains match set to example.com. See [the Envoy documentation][15] for more information. |

 ### Listener Configuration

@@ -539,3 +540,4 @@ connects to Contour:
 [12]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-request-timeout
 [13]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-delayed-close-timeout
 [14]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener.proto#config-listener-v3-listener-connectionbalanceconfig
+[15]: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto?highlight=strip_trailing_host_dot

[saley89]

@tsaarni no problem at all. Those changes should be made now.

Please let me know if you see any issues/require any further changes and thanks for following it up.

[tsaarni]

Thank you @saley89 for the contribution!