Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow wildcard fqdn's for HTTPProxy resources #4145

Merged
merged 7 commits into from
Nov 1, 2021

Conversation

stevesloka
Copy link
Member

Fixes #1228

Signed-off-by: Steve Sloka slokas@vmware.com

Fixes projectcontour#1228

Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka stevesloka added the release-note/small A small change that needs one line of explanation in the release notes. label Oct 29, 2021
@stevesloka stevesloka requested a review from a team as a code owner October 29, 2021 19:18
@stevesloka stevesloka requested review from tsaarni and youngnick and removed request for a team October 29, 2021 19:18
Signed-off-by: Steve Sloka <slokas@vmware.com>
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #4145 (c04b366) into main (0d824ba) will decrease coverage by 1.68%.
The diff coverage is 46.50%.

❗ Current head c04b366 differs from pull request most recent head d018db7. Consider uploading reports for the commit d018db7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4145      +/-   ##
==========================================
- Coverage   74.64%   72.95%   -1.69%     
==========================================
  Files         112      115       +3     
  Lines        9722    10096     +374     
==========================================
+ Hits         7257     7366     +109     
- Misses       2309     2574     +265     
  Partials      156      156              
Impacted Files Coverage Δ
cmd/contour/serve.go 12.60% <0.00%> (-0.43%) ⬇️
internal/controller/gateway.go 0.00% <0.00%> (ø)
internal/controller/helpers.go 0.00% <0.00%> (ø)
internal/controller/tcproute.go 0.00% <0.00%> (ø)
internal/controller/udproute.go 0.00% <0.00%> (ø)
internal/k8s/informers.go 0.00% <0.00%> (ø)
internal/xdscache/v3/route.go 84.47% <72.00%> (-4.12%) ⬇️
internal/envoy/v3/route.go 66.49% <90.47%> (+1.40%) ⬆️
internal/dag/gatewayapi_processor.go 94.45% <98.19%> (+0.36%) ⬆️
internal/dag/cache.go 94.08% <100.00%> (-0.17%) ⬇️
... and 11 more

Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work.

(once the tests are fixed, obviously).

@sunjayBhatia
Copy link
Member

#4147 might be a fix to the test flakes here

// as Envoy's virtualhost hostname wildcard matching can match multiple
// labels. This match ignores a port in the hostname in case it is present.
if strings.HasPrefix(rootProxy.Spec.VirtualHost.Fqdn, "*.") {
r.HeaderMatchConditions = []HeaderMatchCondition{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think maybe only change I would make is to pull this out into a helper now its used in a few places, so we get something like:

if strings.HasPrefix(rootProxy.Spec.VirtualHost.Fqdn, "*.") {
    r.HeaderMatchConditions = append(r.HeaderMatchConditions, wildcardDomainHeaderMatch(rootProxy.Spec.VirtualHost.Fqdn))
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

test/e2e/httpproxy/fqdn_test.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka
Copy link
Member Author

#4147 might be a fix to the test flakes here

That still may help, but what I found was I think there was a timing issue since many of our tests use projectcontour.io, the wildcard *.projectcontour.io was left around and causing routes to direct to the wildcard routes. Changing to a non-projectcontour.io domain seems to have fixed that specific flake.

@stevesloka stevesloka merged commit 893791c into projectcontour:main Nov 1, 2021
@stevesloka stevesloka deleted the wildcardHTTPProxy branch November 1, 2021 18:32
skriss added a commit to skriss/contour-operator that referenced this pull request Nov 1, 2021
Updates operator per Contour changes:

projectcontour/contour#4141
projectcontour/contour#4138
projectcontour/contour#4145

Signed-off-by: Steve Kriss <krisss@vmware.com>
stevesloka pushed a commit to projectcontour/contour-operator that referenced this pull request Nov 2, 2021
Updates operator per Contour changes:

projectcontour/contour#4141
projectcontour/contour#4138
projectcontour/contour#4145

Signed-off-by: Steve Kriss <krisss@vmware.com>
@stevesloka stevesloka added this to the 1.20.0 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add support for wildcard virtual hosts
3 participants