Merge lp://qastaging/~frankban/charms/precise/juju-gui/http-proxy into lp://qastaging/~juju-gui/charms/precise/juju-gui/trunk

Reviewers: mp+203612_code.launchpad.net,

Message:
Please take a look.

Description:
Add putcharm support to the GUI server.

Implement a proxy handler propagating requests/responses from
the GUI to the real juju-core HTTPS endpoint.

Tests: `make unittest`.

QA:
QA must be done using juju-core trunk.
I used revno 2265 for the following.

1) Bootstrap a juju environment, deploy the GUI charm
and get the latest GUI sources:
     juju bootstrap
     make deploy
     juju set juju-gui
juju-gui-source="https://github.com/juju/juju-gui.git"

2) wait for the GUI to be ready
(using `juju debug-log` or `tailf ~/.juju/local/log/unit-juju-gui-0.log`
if you bootstrapped a local environment).
This step takes a while.

3) Open the GUI with Chrome and log in. Open the network tab of the
Chrome JavaScript console and refresh.

4) In the terminal, open and observe the GUI server logs:
     juju ssh juju-gui/0 sudo tailf /var/log/upstart/guiserver.log

5) Drag and drop a zipped archive of a local charm.
Charm files (e.g. metadata.yaml) must be on the zip root.
You can use an haproxy archive I used for tests:
it is available in http://ubuntuone.com/3FRDrcjSECyHp3xLmMIZLY

At this point you should see a 200 response in the gui server logs,
and a notification in the GUI itself.
Moreover, if you inspect the request from the JavaScript console,
you should see a response including info about the charm you
just uploaded: if you used haproxy, you should see
{"CharmURL":"local:precise/haproxy-0"}.

Done!. If you want additional fun, you can now try to
execute the following in the Chrome console:
     app.env.deploy('local:precise/haproxy-0', 'myservice', null, null,
1)
Replace "local:precise/haproxy-0" with the charm URL you used.

In a couple of seconds you should see the "myservice" box appearing
in a pending state. Congratulations, you deployed a local
charm from the GUI! :-)

6) Destroy the environment.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/http-proxy/+merge/203612

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/57820043/

Affected files (+340, -12 lines):
   A [revision details]
   M hooks/backend.py
   M server-requirements.pip
   M server/guiserver/__init__.py
   M server/guiserver/apps.py
   M server/guiserver/handlers.py
   M server/guiserver/manage.py
   M server/guiserver/tests/helpers.py
   M server/guiserver/tests/test_apps.py
   M server/guiserver/tests/test_handlers.py
   M server/guiserver/tests/test_utils.py
   M server/guiserver/utils.py
   M tests/test_backends.py

Deferring to juju-gui

LGTM with various non-blocking comments. Doing QA now. Thank you!

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py
File server/guiserver/handlers.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py#newcode112
server/guiserver/handlers.py:112: return
Why is this better than raising gen.Return()? This is not a blocker;
please just clarify.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py#newcode229
server/guiserver/handlers.py:229: # target URL to the response sent by
the GUI server to the original client.
Is there a concrete reason why you want to filter the headers with a
whitelist, rather than simply letting them all through? If not, I guess
that's fine, but I'd feel more comfortable if there were a reason. It
might be worth calling out in _send_response.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/manage.py#newcode131
server/guiserver/manage.py:131:
'tornado.curl_httpclient.CurlAsyncHTTPClient', max_clients=20)
I'm assuming the max_clients is a random best guess? Or is there a
specific reason for 20? If it's a random best guess...sure...IIUC that
means that we won't be uploading more than 20 local charms
simultaneously, which seems more than reasonable, though I worry this
kind of arbitrary limit might bite us later. Not sure what to do about
it, so leave it.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/tests/test_handlers.py
File server/guiserver/tests/test_handlers.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/tests/test_handlers.py#newcode573
server/guiserver/tests/test_handlers.py:573: # Certificates are
automatically accepted.
What would we have to do to actually verify the certs? (Future branch.)
  If you don't think it is necessary to do so (I can imagine reasonable
arguments :-) ), could you comment why in the code?

https://codereview.appspot.com/57820043/

QA great! Very cool. :-)

https://codereview.appspot.com/57820043/

LGTM Thanks for the hard work on this branch!

https://codereview.appspot.com/57820043/

Thanks for the reviews Gary and Jeff!
Double checked this putCharm implementation works in Firefox too.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py
File server/guiserver/handlers.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py#newcode112
server/guiserver/handlers.py:112: return
On 2014/01/28 21:45:05, gary.poster wrote:
> Why is this better than raising gen.Return()? This is not a blocker;
please
> just clarify.

It's mostly equivalent, but I also feel it's slightly clearer. In
Python2 it is possible to exit from a generator using "return" without a
value. Raising a gen.Return seems like a workaround for when you want a
coroutine generator to exit with a final value. Python3 fixes this, but
in the meanwhile, at least when you can just return None, using "return"
seems more readable to me, and less surprising: in async programming, at
this point, we are used to raise exceptions to return values, but at
first sight it still looks weird IMHO.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/handlers.py#newcode229
server/guiserver/handlers.py:229: # target URL to the response sent by
the GUI server to the original client.
On 2014/01/28 21:45:05, gary.poster wrote:
> Is there a concrete reason why you want to filter the headers with a
whitelist,
> rather than simply letting them all through? If not, I guess that's
fine, but
> I'd feel more comfortable if there were a reason. It might be worth
calling out
> in _send_response.

Good point. Changed to just pass headers through.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/manage.py#newcode131
server/guiserver/manage.py:131:
'tornado.curl_httpclient.CurlAsyncHTTPClient', max_clients=20)
On 2014/01/28 21:45:05, gary.poster wrote:
> I'm assuming the max_clients is a random best guess? Or is there a
specific
> reason for 20? If it's a random best guess...sure...IIUC that means
that we
> won't be uploading more than 20 local charms simultaneously, which
seems more
> than reasonable, though I worry this kind of arbitrary limit might
bite us
> later. Not sure what to do about it, so leave it.

max_clients determines the maximum number of simultaneous fetch()
operations that can execute in parallel on each IOLoop. We can still
serve more than 20 uploads, but queued requests are blocked until a curl
slot becomes available. This seems to be a tradeoff between
memory/sockets consumption and speed. The default is 10, I decided to
double it, as you said, as a best guess. We can tweak this option any
time if we find performance issues.

https://codereview.appspot.com/57820043/diff/1/server/guiserver/tests/test_handlers.py
File server/guiserver/tests/test_handlers.py (right):

https://codereview.appspot.com/57820043/diff/1/server/guiserver/tests/test_handlers.py#newcode573
server/guiserver/tests/test_handlers.py:573: # Certificates are
automatically accepted.
On 2014/01/28 21:45:05, gary.poster wrote:
> What would we have to do to actually verify the certs? (Future
branch.) If you
> d...

*** Submitted:

Add putcharm support to the GUI server.

Implement a proxy handler propagating requests/responses from
the GUI to the real juju-core HTTPS endpoint.

Tests: `make unittest`.

QA:
QA must be done using juju-core trunk.
I used revno 2265 for the following.

1) Bootstrap a juju environment, deploy the GUI charm
and get the latest GUI sources:
     juju bootstrap
     make deploy
     juju set juju-gui
juju-gui-source="https://github.com/juju/juju-gui.git"

2) wait for the GUI to be ready
(using `juju debug-log` or `tailf ~/.juju/local/log/unit-juju-gui-0.log`
if you bootstrapped a local environment).
This step takes a while.

3) Open the GUI with Chrome and log in. Open the network tab of the
Chrome JavaScript console and refresh.

4) In the terminal, open and observe the GUI server logs:
     juju ssh juju-gui/0 sudo tailf /var/log/upstart/guiserver.log

5) Drag and drop a zipped archive of a local charm.
Charm files (e.g. metadata.yaml) must be on the zip root.
You can use an haproxy archive I used for tests:
it is available in http://ubuntuone.com/3FRDrcjSECyHp3xLmMIZLY

At this point you should see a 200 response in the gui server logs,
and a notification in the GUI itself.
Moreover, if you inspect the request from the JavaScript console,
you should see a response including info about the charm you
just uploaded: if you used haproxy, you should see
{"CharmURL":"local:precise/haproxy-0"}.

Done!. If you want additional fun, you can now try to
execute the following in the Chrome console:
     app.env.deploy('local:precise/haproxy-0', 'myservice', null, null,
1)
Replace "local:precise/haproxy-0" with the charm URL you used.

In a couple of seconds you should see the "myservice" box appearing
in a pending state. Congratulations, you deployed a local
charm from the GUI! :-)

6) Destroy the environment.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/57820043

https://codereview.appspot.com/57820043/