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

perf(MuxPath): prefix and fullpath support rewrite_target #553

Merged
merged 4 commits into from
Mar 31, 2022

Conversation

aniaan
Copy link
Collaborator

@aniaan aniaan commented Mar 29, 2022

close: #552

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #553 (03aef3a) into main (0cd6b15) will increase coverage by 0.00%.
The diff coverage is 88.63%.

@@           Coverage Diff           @@
##             main     #553   +/-   ##
=======================================
  Coverage   78.51%   78.51%           
=======================================
  Files          94       95    +1     
  Lines       10878    10940   +62     
=======================================
+ Hits         8541     8590   +49     
- Misses       1850     1867   +17     
+ Partials      487      483    -4     
Impacted Files Coverage Δ
pkg/object/httpserver/mux.go 66.12% <76.19%> (+2.87%) ⬆️
pkg/object/httpserver/spec.go 13.33% <100.00%> (+6.19%) ⬆️
pkg/util/stringtool/stringtool.go 42.85% <100.00%> (ø)
pkg/cluster/cluster.go 53.06% <0.00%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd6b15...03aef3a. Read the comment docs.

Comment on lines 269 to 286
path := ctx.Request().Path()

if mp.path != "" {
ctx.Request().SetPath(mp.rewriteTarget)
return
}

if mp.pathPrefix != "" {
path = strings.Replace(path, mp.pathPrefix, mp.rewriteTarget, 1)
ctx.Request().SetPath(path)
return
}

if mp.pathRE != nil {
path = mp.pathRE.ReplaceAllString(path, mp.rewriteTarget)
ctx.Request().SetPath(path)
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect, for example:
if both mp.path and mp.pathPrefix are not empty and the request matches mp.pathPrefix, the path should be rewritten at L277, but was actually rewritten at L272, which is wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I missed the point that path and prefix and regexp can co-exist, I previously thought they were a mutually exclusive relationship, I'll fix that

@samutamm
Copy link
Contributor

samutamm commented Mar 30, 2022

This change could be documented here: controllers.md . Old doc

Use pathRegexp.[ReplaceAllString](https://golang.org/pkg/regexp/#Regexp.ReplaceAllString)(path, rewriteTarget) to rewrite request path

New doc:

Use pathRegexp.[ReplaceAllString](https://golang.org/pkg/regexp/#Regexp.ReplaceAllString)(path, rewriteTarget) or pathPrefix [strings.Replace](https://pkg.go.dev/strings#Replace) to rewrite request path

@aniaan
Copy link
Collaborator Author

aniaan commented Mar 30, 2022

  1. If path, prefix, and regexp are empty, but rewriteTarget is not empty, the newly created spec will fail to verify. For the existing spec, rewriteTarget will not take effect, because I didn't expect the three to be empty at present, but The scenario where the path is still to be rewritten, so a check is added
  2. There is also the handleRewrite function, you can see if it is appropriate to do so

rule:

  1. if "match path" and rewriteTarget not empty, request.setPath(rewriteTarget)
  2. if "match prefix" and rewriteTarget not empty, request.setPath(strings.Replace(path, prefix, 1))
  3. if "match regexp" and rewriteTarget not empty, request.setPath(regexp.Replace(path, regexpRepl))

@samutamm @localvar

@aniaan aniaan requested review from localvar and samutamm March 30, 2022 08:53
@samutamm samutamm merged commit 0dc0d5a into easegress-io:main Mar 31, 2022
@aniaan aniaan deleted the feature/prefix-support-rewrite branch April 7, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PathPrefix and Full Path support rewriteTarget
4 participants