Merge lp://qastaging/~frankban/charms/trusty/juju-gui/wss-secure into lp://qastaging/~juju-gui/charms/trusty/juju-gui/trunk

Reviewers: mp+261946_code.launchpad.net,

Message:
Please take a look.

Description:
Secure WebSockets on insecure GUI server.

Add the ability to explicitly set the WebSocket
protocol used by the connection established by the GUI
client.

Also implement the cached-website relation, that can be used,
for instance, to balance GUI WebSocket connections
in a relation with Apache2.

Both changes are useful in our production env.

Also fixed a bug on the web relation always sending
port 80 or 443 even when a customized port was specified
in the charm options.

Tests: `make unittest`.

QA:
- `juju bootstrap` an environment;
- `make deploy` this branch;
- wait for the GUI to be ready;
- visit the GUI and check that everything works,
   and no errors are present in the JS console;
- run `juju set juju-gui secure=false`;
- wait for the GUI to be ready;
- visit the GUI on port 80 (insecure mode) and check
   that everything works, and no errors are present
   in the JS console;
- run `juju set juju-gui ws-secure=true`;
- wait for the GUI to be ready;
- visit the GUI on port 80 (insecure mode) and check
   that NOTHING works, and you see an error in the JS
   console similar to this: "WebSocket connection to

'wss://10.0.3.37/ws/environment/c54a52a6-b307-44ca-8af2-2afc84f969e4/api'

   failed";
- done, destroy the environment, thank you!

https://code.launchpad.net/~frankban/charms/trusty/juju-gui/wss-secure/+merge/261946

(do not edit description out of merge proposal)

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

Affected files (+139, -25 lines):
   A [revision details]
   M config.yaml
   M hooks/backend.py
   A hooks/cached-website-relation-joined
   M hooks/utils.py
   M hooks/web-relation-joined
   M metadata.yaml
   M tests/test_backends.py
   M tests/test_utils.py

LGTM with a couple of questions. Thanks for the quick turn around on
this.

https://codereview.appspot.com/242300043/diff/1/config.yaml
File config.yaml (right):

https://codereview.appspot.com/242300043/diff/1/config.yaml#newcode112
config.yaml:112: ws-secure:
there were two things we were messing with. The ws url and the ws
protocol. Is this going to set both? Should this tie specifically to
each param instead of being more generic 'ws-secure'?

https://codereview.appspot.com/242300043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/242300043/diff/1/hooks/backend.py#newcode116
hooks/backend.py:116: secure = config.get('ws-secure')
what do you think about making this explicit as a 'force secure url'
option or the like?

secure = config['secure']
if config.get('force-secure-ws'):
     secure = True

?

https://codereview.appspot.com/242300043/diff/1/metadata.yaml
File metadata.yaml (right):

https://codereview.appspot.com/242300043/diff/1/metadata.yaml#newcode25
metadata.yaml:25: cached-website:
what is the cached website for? Are we going through squid now? At one
point we talked about doing haproxy->apache2 and I was expecting it to
be more of a proxy relation.

https://codereview.appspot.com/242300043/diff/1/tests/test_backends.py
File tests/test_backends.py (right):

https://codereview.appspot.com/242300043/diff/1/tests/test_backends.py#newcode210
tests/test_backends.py:210: # WebSocket connection.
can you add a comment about "This is often used when proxying the GUI
behind an SSL terminating service like Apache2"

https://codereview.appspot.com/242300043/

Please take a look.

https://codereview.appspot.com/242300043/diff/1/config.yaml
File config.yaml (right):

https://codereview.appspot.com/242300043/diff/1/config.yaml#newcode112
config.yaml:112: ws-secure:
On 2015/06/15 11:08:51, rharding wrote:
> there were two things we were messing with. The ws url and the ws
protocol. Is
> this going to set both? Should this tie specifically to each param
instead of
> being more generic 'ws-secure'?

We don't use the ws URL anymore. It is there only for bakward
compatibility with really old GUI versions. So, even if this is also
used to set the ws URL, the really meaningful part here is only the
protocol.

https://codereview.appspot.com/242300043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/242300043/diff/1/hooks/backend.py#newcode116
hooks/backend.py:116: secure = config.get('ws-secure')
On 2015/06/15 11:08:51, rharding wrote:
> what do you think about making this explicit as a 'force secure url'
option or
> the like?

> secure = config['secure']
> if config.get('force-secure-ws'):
> secure = True

> ?

It could work, but wit the current approach we handle all four
possibilities (https+wss, http+ws, http+wss and https+ws).
With a force flag the latter is excluded. However, I don't have a use
case for the latter, so [shrug]. Anyway, I feel a specif flag
(ws-secure) falling back to a more generic one (secure) to be an easy to
understand pattern, perhaps clearer than a force-flag, e.g. what happens
if force is false? is it not forced, is the opposite forced?
[shrug] again.

https://codereview.appspot.com/242300043/diff/1/metadata.yaml
File metadata.yaml (right):

https://codereview.appspot.com/242300043/diff/1/metadata.yaml#newcode25
metadata.yaml:25: cached-website:
On 2015/06/15 11:08:51, rharding wrote:
> what is the cached website for? Are we going through squid now? At one
point we
> talked about doing haproxy->apache2 and I was expecting it to be more
of a proxy
> relation.

This is my proposal in this branch. For http traffic, we leave things as
they are: apache -> squid -> haproxy -> GUI. For WebSockets, we connect
directly apache -> GUI. I don;t think squid or haproxy are really
required for this kind of log running connections, and it makes things
easier on our side. So asically we will connect
jc-jujugui:cached-website and jc-apache2:balancer and have a balancer
entry for the GUI to be used for handling ws on the vhost. What do you
think?

https://codereview.appspot.com/242300043/

On 2015/06/15 12:28:39, fabrice wrote:
> LGTM,
> QA:
> Did follow the instructions and got the desired output.

> https://codereview.appspot.com/242300043/diff/20001/hooks/utils.py
> File hooks/utils.py (right):

https://codereview.appspot.com/242300043/diff/20001/hooks/utils.py#newcode436
> hooks/utils.py:436: return config.get('port', default_port) or
default_port
> Is it really useful ? return config.get('port', default_port) should
be enough

As mentioned on IRC, that does not work if the port is None in the
config.
Thank you both for the reviews!

https://codereview.appspot.com/242300043/

*** Submitted:

Secure WebSockets on insecure GUI server.

Add the ability to explicitly set the WebSocket
protocol used by the connection established by the GUI
client.

Also implement the cached-website relation, that can be used,
for instance, to balance GUI WebSocket connections
in a relation with Apache2.

Both changes are useful in our production env.

Also fixed a bug on the web relation always sending
port 80 or 443 even when a customized port was specified
in the charm options.

Tests: `make unittest`.

QA:
- `juju bootstrap` an environment;
- `make deploy` this branch;
- wait for the GUI to be ready;
- visit the GUI and check that everything works,
   and no errors are present in the JS console;
- run `juju set juju-gui secure=false`;
- wait for the GUI to be ready;
- visit the GUI on port 80 (insecure mode) and check
   that everything works, and no errors are present
   in the JS console;
- run `juju set juju-gui ws-secure=true`;
- wait for the GUI to be ready;
- visit the GUI on port 80 (insecure mode) and check
   that NOTHING works, and you see an error in the JS
   console similar to this: "WebSocket connection to

'wss://10.0.3.37/ws/environment/c54a52a6-b307-44ca-8af2-2afc84f969e4/api'

   failed";
- done, destroy the environment, thank you!

R=rharding, fabrice
CC=
https://codereview.appspot.com/242300043

https://codereview.appspot.com/242300043/

LGTM

https://codereview.appspot.com/242300043/