> On July 27, 2016, 5:47 p.m., Jiang Yan Xu wrote: > > src/master/http.cpp, lines 2039-2051 > > <https://reviews.apache.org/r/50488/diff/1/?file=1455007#file1455007line2039> > > > > It would be great if we don't duplicate code from > > [here](https://github.com/apache/mesos/blob/6d63c6236cec99a6ff79afd690c2287ccf3063eb/3rdparty/libprocess/src/http.cpp#L802). > > > > Actually it did work when you simply had `stringify(request.url)` and > > it looks like when decoding the request on the server side the URL [doesn't > > include the scheme, host and port > > part](https://github.com/apache/mesos/blob/6d63c6236cec99a6ff79afd690c2287ccf3063eb/3rdparty/libprocess/src/decoder.hpp#L153). > > > > If I am not missing anything, it does look like we can just use > > `stringify(request.url)`. > > > > BenM, you have a comment on > > [http.hpp](https://github.com/apache/mesos/blob/6d63c6236cec99a6ff79afd690c2287ccf3063eb/3rdparty/libprocess/include/process/http.hpp#L261), > > is it possible that for server requests the URL is a full URI with scheme, > > host and port? Doesn't look like it? > > haosdent huang wrote: > Yes, I afraid we may set `schema`, `host`, `ip` or `port` in > `DataDecoder` in the future. Then `stringify(request.url)` would not work > here. > > Jiang Yan Xu wrote: > If that's the case we can have a sanity check here to prevent future > changes to silently break this. Better yet if we have something like: > > ``` > URL relative = url.relative(); > return TemporaryRedirect(basePath + stringify(relative)); > ``` > > Jiang Yan Xu wrote: > Strictly speaking a relative URL is relative to **something**. So how > about > > The `Option<URL> base` part could be a TODO thought IMO. > ``` > // If `base` is none, return everything after scheme://host:port. > URL relative(Option<URL> base); > ``` > > What do you think? > > haosdent huang wrote: > Agree, let me update. > > Jiang Yan Xu wrote: > Sorry I thought about it and had a discussion with BenM on slack. > > So here's what I think would be the most standard conforming: > > 1. Request.url on the server side has to be either an > [Absolute-URI](https://tools.ietf.org/html/rfc3986#section-4.3) or an > abs_path (absolute path), but [most often an > abs_path](https://tools.ietf.org/html/rfc2616#section-5.1.2). > 2. golang's URL has a func precisely for what we want (encoded > path?query): > [RequestURI](https://golang.org/src/net/url/url.go?s=24336:24369#L894), it > ignores the possibility in rfc2616 that a Request-URI can be an Absolute-URI > but I think it's acceptable with a comment if we add a method `URL > requestURI()`. > 3. golang also has a func to determine if it's an absoluate URI: > [IsAbs](https://golang.org/pkg/net/url/#URL.IsAbs) > > Given the above I think, without risking implementing things that we > don't need, right now the simplest thing is probably just an `bool > isAbsoluateURI()` so we can `CHECK(request.url.isAbsoluateURI());` here. > > BenM do you agree?
Awesome, thanks a lot for your checks. Let's waiting for @bmahler's reply. - haosdent ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50488/#review143762 ----------------------------------------------------------- On July 27, 2016, 9:32 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50488/ > ----------------------------------------------------------- > > (Updated July 27, 2016, 9:32 a.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Lunøe, Vinod Kone, and > Jiang Yan Xu. > > > Bugs: MESOS-5911 > https://issues.apache.org/jira/browse/MESOS-5911 > > > Repository: mesos > > > Description > ------- > > When the request which contains query parameters or fragment sent to a > non-leading master, the master would redirect to an URL that ignored > the query parameters and fragment. This changes the redirect URL > include the query parameters or fragment. > > > Diffs > ----- > > src/master/http.cpp 1cbec976ccedae81def388c62889624aaf9a794e > > Diff: https://reviews.apache.org/r/50488/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
