Optionally instruct nginx to proxy assets from S3 #126
Conversation
chrisroos
force-pushed
the
proxy-via-nginx
branch
2 times, most recently
from
31b10a8
to
9d716d3
Compare
chrisroos
changed the title
chrisroos
force-pushed
the
proxy-via-nginx
branch
from
9d716d3
to
76bc4d5
Compare
app/controllers/media_controller.rb
Outdated
| elsif proxy_via_nginx? | ||
| url = Services.cloud_storage.presigned_url_for(asset) | ||
| headers['X-Cloud-Storage-URL'] = url | ||
| headers['X-Cloud-Storage-Host'] = URI.parse(url).host |
There was a problem hiding this comment.
It would be useful to explain in the commit note why you need both the full URL (including the host) and the host on its own.
There was a problem hiding this comment.
I've just noticed that you've explained this in an example name below, so I think it's less important to explain in the commit note.
There was a problem hiding this comment.
We're no longer sending the URL and Host so this problem has gone away.
| @@ -31,6 +31,10 @@ def public_url_for(asset) | |||
| object_for(asset).public_url(virtual_host: AssetManager.aws_s3_use_virtual_host) | |||
| end | |||
|
|
|||
| def presigned_url_for(asset) | |||
| object_for(asset).presigned_url(:get, expires_in: 1.minute, virtual_host: AssetManager.aws_s3_use_virtual_host) | |||
There was a problem hiding this comment.
Minor: It would be nice to pull the expires_in option value into the application configuration c.f. AssetManager.aws_s3_use_virtual_host.
There was a problem hiding this comment.
I've decided to leave it where it is at the moment as it wasn't immediately obvious whether we need to change this value. I suggest we move it to a config option if/when we need to update it.
app/controllers/media_controller.rb
Outdated
| url = Services.cloud_storage.presigned_url_for(asset) | ||
| headers['X-Cloud-Storage-URL'] = url | ||
| headers['X-Cloud-Storage-Host'] = URI.parse(url).host | ||
| headers['X-Accel-Redirect'] = "/cloud-storage-proxy/" |
There was a problem hiding this comment.
Very minor: It would be nice to be consistent about using single quotes where possible in this file.
There was a problem hiding this comment.
The value of the X-Accel-Redirect header now includes some string interpolation so I've had to keep the double quotes.
chrisroos
force-pushed
the
proxy-via-nginx
branch
from
76bc4d5
to
d12b03d
Compare
|
Thanks for the review, @floehopper. I've just rebased on master and made some changes based on the updates in alphagov/govuk-puppet#6280. Can you review it again, please? |
| expect(subject.presigned_url_for(asset)).to eq('presigned-url') | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Minor: It would be nice to test the value of the expires_in option separately to (a) reduce the brittleness/duplication in these two examples; and (b) explain the choice of value in an example name.
This complements the change in the asset-manager nginx config[1]. The app sets the `X-Accel-Redirect` header to instruct nginx to proxy the request for an asset to S3. We're generating a presigned URL for the assets on S3 so that we can avoid making them publicly accessible. The presigned URL is valid for 1 minute which should be enough time for nginx to use the URL to make the request. [1]: alphagov/govuk-puppet#6280
chrisroos
force-pushed
the
proxy-via-nginx
branch
from
d12b03d
to
e334116
Compare
This complements the change in the asset-manager nginx config.
The app sets the
X-Accel-Redirectheader to instruct nginx to proxythe request for an asset to S3.
We're generating a presigned URL for the assets on S3 so that we can
avoid making them publicly accessible. The presigned URL is valid for 1
minute which should be enough time for nginx to use the URL to make the
request.