Add Path Replacement Rule #1374

ssttevee · 2017-04-02T01:11:11Z

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

timoreimann

I left a few comments.

We should definitely also add some tests for PathReplace.ServeHTTP and pathReplace.

timoreimann · 2017-04-04T21:30:10Z

docs/basics.md

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


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.

timoreimann · 2017-04-04T21:35:58Z

middlewares/pathReplace.go

+
+func (s *PathReplace) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	r.Header.Add("X-Replaced-Path", r.URL.Path)
+	r.URL.Path = s.Path


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 than r.URL.Path, i.e., we cannot replace it completely?

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.

timoreimann · 2017-04-04T21:36:25Z

server.go

@@ -66,6 +66,7 @@ type serverRoute struct {
 	route         *mux.Route
 	stripPrefixes []string
 	addPrefix     string
+	pathReplace   string


Should be called replacePath if you follow my naming reasoning above.

timoreimann · 2017-04-04T21:36:45Z

rules.go

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


You mean pathReplace / replacePath here.

ssttevee · 2017-04-05T01:19:43Z

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.

timoreimann · 2017-04-05T04:24:02Z

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. :-)

ssttevee · 2017-04-13T20:12:57Z

I have made the changes, but I feel like one might want to customize the header that it is set to too.

emilevauge

Hey Thanks @ssttevee !
One small comment, but other than that, LGTM :)

emilevauge · 2017-04-14T09:34:44Z

middlewares/replacePath.go

+}
+
+// SetHandler sets handler
+func (s *ReplacePath) SetHandler(Handler http.Handler) {


Is it needed (apart from tests) ?

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.

timoreimann

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.

timoreimann · 2017-04-14T10:31:41Z

middlewares/replacePath_test.go

+	}
+
+	for _, path := range paths {
+		req, err := http.NewRequest("ANY", "http://localhost"+path, nil)


Please use subtests and pass path to t.Run. Right now, if a test fails, there's no way to tell which one did.

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.

timoreimann · 2017-04-14T10:32:38Z

middlewares/replacePath_test.go

+package middlewares_test
+
+import (
+	"github.com/containous/traefik/middlewares"


Please move Github imports below stdlib ones, separated by a blank line.

timoreimann · 2017-04-14T10:37:24Z

rules.go

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


What about https://github.com/containous/traefik/pull/1374/files/12e21cd2e9c5c22c8aa2683339b08a70b62e4095#diff-c06ec64a1e1b34f38b2152c5a5169d7c?

timoreimann · 2017-04-14T10:38:38Z

server.go

@@ -791,6 +792,14 @@ func (server *Server) wireFrontendBackend(serverRoute *serverRoute, handler http
 		}
 	}

+	// path replace
+	if len(serverRoute.replacePath) > 0 {


This definitely needs a test.

there is no existing test file for server.go, should I make one to only test this part?

actually, does this function even need to be a method of Server? it doesn't access any of it's fields.

Yeah good catch, we could convert it from a method to a function right away. Would simplify testing.

ldez · 2017-04-24T12:40:45Z

@ssttevee do you the time to update this PR before the upcoming feature freeze for the 1.3 release?

ssttevee · 2017-04-25T06:27:04Z

I couldn't think of a good way to test the wireFrontendBackend method, but everything else should be done.

timoreimann

A few smallish comments left; I think we're almost done. :-)

timoreimann · 2017-04-25T10:06:39Z

middlewares/replace_path_test.go

+		t.Run(path, func(t *testing.T) {
+			req, err := http.NewRequest("GET", "http://localhost"+path, nil)
+			if err != nil {
+				t.Error(err)


This should be a t.Fatal.

timoreimann · 2017-04-25T10:07:03Z

middlewares/replace_path_test.go

+
+			handler.ServeHTTP(nil, req)
+			if newPath != replacementPath {
+				t.Errorf("new path should be \"%s\"", replacementPath)


I suggest to use single quotes (') to avoid escaping.

timoreimann · 2017-04-25T10:07:10Z

middlewares/replace_path_test.go

+			}
+
+			if oldPath != path {
+				t.Errorf("old path should be \"%s\"", path)


Single quotes preferred here too.

timoreimann · 2017-04-25T10:10:52Z

middlewares/replace_path_test.go

+			newPath = r.URL.Path
+			oldPath = r.Header.Get("X-Replaced-Path")
+		}),
+	}


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.

timoreimann · 2017-04-25T10:15:16Z

middlewares/replace_path.go

+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()


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?

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.

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.

ssttevee · 2017-04-25T10:58:27Z

After investigating, I've concluded that setting r.RequestURI is unnecessary (maybe even erroneous). My reasoning is in this comment.

It will probably be best to remove all instances of this from the rest of code base to avoid future contributors copying it.

timoreimann

@ssttevee thanks for bearing with me, and in particular for looking into the RequestURI matter.

PR LGTM. 🎉

timoreimann · 2017-04-25T13:06:52Z

@ssttevee one last request: could you squash your commits for git hygiene reasons?

ssttevee · 2017-04-27T07:03:33Z

it's done!

aantono · 2017-05-09T05:21:58Z

I believe I found a possible bug, or at least a not intuitive behavior when ReplacePath rule is preceded by PathPrefixStrip
Basically because PathPrefixStrip is a Matcher, but also happens to be a Modifier as well, when combined with ReplacePath, like PathPrefixStrip:/management;ReplacePath:/health, due to the order in which the Modifiers are applied in server.go, the above rule-set does not behave as expected.

While one would expect that we match the request to /management url, strip the /management prefix, followed by a replace with /health. This is NOT, however, what is happening. The matcher does the job correctly and matches the request on /management and sends it down the correct Handler chain, but that's when the issue creeps in.

Because ReplacePath Modifier gets executed first in the Handler chain, it replaces /management with /health, and when StripPrefix Modifier gets called, it is trying to match the Path with /management prefix, and obviously failing to do so, thus returning 404-Not Found.

If the Rule is written as PathPrefixStrip:/management;ReplacePath:/management/health, then everything works, but the actual invocation URL Path ends up being NOT /management/health but /health, thus making this configuration be misleading.

Should I open a new Issue to track this improvement?

timoreimann · 2017-05-09T05:26:29Z

@aantono thanks for the report.

Please do file a new bug report and link it here. Appreciated!

aantono · 2017-05-09T05:33:01Z

Done... I'll try to see if I can make PR with a fix tomorrow.
My current thinking is to try to move the ReplacePath block in server.go to be the first in the chain, above the AddPrefix - https://github.com/containous/traefik/blob/master/server/server.go#L778
Thoughts?

timoreimann · 2017-05-09T14:05:50Z

@aantono sounds reasonable on first sight.

We should try to add a few tests to make sure the known combinations work as expected.

IceXPR · 2017-05-15T14:04:20Z

I still have the problem with the route replacements in the containous/traefik image.

IceXPR · 2017-05-15T14:25:59Z

@aantono What version of Traefik did you use for the workaround you mentioned (PathPrefixStrip:/management;ReplacePath:/management/health)?

aantono · 2017-05-15T14:38:53Z

That was the 1.3-RC1 (the one I used was downloaded from the releases page)

ssttevee force-pushed the path-replace-rule branch 2 times, most recently from e4f0e02 to 12e21cd Compare April 2, 2017 01:30

timoreimann suggested changes Apr 4, 2017

View reviewed changes

emilevauge added the contributor/waiting-for-corrections label Apr 6, 2017

ssttevee force-pushed the path-replace-rule branch 5 times, most recently from af428a8 to 1918db3 Compare April 10, 2017 00:49

emilevauge reviewed Apr 14, 2017

View reviewed changes

timoreimann suggested changes Apr 14, 2017

View reviewed changes

ssttevee force-pushed the path-replace-rule branch from 2d0689d to 651befc Compare April 25, 2017 05:07

ssttevee force-pushed the path-replace-rule branch from 64759a8 to 180426d Compare April 25, 2017 05:58

ldez added kind/enhancement a new or improved feature. status/2-needs-review contributor/needs-resolve-conflicts and removed contributor/waiting-for-corrections labels Apr 25, 2017

ssttevee force-pushed the path-replace-rule branch 4 times, most recently from eccc46d to be69f43 Compare April 25, 2017 10:05

timoreimann suggested changes Apr 25, 2017

View reviewed changes

ldez added this to the 1.3 milestone Apr 25, 2017

timoreimann approved these changes Apr 25, 2017

View reviewed changes

ssttevee force-pushed the path-replace-rule branch from 5e340f5 to b16dc60 Compare April 25, 2017 18:14

added path replacement rule

aa8375e

ssttevee force-pushed the path-replace-rule branch from b16dc60 to aa8375e Compare April 27, 2017 06:34

ldez approved these changes Apr 27, 2017

View reviewed changes

ldez merged commit 521e295 into traefik:master Apr 27, 2017

timoreimann removed the contributor/needs-resolve-conflicts label Apr 27, 2017

ldez removed the status/2-needs-review label Apr 29, 2017

aantono mentioned this pull request May 9, 2017

ReplacePath rule executes out of order, when combined with PathPrefixStrip #1569

Closed

ldez added the area/rules label May 25, 2017

Add Path Replacement Rule #1374

Add Path Replacement Rule #1374

Conversation

ssttevee commented Apr 2, 2017

timoreimann left a comment • edited

Choose a reason for hiding this comment

timoreimann Apr 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssttevee commented Apr 5, 2017

timoreimann commented Apr 5, 2017

ssttevee commented Apr 13, 2017

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Apr 24, 2017

ssttevee commented Apr 25, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssttevee Apr 25, 2017 • edited by timoreimann

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssttevee commented Apr 25, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented Apr 25, 2017

ssttevee commented Apr 27, 2017

aantono commented May 9, 2017

timoreimann commented May 9, 2017

aantono commented May 9, 2017

timoreimann commented May 9, 2017

IceXPR commented May 15, 2017

IceXPR commented May 15, 2017

aantono commented May 15, 2017

timoreimann left a comment •

edited

timoreimann Apr 4, 2017 •

edited

ssttevee Apr 25, 2017 •

edited by timoreimann