Use Nginx to make Buildbot logs available via CORS #375
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @jdm: Is this sufficient to pull the logs you needed or do I need to support OPTIONS/preflight requests too? |
aneeshusa
force-pushed
the
add-basic-cors-headers
branch
from
7442488
to
5f27eb0
Compare
Buildbot itself (neither the current version eight nor the upcoming
verison nine) does not seem to have built-in CORS support, so use Nginx
to supply CORS headers while reverse proxying.
This is only needed to download log files, so start with a very
conservative set of CORS headers. In particular:
- Only support basic CORS requests (those that don't require preflight
requests via OPTIONS). This means supporting only GET requests.
- Do not allow sending cookies or exposing any headers.
In return, whitelist all domains for CORS, and send the Allow-Origin
header unconditionally, as the actual value of the Origin header does
not matter.
Because we are using proxy_pass to reverse proxy Buildbot, we need to
tell Nginx to always add the CORS headers, instead of only adding them
for successful responses. (I.e. add CORS headers even for 404s and other
erroneous status codes). This features is not available in the Nginx
packaged for Ubuntu Trusty, so replace it with a newer Nginx from
upstream Nginx's respositories.
Because Debian/Ubuntu modify the default configuration for Nginx, use
Salt to ensure that the installed nginx.conf properly looks into the
sites-enabled directory (which is a Debian-ism) and that the default
handler in /etc/nginx/conf.d/ is purged.
In order to only support CORS on GET requests, use the 'if' directive.
However, because Nginx's if is evil,
(https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/)
add a bunch of tests to ensure that the configuration is working.
Also, update the Nginx states to follow Salt best practices.
aneeshusa
force-pushed
the
add-basic-cors-headers
branch
from
5f27eb0
to
5d93b54
Compare
|
I don't know of any reason that preflight requests should be required. |
|
No longer blocked since #374 merged a while ago. @bors-servo try |
bors-servo
pushed a commit
that referenced
this pull request
Sep 29, 2016
Use Nginx to make Buildbot logs available via CORS
Buildbot itself (neither the current version eight nor the upcoming
verison nine) does not seem to have built-in CORS support, so use Nginx
to supply CORS headers while reverse proxying.
This is only needed to download log files, so start with a very
conservative set of CORS headers. In particular:
- Only support basic CORS requests (those that don't require preflight
requests via OPTIONS). This means supporting only GET requests.
- Do not allow sending cookies or exposing any headers.
In return, whitelist all domains for CORS, and send the Allow-Origin
header unconditionally, as the actual value of the Origin header does
not matter.
Because we are using proxy_pass to reverse proxy Buildbot, we need to
tell Nginx to always add the CORS headers, instead of only adding them
for successful responses. (I.e. add CORS headers even for 404s and other
erroneous status codes). This features is not available in the Nginx
packaged for Ubuntu Trusty, so replace it with a newer Nginx from
upstream Nginx's respositories.
Because Debian/Ubuntu modify the default configuration for Nginx, use
Salt to ensure that the installed nginx.conf properly looks into the
sites-enabled directory (which is a Debian-ism) and that the default
handler in /etc/nginx/conf.d/ is purged.
In order to only support CORS on GET requests, use the 'if' directive.
However, because Nginx's if is evil,
(https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/)
add a bunch of tests to ensure that the configuration is working.
Also, update the Nginx states to follow Salt best practices.
Fixes #354. Blocked on #374 because I encountered various failures
while upgrading from the Ubuntu Nginx to upstream Nginx, and I'd
like to make sure I caught them all.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/375)
<!-- Reviewable:end -->
|
💔 Test failed - status-travis |
|
☔ The latest upstream changes (presumably #544) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Buildbot itself (neither the current version eight nor the upcoming
verison nine) does not seem to have built-in CORS support, so use Nginx
to supply CORS headers while reverse proxying.
This is only needed to download log files, so start with a very
conservative set of CORS headers. In particular:
requests via OPTIONS). This means supporting only GET requests.
In return, whitelist all domains for CORS, and send the Allow-Origin
header unconditionally, as the actual value of the Origin header does
not matter.
Because we are using proxy_pass to reverse proxy Buildbot, we need to
tell Nginx to always add the CORS headers, instead of only adding them
for successful responses. (I.e. add CORS headers even for 404s and other
erroneous status codes). This features is not available in the Nginx
packaged for Ubuntu Trusty, so replace it with a newer Nginx from
upstream Nginx's respositories.
Because Debian/Ubuntu modify the default configuration for Nginx, use
Salt to ensure that the installed nginx.conf properly looks into the
sites-enabled directory (which is a Debian-ism) and that the default
handler in /etc/nginx/conf.d/ is purged.
In order to only support CORS on GET requests, use the 'if' directive.
However, because Nginx's if is evil,
(https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/)
add a bunch of tests to ensure that the configuration is working.
Also, update the Nginx states to follow Salt best practices.
Fixes #354. Blocked on #374 because I encountered various failures
while upgrading from the Ubuntu Nginx to upstream Nginx, and I'd
like to make sure I caught them all.
This change is