Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a USE_FASTBOOT=staging-experimental mode #1907

Merged
merged 4 commits into from
Dec 18, 2019

Conversation

jtgeibel
Copy link
Member

@jtgeibel jtgeibel commented Nov 19, 2019

This adds a new mode to the USE_FASTBOOT environment variable. The logic is now:

  • When deployed on Heroku (staging or production - via script/start-web.sh):
    • If USE_FASTBOOT=staging-experimental then non-backend requests are sent to FastBoot.
    • If USE_FASTBOOT is set and non-zero length, FastBoot is enabled only for allowed paths.
    • Otherwise (USE_FASTBOOT is not set, or is zero length), FastBoot is disabled.
  • When doing local development (via script/ember.sh):
    • If USE_FASTBOOT is set, all (initial) frontend requests are served via FastBoot. This is equivalent to staging-experimental above.
    • If USE_FASTBOOT is not set, FastBoot is disabled.

Because our development environment doesn't go through nginx, it isn't possible to support the "FastBoot is enabled only for allowed paths" mode. However, once the frontend has booted, all further requests hit the backend (unless JS is disabled, then every client navigation results in a new request served via FastBoot).

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys
Copy link
Contributor

kzys commented Nov 19, 2019

As like #1788, this is another PR we can simplify by moving some routes under /api.

@jtgeibel
Copy link
Member Author

As like #1788, this is another PR we can simplify by moving some routes under /api.

Yes, I would love to do that, but this will require coordination with someone who has access to the GitHub settings for rust-lang to update the oauth settings.

We would need to (on both production and staging):

  • Add the new routes, keeping the old routes in place
  • Deploy
  • Update oauth settings on GH
  • Remove the old routes
  • Deploy

@jtgeibel
Copy link
Member Author

To clarify, my last comment applies to the /authorize OAuth callback. The /authorize_url and /logout routes can be moved in sync with a change to the frontend.

@jtgeibel jtgeibel force-pushed the fastboot-staging-experimental branch from 4c33f00 to bf570ca Compare November 23, 2019 19:55
@jtgeibel
Copy link
Member Author

I've updated this PR to remove the hack for the 3 session routes, in anticipation of #1918 landing. Since this is an experimental runtime flag, there is no need to land these PRs in a particular order.

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about some more changes that might need to be made

script/start-web.sh Outdated Show resolved Hide resolved
This script is reference from `package.json` and devlopment should behave
the same as production.
bors added a commit that referenced this pull request Dec 3, 2019
Make /install work under FastBoot

Will add nginx.conf change after #1907. Early feedback is welcome!
Explicitly default to an empty value, to work with `set -u`.
@kzys kzys mentioned this pull request Dec 5, 2019
19 tasks
@carols10cents
Copy link
Member

I think the logic in ember.sh is backwards somehow? Or I might be super confused. I haven't looked closely and thought hard about all the negatives in there, but here's what happens:

On master (3e5dd28)

The behavior on master is what I expect.

No fastboot

What I think is "without any fastboot" if I use npm run start:local and then make a hard reloaded request in the browser to http://localhost:4200/policies, while I'm logged in, with some uninteresting lines elided:

$ npm run start:local

> [email protected] start:local /Users/carolnichols/rust/crates.io
> ./script/ember.sh serve --proxy http://127.0.0.1:8888

[...]

Proxying to http://127.0.0.1:8888

Build successful (11597ms) – Serving on http://localhost:4200/

[...]

GET /livereload.js 404 8.165 ms - 35
GET /api/v1/me 200 13.505 ms - 399

(livereload hasn't been working for me for a while but I don't think that's relevant)

Fastboot only on some pages

What I think is "with fastboot for /policies":

$ USE_FASTBOOT=1 npm run start:local

> [email protected] start:local /Users/carolnichols/rust/crates.io
> ./script/ember.sh serve --proxy http://127.0.0.1:8888

[...]

Proxying to http://127.0.0.1:8888

Build successful (12667ms) – Serving on http://localhost:4200/

[...]
App is being served by FastBoot
DEBUG: -------------------------------
DEBUG: Ember      : 3.1.2
DEBUG: Ember Data : 2.18.2
DEBUG: -------------------------------
(node:40174) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
2019-12-05T20:42:45.536Z 200 OK /policies
GET /livereload.js 404 63.265 ms - 35
GET /api/v1/me 200 8.487 ms - 399

I think fastboot is on because it says "App is being served by FastBoot" (I hope that's not lying to me!) and because the format of the logs is what we talked about in #1908.

On this branch (32a2295)

No fastboot

If I try the same steps as I did on master, this is what I see:

$ npm run start:local

> [email protected] start:local /Users/carolnichols/rust/crates.io
> ./script/ember.sh serve --proxy http://127.0.0.1:8888

[...]

Proxying to http://127.0.0.1:8888

Build successful (13345ms) – Serving on http://localhost:4200/

[...]

App is being served by FastBoot
DEBUG: -------------------------------
DEBUG: Ember      : 3.1.2
DEBUG: Ember Data : 2.18.2
DEBUG: -------------------------------
(node:40416) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
2019-12-05T20:45:49.465Z 200 OK /policies
GET /livereload.js 404 146.684 ms - 35
GET /api/v1/me 200 23.670 ms - 399

I didn't expect to see "App is being served by FastBoot", but I do.

Fastboot only on some pages

If I try the same steps as I did on master, this is what I see:

$ USE_FASTBOOT=1 npm run start:local

> [email protected] start:local /Users/carolnichols/rust/crates.io
> ./script/ember.sh serve --proxy http://127.0.0.1:8888

[...]

Proxying to http://127.0.0.1:8888

Build successful (11761ms) – Serving on http://localhost:4200/

[...]

GET /livereload.js 404 100.218 ms - 35
GET /api/v1/me 200 114.022 ms - 399

I expected fastboot to be on, but this looks like what I see on master when fastboot is off.

Fastboot staging experimental

Similarly, I expected to see signs fastboot is on here, but I don't:

$ USE_FASTBOOT=staging-experimental npm run start:local

> [email protected] start:local /Users/carolnichols/rust/crates.io
> ./script/ember.sh serve --proxy http://127.0.0.1:8888

[...]

Proxying to http://127.0.0.1:8888

Build successful (13155ms) – Serving on http://localhost:4200/

[...]

GET /livereload.js 404 9.405 ms - 35
GET /api/v1/me 200 5.611 ms - 399

So this feels backwards?

I've also deployed this to staging, will update on what I'm seeing there next.

@carols10cents
Copy link
Member

I have this on staging and I set USE_FASTBOOT=staging-experimental... I'm not sure what I should expect to be seeing differently than when USE_FASTBOOT=1 though?

I think it's working though, because when staging has USE_FASTBOOT=staging-experimental and I log in, I briefly see an error in the popup window. In the logs on heroku when this happens, I see:

2019-12-05T21:02:15.652152+00:00 app[web.1]: Error while processing route: github-authorize window.close is not a function TypeError: window.close is not a function
2019-12-05T21:02:15.652225+00:00 app[web.1]: at o.<anonymous> (/app/dist/assets/cargo-344fe2ad42eb900078e166e1e7a8ab40.js:213:33)
2019-12-05T21:02:15.652228+00:00 app[web.1]: at r (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:47:138)
2019-12-05T21:02:15.652230+00:00 app[web.1]: at Generator._invoke (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:43:7)
2019-12-05T21:02:15.652233+00:00 app[web.1]: at Generator.e.<computed> [as next] (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:47:317)
2019-12-05T21:02:15.652235+00:00 app[web.1]: at r (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:47:138)
2019-12-05T21:02:15.652237+00:00 app[web.1]: at t (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:47:398)
2019-12-05T21:02:15.652239+00:00 app[web.1]: at /app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:49:63
2019-12-05T21:02:15.652241+00:00 app[web.1]: at l (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:3431:25)
2019-12-05T21:02:15.652243+00:00 app[web.1]: at v (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:3439:9)
2019-12-05T21:02:15.652246+00:00 app[web.1]: at g (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:3437:43)
2019-12-05T21:02:15.652248+00:00 app[web.1]: at e.invokeWithOnError (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:1596:275)
2019-12-05T21:02:15.652250+00:00 app[web.1]: at e.flush (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:1589:147)
2019-12-05T21:02:15.652252+00:00 app[web.1]: at e.flush (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:1601:15)
2019-12-05T21:02:15.652255+00:00 app[web.1]: at e.end (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:1609:9)
2019-12-05T21:02:15.652257+00:00 app[web.1]: at Timeout._boundAutorunEnd [as _onTimeout] (/app/dist/assets/vendor-bf6f058822ccaed151acd51fae04766b.js:1605:388)
2019-12-05T21:02:15.652259+00:00 app[web.1]: at listOnTimeout (internal/timers.js:531:17)
2019-12-05T21:02:15.652262+00:00 app[web.1]: at processTimers (internal/timers.js:475:7)
2019-12-05T21:02:15.862734+00:00 app[web.1]: 2019-12-05T21:02:15.862Z 200 OK /authorize/github?code=[...]&state=[...]
2019-12-05T21:02:15.922219+00:00 app[web.1]: measure#nginx.service=1.196 request_id=44befcc7-5f10-4603-bf88-96e115ce1a60
2019-12-05T21:02:16.002594+00:00 app[web.1]: at=info method=GET path="/authorize/github?code=[...]&state=[...]" request_id=44befcc7-5f10-4603-bf88-96e115ce1a60 fwd="[...]" user_agent="Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:73.0) Gecko/20100101 Firefox/73.0"

But I do get logged in 😂 I know we don't have everything working under fastboot yet, so does this mean fastboot is enabled?

I am seeing log lines like:

2019-12-05T21:12:44.911018+00:00 app[web.1]: 2019-12-05T21:12:44.910Z 200 OK /crates

when I do a hard reload with USE_FASTBOOT=staging-experimental, as I expect.

When I have USE_FASTBOOT=1, I only see log lines that look like that when I go to /policies or /install, and logging in doesn't produce any errors.

When I unset USE_FASTBOOT, everything indeed looks disabled.

So I think the nginx stuff is working for production-like environments? Let me know if there's anything else you think I should check.

@jtgeibel
Copy link
Member Author

jtgeibel commented Dec 5, 2019 via email

@jtgeibel
Copy link
Member Author

jtgeibel commented Dec 6, 2019

@carols10cents I've updated ember.sh to fix the logic inversion there. I've also clarified things in the PR description above.

I have this on staging and I set USE_FASTBOOT=staging-experimental... I'm not sure what I should expect to be seeing differently than when USE_FASTBOOT=1 though?

If you disable JS you should notice a bigger difference with USE_FASTBOOT=staging-experimental. With JS enabled, once the app has booted then Ember behaves as normal. With JS disabled, each navigation is forced to hit the FastBoot server to serve up static HTML (and you'll see the remaining bugs outlined in #1811).

With JS enabled, Ember will also load data from the backend once it has booted. This means that bugs in our FastBoot implementation may generate incomplete static HTML, but as soon as Ember loads the data is quickly populated making it difficult to see the difference.

@carols10cents
Copy link
Member

If you disable JS you should notice a bigger difference

Aha, that's what I was missing, oops!

Just tested locally with the last commit and it works as I'd expect now!!!

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 18, 2019

📌 Commit 8e02c1e has been approved by carols10cents

@bors
Copy link
Contributor

bors commented Dec 18, 2019

⌛ Testing commit 8e02c1e with merge 48a64ae...

bors added a commit that referenced this pull request Dec 18, 2019
…s10cents

Add a `USE_FASTBOOT=staging-experimental` mode

This adds a new mode to the `USE_FASTBOOT` environment variable.  The logic is now:

* When deployed on Heroku (staging or production - via `script/start-web.sh`):
  * If `USE_FASTBOOT=staging-experimental` then non-backend requests are sent to FastBoot.
  * If `USE_FASTBOOT` is set and non-zero length, FastBoot is enabled only for allowed paths.
  * Otherwise (`USE_FASTBOOT` is not set, or is zero length), FastBoot is disabled.
* When doing local development (via `script/ember.sh`):
  * If `USE_FASTBOOT` is set, all (initial) frontend requests are served via FastBoot.  This is equivalent to `staging-experimental` above.
  * If `USE_FASTBOOT` is not set, FastBoot is disabled.

Because our development environment doesn't go through nginx, it isn't possible to support the "FastBoot is enabled only for allowed paths" mode.  However, once the frontend has booted, all further requests hit the backend (unless JS is disabled, then every client navigation results in a new request served via FastBoot).
@bors
Copy link
Contributor

bors commented Dec 18, 2019

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 48a64ae to master...

@bors bors merged commit 8e02c1e into rust-lang:master Dec 18, 2019
jtgeibel added a commit to jtgeibel/crates.io that referenced this pull request Dec 25, 2019
In production we're currently returning a 502 Bad Gateway for
`/policies`.  I intend to merge this and deploy to production shortly.

This bug was introduced in rust-lang#1907.  The bad check always evaluated to
true.  An explicit check for an empty string is added as well.  I've
tested this locally with `erb` and different values and the generated
configuration is now working correctly.
bors added a commit that referenced this pull request Dec 25, 2019
Fix fastboot check in nginx erb template

In production we're currently returning a 502 Bad Gateway for
`/policies`.  I intend to merge this and deploy to production shortly.

This bug was introduced in #1907.  The bad check always evaluated to
true.  An explicit check for an empty string is added as well.  I've
tested this locally with `erb` and different values and the generated
configuration is now working correctly.

r? @jtgeibel
@jtgeibel jtgeibel deleted the fastboot-staging-experimental branch December 26, 2019 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants