Add support for wildcard hosts on Ingresses

[sunjayBhatia]

• Wildcard virtualhosts now allowed
• Envoy's virtualhost matching does not match on a single DNS label like
  we want to implement
• For wildcard host routes, we add a header match rule
  to match on the ":authority" header that is more strict and will only
  match one DNS label wildcard
• Adds an integration test to validate behavior with a wildcard
  certificate
• Passes ingress conformance tests for wildcards
• Adds the "regex" header match strategy (only internal for now)

Fixes #2138
Updates #2139

[codecov]

Codecov Report

Merging #3381 (5584e98) into main (2982d58) will increase coverage by 0.04%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main    #3381      +/-   ##
==========================================
+ Coverage   76.66%   76.70%   +0.04%     
==========================================
  Files         100      100              
  Lines        7066     7088      +22     
==========================================
+ Hits         5417     5437      +20     
- Misses       1533     1535       +2     
  Partials      116      116              

Impacted Files	Coverage Δ
internal/dag/dag.go	96.93% <ø> (ø)
internal/featuretests/kubernetes.go	100.00% <ø> (ø)
internal/sorter/sorter.go	97.46% <80.00%> (-1.19%)	⬇️
internal/dag/ingress_processor.go	94.92% <100.00%> (+0.15%)	⬆️
internal/envoy/v3/listener.go	98.15% <100.00%> (+0.02%)	⬆️
internal/envoy/v3/route.go	99.02% <100.00%> (+0.01%)	⬆️

[sunjayBhatia]

How to run Ingress conformance tests:

• Check out this branch of the tests: https://github.com/sunjayBhatia/ingress-controller-conformance/tree/focus-host-rules-tests
  • Focuses the host rules tests (some non-wildcard ones are a bit flaky so beware) and uses a k8s TLS secret instead of an Opaque one
• Run make build-image
• Load the built image into your kind cluster
• Add --image-pull-policy=IfNotPresent to the sonobuoy args in _integration/testsuite/run-ingress-conformance.sh and set INGRESS_CONFORMANCE_IMAGE to the name of the image built above
• run make check-ingress-conformance
• Results are printed to the screen and a tarball in your working directory with the results should show the wildcard tests pass

[sunjayBhatia]

Current caveats/TODOs:

• Docs
• Mismatched SNI and :authority header is not detected (integration test failure should show this)
  • We don't have the ability to get the requested server name in Lua at the moment so the Lua script for checking the above is disabled for wildcard hosts
  • If we didn't disable the Lua script, no HTTPS requests would work going to a wildcard host (checking for equality between bar.foo.com and *.foo.com cannot work!)
• Currently to use a wildcard route with TLS you need a cert that is intended to exactly match the hostname, e.g. as in the integration/feature tests:
  • This is the only way to get TLS on a wildcard route, until we also support no hostnames on a secret

apiVersion: networking.k8s.io/v1
kind: Ingress
...
spec:
  - hosts:
      - "*.foo.com"
    secretName: wildcard
  rules:
  - host: *.foo.com
     ...

• Still have to make a decision on if allowing a secret for a specific hostname to be applied on a wildcard Ingress rule is allowed, for example for the below, should bar be applied to *.foo.com, should it be an error, ignored? (seems like the opinion was to ignore this combo for the wildcard host/raise an error, though with Ingress we don't have status so it might be easiest to leave it alone)
  • As per the spec, you only get TLS on a hostname if the secret has the exact same hostname string or no hostnames in the list so this is a non issue, bar is not applied to *.foo.com, same for below

apiVersion: networking.k8s.io/v1
kind: Ingress
...
spec:
  - hosts:
      - bar.foo.com
    secretName: bar
  rules:
  - host: "*.foo.com"
     ...

• Something that could be a separate PR: certs for wildcard hosts allowed to be applied to Ingress rules that match the wildcard, e.g in the below, wildcard would be applied to the bar.foo.com rule

apiVersion: networking.k8s.io/v1
kind: Ingress
...
spec:
  - hosts:
      - "*.foo.com"
    secretName: wildcard
  rules:
  - host: bar.foo.com
     ...

[sunjayBhatia]

Going to mark this as ready just so it notifies people to review to get some feedback

[sunjayBhatia]

See envoyproxy/envoy#15142

[sunjayBhatia]

Still TODO: Add testing and fix for combining a catchall virtualhost (default backend or no supplied hostname in a rule) with a wildcard route b/c as of right now they are both using * for virtualhost domain which is invalid

Right now this change attempts to do the following:

• for HTTP and HTTPS wildcard virtualhosts, match on * and only match routes if the :authority header matches something like *.foo.com:80 (where the port is optional)
  • this is because we allow you to supply bar.foo.com:<port> as a hostname that should match bar.foo.com so to match this for wildcards we have to jump through some hoops
• this runs into an issue b/c if you have a "default backend" or Ingress rule without a host, that gets mapped to the * virtualhost as well
  • this is not allowed by Envoy
  • See: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-virtualhost-domains

Notes:

• cannot add a secret that maps to host * in an Ingress (so is it ever possible to do TLS on a default backend?)
• we could have a Lua filter or something strip off the port from the :authority field so once a request gets to the router we don't have to care about trying to match the optional port and can continue to have the virtualhost match on the wildcard domain so we don't have conflicts with *
• This is mainly just an issue for the HTTP listener as there is one route config ingress_http for that listener and * has to be unique as a domain per route config
  • Could become a problem in Gateway APIs as we can have * as a hostname match and allow TLS

[youngnick]

Still TODO: Add testing and fix for combining a catchall virtualhost (default backend or no supplied hostname in a rule) with a wildcard route b/c as of right now they are both using * for virtualhost domain which is invalid

I don't get this one. Are you saying the current catchall vhost is implemented using a * domain when we send the config to Envoy? Isn't a wildcard route more specific than that, because only the first label can be a wildcard, and there has to be more than one label in a wildcard hostname?

Right now this change attempts to do the following:

• for HTTP and HTTPS wildcard virtualhosts, match on * and only match routes if the :authority header matches something
  • this is because we allow you to supply bar.foo.com:<port> as a hostname that should match bar.foo.com so to match this for wildcards we have to jump through some hoops

Is that part of the Ingress spec? Really? Doesn't feel like we should be doing that.

...

• Could become a problem in Gateway APIs as we can have * as a hostname match and allow TLS

I didn't think that * by itself was a valid hostname, I thought you needed at least two labels to be able to do a wildcard.

[sunjayBhatia]

Is that part of the Ingress spec? Really? Doesn't feel like we should be doing that.

We have this:

contour/internal/envoy/v3/route.go

Lines 263 to 266 in 711001c

	if hostname != "*" {
	// NOTE(jpeach) see also envoy.FilterMisdirectedRequests().
	domains = append(domains, hostname+":*")
	}

Which was introduced b/c of 8b16fd8

This is a contour-specific thing but presumably we would want wildcard routes to ignore the port if present just as on a precise hostname.

If we try to do a route match for *.foo.com we would also generate *.foo.com:* which Envoy accepts but it doesn't actually work to do pre AND post wildcard matching (the docs say either-or and empirically this is true as well, this test case fails: https://github.com/projectcontour/contour/pull/3381/files#diff-0261dd69a192ff188fa76df82a9df9b5c003c1c92fff658d47e5151f2894e857R126).

I don't get this one. Are you saying the current catchall vhost is implemented using a * domain when we send the config to Envoy? Isn't a wildcard route more specific than that, because only the first label can be a wildcard, and there has to be more than one label in a wildcard hostname?

To clarify, this is only implemented with a less specific route domain match b/c of the port issue as described above. To implement ignoring a supplied port in the host, I've made it so wildcard routes match * and then we use the header match rules to match the backend (with a regex that makes the port optional) for the wildcard as specified by the user (we already do need the header match regex to limit Envoy to matching a single DNS label in a wildcard route, otherwise it will allow *.foo.com to match a.b.foo.com). Not the greatest as we then end up making Envoy do a lot more work and it results in some convoluted logic in Contour.

Another solution would be to strip the port from the :authority header in the early Lua filter so the router doesn't even need to think about matching the port with a post-wildcard and the matched domains could just be a single entry, making precise and wildcard hostname matches simpler. Downsides of this are more Lua, maybe the port could be relevant to some application so stripping it is no good? (Added a PR here: #3422)

[sunjayBhatia]

I didn't think that * by itself was a valid hostname, I thought you needed at least two labels to be able to do a wildcard.

GatewayAPI Listener.hostname says it can be empty or *: https://gateway-api.sigs.k8s.io/spec/#networking.x-k8s.io/v1alpha1.Listener

[sunjayBhatia]

This requires #3458 as it makes life a lot simpler!

[sunjayBhatia]

This still relies on envoyproxy/envoy#15215 in an Envoy release, waiting for Envoy 1.18

[sunjayBhatia]

Now that we have #3458 it removes all the logic we would have needed around polluting the * wildcard host used for default backends or rules without a hostname so those comments can be disregarded

[sunjayBhatia]

Still validating ingress conformance tests again since they are flaky

Getting a 503 on https://github.com/kubernetes-sigs/ingress-controller-conformance/blob/79bd068cbb31d77c2a060f61eea795f9791a3d48/features/host_rules.feature#L70-L76 when running the tests

[sunjayBhatia]

Note: With this change we will need to update Envoy first before Contour b/c we are using a net new Envoy Lua API

[skriss]

this looks pretty good to me, pending a new Envoy release.

[sunjayBhatia]

TODO: Add sorting of regex header match type, realized while working on path matches that this needs to be done as well, at least tested since we are doing an interesting comparison of string version of proto types right now Done

[youngnick]

LGTM with one small followup.

[sunjayBhatia]

Going to try to fix siteproof in a separate PR

[sunjayBhatia]

See #3596 for siteproof fixes

[sunjayBhatia]

This should be ready for review again

Note if we merge this for 1.15 we will need to mention in upgrade instructions that the Envoy deployment has to be updated first before Contour. This is a hard requirement if you are using Ingresses with wildcard hostnames. If you are not, this is not as critical (things will work if you upgrade Contour first).

[skriss]

LGTM