-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add Path Replacement Rule #1374
Conversation
e4f0e02
to
12e21cd
Compare
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.
I left a few comments.
We should definitely also add some tests for PathReplace.ServeHTTP
and pathReplace
.
docs/basics.md
Outdated
@@ -82,6 +82,7 @@ Frontends can be defined using the following rules: | |||
- `HostRegexp: traefik.io, {subdomain:[a-z]+}.traefik.io`: Adds a matcher for the URL hosts. It accepts templates with zero or more URL variables enclosed by `{}`. Variables can define an optional regexp pattern to be matched. | |||
- `Method: GET, POST, PUT`: Method adds a matcher for HTTP methods. It accepts a sequence of one or more methods to be matched. | |||
- `Path: /products/, /articles/{category}/{id:[0-9]+}`: Path adds a matcher for the URL paths. It accepts templates with zero or more URL variables enclosed by `{}`. | |||
- `PathReplace: /serverless-path`: Replaces the path and adds the old path to the `X-Replaced-Path` header. Useful for mapping to AWS Lambda or Google Cloud Functions. |
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.
I'm not sure if it was a conscious decision, but we seem to have a pattern that Path*
match a path and optionally modify it (e.g., Path
, PathPrefix
, PathPrefixStrip
), while *Path
do modifications only (AddPrefix
). Regardless of how deliberate this naming choice was, I like it. :-)
Your new rule seems to fall into the second category. If that was your intention too, then I'd suggest to call it ReplacePath
for naming consistency reasons.
middlewares/pathReplace.go
Outdated
|
||
func (s *PathReplace) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
r.Header.Add("X-Replaced-Path", r.URL.Path) | ||
r.URL.Path = s.Path |
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.
A few questions:
- Is it intentional that you replace the entire path?
- Shouldn't we check if the path we intend to replace actually exists in
r.URL.Path
and only replace / add the header if it does? - What's the desired behavior if
s.Path
is shorter thanr.URL.Path
, i.e., we cannot replace it completely?
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.
Yes, the idea is to replace the entire path so that all paths will be forwarded to a single path on the backend.
For example, if I'm using Google Cloud Functions, I want both http://example.com/
and http://example.com/some/long/url.html
to be replaced with /some-function
, then I can simulate having multiple paths on https://some-project.cloudfunctions.net/some-function
.
Regarding your third question, I'm not sure why we would ever be unable to replace the path completely.
server.go
Outdated
@@ -66,6 +66,7 @@ type serverRoute struct { | |||
route *mux.Route | |||
stripPrefixes []string | |||
addPrefix string | |||
pathReplace string |
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.
Should be called replacePath
if you follow my naming reasoning above.
rules.go
Outdated
@@ -74,6 +74,13 @@ func (r *Rules) pathStrip(paths ...string) *mux.Route { | |||
return r.route.route | |||
} | |||
|
|||
func (r *Rules) pathReplace(paths ...string) *mux.Route { | |||
for _, path := range paths { | |||
r.route.addPrefix = path |
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.
You mean pathReplace
/ replacePath
here.
I agree with your suggestion to rename it, however I think you're missing the point of my changes. My reply to your comment further explains the idea. |
Thanks, I got a clearer picture now. Makes total sense to me. If you could do the rename and add a test or two, I'd be glad. :-) |
af428a8
to
1918db3
Compare
I have made the changes, but I feel like one might want to customize the header that it is set to too. |
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.
Hey Thanks @ssttevee !
One small comment, but other than that, LGTM :)
middlewares/replacePath.go
Outdated
} | ||
|
||
// SetHandler sets handler | ||
func (s *ReplacePath) SetHandler(Handler http.Handler) { |
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.
Is it needed (apart from tests) ?
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.
I was wondering the same thing for the AddPrefix
handler. I thought it might be part of an interface so I copied it anyways.
I'll remove it since you don't think it's needed either.
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.
Some more comments.
I'd also prefer snail case instead of camel case in file names. I know we use the later elsewhere, but it's really not Go idiomatic.
middlewares/replacePath_test.go
Outdated
} | ||
|
||
for _, path := range paths { | ||
req, err := http.NewRequest("ANY", "http://localhost"+path, nil) |
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.
Please use subtests and pass path
to t.Run
. Right now, if a test fails, there's no way to tell which one did.
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.
Unless ANY
is some kind of special place holder accepted by Go/HTTP, I'd rather like to use GET
. This will be safe to work in the future too.
middlewares/replacePath_test.go
Outdated
package middlewares_test | ||
|
||
import ( | ||
"github.com/containous/traefik/middlewares" |
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.
Please move Github imports below stdlib ones, separated by a blank line.
rules.go
Outdated
@@ -74,6 +74,13 @@ func (r *Rules) pathStrip(paths ...string) *mux.Route { | |||
return r.route.route | |||
} | |||
|
|||
func (r *Rules) replacePath(paths ...string) *mux.Route { | |||
for _, path := range paths { | |||
r.route.addPrefix = path |
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.
server.go
Outdated
@@ -791,6 +792,14 @@ func (server *Server) wireFrontendBackend(serverRoute *serverRoute, handler http | |||
} | |||
} | |||
|
|||
// path replace | |||
if len(serverRoute.replacePath) > 0 { |
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.
This definitely needs a 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.
there is no existing test file for server.go, should I make one to only test this part?
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.
Please do.
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.
actually, does this function even need to be a method of Server
? it doesn't access any of it's fields.
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.
Yeah good catch, we could convert it from a method to a function right away. Would simplify testing.
@ssttevee do you the time to update this PR before the upcoming feature freeze for the 1.3 release? |
2d0689d
to
651befc
Compare
64759a8
to
180426d
Compare
I couldn't think of a good way to test the |
eccc46d
to
be69f43
Compare
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.
A few smallish comments left; I think we're almost done. :-)
middlewares/replace_path_test.go
Outdated
t.Run(path, func(t *testing.T) { | ||
req, err := http.NewRequest("GET", "http://localhost"+path, nil) | ||
if err != nil { | ||
t.Error(err) |
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.
This should be a t.Fatal
.
middlewares/replace_path_test.go
Outdated
|
||
handler.ServeHTTP(nil, req) | ||
if newPath != replacementPath { | ||
t.Errorf("new path should be \"%s\"", replacementPath) |
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.
I suggest to use single quotes ('
) to avoid escaping.
middlewares/replace_path_test.go
Outdated
} | ||
|
||
if oldPath != path { | ||
t.Errorf("old path should be \"%s\"", path) |
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.
Single quotes preferred here too.
middlewares/replace_path_test.go
Outdated
newPath = r.URL.Path | ||
oldPath = r.Header.Get("X-Replaced-Path") | ||
}), | ||
} |
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.
Please move the block from line 13 to 20 into the test loop body in order to make sure the tests work okay even if we run them in parallel.
middlewares/replace_path.go
Outdated
func (s *ReplacePath) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
r.Header.Add(ReplacedPathHeader, r.URL.Path) | ||
r.URL.Path = s.Path | ||
r.RequestURI = r.URL.RequestURI() |
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.
I'm not too deep into this part of HTTP, but is it legitimate to change this?
This is what net/http says:
// RequestURI is the unmodified Request-URI of the
// Request-Line (RFC 2616, Section 5.1) as sent by the client
// to a server. Usually the URL field should be used instead.
// It is an error to set this field in an HTTP client request.
RequestURI string
WDYT?
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.
I did find it a bit strange too, but iirc, it does not directly affect a client request.
I'm probably wrong, but at one point there might have been some code that read r.RequestURI` to use for the path to use for the backend request. That would also explain why the same expression elsewhere in the code base.
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.
In the github.com/vulcand/oxy
, when creating a new forwarder, it is used for r.Opaque
which in http(s) requests has no effect.
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.
👍
After investigating, I've concluded that setting It will probably be best to remove all instances of this from the rest of code base to avoid future contributors copying it. |
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.
@ssttevee thanks for bearing with me, and in particular for looking into the RequestURI matter.
PR LGTM. 🎉
@ssttevee one last request: could you squash your commits for git hygiene reasons? |
5e340f5
to
b16dc60
Compare
b16dc60
to
aa8375e
Compare
it's done! |
I believe I found a possible bug, or at least a not intuitive behavior when While one would expect that we match the request to Because If the Rule is written as Should I open a new Issue to track this improvement? |
@aantono thanks for the report. Please do file a new bug report and link it here. Appreciated! |
Done... I'll try to see if I can make PR with a fix tomorrow. |
@aantono sounds reasonable on first sight. We should try to add a few tests to make sure the known combinations work as expected. |
I still have the problem with the route replacements in the containous/traefik image. |
@aantono What version of Traefik did you use for the workaround you mentioned (PathPrefixStrip:/management;ReplacePath:/management/health)? |
That was the 1.3-RC1 (the one I used was downloaded from the releases page) |
Replaces the old path with the new one and adds the old path to the
X-Replaced-Path
header.My use case is to map to a FAAS web triggers (i.e. google cloud functions, aws lambda).