Add HTTP handler that redirects requests for Mojo apps

Add HTTP handler that redirects requests for Mojo apps

This CL adds an HTTP handler whose purpose is to redirect requests for Mojo
apps to the locations of the current versions of the binaries. It installs
itself as a handler for all requests and operates as follows:

(1) Ensure that the request is of the form "/platform/app", e.g.,
    "/linux-x64/view_manager.mojo".

(2) Load the file containing the up-to-date version of the requested app, which
    resides in a known location for the app and platform, e.g.,
"https://storage.googleapis.com/mojo/services/linux-x64/view_manager_location".
(3) Return a redirect to the contents of that file.

The expected usage of this app is to make it accessible from a server such as
https://foo.com and then to direct the Mojo shell to send requests for mojo:
apps to this server, e.g. "--origin=https://foo.com/linux-64". Note that the
platform must be specified explicitly since the shell does not send that
information at this time.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3a9bcfc3676aea5ac38f06f434a7c0827dfc9dbe

[blundell, 2015-02-25 15:06:18]

blundell@chromium.org changed reviewers:
+ etiennej@chromium.org, qsr@chromium.org

[blundell, 2015-02-25 15:06:18]

I've left in the print statements that I was using for debugging because I'd
like Etienne's feedback on what logging strategy we should be using here.

[qsr, 2015-02-25 15:27:05]

On 2015/02/25 15:06:18, blundell wrote:
> I've left in the print statements that I was using for debugging because I'd
> like Etienne's feedback on what logging strategy we should be using here.

Use logging, then you will be enabled/disabled depending on whether content
handler has debug on or off.

[qsr, 2015-02-25 15:34:07]

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
File services/mojo_url_redirector/BUILD.gn (right):

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/BUILD.gn:21: "//mojo/public/python:mojo_system",
I'm surprised you need this.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
File services/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:12: import os
Maybe move this in its own section.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:25: 
2 blank lines.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:39: response.status_code = 400
Why isn't this a 404

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:52: app_name =
os.path.splitext(requested_app)[0]
You can write this as:
app_name, _ = os.path.splitext(requested_app)

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:64: on_version_fetched = lambda
response: self.OnFoundAppLocation(
You might want to use functools.partial instead. You would just need to have
response as the last argument.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:69: onFullfilled=on_version_fetched)
Do you want to have an onRejected with some logging?

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:76: response.status_code = 404
What about a function returning this, and calling it when returning the 404. I
think it will be clearer.

[blundell, 2015-02-26 13:29:23]

Patchset #2 (id:20001) has been deleted

[blundell, 2015-02-26 13:29:53]

Thanks for the review!

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
File services/mojo_url_redirector/BUILD.gn (right):

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/BUILD.gn:21: "//mojo/public/python:mojo_system",
On 2015/02/25 15:34:07, qsr wrote:
> I'm surprised you need this.

Removed. I had thought I needed it from looking at a different file, but it
works fine without it.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
File services/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:12: import os
On 2015/02/25 15:34:07, qsr wrote:
> Maybe move this in its own section.

Done.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:25: 
On 2015/02/25 15:34:07, qsr wrote:
> 2 blank lines.

Done.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:39: response.status_code = 400
On 2015/02/25 15:34:07, qsr wrote:
> Why isn't this a 404

400 is "Bad Request": "The server cannot or will not process the request due to
something that is perceived to be a client error (e.g., malformed request
syntax, invalid request message framing, or deceptive request routing)."

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:52: app_name =
os.path.splitext(requested_app)[0]
On 2015/02/25 15:34:07, qsr wrote:
> You can write this as:
> app_name, _ = os.path.splitext(requested_app)

Done.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:64: on_version_fetched = lambda
response: self.OnFoundAppLocation(
On 2015/02/25 15:34:07, qsr wrote:
> You might want to use functools.partial instead. You would just need to have
> response as the last argument.

Done.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:69: onFullfilled=on_version_fetched)
On 2015/02/25 15:34:07, qsr wrote:
> Do you want to have an onRejected with some logging?

Done.

https://codereview.chromium.org/953353002/diff/1/services/mojo_url_redirector...
services/mojo_url_redirector/__mojo__.py:76: response.status_code = 404
On 2015/02/25 15:34:07, qsr wrote:
> What about a function returning this, and calling it when returning the 404. I
> think it will be clearer.

Done.

[qsr, 2015-02-26 14:27:06]

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:109: status, app_location =
data_pipe_utils.BlockingCopyFromDataPipeIntoString(
So, yes, I really think you should use a promise here, so that this is
asynchronous, otherwise, you are blocking your server while you are reading the
response -> you cannot handle any new call.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:121: return reason
You do not really need to return anything.

[etiennej, 2015-02-26 14:57:46]

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:57: logging.info("Handling
request for " + request.relative_url)
I don't think this one is necessary, or at a debug level.

We already log when we exit this code, and I feel that this type of basic
logging (logging the request, and maybe the response error code) should be done
by http_server, not by handlers.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:99: logging.error("Network error
from app location fetch: " +
Suggestion: use some common pattern to log error and info messages, such as:
REQUESTED_PLATFORM REQUESTED_APP MESSAGE

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:119: def
OnAppLocationLoadRejected(self, reason):
Do we have a way here to know which app/platform has been requested? It would be
useful for logging.

[blundell, 2015-02-26 16:35:32]

Changed to build on the data_pipe_utils API that returns a Promise. Etienne, I
will address your comments about logging tomorrow.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:121: return reason
On 2015/02/26 14:27:06, qsr wrote:
> You do not really need to return anything. 

Done.

[blundell, 2015-03-04 13:52:06]

Patchset #3 (id:60001) has been deleted

[blundell, 2015-03-04 14:20:46]

Patchset #3 (id:80001) has been deleted

[blundell, 2015-03-04 14:21:59]

Patchset #3 (id:100001) has been deleted

[blundell, 2015-03-04 14:22:54]

All comments addressed.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:57: logging.info("Handling
request for " + request.relative_url)
On 2015/02/26 14:57:45, etiennej wrote:
> I don't think this one is necessary, or at a debug level.
> 
> We already log when we exit this code, and I feel that this type of basic
> logging (logging the request, and maybe the response error code) should be
done
> by http_server, not by handlers.

Done.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:99: logging.error("Network error
from app location fetch: " +
On 2015/02/26 14:57:45, etiennej wrote:
> Suggestion: use some common pattern to log error and info messages, such as:
> REQUESTED_PLATFORM REQUESTED_APP MESSAGE

Done.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:109: status, app_location =
data_pipe_utils.BlockingCopyFromDataPipeIntoString(
On 2015/02/26 14:27:06, qsr wrote:
> So, yes, I really think you should use a promise here, so that this is
> asynchronous, otherwise, you are blocking your server while you are reading
the
> response -> you cannot handle any new call.

Done.

https://codereview.chromium.org/953353002/diff/40001/services/python/mojo_url...
services/python/mojo_url_redirector/__mojo__.py:119: def
OnAppLocationLoadRejected(self, reason):
On 2015/02/26 14:57:45, etiennej wrote:
> Do we have a way here to know which app/platform has been requested? It would
be
> useful for logging.

Done.

[qsr, 2015-03-04 14:40:38]

LGTM with nit.

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:8: import functools
Not needed anymore.

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:92: @promise.async
This doesn't seem to take any promise as parameter, it doesn't need to be @async

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:161: self.http_server, request =
\
Use parenthesis instead of antislash for line continuation.

[blundell, 2015-03-04 15:00:21]

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
File services/python/mojo_url_redirector/__mojo__.py (right):

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:8: import functools
On 2015/03/04 14:40:37, qsr wrote:
> Not needed anymore.

Done.

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:92: @promise.async
On 2015/03/04 14:40:38, qsr wrote:
> This doesn't seem to take any promise as parameter, it doesn't need to be
@async

Done.

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:161: self.http_server, request =
\
On 2015/03/04 14:40:38, qsr wrote:
> Use parenthesis instead of antislash for line continuation.

Done.

https://codereview.chromium.org/953353002/diff/120001/services/python/mojo_ur...
services/python/mojo_url_redirector/__mojo__.py:161: self.http_server, request =
\
On 2015/03/04 14:40:38, qsr wrote:
> Use parenthesis instead of antislash for line continuation.

Done.

[blundell, 2015-03-04 15:05:00]

Committed patchset #4 (id:140001) manually as
3a9bcfc3676aea5ac38f06f434a7c0827dfc9dbe (presubmit successful).