Skip to content

Commit 04cf68b

Browse files
committed
uploads: Serve S3 uploads directly from nginx.
When file uploads are stored in S3, this means that Zulip serves as a 302 to S3. Because browsers do not cache redirects, this means that no image contents can be cached -- and upon every page load or reload, every recently-posted image must be re-fetched. This incurs extra load on the Zulip server, as well as potentially excessive bandwidth usage from S3, and on the client's connection. Switch to fetching the content from S3 in nginx, and serving the content from nginx. These have `Cache-control: private, immutable` headers set on the response, allowing browsers to cache them locally. Because nginx fetching from S3 can be slow, and requests for uploads will generally be bunched around when a message containing them are first posted, we instruct nginx to cache the contents locally. This is safe because uploaded file contents are immutable; access control is still mediated by Django. The nginx cache key is the URL without query parameters, as those parameters include a time-limited signed authentication parameter which lets nginx fetch the non-public file. This adds a number of nginx-level configuration parameters to control the caching which nginx performs, including the amount of in-memory index for he cache, the maximum storage of the cache on disk, and how long data is retained in the cache. The currently-chosen figures are reasonable for small to medium deployments. The most notable effect of this change is in allowing browsers to cache uploaded image content; however, while there will be many fewer requests, it also has an improvement on request latency. The following tests were done with a non-AWS client in SFO, a server and S3 storage in us-east-1, and with 100 requests after 10 requests of warm-up (to fill the nginx cache). The mean and standard deviation are shown. | | Redirect to S3 | Caching proxy, hot | Caching proxy, cold | | ----------------- | ------------------- | ------------------- | ------------------- | | Time in Django | 263.0 ms ± 28.3 ms | 258.0 ms ± 12.3 ms | 258.0 ms ± 12.3 ms | | Small file (842b) | 586.1 ms ± 21.1 ms | 266.1 ms ± 67.4 ms | 288.6 ms ± 17.7 ms | | Large file (660k) | 959.6 ms ± 137.9 ms | 609.5 ms ± 13.0 ms | 648.1 ms ± 43.2 ms | The hot-cache performance is faster for both large and small files, since it saves the client the time having to make a second request to a separate host. This performance improvement remains at least 100ms even if the client is on the same coast as the server. Cold nginx caches are only slightly slower than hot caches, because VPC access to S3 endpoints is extremely fast (assuming it is in the same region as the host), and nginx can pool connections to S3 and reuse them. However, all of the 648ms taken to serve a cold-cache large file is occupied in nginx, as opposed to the only 263ms which was spent in nginx when using redirects to S3. This means that to overall spend less time responding to uploaded-file requests in nginx, clients will need to find files in their local cache, and skip making an uploaded-file request, at least 60% of the time. Modeling shows a reduction in the number of client requests by about 70% - 80%. The `Content-Disposition` header logic can now also be entirely shared with the local-file codepath, as can the `url_only` path used by mobile clients. While we could provide the direct-to-S3 temporary signed URL to mobile clients, we choose to provide the served-from-Zulip signed URL, to better control caching headers on it, and greater consistency. In doing so, we adjust the salt used for the URL; since these URLs are only valid for 60s, the effect of this salt change is minimal.
1 parent 58dc105 commit 04cf68b

File tree

12 files changed

+211
-59
lines changed

12 files changed

+211
-59
lines changed

Diff for: docs/production/deployment.md

+23
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,29 @@ all at once. This decreases the number of 502's served to clients, at
684684
the cost of slightly increased memory usage, and the possibility that
685685
different requests will be served by different versions of the code.
686686

687+
#### `s3_memory_cache_size`
688+
689+
Used only when the [S3 storage backend][s3-backend] is in use.
690+
Controls the in-memory size of the cache _index_; the default is 1MB,
691+
which is enough to store about 8 thousand entries.
692+
693+
#### `s3_disk_cache_size`
694+
695+
Used only when the [S3 storage backend][s3-backend] is in use.
696+
Controls the on-disk size of the cache _contents_; the default is
697+
200MB.
698+
699+
#### `s3_cache_inactive_time`
700+
701+
Used only when the [S3 storage backend][s3-backend] is in use.
702+
Controls the longest amount of time an entry will be cached since last
703+
use; the default is 30 days. Since the contents of the cache are
704+
immutable, this serves only as a potential additional limit on the
705+
size of the contents on disk; `s3_disk_cache_size` is expected to be
706+
the primary control for cache sizing.
707+
708+
[s3-backend]: upload-backends.md
709+
687710
#### `uwsgi_listen_backlog_limit`
688711

689712
Override the default uwsgi backlog of 128 connections.

Diff for: docs/production/upload-backends.md

+30
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,36 @@ uploading files, this process does not upload them to Amazon S3; see
5555
[migration instructions](#migrating-from-local-uploads-to-amazon-s3-backend)
5656
below for those steps.
5757

58+
## S3 local caching
59+
60+
For performance reasons, Zulip stores a cache of recently served user
61+
uploads on disk locally, even though the durable storage is kept in
62+
S3. There are a number of parameters which control the size and usage
63+
of this cache, which is maintained by nginx:
64+
65+
- `s3_memory_cache_size` controls the in-memory size of the cache
66+
_index_; the default is 1MB, which is enough to store about 8 thousand
67+
entries.
68+
- `s3_disk_cache_size` controls the on-disk size of the cache
69+
_contents_; the default is 200MB.
70+
- `s3_cache_inactive_time` controls the longest amount of time an
71+
entry will be cached since last use; the default is 30 days. Since
72+
the contents of the cache are immutable, this serves only as a
73+
potential additional limit on the size of the contents on disk;
74+
`s3_disk_cache_size` is expected to be the primary control for cache
75+
sizing.
76+
77+
These defaults are likely sufficient for small-to-medium deployments.
78+
Large deployments, or deployments with image-heavy use cases, will
79+
want to increase `s3_disk_cache_size`, potentially to be several
80+
gigabytes. `s3_memory_cache_size` should potentially be increased,
81+
based on estimating the number of files that the larger disk cache
82+
will hold.
83+
84+
You may also wish to increase the cache sizes if the S3 storage (or
85+
S3-compatible equivalent) is not closely located to your Zulip server,
86+
as cache misses will be more expensive.
87+
5888
## S3 bucket policy
5989

6090
The best way to do the S3 integration with Amazon is to create a new IAM user

Diff for: puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf

+42-2
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,52 @@
1-
location /internal/uploads {
1+
# Handle redirects to S3
2+
location ~ ^/internal/s3/([^/]+)/(.*) {
3+
internal;
4+
include /etc/nginx/zulip-include/headers;
5+
6+
set $download_url https://$1/$2;
7+
proxy_set_header Host $1;
8+
9+
# Ensure that we only get _one_ of these headers: the one that
10+
# Django added, not the one from S3.
11+
proxy_hide_header Content-Disposition;
12+
proxy_hide_header Cache-Control;
13+
proxy_hide_header Expires;
14+
proxy_hide_header Set-Cookie;
15+
# We are _leaving_ S3 to provide Content-Type and Accept-Ranges
16+
# headers, which are the two remaining headers which nginx would
17+
# also pass through from the first response. Django explicitly
18+
# unsets the former, and does not set the latter.
19+
20+
# nginx does its own DNS resolution, which is necessary here to
21+
# resolve the IP of the S3 server. Point it at the local caching
22+
# systemd resolved service. The validity duration is set to match
23+
# S3's DNS validity.
24+
resolver 127.0.0.53 valid=300s;
25+
resolver_timeout 10s;
26+
27+
proxy_pass $download_url$is_args$args;
28+
proxy_cache uploads;
29+
# If the S3 response doesn't contain Cache-Control headers (which
30+
# we don't expect it to) then we assume they are valid for a very
31+
# long time. The size of the cache is controlled by
32+
# `s3_disk_cache_size` and read frequency, set via
33+
# `s3_cache_inactive_time`.
34+
proxy_cache_valid 200 1y;
35+
# Don't include query parameters in the cache key, since those
36+
# include a time-based auth token
37+
proxy_cache_key $download_url;
38+
}
39+
40+
# Internal file-serving
41+
location /internal/local/uploads {
242
internal;
343
include /etc/nginx/zulip-include/api_headers;
444
add_header Content-Security-Policy "default-src 'none'; style-src 'self' 'unsafe-inline'; img-src 'self'; object-src 'self'; plugin-types application/pdf;";
545
include /etc/nginx/zulip-include/uploads.types;
646
alias /home/zulip/uploads/files;
747
}
848

9-
location /internal/user_avatars {
49+
location /internal/local/user_avatars {
1050
internal;
1151
include /etc/nginx/zulip-include/headers;
1252
add_header Content-Security-Policy "default-src 'none' img-src 'self'";

Diff for: puppet/zulip/manifests/app_frontend_base.pp

+18-1
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,18 @@
6969
notify => Service['nginx'],
7070
}
7171

72+
$s3_memory_cache_size = zulipconf('application_server', 's3_memory_cache_size', '1M')
73+
$s3_disk_cache_size = zulipconf('application_server', 's3_disk_cache_size', '200M')
74+
$s3_cache_inactive_time = zulipconf('application_server', 's3_cache_inactive_time', '30d')
75+
file { '/etc/nginx/zulip-include/s3-cache':
76+
require => [Package[$zulip::common::nginx], File['/srv/zulip-uploaded-files-cache']],
77+
owner => 'root',
78+
group => 'root',
79+
mode => '0644',
80+
content => template('zulip/nginx/s3-cache.template.erb'),
81+
notify => Service['nginx'],
82+
}
83+
7284
file { '/etc/nginx/zulip-include/app.d/uploads-internal.conf':
7385
ensure => file,
7486
require => Package[$zulip::common::nginx],
@@ -200,7 +212,12 @@
200212
group => 'zulip',
201213
mode => '0755',
202214
}
203-
215+
file { '/srv/zulip-uploaded-files-cache':
216+
ensure => directory,
217+
owner => 'zulip',
218+
group => 'zulip',
219+
mode => '0755',
220+
}
204221
file { '/var/log/zulip/queue_error':
205222
ensure => directory,
206223
owner => 'zulip',

Diff for: puppet/zulip/templates/nginx/s3-cache.template.erb

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# This cache is only used if S3 file storage is configured.
2+
proxy_cache_path /srv/zulip-uploaded-files-cache
3+
levels=1:2
4+
keys_zone=uploads:<%= @s3_memory_cache_size %>
5+
inactive=<%= @s3_cache_inactive_time %>
6+
max_size=<%= @s3_disk_cache_size %>;

Diff for: puppet/zulip/templates/nginx/zulip-enterprise.template.erb

+2
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@ server {
1212
}
1313
<% end -%>
1414

15+
include /etc/nginx/zulip-include/s3-cache;
1516
include /etc/nginx/zulip-include/upstreams;
1617
include /etc/zulip/nginx_sharding_map.conf;
1718

19+
1820
server {
1921
<% if @nginx_http_only -%>
2022
listen <%= @nginx_listen_port %>;

Diff for: puppet/zulip_ops/files/nginx/sites-available/zulip

+1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
include /etc/nginx/zulip-include/s3-cache;
12
include /etc/nginx/zulip-include/upstreams;
23
include /etc/zulip/nginx_sharding_map.conf;
34

Diff for: puppet/zulip_ops/files/nginx/sites-available/zulip-staging

+1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ server {
55
return 301 https://$server_name$request_uri;
66
}
77

8+
include /etc/nginx/zulip-include/s3-cache;
89
include /etc/nginx/zulip-include/upstreams;
910
include /etc/zulip/nginx_sharding_map.conf;
1011

Diff for: zerver/lib/upload/s3.py

+5-8
Original file line numberDiff line numberDiff line change
@@ -91,24 +91,21 @@ def upload_image_to_s3(
9191
)
9292

9393

94-
def get_signed_upload_url(path: str, download: bool = False) -> str:
94+
def get_signed_upload_url(path: str) -> str:
9595
client = boto3.client(
9696
"s3",
9797
aws_access_key_id=settings.S3_KEY,
9898
aws_secret_access_key=settings.S3_SECRET_KEY,
9999
region_name=settings.S3_REGION,
100100
endpoint_url=settings.S3_ENDPOINT_URL,
101101
)
102-
params = {
103-
"Bucket": settings.S3_AUTH_UPLOADS_BUCKET,
104-
"Key": path,
105-
}
106-
if download:
107-
params["ResponseContentDisposition"] = "attachment"
108102

109103
return client.generate_presigned_url(
110104
ClientMethod="get_object",
111-
Params=params,
105+
Params={
106+
"Bucket": settings.S3_AUTH_UPLOADS_BUCKET,
107+
"Key": path,
108+
},
112109
ExpiresIn=SIGNED_UPLOAD_URL_DURATION,
113110
HttpMethod="GET",
114111
)

Diff for: zerver/tests/test_upload.py

+35-13
Original file line numberDiff line numberDiff line change
@@ -949,7 +949,7 @@ def check_xsend_links(
949949
test_run, worker = os.path.split(os.path.dirname(settings.LOCAL_UPLOADS_DIR))
950950
self.assertEqual(
951951
response["X-Accel-Redirect"],
952-
"/internal/uploads/" + fp_path + "/" + name_str_for_test,
952+
"/internal/local/uploads/" + fp_path + "/" + name_str_for_test,
953953
)
954954
if content_disposition != "":
955955
self.assertIn("attachment;", response["Content-disposition"])
@@ -1882,7 +1882,7 @@ def test_avatar_url_local(self) -> None:
18821882
result = self.client_get(url)
18831883
self.assertEqual(result.status_code, 200)
18841884
internal_redirect_path = urlparse(url).path.replace(
1885-
"/user_avatars/", "/internal/user_avatars/"
1885+
"/user_avatars/", "/internal/local/user_avatars/"
18861886
)
18871887
self.assertEqual(result["X-Accel-Redirect"], internal_redirect_path)
18881888
self.assertEqual(b"", result.content)
@@ -2098,35 +2098,57 @@ def test_file_upload_authed(self) -> None:
20982098
uri = response_dict["uri"]
20992099
self.assertEqual(base, uri[: len(base)])
21002100

2101+
# In development, this is just a redirect
21012102
response = self.client_get(uri)
21022103
redirect_url = response["Location"]
21032104
path = urllib.parse.urlparse(redirect_url).path
21042105
assert path.startswith("/")
2105-
key = path[1:]
2106+
key = path[len("/") :]
2107+
self.assertEqual(b"zulip!", bucket.Object(key).get()["Body"].read())
2108+
2109+
prefix = f"/internal/s3/{settings.S3_AUTH_UPLOADS_BUCKET}.s3.amazonaws.com/"
2110+
with self.settings(DEVELOPMENT=False):
2111+
response = self.client_get(uri)
2112+
redirect_url = response["X-Accel-Redirect"]
2113+
path = urllib.parse.urlparse(redirect_url).path
2114+
assert path.startswith(prefix)
2115+
key = path[len(prefix) :]
21062116
self.assertEqual(b"zulip!", bucket.Object(key).get()["Body"].read())
21072117

21082118
# Check the download endpoint
21092119
download_uri = uri.replace("/user_uploads/", "/user_uploads/download/")
2110-
response = self.client_get(download_uri)
2111-
redirect_url = response["Location"]
2120+
with self.settings(DEVELOPMENT=False):
2121+
response = self.client_get(download_uri)
2122+
redirect_url = response["X-Accel-Redirect"]
21122123
path = urllib.parse.urlparse(redirect_url).path
2113-
assert path.startswith("/")
2114-
key = path[1:]
2124+
assert path.startswith(prefix)
2125+
key = path[len(prefix) :]
21152126
self.assertEqual(b"zulip!", bucket.Object(key).get()["Body"].read())
21162127

21172128
# Now try the endpoint that's supposed to return a temporary URL for access
21182129
# to the file.
21192130
result = self.client_get("/json" + uri)
21202131
data = self.assert_json_success(result)
21212132
url_only_url = data["url"]
2122-
path = urllib.parse.urlparse(url_only_url).path
2123-
assert path.startswith("/")
2124-
key = path[1:]
2133+
2134+
self.assertNotEqual(url_only_url, uri)
2135+
self.assertIn("user_uploads/temporary/", url_only_url)
2136+
self.assertTrue(url_only_url.endswith("zulip.txt"))
2137+
# The generated URL has a token authorizing the requestor to access the file
2138+
# without being logged in.
2139+
self.logout()
2140+
with self.settings(DEVELOPMENT=False):
2141+
self.client_get(url_only_url)
2142+
redirect_url = response["X-Accel-Redirect"]
2143+
path = urllib.parse.urlparse(redirect_url).path
2144+
assert path.startswith(prefix)
2145+
key = path[len(prefix) :]
21252146
self.assertEqual(b"zulip!", bucket.Object(key).get()["Body"].read())
21262147

2127-
# Note: Depending on whether the calls happened in the same
2128-
# second (resulting in the same timestamp+signature),
2129-
# url_only_url may or may not equal redirect_url.
2148+
# The original uri shouldn't work when logged out:
2149+
with self.settings(DEVELOPMENT=False):
2150+
result = self.client_get(uri)
2151+
self.assertEqual(result.status_code, 403)
21302152

21312153
hamlet = self.example_user("hamlet")
21322154
self.subscribe(hamlet, "Denmark")

0 commit comments

Comments
 (0)