-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
gateway-api: Fix double slash when trying to strip path prefix #28294
Conversation
Commit e629c88f15ebe94171352dd68f4dc68049252a97 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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Okay, so I found the root cause of the issue. It is indeed envoy problem. For whatever reason merging slashes only works between path segments (it seems like they are first merging slashes and only then doing path rewrites). |
Commit 2185aa43bb39751c1f54b7829ca6d8b38b5d36f7 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 can you take a look please? |
@nxy7 seems like your PR includes a merge of the main branch, please rebase your changes on top of main instead. Also try squashing into as few commits as possible please. |
ab9f994
to
edfb1f8
Compare
I've kind of made a mess with this git history (sorry for that, I'm not very good at using git), but it should be good now. I've reduced commits to 3 and rebased all changes on top of main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR.
Please find below my comments. Also, can you help to merge all commits into one as we are using rebase and merge strategy to keep main history clean.
vendor/sigs.k8s.io/gateway-api/conformance/tests/httproute-rewrite-path.go
Outdated
Show resolved
Hide resolved
I've applied all your suggestions @sayboras, hope it's okay now. |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and LGTM ✔️
I tried to resolve the conflict and merge this in, however, it seems like I didn't have permission to push to your fork. Can you help to resolve the conflict? It should be quite straightforward like the below main...sayboras:cilium:tam/resolve-conflict-28294 |
Path prefixes could not be stripped which lead to double slashes. Fixes: cilium#28270 Signed-off-by: Dawid Danieluk <lolnoxy@gmail.com>
Done :-) |
/test |
/test |
Thanks a lot for your help from analysing to sending the patch 🥇 |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Stripping path prefix leads to double slashes in URI path. To fix that I've enabled envoy
merge_slashes
option.Fixes: #28270