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.

Sign up for GitHub

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

Jump to bottom

gateway-api: Avoid redirect loop when the same host name is used for http and https listeners #29115

Merged
merged 3 commits into from
Nov 13, 2023
Merged

gateway-api: Avoid redirect loop when the same host name is used for http and https listeners #29115

merged 3 commits into from
Nov 13, 2023

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Nov 12, 2023 •
edited

This is to fix the issue in which the wrong list of HTTP routes is used
to construct virtual host for insecure and secure routes. Ideally, only
related HTTP routes for the same listener ports should be considered.

Normally, this should not have any side effect, except using weightage
clusters for the multiple, but the same, backends. However, in case of
redirect filter, this will cause redirect loop as mentioned in the below
issue.

Fixes: #28186
Signed-off-by: Tam Mach tam.mach@cilium.io

gateway-api: Avoid redirect loop when the same host name is used for http and https listeners

@maintainer-s-little-helper
Copy link

Commit b839909 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 12, 2023
@sayboras
gateway-api: Update import name in unit test
f431768 
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
gateway-api: Refactor common component to its own func
fc06e05 
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras added release-note/bug This PR fixes an issue in a previous release of Cilium. area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-gateway-api needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Nov 12, 2023
@maintainer-s-little-helper
Copy link

Commit b839909 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@sayboras sayboras removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 12, 2023
@sayboras sayboras closed this Nov 13, 2023
@sayboras sayboras reopened this Nov 13, 2023
@sayboras
gateway-api: Correct list of HTTPRoutes in VirtualHost
97e1dcb 
This is to fix the issue in which the wrong list of HTTP routes is used
to construct virtual host for insecure and secure routes. Ideally, only
related HTTP routes for the same listener ports should be considered.

Normally, this should not have any side effect, except using weightage
clusters for the multiple, but the same, backends. However, in case of
redirect filter, this will cause redirect loop as mentioned in the below
issue.

Fixes: 299648f
Fixes: cilium#28186
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras
Copy link
Member Author

/test

@sayboras sayboras marked this pull request as ready for review November 13, 2023 02:28
@sayboras sayboras requested a review from a team as a code owner November 13, 2023 02:28
meyskens
meyskens approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM, this fixes an issue I was investigating last week :D

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 13, 2023
@julianwiedmann julianwiedmann merged commit e76fe9d into cilium:main Nov 13, 2023
61 checks passed
@julianwiedmann
Copy link
Member

This is your friendly reminder that the current release-note isn't very user-friendly :).

@julianwiedmann julianwiedmann mentioned this pull request Nov 13, 2023
Merged
@sayboras sayboras deleted the tam/redirect-loop branch November 13, 2023 21:36
@sayboras
Copy link
Member Author

This is your friendly reminder that the current release-note isn't very user-friendly :).

Thanks, I have updated the release note to give me little more context.

@sayboras sayboras changed the title gateway-api: Fix the HTTPS redirect loop gateway-api: Avoid redirect loop when the same host name is used for http and https listeners Nov 13, 2023
@gandro gandro mentioned this pull request Nov 15, 2023
Merged
6 tasks
@gandro
Copy link
Member

gandro commented Nov 15, 2023

I had troubles backporting this to v1.14 and had to give up. In particular, I think the first commit needs to be skipped and some of the refactored code in the second commit does not exist in v1.14. Thus marking as "backport/author"

@gandro gandro added the backport/author The backport will be carried out by the author of the PR. label Nov 15, 2023
@sayboras
Copy link
Member Author

Noted and thanks, I will perform the backport soon.

@rouke-broersma rouke-broersma mentioned this pull request Nov 28, 2023
Closed
2 tasks
This was referenced Nov 28, 2023
Closed
Merged
@sayboras sayboras added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.4 Nov 28, 2023
@sayboras sayboras added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.4 Nov 29, 2023
@nebril nebril mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meyskens meyskens meyskens approved these changes

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh backport/author The backport will be carried out by the author of the PR. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. feature/k8s-gateway-api ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.14.4
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTPRoute not accepted if namespace selector is used
4 participants
@sayboras @julianwiedmann @gandro @meyskens