Closed Bug 987086 Opened 10 years ago Closed 10 years ago

Loop web client should use configuration for determining server url


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



(Not tracked)



(Reporter: NiKo, Assigned: alexis+bugs)



(Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1])


(1 file, 2 obsolete files)

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?
We should make a decision (and patch!) on this soon, I've had to manually change the loop client server for the time being.
Flags: needinfo?(alexis+bugs)
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.
Flags: needinfo?(alexis+bugs)
This is more of a deployment issue, so assigning to Alexis to look at resolving it.
Assignee: nobody → alexis+bugs
OS: Mac OS X → All
Hardware: x86 → All
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)

Flags: needinfo?(alexis+bugs)
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.
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.
backlog: --- → Fx32?
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1]
Target Milestone: --- → mozilla32
Priority: -- → P1
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?
Flags: needinfo?(alexis+bugs) → needinfo?(bwong)
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.
Flags: needinfo?(bwong)
loop-server is not a static server I think you were thinking about serving the loop-client webapp.
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.
OK this line:

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.

- change onload="loop.webapp.init()" to onload="loop.webapp.init('http://baseurl...')"
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.
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.
Flags: needinfo?(bwong)
Attached file link to github PR (obsolete) —
Here is one way to implement it. Deploying the server now requires to do "make config" and setting the appropriate ENV variable.
Attachment #8433401 - Flags: review?(nperriault)
Flags: needinfo?(bwong)
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 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.
Attachment #8433401 - Flags: feedback-
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1] → [p=1, 1.5:p1, ft:webrtc, est:1] [qa+]
Depends on: 1018866
: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.
Attached patch 987086.diff (obsolete) — Splinter Review
Patch against Elm, attaching to loop.config.
Attachment #8433401 - Attachment is obsolete: true
Attachment #8433401 - Flags: review?(nperriault)
Attachment #8434809 - Flags: review?(nperriault)
Attachment #8434809 - Flags: feedback?(standard8)
Comment on attachment 8434809 [details] [diff] [review]

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

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

::: browser/components/loop/standalone/
@@ +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
Attachment #8434809 - Flags: review?(nperriault) → review+
Attached patch 987086.diffSplinter Review
Adressed nits.
Attachment #8434809 - Attachment is obsolete: true
Attachment #8434809 - Flags: feedback?(standard8)
Comment on attachment 8434857 [details] [diff] [review]

Looks good, r=Standard8
Attachment #8434857 - Flags: review+

Closing for tracking purposes.
backlog: Fx32? → ---
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla32 → mozilla33
Flags: qe-verify+
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1] [qa+] → [p=1, 1.5:p1, ft:webrtc, est:1]
What needs to be tested here ?
(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.
I think we can mark this qe- then.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.