Do a case insensitive match when checking for duplicate FQDNs

[erwbgy]

Do a case insensitive match when checking for duplicate FQDNs to avoid RouteConfiguration failures.

Fixes #3230

Signed-off-by: Keith Burdis keith.burdis@gs.com

[codecov]

Codecov Report

Merging #3231 (953149c) into main (88805b8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3231      +/-   ##
==========================================
- Coverage   75.44%   75.42%   -0.02%     
==========================================
  Files          97       97              
  Lines        6022     6023       +1     
==========================================
  Hits         4543     4543              
- Misses       1369     1371       +2     
+ Partials      110      109       -1     

Impacted Files	Coverage Δ
internal/dag/httpproxy_processor.go	94.16% <100.00%> (+0.01%)	⬆️
internal/k8s/log.go	63.04% <0.00%> (-6.53%)	⬇️
internal/dag/cache.go	96.04% <0.00%> (+0.79%)	⬆️

[youngnick]

LGTM, thanks for this!

[youngnick]

We'll need one more review on this one, we recently changed our review settings to require 2 reviews. But this will definitely be in 1.12 (due January 2021).

[sunjayBhatia]

Looks got to me, only additional change I could see would be some detail in an error message but given people should know virtual host names should be case insensitive it also seems unnecessary

[jpeach]

We'll need one more review on this one, we recently changed our review settings to require 2 reviews. But this will definitely be in 1.12 (due January 2021).

Maybe just verify whether we should lowercase the name before sending it to envoy?

[sunjayBhatia]

General question, are there any higher level tests we should add for this? Worth adding an integration test?

[youngnick]

Envoy only performs lower-case host name matches itself anyway, so we are not gaining anything by case insensitivity. See envoyproxy/envoy#2412 for the fix there.

[youngnick]

Seems like there is also currently case sensitivity for SNI hostnames, although it will be changed at some point, see envoyproxy/envoy#6199 for more.

This definitely will require a release note and some explanation, since in the event that people are relying on the non-RFC-compliant case sensitivity we currently have, they need to know.

However, given we have a reasonably important bug here (#3230), we need to have a solution, which this is. I don't feel that we should be tying ourselves in knots to maintain compatibility when that compatibility is against the relevant RFCs anyway. I won't merge this until the new year, to give people a chance to see this PR and comment if it will affect them.

[erwbgy]

One thing to note in the release notes is that configuring an FQDN like EXPLORER.domain.com still works after this fix, so won't break any users with uppercase in their URL hostnames. They just won't be able to configure multiple FQDNs that differ only by case.

The multiple case-insensitive FQDNs only works with tcpproxy today and fails when routes are used, so that is the only use-case affected by this change. On the other hand it is trivially easy to accidentally (or on purpose) break a multi-tenant Contour setup, though unlikely (one of our users did it twice).

[youngnick]

I munged the commit message a bit, @erwbgy.