Page MenuHomePhabricator

Add a wide range of HTTP-request-based setup checks
ClosedPublic

Authored by epriestley on Apr 30 2015, 1:29 PM.

Details

Summary

Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves.

I use a magical header to make this request because we want to test everything else (parameters, path).

  • Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this.
  • Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA").
  • Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B").
  • Fixes T4921. We now test that Authorization survives the request.
  • Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level.
  • Fixes <space space newline newline space><?php in preamble.php.
Test Plan

Tested all of these setup warnings, although mostly by faking them.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

joshuaspence retitled this revision from to Add a setup check for gzip.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley requested changes to this revision.Apr 30 2015, 2:42 PM
epriestley edited edge metadata.

I think this is trickier than it appears:

  • phabricator.base-uri may not be set.
  • Phabricator may be on a non-default port.
  • Phabricator may be configured not to accept local traffic, only traffic via a load balancer. This is very unlikely, but could cause these requests to fail.
  • The localhost may hang indefinitely.
  • The localhost may work fine, execute this setup check itself, make another request, which works fine, executes this setup check, etc.

I think we need to:

  • Don't run the check if there's no base URI.
  • Use the entire base URI.
  • (You can -- and should -- always use HTTPSFuture regardless of the protocol).
  • Set a short (5 second?) timeout on the request.
  • Add an X-Phabricator-Setup-Check header.
  • Don't run the check if the header is present in the request.
  • Ideally, make this request to some purpose-built controller which also lets us test for T5067 in one request, perhaps in a future diff.
This revision now requires changes to proceed.Apr 30 2015, 2:42 PM
This comment was removed by epriestley.
This comment was removed by epriestley.

I'm going to come back to this as my latest changes don't seem to work at the moment. Before I forget, I originally had to change HTTPFuture to use HTTP 1.1 instead of HTTP 1.0. My local nginx configuration doesn't gzip responses for HTTP 1.0. I then, however, had to add the Connection: Close header otherwise the request would remain open and eventually timeout.

joshuaspence edited edge metadata.
  • Use HTTPSFuture
  • Check that phabricator.base-uri is set.
  • Set reasonable timeout value.
  • Add X-Phabricator-Setup-Check header.
epriestley requested changes to this revision.May 1 2015, 6:07 PM
epriestley edited edge metadata.

Cool, this looks pretty reasonable.

src/applications/config/check/PhabricatorWebServerSetupCheck.php
11

Debugging?

26

What's the reasoning for this? I'd expect us to just use the same domain?

41

We should expand this but I can counter-diff you on that.

This revision now requires changes to proceed.May 1 2015, 6:07 PM
joshuaspence added inline comments.May 4 2015, 10:37 PM
src/applications/config/check/PhabricatorWebServerSetupCheck.php
11

Oops, yeah.

26

I guess it's probably not worth it, but I figured that you might have to hit the web server directly instead of potentially going through a load balancer.

41

Yeah, I'll expand on this a bit but it's probably better that the copy comes from you.

joshuaspence edited edge metadata.
joshuaspence marked 6 inline comments as done.
joshuaspence edited edge metadata.

Remove some unnecessary code

This setup check doesn't seem to work for me anymore. Instead of getting a Content-Encoding header in the response I am now getting Transfer-Encoding. I spent ~5 minutes looking into this but couldn't establish the relationship between the two headers.

epriestley commandeered this revision.Dec 8 2016, 9:01 PM
epriestley edited reviewers, added: joshuaspence; removed: epriestley.
epriestley updated this revision to Diff 40927.Dec 8 2016, 11:30 PM
  • Perform many more checks.

oh no my beautiful --edit

chad accepted this revision.Dec 8 2016, 11:43 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Dec 8 2016, 11:43 PM

If your load balancer somehow drops this header we'll end up looping forever, but I don't know that there's really much we can do about that -- I think it's an inherent risk in making requests to ourselves. Load balancers also generally should not be dropping random headers that they decide they don't like the look of.

This revision was automatically updated to reflect the committed changes.