HTTP Router module to discard proxy side-effects #553
Conversation
|
@olix0r, another PR to your attention - as we agreed yesterday :) |
| def apply(req: Request, svc: Service[Request, Response]) = { | ||
| if (!req.uri.startsWith("/")) { // Proxied request | ||
| // split URI like http://acme.co/foo/bar?x=1 into host 'acme.co' and uri 'foo/bar?x=1' | ||
| val Array(_, _, host, uri) = req.uri.split("/", 4) |
There was a problem hiding this comment.
this will throw an exception if there aren't 4 elements returned. we should be more tolerant of unexpected input:
req.uri.split("/", 4) match {
case Array(...) =>
case _ => // doesn't match ...
}
There was a problem hiding this comment.
If at all possible i'd prefer to use a real url parser. URLs come in some unexpected shapes and sizes. Does java.net have something that does this job?
There was a problem hiding this comment.
Yeah, that makes sense. At first I thought about keeping it simple but I was still in doubts. I'll dig API, thanks for an advice.
|
All in all this looks good! A few small comments... |
| @@ -32,6 +32,7 @@ object Http extends Router[Request, Response] with FinagleServer[Request, Respon | |||
| val client: StackClient[Request, Response] = FinagleHttp.client | |||
| .transformed(StackRouter.Client.mkStack(_)) | |||
| .transformed(_.replace(http.TracingFilter.role, http.TracingFilter.module)) | |||
| .transformed(_.insertBefore(http.TracingFilter.role, DiscardProxySideEffects.module)) | |||
There was a problem hiding this comment.
Finally, I think that this needs to be installed on the serverside and not the client-side.
We want this rewriting to happen before we apply an Identifier to the request (which happens above the client).
There was a problem hiding this comment.
Looks like I failed and put it in wrong place although you initially told me to put it onto the server side. %) Will fix, thanks.
6ff62dd to
95633d8
Compare
|
@olix0r pushed an updated - please take another look |
| import com.twitter.finagle.client.StackClient | ||
| import com.twitter.finagle.http.{Request, Response, TlsFilter} | ||
| import com.twitter.finagle.param.ProtocolLibrary | ||
| import com.twitter.finagle.server.StackServer | ||
| import com.twitter.finagle.{Http => FinagleHttp, Server => FinagleServer, http => fhttp, _} | ||
| import io.buoyant.router.Http.param.HttpIdentifier | ||
| import io.buoyant.router.http.{ForwardedFilter, MethodAndHostIdentifier, StripConnectionHeader} | ||
| import java.net.SocketAddress | ||
| import io.buoyant.router.http.{DiscardProxySideEffects, ForwardedFilter, MethodAndHostIdentifier, StripConnectionHeader} |
There was a problem hiding this comment.
super minor: can yiou please sort imports without breaking out sections.
From Effective Scala
Sort import lines alphabetically
This makes it easy to examine visually, and is simple to automate.
IntelliJ, etc have settings to configure this.
|
Sorry, just a few more nits. Once we fix that test, I think this is good to go. |
95633d8 to
9d738f0
Compare
* Strip authority from the URI * Restore Host header value (HTTP/1.1) * Drop Proxy-* headers Request considered being proxied if its URI is absolute
9d738f0 to
d69f4a7
Compare
|
lgtm! |
* Strip authority from the URI * Restore Host header value (HTTP/1.1) * Drop Proxy-* headers Request considered being proxied if its URI is absolute
* Strip authority from the URI * Restore Host header value (HTTP/1.1) * Drop Proxy-* headers Request considered being proxied if its URI is absolute
Applies timeout of 5s to check request contexts. This overrides 30s timeout applied at client transport level, and stops the conduit check command from taking > 90s to complete. Fixes linkerd#553 Signed-off-by: Andy Hume <andyhume@gmail.com>
Proxy-*requestsRequest considered being proxied if its URI is absolute