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

RequestURI path is modified when path has multiple `/` characters #60

Closed
youngm opened this Issue Oct 6, 2014 · 32 comments

Comments

Projects
None yet
@youngm
Contributor

youngm commented Oct 6, 2014

We are running cf-release 183.

It appears that the gorouter is modifying the Request Path of a request if prefixed with multiple '/' characters.

Example Request: http://test.app.com/bob
Router Log in Loggregator: GET /bob HTTP/1.1" 404 15

Example Request: http://test.app.com//bob (node the //)
Router Log in Loggregator: GET http://bob HTTP/1.1

Note the addition of "http:" to the request path. This is what I'd expect to see:
Router Log in Loggregator: GET //bob HTTP/1.1

I would expect the go router to pass this path to the server unchanged.

You can see similar behaviour on pws. Though something appears to be stripping one of the slashes as you only get an error with 3 ///.

Example pws Request: http://boshartifacts.cloudfoundry.org///joe
WEBrick response: Bad Request bad URIhttp://joe'.`

@cf-gitbot

This comment has been minimized.

Show comment
Hide comment
@cf-gitbot

cf-gitbot Oct 6, 2014

Collaborator

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/80170710.

Collaborator

cf-gitbot commented Oct 6, 2014

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/80170710.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 6, 2014

Contributor

I think this test duplicates the issue in proxy_test.go:

    It("maintains double /", func() {
        shouldEcho("//abc", "//abc")
    })

Returns a 400 Status Code instead of 200 from the proxy server.

Contributor

youngm commented Oct 6, 2014

I think this test duplicates the issue in proxy_test.go:

    It("maintains double /", func() {
        shouldEcho("//abc", "//abc")
    })

Returns a 400 Status Code instead of 200 from the proxy server.

@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Oct 7, 2014

Contributor

According to rfc3986, an absolute path begins with / but must not '//'. The 400 is a valid response to a request the is invalid.
I wouldn't be surprised if many runtimes are being loose in their definition/parsing of a request path.
If we augment your test to something like /a/////b, you will see that it passes.

The failure occurs when the path is parsed as a valid url.ParseRequestURI inside request.go

Contributor

fraenkel commented Oct 7, 2014

According to rfc3986, an absolute path begins with / but must not '//'. The 400 is a valid response to a request the is invalid.
I wouldn't be surprised if many runtimes are being loose in their definition/parsing of a request path.
If we augment your test to something like /a/////b, you will see that it passes.

The failure occurs when the path is parsed as a valid url.ParseRequestURI inside request.go

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 7, 2014

Contributor

Thanks for responding @fraenkel I agree that // is an invalid Request. My test was wrong. But don't you think the router should pass it through unchanged anyway instead of munging it into http://?

Perhaps I'll have to muddle my way through creating a better unit test to show that the url is getting munged.

Contributor

youngm commented Oct 7, 2014

Thanks for responding @fraenkel I agree that // is an invalid Request. My test was wrong. But don't you think the router should pass it through unchanged anyway instead of munging it into http://?

Perhaps I'll have to muddle my way through creating a better unit test to show that the url is getting munged.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 7, 2014

Contributor

@fraenkel This is hopefully a better test that shows the router is modifying paths that start with //. Even though // is an invalid path I don't think the router should mess with it.

    It("maintains a double slash at beginning of path", func() {
        ln := registerHandler(r, "test", func(x *test_util.HttpConn) {
            x.CheckLine("GET //test_path HTTP/1.1")
        })
        defer ln.Close()

        x := dialProxy(proxyServer)

        x.WriteLines([]string{
            "GET //test_path HTTP/1.1",
            "Host: test",
        })

        x.ReadResponse()
    })
Contributor

youngm commented Oct 7, 2014

@fraenkel This is hopefully a better test that shows the router is modifying paths that start with //. Even though // is an invalid path I don't think the router should mess with it.

    It("maintains a double slash at beginning of path", func() {
        ln := registerHandler(r, "test", func(x *test_util.HttpConn) {
            x.CheckLine("GET //test_path HTTP/1.1")
        })
        defer ln.Close()

        x := dialProxy(proxyServer)

        x.WriteLines([]string{
            "GET //test_path HTTP/1.1",
            "Host: test",
        })

        x.ReadResponse()
    })
@fraenkel

This comment has been minimized.

Show comment
Hide comment
@fraenkel

fraenkel Oct 8, 2014

Contributor

For kicks, I tried a few other servers to see what happens with your //test_path request.
What I got back were:

404 Not Found: Requested route ('test_path') does not exist.
and
404 The requested URL /test_path

// is not valid period, and there are many translations to make it valid all of which change what is there. Gorouter is actually not doing anything but the underlying golang http server is doing all the munging that is being passed through. If any change is to be made its in the core golang code.

Contributor

fraenkel commented Oct 8, 2014

For kicks, I tried a few other servers to see what happens with your //test_path request.
What I got back were:

404 Not Found: Requested route ('test_path') does not exist.
and
404 The requested URL /test_path

// is not valid period, and there are many translations to make it valid all of which change what is there. Gorouter is actually not doing anything but the underlying golang http server is doing all the munging that is being passed through. If any change is to be made its in the core golang code.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 8, 2014

Contributor

@fraenkel Hmm....can you confirm you are seeing the same test results as me?

When I run that test I get the following failure:

  Expected
      <string>: GET http://test_path HTTP/1.1
  to equal
      <string>: GET //test_path HTTP/1.1

You think that it is correct that the path is changing from //test_path to http://test_path? Or do you think the backend server is changing that path? That path change shows up in loggregator logs so I don't think it is the back end server changing the path then somehow returning it back to the router to be logged?

Contributor

youngm commented Oct 8, 2014

@fraenkel Hmm....can you confirm you are seeing the same test results as me?

When I run that test I get the following failure:

  Expected
      <string>: GET http://test_path HTTP/1.1
  to equal
      <string>: GET //test_path HTTP/1.1

You think that it is correct that the path is changing from //test_path to http://test_path? Or do you think the backend server is changing that path? That path change shows up in loggregator logs so I don't think it is the back end server changing the path then somehow returning it back to the router to be logged?

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 8, 2014

Contributor

@fraenkel I re-read your comment: Gorouter is actually not doing anything but the underlying golang http server is doing all the munging that is being passed through. If the munging you are referring to is the "http://" munging I can acknowledge that it could be a core golang code bug. But, I submit it here because I believe it is invalid functionality exibited in the gorouter component regardless of if the fault lies with golang code or gorouter code. // is an invalid path. I get that. But I don't think it is valid behaviour for a reverse proxy to be changing requests invalid or not when it could just forward it on.

Let me give you background on this issue. We are trying to deploy a thirdparty commercial product to Cloud Foundry. This commercial product has a compiled thick client with a "bug" in it that sends all it's rest requests with //. It works great if they hit a dev server directly but fails if they deploy their app to cloud foundry. So, this issue isn't simply a matter of someone fat fingering a url in their browser. We cannot change the code that is adding //. It works when they hit their server directly. Fails when going through a CF router. From their perspective CF is not acting as a transparent platform since it is munging the requests sent to their server.

Hopefully that background helps.

Contributor

youngm commented Oct 8, 2014

@fraenkel I re-read your comment: Gorouter is actually not doing anything but the underlying golang http server is doing all the munging that is being passed through. If the munging you are referring to is the "http://" munging I can acknowledge that it could be a core golang code bug. But, I submit it here because I believe it is invalid functionality exibited in the gorouter component regardless of if the fault lies with golang code or gorouter code. // is an invalid path. I get that. But I don't think it is valid behaviour for a reverse proxy to be changing requests invalid or not when it could just forward it on.

Let me give you background on this issue. We are trying to deploy a thirdparty commercial product to Cloud Foundry. This commercial product has a compiled thick client with a "bug" in it that sends all it's rest requests with //. It works great if they hit a dev server directly but fails if they deploy their app to cloud foundry. So, this issue isn't simply a matter of someone fat fingering a url in their browser. We cannot change the code that is adding //. It works when they hit their server directly. Fails when going through a CF router. From their perspective CF is not acting as a transparent platform since it is munging the requests sent to their server.

Hopefully that background helps.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 8, 2014

Contributor

Here is another test case going against the loggregator log produced by the router. I don't see how the back end server could be changing the url the router is logging, right?

    It("Logs a doubleslash", func() {
        ln := registerHandler(r, "test", func(x *test_util.HttpConn) {
            x.ReadRequest()
            x.WriteLines([]string{
                "HTTP/1.1 200 OK",
                "Content-Length: 0",
            })
        })
        defer ln.Close()

        x := dialProxy(proxyServer)

        x.WriteLines([]string{
            "GET //test_path HTTP/1.0",
            "Host: test",
        })

        x.CheckLine("HTTP/1.0 200 OK")

        var payload []byte
        Eventually(func() int {
            accessLogFile.Read(&payload)
            return len(payload)
        }).ShouldNot(BeZero())
        Ω(string(payload)).To(MatchRegexp("GET //test_path"))
    })

To be clear. This is the output I get when this test fails:

  Expected
      <string>: test - [08/10/2014:10:03:47 -0600] "GET http://test_path HTTP/1.0" 200 0 "-" "-" 127.0.0.1:43082 x_forwarded_for:"127.0.0.1" vcap_request_id:78486ac8-b3e7-4aad-4e3e-c95f994b9df9 response_time:0.007782678 app_id:

  to match regular expression
      <string>: GET //test_path
Contributor

youngm commented Oct 8, 2014

Here is another test case going against the loggregator log produced by the router. I don't see how the back end server could be changing the url the router is logging, right?

    It("Logs a doubleslash", func() {
        ln := registerHandler(r, "test", func(x *test_util.HttpConn) {
            x.ReadRequest()
            x.WriteLines([]string{
                "HTTP/1.1 200 OK",
                "Content-Length: 0",
            })
        })
        defer ln.Close()

        x := dialProxy(proxyServer)

        x.WriteLines([]string{
            "GET //test_path HTTP/1.0",
            "Host: test",
        })

        x.CheckLine("HTTP/1.0 200 OK")

        var payload []byte
        Eventually(func() int {
            accessLogFile.Read(&payload)
            return len(payload)
        }).ShouldNot(BeZero())
        Ω(string(payload)).To(MatchRegexp("GET //test_path"))
    })

To be clear. This is the output I get when this test fails:

  Expected
      <string>: test - [08/10/2014:10:03:47 -0600] "GET http://test_path HTTP/1.0" 200 0 "-" "-" 127.0.0.1:43082 x_forwarded_for:"127.0.0.1" vcap_request_id:78486ac8-b3e7-4aad-4e3e-c95f994b9df9 response_time:0.007782678 app_id:

  to match regular expression
      <string>: GET //test_path
@dieucao

This comment has been minimized.

Show comment
Hide comment
@dieucao

dieucao Oct 8, 2014

Member

@youngm We would be fine to take a PR on this if you wouldn't mind working on it.

Member

dieucao commented Oct 8, 2014

@youngm We would be fine to take a PR on this if you wouldn't mind working on it.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 8, 2014

Contributor

@dieucao This is one of those hard ones I was hoping you guys could do. :) But, I can see it isn't a priority for you so I'll take a look. Hopefully it doesn't involve rewriting all the golang http server libraries.

Thanks,
Mike

Contributor

youngm commented Oct 8, 2014

@dieucao This is one of those hard ones I was hoping you guys could do. :) But, I can see it isn't a priority for you so I'll take a look. Hopefully it doesn't involve rewriting all the golang http server libraries.

Thanks,
Mike

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 9, 2014

Contributor

Well, I found the problem code: http://golang.org/src/pkg/net/url/url.go#692

In all other ways it appears that the Opaque variable is truly Opaque in every way except in this one way. And I can see no way to supply a truly Opaque path to request.go without completely rewriting the request package. Why is URL not an interface? I hate go lang. :)

Contributor

youngm commented Oct 9, 2014

Well, I found the problem code: http://golang.org/src/pkg/net/url/url.go#692

In all other ways it appears that the Opaque variable is truly Opaque in every way except in this one way. And I can see no way to supply a truly Opaque path to request.go without completely rewriting the request package. Why is URL not an interface? I hate go lang. :)

@jfmyers9

This comment has been minimized.

Show comment
Hide comment
@jfmyers9

jfmyers9 Oct 16, 2014

Member

Hi @youngm,

We agree that the gorouter should not modify requests, however we feel uncomfortable putting a work around in the gorouter for a bug from golang core. If you wish to pursue this further, we suggest that you open up an issue with golang.

Thanks.

@jfmyers9 && @sergueif, CF Runtime Team

Member

jfmyers9 commented Oct 16, 2014

Hi @youngm,

We agree that the gorouter should not modify requests, however we feel uncomfortable putting a work around in the gorouter for a bug from golang core. If you wish to pursue this further, we suggest that you open up an issue with golang.

Thanks.

@jfmyers9 && @sergueif, CF Runtime Team

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Oct 16, 2014

Contributor

Sounds fair. I looked into it and it is not worth the effort for me to lobby golang for a fix.

  • I don't have a relationship with the golang community.
  • This issue is really only an issue when using golang's http package as a transparent proxy probably not the original purpose of the http package. Not something I feel like putting effort into debating in the golang community.

You can close the issue if you wish. At the very least perhaps you can keep this issue in mind next time the router is up for a rewrite.

Mike

Contributor

youngm commented Oct 16, 2014

Sounds fair. I looked into it and it is not worth the effort for me to lobby golang for a fix.

  • I don't have a relationship with the golang community.
  • This issue is really only an issue when using golang's http package as a transparent proxy probably not the original purpose of the http package. Not something I feel like putting effort into debating in the golang community.

You can close the issue if you wish. At the very least perhaps you can keep this issue in mind next time the router is up for a rewrite.

Mike

@zrob

This comment has been minimized.

Show comment
Hide comment
@zrob

zrob Nov 4, 2014

Member

We have no plans to resolve this issue at this point.

We will leave the issue open as a KB for others running into this issue.

Member

zrob commented Nov 4, 2014

We have no plans to resolve this issue at this point.

We will leave the issue open as a KB for others running into this issue.

@narma

This comment has been minimized.

Show comment
Hide comment
@narma

narma Jul 27, 2015

Related golang issue golang/go#10433

narma commented Jul 27, 2015

Related golang issue golang/go#10433

@shalako

This comment has been minimized.

Show comment
Hide comment
@shalako

shalako Feb 2, 2016

Member

golang/go#10433 says this was fixed in golang 1.5. gorouter has been updated to golang 1.5.3 in cf-release v230.

@youngm could you try v230 and see if the upgrade addresses this issue?

Member

shalako commented Feb 2, 2016

golang/go#10433 says this was fixed in golang 1.5. gorouter has been updated to golang 1.5.3 in cf-release v230.

@youngm could you try v230 and see if the upgrade addresses this issue?

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Feb 2, 2016

Contributor

@shalako Yes we'll probably be upgrading to v230+ in 3 weeks or so. I'll test it then. If you want to close this now I can reopen if it is still not fixed.

Contributor

youngm commented Feb 2, 2016

@shalako Yes we'll probably be upgrading to v230+ in 3 weeks or so. I'll test it then. If you want to close this now I can reopen if it is still not fixed.

@shalako

This comment has been minimized.

Show comment
Hide comment
@shalako

shalako Feb 3, 2016

Member

According to a comment in golang/go#10433:

there is now a way to construct this request URI. Don't use Opaque, use RawPath (or just ParseRequestURI).

I bypassed the ELB and opened a telnet session directly with gorouter so I could specify the absoluteURI.

# telnet 10.0.32.15 80
Trying 10.0.32.15...
Connected to 10.0.32.15.
Escape character is '^]'.
GET http://lattice-scoen.superman.cf-app.com/foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:02 GMT
X-Cf-Requestid: 2a5b82a1-5e33-4092-6923-97f4e5a5f334

404 page not found
GET http://lattice-scoen.superman.cf-app.com//foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:13 GMT
X-Cf-Requestid: 5a2ee300-4c33-4597-7e0a-d9eadbac5a43

404 page not found
GET http://lattice-scoen.superman.cf-app.com///foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:23 GMT
X-Cf-Requestid: 72ba5e74-cf5d-4444-4240-3b7ea3f8bc89

404 page not found

Corresponding logs for these were:

2016-02-02T16:49:49.70-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:49 +0000] "GET http://lattice-scoen.superman.cf-app.com/foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:e19d0e6b-2808-46f4-7d0b-04e5e237ce87 response_time:0.002659661 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:49:59.91-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:59 +0000] "GET http://lattice-scoen.superman.cf-app.com//foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:13b527df-a598-4ba3-78a9-29f92ce8d06c response_time:0.002781978 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:50:09.12-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:50:09 +0000] "GET http://lattice-scoen.superman.cf-app.com///foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:0db0fca0-aeb5-4f2e-47c0-679430707729 response_time:0.00276622 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

Let me know if this sounds like a reasonable validation.

Member

shalako commented Feb 3, 2016

According to a comment in golang/go#10433:

there is now a way to construct this request URI. Don't use Opaque, use RawPath (or just ParseRequestURI).

I bypassed the ELB and opened a telnet session directly with gorouter so I could specify the absoluteURI.

# telnet 10.0.32.15 80
Trying 10.0.32.15...
Connected to 10.0.32.15.
Escape character is '^]'.
GET http://lattice-scoen.superman.cf-app.com/foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:02 GMT
X-Cf-Requestid: 2a5b82a1-5e33-4092-6923-97f4e5a5f334

404 page not found
GET http://lattice-scoen.superman.cf-app.com//foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:13 GMT
X-Cf-Requestid: 5a2ee300-4c33-4597-7e0a-d9eadbac5a43

404 page not found
GET http://lattice-scoen.superman.cf-app.com///foo HTTP/1.1
Host: lattice-scoen.superman.cf-app.com

HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Wed, 03 Feb 2016 00:48:23 GMT
X-Cf-Requestid: 72ba5e74-cf5d-4444-4240-3b7ea3f8bc89

404 page not found

Corresponding logs for these were:

2016-02-02T16:49:49.70-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:49 +0000] "GET http://lattice-scoen.superman.cf-app.com/foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:e19d0e6b-2808-46f4-7d0b-04e5e237ce87 response_time:0.002659661 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:49:59.91-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:59 +0000] "GET http://lattice-scoen.superman.cf-app.com//foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:13b527df-a598-4ba3-78a9-29f92ce8d06c response_time:0.002781978 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:50:09.12-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:50:09 +0000] "GET http://lattice-scoen.superman.cf-app.com///foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:0db0fca0-aeb5-4f2e-47c0-679430707729 response_time:0.00276622 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

Let me know if this sounds like a reasonable validation.

@shalako shalako closed this Feb 3, 2016

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Mar 11, 2016

Contributor

@shalako I finally got around to validating this and it is not fixed. It appears you had a small error in your test case. Instead of putting the entire url in the first line of the request you should have only put the path and then added a host header for the host.

The good news is the problem can be duplicated with curl. With the exception of bypassing the ELB I guess you can update your hosts file for the test if you'd prefer to use curl and by pass the ELB.

Regardless I've updated your test cases below with the results I see with 231:

curl http://lattice-scoen.superman.cf-app.com//foo

curl http://lattice-scoen.superman.cf-app.com///foo

Outputs in loggregator:

2016-02-02T16:49:49.70-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:49 +0000] "GET http://foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:e19d0e6b-2808-46f4-7d0b-04e5e237ce87 response_time:0.002659661 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:49:59.91-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:59 +0000] "GET http:///foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:13b527df-a598-4ba3-78a9-29f92ce8d06c response_time:0.002781978 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79
Contributor

youngm commented Mar 11, 2016

@shalako I finally got around to validating this and it is not fixed. It appears you had a small error in your test case. Instead of putting the entire url in the first line of the request you should have only put the path and then added a host header for the host.

The good news is the problem can be duplicated with curl. With the exception of bypassing the ELB I guess you can update your hosts file for the test if you'd prefer to use curl and by pass the ELB.

Regardless I've updated your test cases below with the results I see with 231:

curl http://lattice-scoen.superman.cf-app.com//foo

curl http://lattice-scoen.superman.cf-app.com///foo

Outputs in loggregator:

2016-02-02T16:49:49.70-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:49 +0000] "GET http://foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:e19d0e6b-2808-46f4-7d0b-04e5e237ce87 response_time:0.002659661 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79

2016-02-02T16:49:59.91-0800 [RTR/0]      OUT lattice-scoen.superman.cf-app.com - [03/02/2016:00:49:59 +0000] "GET http:///foo HTTP/1.1" 404 0 19 "-" "-" 10.0.32.15:2794 x_forwarded_for:"10.0.32.15" x_forwarded_proto:"http" vcap_request_id:13b527df-a598-4ba3-78a9-29f92ce8d06c response_time:0.002781978 app_id:e7b285c1-6c6d-4272-bc2d-054b826bce79
@shalako

This comment has been minimized.

Show comment
Hide comment
@shalako

shalako Mar 11, 2016

Member

Thank you for taking a closer look. I redid my test using the relative requestURI as you suggested.

GET / HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 200 OK
Content-Length: 1677
Date: Fri, 11 Mar 2016 22:44:37 GMT
X-Vcap-Request-Id: 3b3b19d5-9259-4351-6516-e9f63c27d58e
2016-03-11T14:45:22.35-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:45:22 +0000] "GET / HTTP/1.1" 200 0 1677 "-" "-" 10.0.48.66:40314 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:d8869cec-a384-411c-608b-17a7bcad5c98 response_time:0.002493097 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
GET // HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 301 Moved Permanently
Content-Length: 36
Content-Type: text/html; charset=utf-8
Date: Fri, 11 Mar 2016 22:26:59 GMT
Location: /
X-Vcap-Request-Id: 65118383-db82-4f5f-631c-91abccf52774

<a href="/">Moved Permanently</a>.
2016-03-11T14:27:44.23-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:27:44 +0000] "GET http:// HTTP/1.1" 301 0 36 "-" "-" 10.0.48.66:39758 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:61e9a0c6-667a-489c-6b92-b3e2a27bcae2 response_time:0.002656648 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
GET /// HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 200 OK
Content-Length: 1677
Date: Fri, 11 Mar 2016 22:28:11 GMT
X-Vcap-Request-Id: 6aede386-9ae7-4856-5621-0667cd149837

2016-03-11T14:28:56.87-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:28:56 +0000] "GET http:/// HTTP/1.1" 200 0 1677 "-" "-" 10.0.48.66:39758 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:53a6f182-4dc7-4c4b-75dd-cc239e0cab23 response_time:0.002785987 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94

I see in the logs that http: is prepended to the requests for // and /// but not /.

Also, // appears to be redirected to / while /// gets passed through unchanged.

I see similar behavior when going through the ELB:

$ curl lattice.superman.cf-app.com/ -v
* Adding handle: conn: 0x7fc653004400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fc653004400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Length: 1676
< Date: Fri, 11 Mar 2016 22:42:59 GMT
< X-Vcap-Request-Id: 7a320dba-b943-4c9c-5d1a-53ff62984334
<
2016-03-11T14:43:44.64-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:43:44 +0000] "GET / HTTP/1.1" 200 0 1676 "-" "curl/7.30.0" 10.0.2.160:47008 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:a33b7efd-7d3c-44f4-5ffd-cbe4a57047d7 response_time:0.002505251 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
$ curl lattice.superman.cf-app.com// -v* Adding handle: conn: 0x7fd8bb004400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fd8bb004400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET // HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 301 Moved Permanently
< Content-Length: 36
< Content-Type: text/html; charset=utf-8
< Date: Fri, 11 Mar 2016 22:31:15 GMT
< Location: /
< X-Vcap-Request-Id: c15487ba-e791-494e-7665-8f9043495322
<
<a href="/">Moved Permanently</a>.

* Connection #0 to host lattice.superman.cf-app.com left intact
2016-03-11T14:32:00.14-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:32:00 +0000] "GET http:// HTTP/1.1" 301 0 36 "-" "curl/7.30.0" 10.0.2.160:46403 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:3a90efee-0e0b-438b-4dbd-d875e4a36aba response_time:0.002452937 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
$ curl lattice.superman.cf-app.com/// -v
* Adding handle: conn: 0x7fc200804400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fc200804400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET /// HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Length: 1677
< Date: Fri, 11 Mar 2016 22:32:19 GMT
< X-Vcap-Request-Id: 69b3117a-ed15-4503-64b9-83d5c679774f
<
...
* Connection #0 to host lattice.superman.cf-app.com left intact
2016-03-11T14:33:04.08-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:33:04 +0000] "GET http:/// HTTP/1.1" 200 0 1677 "-" "curl/7.30.0" 10.0.2.160:46458 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:6a9e9bdc-6cd9-4ad8-5f2c-bfb6bc3c0ead response_time:0.002803724 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94

It appears http: is prepended all the time. I'm also curious about the redirect. We'll look into it.

Member

shalako commented Mar 11, 2016

Thank you for taking a closer look. I redid my test using the relative requestURI as you suggested.

GET / HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 200 OK
Content-Length: 1677
Date: Fri, 11 Mar 2016 22:44:37 GMT
X-Vcap-Request-Id: 3b3b19d5-9259-4351-6516-e9f63c27d58e
2016-03-11T14:45:22.35-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:45:22 +0000] "GET / HTTP/1.1" 200 0 1677 "-" "-" 10.0.48.66:40314 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:d8869cec-a384-411c-608b-17a7bcad5c98 response_time:0.002493097 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
GET // HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 301 Moved Permanently
Content-Length: 36
Content-Type: text/html; charset=utf-8
Date: Fri, 11 Mar 2016 22:26:59 GMT
Location: /
X-Vcap-Request-Id: 65118383-db82-4f5f-631c-91abccf52774

<a href="/">Moved Permanently</a>.
2016-03-11T14:27:44.23-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:27:44 +0000] "GET http:// HTTP/1.1" 301 0 36 "-" "-" 10.0.48.66:39758 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:61e9a0c6-667a-489c-6b92-b3e2a27bcae2 response_time:0.002656648 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
GET /// HTTP/1.1
Host: lattice.superman.cf-app.com

HTTP/1.1 200 OK
Content-Length: 1677
Date: Fri, 11 Mar 2016 22:28:11 GMT
X-Vcap-Request-Id: 6aede386-9ae7-4856-5621-0667cd149837

2016-03-11T14:28:56.87-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:28:56 +0000] "GET http:/// HTTP/1.1" 200 0 1677 "-" "-" 10.0.48.66:39758 x_forwarded_for:"10.0.48.66" x_forwarded_proto:"http" vcap_request_id:53a6f182-4dc7-4c4b-75dd-cc239e0cab23 response_time:0.002785987 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94

I see in the logs that http: is prepended to the requests for // and /// but not /.

Also, // appears to be redirected to / while /// gets passed through unchanged.

I see similar behavior when going through the ELB:

$ curl lattice.superman.cf-app.com/ -v
* Adding handle: conn: 0x7fc653004400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fc653004400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Length: 1676
< Date: Fri, 11 Mar 2016 22:42:59 GMT
< X-Vcap-Request-Id: 7a320dba-b943-4c9c-5d1a-53ff62984334
<
2016-03-11T14:43:44.64-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:43:44 +0000] "GET / HTTP/1.1" 200 0 1676 "-" "curl/7.30.0" 10.0.2.160:47008 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:a33b7efd-7d3c-44f4-5ffd-cbe4a57047d7 response_time:0.002505251 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
$ curl lattice.superman.cf-app.com// -v* Adding handle: conn: 0x7fd8bb004400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fd8bb004400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET // HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 301 Moved Permanently
< Content-Length: 36
< Content-Type: text/html; charset=utf-8
< Date: Fri, 11 Mar 2016 22:31:15 GMT
< Location: /
< X-Vcap-Request-Id: c15487ba-e791-494e-7665-8f9043495322
<
<a href="/">Moved Permanently</a>.

* Connection #0 to host lattice.superman.cf-app.com left intact
2016-03-11T14:32:00.14-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:32:00 +0000] "GET http:// HTTP/1.1" 301 0 36 "-" "curl/7.30.0" 10.0.2.160:46403 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:3a90efee-0e0b-438b-4dbd-d875e4a36aba response_time:0.002452937 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94
$ curl lattice.superman.cf-app.com/// -v
* Adding handle: conn: 0x7fc200804400
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fc200804400) send_pipe: 1, recv_pipe: 0
* About to connect() to lattice.superman.cf-app.com port 80 (#0)
*   Trying 52.72.161.118...
* Connected to lattice.superman.cf-app.com (52.72.161.118) port 80 (#0)
> GET /// HTTP/1.1
> User-Agent: curl/7.30.0
> Host: lattice.superman.cf-app.com
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Length: 1677
< Date: Fri, 11 Mar 2016 22:32:19 GMT
< X-Vcap-Request-Id: 69b3117a-ed15-4503-64b9-83d5c679774f
<
...
* Connection #0 to host lattice.superman.cf-app.com left intact
2016-03-11T14:33:04.08-0800 [RTR/0]      OUT lattice.superman.cf-app.com - [11/03/2016:22:33:04 +0000] "GET http:/// HTTP/1.1" 200 0 1677 "-" "curl/7.30.0" 10.0.2.160:46458 x_forwarded_for:"10.0.2.160" x_forwarded_proto:"http" vcap_request_id:6a9e9bdc-6cd9-4ad8-5f2c-bfb6bc3c0ead response_time:0.002803724 app_id:12d5c752-5c37-46bd-9bfa-00740af27b94

It appears http: is prepended all the time. I'm also curious about the redirect. We'll look into it.

@shalako shalako reopened this Mar 11, 2016

@cf-gitbot

This comment has been minimized.

Show comment
Hide comment
@cf-gitbot

cf-gitbot Mar 11, 2016

Collaborator

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/115531505.

Collaborator

cf-gitbot commented Mar 11, 2016

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/115531505.

@shashwathi

This comment has been minimized.

Show comment
Hide comment
@shashwathi

shashwathi Mar 16, 2016

Member

Using RawPath in URL in place of Opaque solves the issues with double (multiple) slashes but fails for absolute URI (https://github.com/cloudfoundry/gorouter/blob/master/proxy/proxy_test.go#L233) and reserved characters (https://github.com/cloudfoundry/gorouter/blob/master/proxy/proxy_test.go#L890). We are not sure we want to break other usecases to support this case.

Regards,
@atulkc and @shashwathi

Member

shashwathi commented Mar 16, 2016

Using RawPath in URL in place of Opaque solves the issues with double (multiple) slashes but fails for absolute URI (https://github.com/cloudfoundry/gorouter/blob/master/proxy/proxy_test.go#L233) and reserved characters (https://github.com/cloudfoundry/gorouter/blob/master/proxy/proxy_test.go#L890). We are not sure we want to break other usecases to support this case.

Regards,
@atulkc and @shashwathi

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Mar 16, 2016

Contributor

@shashwathi @atulkc Understandable and I agree. So, do we put this back into the golang bug tracker and continue waiting?

Contributor

youngm commented Mar 16, 2016

@shashwathi @atulkc Understandable and I agree. So, do we put this back into the golang bug tracker and continue waiting?

@atulkc

This comment has been minimized.

Show comment
Hide comment
@atulkc

atulkc Mar 17, 2016

Contributor

@youngm Yes, I think we should wait on golang fix for this one.

Do you want to close this issue or keep it open for tracking?

Contributor

atulkc commented Mar 17, 2016

@youngm Yes, I think we should wait on golang fix for this one.

Do you want to close this issue or keep it open for tracking?

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Mar 17, 2016

Contributor

@atulkc I'd like to keep it open. Have you submitted a golang bug for this?

Contributor

youngm commented Mar 17, 2016

@atulkc I'd like to keep it open. Have you submitted a golang bug for this?

@crhino

This comment has been minimized.

Show comment
Hide comment
@crhino

crhino Mar 24, 2016

Member

This is the issue on Golang that specified this behavior in 2013: golang/go#4860
This is the code that is implementing this behavior: https://github.com/golang/go/blob/release-branch.go1.5/src/net/url/url.go#L861-L863

We submitted issue golang/go#14952 to the golang repo.

@youngm

Member

crhino commented Mar 24, 2016

This is the issue on Golang that specified this behavior in 2013: golang/go#4860
This is the code that is implementing this behavior: https://github.com/golang/go/blob/release-branch.go1.5/src/net/url/url.go#L861-L863

We submitted issue golang/go#14952 to the golang repo.

@youngm

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Mar 24, 2016

Contributor

Looks great. Thanks @crhino

Contributor

youngm commented Mar 24, 2016

Looks great. Thanks @crhino

@shalako

This comment has been minimized.

Show comment
Hide comment
@shalako

shalako Apr 4, 2016

Member

@youngm Looking at latest comments in golang/go#14952, I would opt for the switch strategy, rather than writing an alternate version of gorouter. Consider that even if @bradfitz takes this feature on, I expect it wouldn't be available until golang 1.7, so this doesn't appear to be a near-term solution.

Member

shalako commented Apr 4, 2016

@youngm Looking at latest comments in golang/go#14952, I would opt for the switch strategy, rather than writing an alternate version of gorouter. Consider that even if @bradfitz takes this feature on, I expect it wouldn't be available until golang 1.7, so this doesn't appear to be a near-term solution.

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Apr 4, 2016

Contributor

@shalako I agree. My hunch is that a switch strategy is extremely unlikely for golang anytime in the future let alone the next couple of versions unless your team can make a really good case for one. :)

That said, I hope that this limitation and perhaps other missing features like HTTP/2, persistent connections, higher layer support for more protocols than http, more commonality with tcp routing, etc. might eventually make for enough reasons rewrite all of gorouter's as a net product.

Do you see any possibility of something like that happening in the nearish to mid term future?

Contributor

youngm commented Apr 4, 2016

@shalako I agree. My hunch is that a switch strategy is extremely unlikely for golang anytime in the future let alone the next couple of versions unless your team can make a really good case for one. :)

That said, I hope that this limitation and perhaps other missing features like HTTP/2, persistent connections, higher layer support for more protocols than http, more commonality with tcp routing, etc. might eventually make for enough reasons rewrite all of gorouter's as a net product.

Do you see any possibility of something like that happening in the nearish to mid term future?

@shalako

This comment has been minimized.

Show comment
Hide comment
@shalako

shalako Apr 5, 2016

Member

I don't believe we have any plans to rewrite gorouter anytime soon. Using the net library instead of net/http is an implementation detail. Though this may be driven by use cases eventually; it's very hard to say at this point.

I have received requests for HTTP/2 support, and will be taking a closer look when we upgrade to golang 1.6. However, there are some issues with golang's implementation as I recall.

For protocols other than http, we'll be offering TCP routing. This is lower-level support, however. There has been a lot of interest in support for specific UDP protocols, and rather than stuff this all into one component, our current strategy will be to enable a "bring your own router" workflow (exposing Routing API as a multi-tenant service). This could also support routers supporting higher-layer TCP protocols.

Except for this one, I don't currently have use cases for turning gorouter into a something which is not http-specific. I would love to hear what you're imagining.

Thank you!

Member

shalako commented Apr 5, 2016

I don't believe we have any plans to rewrite gorouter anytime soon. Using the net library instead of net/http is an implementation detail. Though this may be driven by use cases eventually; it's very hard to say at this point.

I have received requests for HTTP/2 support, and will be taking a closer look when we upgrade to golang 1.6. However, there are some issues with golang's implementation as I recall.

For protocols other than http, we'll be offering TCP routing. This is lower-level support, however. There has been a lot of interest in support for specific UDP protocols, and rather than stuff this all into one component, our current strategy will be to enable a "bring your own router" workflow (exposing Routing API as a multi-tenant service). This could also support routers supporting higher-layer TCP protocols.

Except for this one, I don't currently have use cases for turning gorouter into a something which is not http-specific. I would love to hear what you're imagining.

Thank you!

@youngm

This comment has been minimized.

Show comment
Hide comment
@youngm

youngm Apr 5, 2016

Contributor

@shalako I was under the impression from conversations with others in the past that making the gorouter a replaceable component wasn't something CF was interested in considering.

If the thought is that you want to keep gorouter relatively light and press more specialized load balancing features towards thirdparty ELBs then I agree that building a net based load balancer is not the direction you want to go in.

I whole heatedly agree that a "bring your own router" approach is an excellent long term strategic approach to take for this issue and ones that may popup like it.

I'd be fine if you want to close this issue for now. If the situation changes we can reopen.

Contributor

youngm commented Apr 5, 2016

@shalako I was under the impression from conversations with others in the past that making the gorouter a replaceable component wasn't something CF was interested in considering.

If the thought is that you want to keep gorouter relatively light and press more specialized load balancing features towards thirdparty ELBs then I agree that building a net based load balancer is not the direction you want to go in.

I whole heatedly agree that a "bring your own router" approach is an excellent long term strategic approach to take for this issue and ones that may popup like it.

I'd be fine if you want to close this issue for now. If the situation changes we can reopen.

@shalako shalako closed this Apr 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment