fix: higher routing path match priority for longer pathnames #1709
Conversation
Signed-off-by: David Kwon <[email protected]>
dkwon17
requested review from
nickboldt,
tolusha,
ibuziuk and
l0rd
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1709 +/- ##
==========================================
+ Coverage 60.22% 60.24% +0.01%
==========================================
Files 71 71
Lines 8438 8441 +3
==========================================
+ Hits 5082 5085 +3
Misses 3008 3008
Partials 348 348
|
|
@dkwon17 great catch! I have one question for my understanding: why can't we set |
|
@l0rd, thank you, I considered it, setting priority = 0 to use the default priority could still cause eclipse-che/che#22288 for existing workspaces when updating Che, since the traefik config for a workspace is updated when starting that workspace. For example, in today's Che, if we have these workspaces we may have trouble opening the If Che is updated to use default priorities for workspaces, and if we try to start and we can still have the problem since the rule for |
|
Oops, I had to make a few edits in the comment above |
There was a problem hiding this comment.
@dmytro-ndp @dkwon17 folks, once merged we need to backport for 3.7 since it is the last fix for the friendly routes blocker
|
@dkwon17 : how many times do you think it's needed to repeat steps 3-5 from PR description to make sure the issue is fixed in quay.io/dkwon17/che-operator:routing-priority ? I had successfully started 6 Python workspaces in Eclipse Che Next with quay.io/dkwon17/che-operator:routing-priority aboard not facing eclipse-che/che#22288 issue. |
|
Thank you @dmytro-ndp , I would say 6 workspaces are enough to verify this PR |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dkwon17, dmytro-ndp, ibuziuk, tolusha The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build 3.8 :: operator-bundle_3.x/1528: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3685: Console, Changes, Git Data |
|
Build 3.8 :: get-sources-rhpkg-container-build_3.x/3529: devspaces-operator-bundle : 3.x :: Failed in 53504017 : BREW:BUILD/STATUS:UNKNOWN |
|
Build 3.8 :: operator-bundle_3.x/1529: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3686: Console, Changes, Git Data |
|
Build 3.8 :: operator-bundle_3.x/1563: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3730: Console, Changes, Git Data |
|
Build 3.8 :: get-sources-rhpkg-container-build_3.x/3575: devspaces-operator-bundle : 3.x :: Failed in 53609179 : BREW:BUILD/STATUS:UNKNOWN |
|
Build 3.8 :: operator-bundle_3.x/1564: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3731: Console, Changes, Git Data |
|
Build 3.8 :: push-latest-container-to-quay_3.x/2713: Console, Changes, Git Data |
|
Build 3.8 :: copyIIBsToQuay/1501: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3731: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/3576 triggered; /job/DS_CI/job/dsc_3.x triggered; |
|
Build 3.8 :: operator-bundle_3.x/1564: Upstream sync done; /DS_CI/sync-to-downstream_3.x/3731 triggered |
|
Build 3.8 :: dsc_3.x/1036: Console, Changes, Git Data |
|
Build 3.8 :: dsc_3.x/1037: Console, Changes, Git Data |
|
Build 3.8 :: operator-bundle_3.x/1565: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3732: Console, Changes, Git Data |
|
Build 3.8 :: push-latest-container-to-quay_3.x/2714: Console, Changes, Git Data |
|
Build 3.8 :: copyIIBsToQuay/1502: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3732: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/3577 triggered; /job/DS_CI/job/dsc_3.x triggered; |
|
Build 3.8 :: operator-bundle_3.x/1565: Upstream sync done; /DS_CI/sync-to-downstream_3.x/3732 triggered |
|
Build 3.8 :: dsc_3.x/1038: Console, Changes, Git Data |
|
Build 3.8 :: copyIIBsToQuay/1506: Console, Changes, Git Data |
|
Build 3.8 :: operator_3.x/255: Console, Changes, Git Data |
|
Build 3.8 :: sync-to-downstream_3.x/3740: Console, Changes, Git Data |
|
Build 3.8 :: get-sources-rhpkg-container-build_3.x/3584: devspaces-operator : 3.x :: |
What does this PR do?
Updates priorities according to the PathPrefix length to the routing pathprefix rule for the traefik config.
If no priority is set, longer rule length results in a higher priority by default: https://doc.traefik.io/traefik/routing/routers/#priority, but since priority is set to 100 today (see here, here), rule length is not being considered. To mitigate this, this PR takes the rule length (in this case, PathPrefix length) into account for the priority.
Before & after example in the Che traefik config for the workspace:
routers: workspace0d368dea85dc49aa: middlewares: - workspace0d368dea85dc49aa-strip-prefix - workspace0d368dea85dc49aa-header-rewrite - workspace0d368dea85dc49aa-auth - workspace0d368dea85dc49aa-headers - workspace0d368dea85dc49aa-errors - workspace0d368dea85dc49aa-retry - priority: 100 + priority: 133 rule: PathPrefix(`/cluster-admin/python-hello-world`) service: workspace0d368dea85dc49aa workspace0d368dea85dc49aa-3100-healthz: middlewares: - workspace0d368dea85dc49aa-strip-prefix - workspace0d368dea85dc49aa-header-rewrite - priority: 101 + priority: 134 rule: Path(`/cluster-admin/python-hello-world/3100/healthz`) service: workspace0d368dea85dc49aaImage: quay.io/dkwon17/che-operator:routing-priority
Screenshot/screencast of this PR
What issues does this PR fix or reference?
Fixes eclipse-che/che#22288
How to test this PR?
To test on OpenShift,
nextversionchectl server:deploy -p openshiftCreate a new workspacequay.io/dkwon17/che-operator:routing-priorityby editing the Che CSVWait a couple of minutes for the new che-operator pod to be created.
Confirm that On dogfooding, sometimes cannot open workspace, redirected to dashboard instead che#22288 cannot be reproduced
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.