Loop web client should use configuration for determining server url

RESOLVED FIXED in mozilla33

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: NiKo, Assigned: alexis+bugs)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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)
Assignee

Comment 2

5 years ago
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
Blocks: 994360
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?
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.

Updated

5 years ago
backlog: --- → Fx32?
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1]
Target Milestone: --- → mozilla32

Updated

5 years ago
Priority: -- → P1
Assignee

Comment 7

5 years ago
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.
Assignee

Comment 10

5 years ago
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: 

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.
oops: 

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

Comment 13

5 years ago
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.
Assignee

Comment 14

5 years ago
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)
Assignee

Comment 15

5 years ago
Posted 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
Assignee

Comment 18

5 years ago
: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.
Assignee

Comment 19

5 years ago
Posted 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]
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
Attachment #8434809 - Flags: review?(nperriault) → review+
Assignee

Comment 21

5 years ago
Posted patch 987086.diffSplinter Review
Adressed nits.
Attachment #8434809 - Attachment is obsolete: true
Attachment #8434809 - Flags: feedback?(standard8)
Comment on attachment 8434857 [details] [diff] [review]
987086.diff

Looks good, r=Standard8
Attachment #8434857 - Flags: review+
https://hg.mozilla.org/projects/elm/rev/e8a128be7a6d

Closing for tracking purposes.
Status: NEW → RESOLVED
backlog: Fx32? → ---
Closed: 5 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.