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
nixos/mastodon: sync nginx configuration with upstream's recommendation #198130
base: master
Are you sure you want to change the base?
Conversation
5f92424
to
ac3788b
Compare
As always: Please explain what you did there and why it is necessary. You are adding 170LOC nginx config and your only comment is "update nginx config". I just don't have time for figuring out where you are going here, you have to tell me. |
Changes:
Based on this PR - mastodon/mastodon#19438 |
Looks good, I'd wait with review until the upstream PR is merged. |
|
PR merged in upstream |
ac3788b
to
c1f16e9
Compare
Small fix. |
c1f16e9
to
498c781
Compare
proxy_buffering off; | ||
proxy_redirect off; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is nginx buffering websockets at all? Also upstream recommends against this unless long polling is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it difficult to answer.
I have been using this configuration on my instance for a long time. I didn't notice any suspicious errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be that errors only occur in very specific scenarios and not at all on lightly used instances. In question I'd rather would like to drop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In upstream the proxy_redirect
parameter is disabled:
location ^~ /api/v1/streaming {
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header Proxy "";
proxy_pass http://streaming;
proxy_buffering off;
proxy_redirect off;
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this looks fine, as in it replicates what upstream does? That is the only thing I understand about this tbh, wether it replicates the behaviour recommended by upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set the same parameters and values as in the upstream.
I don't know much about proxying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not feeling to great about just copying upstream values especially if we are not sure if they also not just copied that from somewhere without deeper thought.
cdc2c7c
to
01a0c97
Compare
Added missing proxy headers. |
01a0c97
to
53e8986
Compare
Removed |
53e8986
to
7c7d94f
Compare
Added location and settings for sw.js.map file (mastodon ver 4.0.0rc1). |
locations."= /sw.js" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2110; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=604800, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."= /sw.js.map" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2120; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=604800, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /assets/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2210; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of repetition here. Can you use listToAttrs
or so for locations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate lines have already been moved to the nginxCommonHeaders
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's still a lot of duplication, particularly the tryFiles
and Cache-Control
headers, which are the same for each location except sw.js. +1 to turion's suggestion of listToAttrs
, especially if priority
isn't necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how it can be done. Are there any examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in #202408 i declare sidekiqUnits
which calls lib.attrsets.mapAttrs'
to transform cfg.sidekiqProcesses
(an attrset of attrsets containing jobClasses
and threads
) into a bunch of systemd units based on a template. I then merge those units with the rest of the config using lib.attrsets.recursiveUpdate
.
I think you could take a similar approach here by declaring an attrset of attrsets containing things like the cache-control max-age and the headers to use. you could also have an extraAttrs
attr that you merge into the location's attrset and taking precedence over whatever's in the template, if you wanted maximum flexibility for setting location-specific settings, or you could have an extraConfig
attr that is simply appended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will make it harder to read nginx configuration.
I used a similar variant in PeerTube service:
nixpkgs/nixos/modules/services/web-apps/peertube.nix
Lines 467 to 744 in 2afac7a
services.nginx = lib.mkIf cfg.configureNginx { | |
enable = true; | |
virtualHosts."${cfg.localDomain}" = { | |
root = "/var/lib/peertube"; | |
# Application | |
locations."/" = { | |
tryFiles = "/dev/null @api"; | |
priority = 1110; | |
}; | |
locations."= /api/v1/videos/upload-resumable" = { | |
tryFiles = "/dev/null @api"; | |
priority = 1120; | |
extraConfig = '' | |
client_max_body_size 0; | |
proxy_request_buffering off; | |
''; | |
}; | |
locations."~ ^/api/v1/videos/(upload|([^/]+/studio/edit))$" = { | |
tryFiles = "/dev/null @api"; | |
root = cfg.settings.storage.tmp; | |
priority = 1130; | |
extraConfig = '' | |
client_max_body_size 12G; | |
add_header X-File-Maximum-Size 8G always; | |
'' + lib.optionalString cfg.enableWebHttps '' | |
add_header Strict-Transport-Security 'max-age=63072000; includeSubDomains'; | |
'' + lib.optionalString config.services.nginx.virtualHosts.${cfg.localDomain}.http3 '' | |
add_header Alt-Svc 'h3=":443"; ma=86400'; | |
''; | |
}; | |
locations."~ ^/api/v1/(videos|video-playlists|video-channels|users/me)" = { | |
tryFiles = "/dev/null @api"; | |
priority = 1140; | |
extraConfig = '' | |
client_max_body_size 6M; | |
add_header X-File-Maximum-Size 4M always; | |
'' + lib.optionalString cfg.enableWebHttps '' | |
add_header Strict-Transport-Security 'max-age=63072000; includeSubDomains'; | |
'' + lib.optionalString config.services.nginx.virtualHosts.${cfg.localDomain}.http3 '' | |
add_header Alt-Svc 'h3=":443"; ma=86400'; | |
''; | |
}; | |
locations."@api" = { | |
proxyPass = "http://127.0.0.1:${toString cfg.listenHttp}"; | |
priority = 1150; | |
extraConfig = '' | |
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | |
proxy_set_header Host $host; | |
proxy_set_header X-Real-IP $remote_addr; | |
proxy_connect_timeout 10m; | |
proxy_send_timeout 10m; | |
proxy_read_timeout 10m; | |
client_max_body_size 100k; | |
send_timeout 10m; | |
''; | |
}; | |
# Websocket | |
locations."/socket.io" = { | |
tryFiles = "/dev/null @api_websocket"; | |
priority = 1210; | |
}; | |
locations."/tracker/socket" = { | |
tryFiles = "/dev/null @api_websocket"; | |
priority = 1220; | |
extraConfig = '' | |
proxy_read_timeout 15m; | |
''; | |
}; | |
locations."@api_websocket" = { | |
proxyPass = "http://127.0.0.1:${toString cfg.listenHttp}"; | |
priority = 1230; | |
extraConfig = '' | |
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | |
proxy_set_header Host $host; | |
proxy_set_header X-Real-IP $remote_addr; | |
proxy_set_header Upgrade $http_upgrade; | |
proxy_set_header Connection 'upgrade'; | |
proxy_http_version 1.1; | |
''; | |
}; | |
# Bypass PeerTube for performance reasons. | |
locations."~ ^/client/(assets/images/(icons/icon-36x36\.png|icons/icon-48x48\.png|icons/icon-72x72\.png|icons/icon-96x96\.png|icons/icon-144x144\.png|icons/icon-192x192\.png|icons/icon-512x512\.png|logo\.svg|favicon\.png|default-playlist\.jpg|default-avatar-account\.png|default-avatar-account-48x48\.png|default-avatar-video-channel\.png|default-avatar-video-channel-48x48\.png))$" = { | |
tryFiles = "/www/client-overrides/$1 /www/client/$1 $1"; | |
priority = 1310; | |
}; | |
locations."~ ^/client/(.*\.(js|css|png|svg|woff2|otf|ttf|woff|eot))$" = { | |
alias = "${cfg.package}/client/dist/$1"; | |
priority = 1320; | |
extraConfig = '' | |
add_header Cache-Control 'public, max-age=604800, immutable'; | |
'' + lib.optionalString cfg.enableWebHttps '' | |
add_header Strict-Transport-Security 'max-age=63072000; includeSubDomains'; | |
'' + lib.optionalString config.services.nginx.virtualHosts.${cfg.localDomain}.http3 '' | |
add_header Alt-Svc 'h3=":443"; ma=86400'; | |
''; | |
}; | |
locations."~ ^/lazy-static/(avatars|banners)/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.avatars; | |
priority = 1330; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Cache-Control 'no-cache'; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
${nginxCommonHeaders} | |
add_header Cache-Control 'public, max-age=7200'; | |
rewrite ^/lazy-static/avatars/(.*)$ /$1 break; | |
rewrite ^/lazy-static/banners/(.*)$ /$1 break; | |
''; | |
}; | |
locations."^~ /lazy-static/previews/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.previews; | |
priority = 1340; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Cache-Control 'no-cache'; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
${nginxCommonHeaders} | |
add_header Cache-Control 'public, max-age=7200'; | |
rewrite ^/lazy-static/previews/(.*)$ /$1 break; | |
''; | |
}; | |
locations."^~ /static/thumbnails/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.thumbnails; | |
priority = 1350; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Cache-Control 'no-cache'; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
${nginxCommonHeaders} | |
add_header Cache-Control 'public, max-age=7200'; | |
rewrite ^/static/thumbnails/(.*)$ /$1 break; | |
''; | |
}; | |
locations."^~ /static/redundancy/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.redundancy; | |
priority = 1360; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
if ($request_method = 'GET') { | |
${nginxCommonHeaders} | |
access_log off; | |
} | |
aio threads; | |
sendfile on; | |
sendfile_max_chunk 1M; | |
limit_rate_after 5M; | |
set $peertube_limit_rate 800k; | |
set $limit_rate $peertube_limit_rate; | |
rewrite ^/static/redundancy/(.*)$ /$1 break; | |
''; | |
}; | |
locations."^~ /static/streaming-playlists/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.streaming_playlists; | |
priority = 1370; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
if ($request_method = 'GET') { | |
${nginxCommonHeaders} | |
access_log off; | |
} | |
aio threads; | |
sendfile on; | |
sendfile_max_chunk 1M; | |
limit_rate_after 5M; | |
set $peertube_limit_rate 5M; | |
set $limit_rate $peertube_limit_rate; | |
rewrite ^/static/streaming-playlists/(.*)$ /$1 break; | |
''; | |
}; | |
locations."~ ^/static/webseed/" = { | |
tryFiles = "$uri @api"; | |
root = cfg.settings.storage.videos; | |
priority = 1380; | |
extraConfig = '' | |
if ($request_method = 'OPTIONS') { | |
${nginxCommonHeaders} | |
add_header Access-Control-Max-Age 1728000; | |
add_header Content-Type 'text/plain charset=UTF-8'; | |
add_header Content-Length 0; | |
return 204; | |
} | |
if ($request_method = 'GET') { | |
${nginxCommonHeaders} | |
access_log off; | |
} | |
aio threads; | |
sendfile on; | |
sendfile_max_chunk 1M; | |
limit_rate_after 5M; | |
set $peertube_limit_rate 800k; | |
set $limit_rate $peertube_limit_rate; | |
rewrite ^/static/webseed/(.*)$ /$1 break; | |
''; | |
}; | |
extraConfig = lib.optionalString cfg.enableWebHttps '' | |
add_header Strict-Transport-Security 'max-age=63072000; includeSubDomains'; | |
''; | |
}; | |
}; |
This is a lot of custom config. It is bound to go out of sync with the upstream recommendations eventually. Can we maybe instead simply use the upstream source config file and include it instead of replicating it? https://github.com/mastodon/mastodon/pull/19438/files#diff-47fc0317b265a70f29e18f9c34069ba84a520061e96d0b2dd03c9ee16cfc6b3e |
I don't think we have another way than to configure nginx through the NixOS module system, right? |
Yes, but I would have thought one can use |
Probably rather |
This is difficult to adapt, also the flexibility of the host configuration will be lost. For example it will not be possible to activate http2/3 protocols. |
Updated and rebased PR. |
af237ae
to
a51989b
Compare
nginxCommonHeaders = '' | ||
add_header Strict-Transport-Security 'max-age=63072000; includeSubDomains'; | ||
'' | ||
# Mastodon in upstream is not supported HTTP3 protocol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Mastodon in upstream is not supported HTTP3 protocol. | |
# Upstream does not supported HTTP3 protocol |
Why do we have this, if upstream is not supporting it? Wouldn't we break things then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
It won't break. I've been using the HTTP3 protocol on my copy for a long time.
add_header Alt-Svc 'h3=":443"; ma=86400'; | ||
''; | ||
|
||
nginxProxyHeaders = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we generally consider double proxies. I would just go with recommendedProxyConfig and write a changelog entry that if you are double proxying, you need to change your config.
locations."= /sw.js" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2110; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=604800, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."= /sw.js.map" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2120; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=604800, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /assets/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2230; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /avatars/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2240; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /emoji/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2250; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /headers/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2260; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /packs/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2270; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /sounds/" = { | ||
tryFiles = "$uri =404"; | ||
priority = 2280; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, must-revalidate'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; | ||
|
||
locations."^~ /system/" = { | ||
tryFiles = "$uri =404"; | ||
alias = "/var/lib/mastodon/public-system/"; | ||
priority = 2290; | ||
|
||
extraConfig = '' | ||
add_header Cache-Control 'public, max-age=2419200, immutable'; | ||
${nginxCommonHeaders} | ||
''; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those all seem to be duplicated except of the priority. I would say we move the common things in to a variable and merge the priority into that.
let
commonLocation = {
tryFiles = "$uri =404";
extraConfig = ''
add_header Cache-Control 'public, max-age=2419200, immutable';
${nginxCommonHeaders}
'';
};
in
locations."^~ /system/" = commonLocatuion / {
alias = "/var/lib/mastodon/public-system/";
priority = 2290;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several different options, and you have to include variables for each one. It seems to me that this just makes it harder to view configuration.
proxy_buffering off; | ||
proxy_redirect off; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not feeling to great about just copying upstream values especially if we are not sure if they also not just copied that from somewhere without deeper thought.
0673157
to
accd5a4
Compare
Add hardened headers to user-uploaded files:
|
accd5a4
to
f94eb38
Compare
Rebased PR. diff --git a/nixos/modules/services/web-apps/mastodon.nix b/nixos/modules/services/web-apps/mastodon.nix
index 52a2708bde64..f5756b339396 100644
--- a/nixos/modules/services/web-apps/mastodon.nix
+++ b/nixos/modules/services/web-apps/mastodon.nix
@@ -789,6 +789,9 @@ in {
upstreams = {
"backend-mastodon-streaming" = {
servers = { ${if cfg.enableUnixSocket then "unix:/run/mastodon-streaming/streaming.socket" else "127.0.0.1:${toString cfg.streamingPort}"} = { fail_timeout = "0"; }; };
+ extraConfig = ''
+ least_conn;
+ '';
};
"backend-mastodon-web" = {
servers = { ${if cfg.enableUnixSocket then "unix:/run/mastodon-web/web.socket" else "127.0.0.1:${toString cfg.webPort}"} = { fail_timeout = "0"; }; };
cc @erictapen |
The rest of it would be good to merge as well. |
f94eb38
to
814eebd
Compare
Rebased and updated PR. |
814eebd
to
9e6e4b0
Compare
Update nginx configuration. |
9281915
to
484beb7
Compare
Remove |
484beb7
to
d580ed7
Compare
Update nginx locations block. |
fae33b5
to
9cc7c5e
Compare
9cc7c5e
to
638821d
Compare
Description of changes
Update nginx configuration.
cc @erictapen @SuperSandro2000
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes