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

gateway-api: Fix double slash when trying to strip path prefix #28294

Merged
merged 1 commit into from Oct 6, 2023

Conversation

nxy7
Copy link
Contributor

@nxy7 nxy7 commented Sep 26, 2023

  • Merge slashes in URI path
  • Gateway to CEC translation fix

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Stripping path prefix leads to double slashes in URI path. To fix that I've enabled envoy merge_slashes option.

Fixes: #28270

Fix: Gateway API double slash while stripping path prefix

@nxy7 nxy7 requested review from a team as code owners September 26, 2023 16:04
@nxy7 nxy7 requested a review from sayboras September 26, 2023 16:04
@maintainer-s-little-helper
Copy link

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

@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 Sep 26, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 26, 2023
@nxy7

This comment was marked as outdated.

@nxy7

This comment was marked as outdated.

@nxy7

This comment was marked as outdated.

@nxy7
Copy link
Contributor Author

nxy7 commented Sep 27, 2023

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).
Also, we cannot pass empty rewritePrefix as that's ignored by envoy. The only solution to this issue seems to be using regexp rewrite, that can in fact accept empty substitution string.
I've tested ebb5dce and it strips prefixes correctly. To do that I've had to pass *model.HTTPRoute to getRouteAction and pathPrefixMutation because regexp in pathPrefixMutation needs to be aware of path that it's supposed to substitute.
What do you think about that solution @sayboras ?

@maintainer-s-little-helper
Copy link

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

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 28, 2023
@lmb
Copy link
Contributor

lmb commented Oct 2, 2023

@sayboras can you take a look please?

@lmb
Copy link
Contributor

lmb commented Oct 2, 2023

@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.

@lmb lmb added sig/agent Cilium agent related. area/servicemesh GH issues or PRs regarding servicemesh release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 2, 2023
@nxy7 nxy7 force-pushed the main branch 2 times, most recently from ab9f994 to edfb1f8 Compare October 2, 2023 11:33
@nxy7
Copy link
Contributor Author

nxy7 commented Oct 2, 2023

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.

Copy link
Member

@sayboras sayboras left a 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.

@nxy7
Copy link
Contributor Author

nxy7 commented Oct 2, 2023

I've applied all your suggestions @sayboras, hope it's okay now.

@sayboras
Copy link
Member

sayboras commented Oct 2, 2023

/test

@sayboras sayboras changed the title Fix Gateway API double slash when trying to strip path prefix gateway-api: Fix double slash when trying to strip path prefix Oct 2, 2023
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks and LGTM ✔️

@sayboras sayboras added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Oct 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.3 Oct 3, 2023
@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 Oct 3, 2023
@sayboras
Copy link
Member

sayboras commented Oct 3, 2023

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

@sayboras sayboras added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 3, 2023
@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 Oct 3, 2023
Path prefixes could not be stripped which lead to double slashes.

Fixes: cilium#28270
Signed-off-by: Dawid Danieluk <lolnoxy@gmail.com>
@nxy7
Copy link
Contributor Author

nxy7 commented Oct 3, 2023

Done :-)

@sayboras
Copy link
Member

sayboras commented Oct 3, 2023

/test

@sayboras sayboras removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 3, 2023
@ti-mo ti-mo removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 4, 2023
@ti-mo
Copy link
Contributor

ti-mo commented Oct 4, 2023

/test

@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 Oct 4, 2023
@sayboras
Copy link
Member

sayboras commented Oct 6, 2023

Thanks a lot for your help from analysing to sending the patch 🥇

@sayboras sayboras merged commit b825f4e into cilium:main Oct 6, 2023
59 of 61 checks passed
@sayboras sayboras mentioned this pull request Oct 6, 2023
9 tasks
@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 Oct 6, 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.3 Oct 6, 2023
@github-actions github-actions bot 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 Oct 9, 2023
@jrajahalme jrajahalme moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.3 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/community-contribution This was a contribution made by a community member. 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. sig/agent Cilium agent related.
Projects
No open projects
1.14.3
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

Gateway API HttpRoute cannot strip path prefix - double slash problem
4 participants