router: support secondary lookup without port if a port is in host/authority header

[gnz00]

Description: A naive attempt to add the behavior requested in #886. The router will try to match without the port suffix if it couldn't find a match with the original host or :authority header value. I'm inclined to put this behind configuration as it adds implicit behavior.
Risk Level: High
Testing: Added 2 unit test cases.
Docs Changes: Added entry to route_matching.rst
Release Notes: Added entry to version_history.rst
Fixes: #886

Disclaimer: Not familiar with C++
Signed-off-by: Alexander Mays amays@homeaway.com

[dio]

@junr03 would you mind to look at this? Thanks!

[zuercher]

By my reading of the RFC, the port in the Host header comes from the URI. Since it may be omitted in the URI if it's the default for the protocol, it may also be omitted from the Host header in that case.

I think it would acceptable to modify the virtual host lookup to handle that case without a configuration option. For example, an inbound request with Host: hostname:80 would match a virtual host with the domain "hostname" for http requests and Host: hostname:443 would match the domain hostname for https).

This change seems to modify virtual host lookup to match Host headers with any port to any virtual host configured, which I think merits a flag since it's unexpected. Is there a specific use case you have for this? Perhaps a suitable compromise would be to allow VirtualHost domain names of the form "hostname:*", which would allow this behavior if specifically configured?

[gnz00]

I think there are solid arguments for both of your suggestions:

1. Implicitly match only well known protocols
2. Allow VHs to be configured to explicitly match a port wildcard

However, I can see some users expecting domains to match without the explicit port definition in VH, e.g. other unofficial well known HTTP ports such as 8080, or debug/admin/management ports. I believe VHs matching by domain is particularly confusing to users as Domains in the context of DNS cannot contain ports. Perhaps changing the configuration to reflect that the match is actually on :authority/host might lead people to naturally conclude that they should enumerate the ports or use the suggested port wildcard syntax.

[gnz00]

The fundamental question is what is a VH domain? Is it a DNS-1123 domain? If so, would we rather add validation to the configuration?

In it’s current state, the PR would bring Envoy behavior inline with the spec without breaking existing configuration, while also permitting explicit matching on ports.

Alternatively, we could determine that a VH domain is actually a matcher on the authority header and add the ability to specify a wildcard port.

From the linked issue, it seems that validating domain per DNS-1123 would bring Envoy behavior closer to that of Kubernetes Ingress controllers and Nginx, although I have not validated that myself.

[zuercher]

The docs for VirtualHost.domain specify that it is the host/authority value.

I think the reference to Kubernetes ingress controllers in #886 is orthogonal because that's a kubernetes limitation: "The : delimiter is not respected because ports are not allowed. Currently the port of an Ingress is implicitly :80 for http and :443 for https." (per https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#ingressrule-v1beta1-networking-k8s-io).

I think it's also already possible to configure a Virtual host that accepts requests for a plain hostname as well a hostname with any port by specifying domains: ["host", "host:*"], which seems like it would be equivalent to what this changes does by default. (What I had suggested in an earlier comment is already implemented, as it turns out.)

I think the only bug is that a user-agent might send Host: foo:80 or Host: foo:443 if the target URL has a redundant port and Envoy doesn't recognize those as being equivalent to Host: foo.

So maybe we need to go back to #886 and see what the actual bug problem is that needs resolving?

[gnz00]

Well that makes things easier. If we are to continue with just adding implicit matching for well known ports, do you have any opinions on what ports should be supported and how that mapping should be maintained? We should also assert in the tests that an explicit match on a port for the same host is preferred to an explicit match on a wildcard. I’m aware this is generally handled by the wildcard matching logic, but would be nice to capture that specific use case.

[zuercher]

I think you just need to handle :80 for http and :443 for https as those are the only schemes defined in the standard. IIRC, GRPC will present as http or https as well. You can get the scheme out of the HeaderMap.

cc @alyssawilk and @PiotrSikora in case they have more input

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[PiotrSikora]

@gnz00 Virtual Hosts are matched against Host / :authority headers, which contain authority portion of the URI, not a DNS domain.

Regarding default ports, see RFC for http & https URI schemes. Basically, missing port number implies default port number for the URI scheme, so http://example.com implies http://example.com:80, and we can treat those 2 interchangeably, but that's the only valid conversion.

This PR strips port numbers and treats http://example.com:8080 and http://example.com:9999 the same, which is definitely wrong for the purpose of establishing authority.

[gnz00]

Hi Piotr, I think that is the conclusion we came to for pragmatic reasons. The remaining question is whether or not “domains” as a property of a virtual host is the correct nomenclature as they do not actually conform to the spec in RFC-1123: https://tools.ietf.org/html/rfc1123. If there is consensus that this is the intended behavior or not worth the risk, I can update the PR to only match the default ports well known schemes. I have also tested that wildcard matches for ports is working as @zuercher indicated.

…

On May 23, 2019, at 10:31 PM, Piotr Sikora ***@***.***> wrote: @PiotrSikora requested changes on this pull request. @gnz00 Virtual Hosts are matched against Host / :authority headers, which contain authority portion of the URI, not a DNS domain. Regarding default ports, see RFC for http & https URI schemes. Basically, missing port number implies default port number for the URI scheme, so http://example.com implies http://example.com:80, and we can treat those 2 interchangeably, but that's the only valid conversion. This PR strips port numbers and treats http://example.com:8080 and http://example.com:9999 the same, which is definitely wrong for the purpose of establishing authority. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[zuercher]

My opinion is that we should just update the PR to only match the well known schemes.

I agree that the field name would be more correct if it were authority, but that change would be painful and I don't think it should be undertaken at this point. A clarifying note in the field's comment would be nice.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!