987086 - Loop web client should use configuration for determining server url

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

The Loop web client is served statically, but we need configuring some settings, eg. the Loop server base url. Any idea what'd be the best strategy to achieve that?

Comment 1

[Mark Banner (:standard8)]

We should make a decision (and patch!) on this soon, I've had to manually change the loop client server for the time being.

Comment 2

[Alexis Metaireau (:alexis)]

The simplest path forward seem to add a configuration step on the server when deploying the webapp, in order to assign it a server domain.

Or, if the client keeps headers on a 302 redirect, or sets something like the "referer" header, maybe we could use this, but I'm unclear about this solution.

Comment 3

[Mark Banner (:standard8)]

This is more of a deployment issue, so assigning to Alexis to look at resolving it.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

As proposed on IRC, how about a JSON static file defining a mapping between loop-client and loop-server hosts? The webapp could load that file, check for current location host and determine the right loop-server url to load accordingly; that way we avoid building files or relying on dynamic complicated stuff

This mapping could even be defined once for all and embedded in the HTML file (but that would be less fexible if we want to deploy & tweak it easily)

Thoughts?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

What problem are we trying to solve here?  Is this about load-balancing, failover, or something else?

The current plan for fixing cookie sessions for MLP assumes that the loop server host will be the one set in the default preferences, so if for some reason we need to do something else, we'll have to coordinate these decisions.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Oh, I see.  This is because we're currently doing a redirect from the loop-server host to the loop-client host, which I had forgotten about.  I'll need to think about this a bit.

Comment 7

[Alexis Metaireau (:alexis)]

I'm not a big fan of having the loop-client WebApp do a lookup into a file to know where the server should be, because each WebApp will be tied to only one server, so we can do it once and be all set. Also it means we would need to distribute yet another file alongside the static app.

I believe this should be solved at the deployment level, rather than at the development one. For instance, when deploying the loop-webapp, ops could setup a rule in the puppet files that will change the location of the server.

I would be interested in having Benson thoughts on that: would it be okay to define a puppet action to solve this problem? Or maybe is it something already solved in a different way in other projects?

Comment 8

[Benson Wong [:mostlygeek]]

I'm not too clear on what the loop-server url is used for. Since it is a static server, I'm going to assume:

- the value gets rendered into some HTML or JSON that is sent back to the user. 
- the server is stateless and doesn't accept POST/PUT requests
- the server does nothing with cookies
- it is a nodejs server serving/generating static content

If the above is accurate, then putting it into puppet is best. For static sites we like to put nginx in front w/ proxy-cache. nodejs makes the content dynamically, nginx caches it and serves it out of RAM transparently.

Comment 9

[Rémy Hubscher (:natim)]

loop-server is not a static server I think you were thinking about serving the loop-client webapp.

Comment 10

[Alexis Metaireau (:alexis)]

the loop-server url is needed because the WebApp need to know who to talk with.

There is no node.js here, it's only static content, but the static content will need to be modified in order to make things work.

Comment 11

[Benson Wong [:mostlygeek]]

OK this line: 

https://github.com/mozilla/loop-client/blob/master/content/js/webapp.js#L22

So this is a *simple* thing but is complicated by: 

- we deploy things, pre-baked via RPMS 
- we have dev, stage, prod env that will have different url settings
- static UI content should be sent w/ long cache headers
  - which means we should probably bust the cache by fingerprinting files 

I would suggest: 

- use grunt to build static assets into a /dist directory
 - minify / combine js files, ie: /dist/js/combined_<hash>.js
 - minify / combine css files, /dist/css/combined_<hash>.js
 - optimize png/jpeg/etc
- change onload="loop.webapp.onload()" to onload="loop.webapp.onload('http://baseurl...')"
- have nodejs serve the index.html file (now a template), with the baseurl inserted at request time
  - we can pass baseURL to nodejs via an ENV variable or whatever on the server side
  - this can be served with an appropriate Cache-control header (1min? 5m? 1 day?) 
- nginx sits in front
  - serves static files from /dist directly off of disk
  - reverse proxy for index.html from nodejs
- ELBs in front to offload SSL 

That is basically how we do fxa-content-server. It gets a really high performance score and it is easy to package into an RPM that works for dev, stage and prod.

Comment 12

[Benson Wong [:mostlygeek]]

oops: 

- change onload="loop.webapp.init()" to onload="loop.webapp.init('http://baseurl...')"

Comment 13

[Alexis Metaireau (:alexis)]

I don't understand why we would need to put nodejs here, in order to serve static files?

The solution I had in mind was basically to change the static file on the fly when it's being deployed. If that's not possible then I'll go for a different solution (compile to static or use a configuration file), but I don't think adding nodejs to the stack is helping here.

Comment 14

[Alexis Metaireau (:alexis)]

Benson, would it be actionable for you to have a command to run that consumes an env var / a configuration file and outputs the static files?

If yes let's build that.

Comment 15

[Alexis Metaireau (:alexis)]

Attachment: link to github PR

Here is one way to implement it. Deploying the server now requires to do "make config" and setting the appropriate ENV variable.

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8433401 [details]
link to github PR

Commented appropriately. LGTM but I'd rather want the config object being attached to the loop root one.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8433401 [details]
link to github PR

A couple of issues:

Firstly, our source for what was loop-client is now currently "Elm" as advertised before the move, so the patch should be against Elm.

Secondly, we need this to be compatible with running unit tests in marionette. We currently run these with:

./mach marionette-test browser/components/loop/test/manifest.ini

With the preprocessed file, these would fail. Could we either have a file that's present by default? Alternately, we might have to consider if there's a way to generate the file at build time, and still integrate it into tests - which I'm currently working on in bug 994483.

Comment 18

[Alexis Metaireau (:alexis)]

:Standar8, We could have a default value in the app in case the config is not parsed, so that it can be run in marionette without problems.

We don't want this configuration file to be versionned, because configuration should be versionned somewhere else than the repository with the code.

:Niko, thanks. I'll update the code to reflect your changes.

Comment 19

[Alexis Metaireau (:alexis)]

Attachment: 987086.diff

Patch against Elm, attaching to loop.config.

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8434809 [details] [diff] [review]
987086.diff

Review of attachment 8434809 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, minor nits which should probably be addressed before landing.

::: browser/components/loop/standalone/README.md
@@ +18,5 @@
> +
> +	$ make config
> +
> +It will read the configuration from the `LOOP_SERVER_URL` env variable and
> +generate the appropriate configuration file.

nit: maybe precise that LOOP_SERVER_URL defines the root url of the loop server root with no trailing slash.

::: browser/components/loop/standalone/content/index.html
@@ +29,4 @@
>      <script type="text/javascript" src="shared/libs/backbone-1.1.2.js"></script>
>  
>      <!-- app scripts -->
> +	<script type="text/javascript" src="config.js"> </script>

nit: unexpected white spaces here

::: browser/components/loop/standalone/content/js/webapp.js
@@ +9,5 @@
>    "use strict";
>  
> +  if (loop.config === undefined) {
> +    loop.config = {};
> +  }

nit: loop.config = loop.config || {}; is probably more idiomatic

@@ +12,5 @@
> +    loop.config = {};
> +  }
> +  if (loop.config.serverUrl === undefined) {
> +    loop.config.serverUrl = "http://localhost:5000";
> +  }

nit: loop.config.serverUrl = loop.config.serverUrl || {}; is probably more idiomatic

Comment 21

[Alexis Metaireau (:alexis)]

Attachment: 987086.diff

Adressed nits.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8434857 [details] [diff] [review]
987086.diff

Looks good, r=Standard8

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/e8a128be7a6d

Closing for tracking purposes.

Comment 24

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/e8a128be7a6d

Comment 25

[Paul Silaghi, QA [:pauly]]

What needs to be tested here ?

Comment 26

[Mark Banner (:standard8)]

(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> What needs to be tested here ?

Nothing specific. This is a configuration open, and as such, is tested whenever a developer runs the standalone client or it is run on dev/staging/production.

Comment 27

[Paul Silaghi, QA [:pauly]]

I think we can mark this qe- then.