Code review comment for lp://qastaging/~frankban/charms/trusty/juju-gui/wss-secure

Revision history for this message
Francesco Banconi (frankban) wrote :

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/

« Back to merge proposal