Skip to content

Allow push #59

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 2, 2020
Merged

Allow push #59

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9f781f2
allow push
Nov 11, 2020
de7c881
allow to upload big layers
Nov 12, 2020
496b89f
allow push
Nov 11, 2020
d826a7f
allow to upload big layers
Nov 12, 2020
2d9630c
address review comments
fgimenez Nov 15, 2020
6610e36
Fix nginx config
fgimenez Nov 15, 2020
184881b
revert changes
fgimenez Nov 15, 2020
4409f7a
actually remove nginx.conf changes
fgimenez Nov 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,9 @@ echo "Docker configured with HTTPS_PROXY=$scheme://$http_host/"
include /etc/nginx/caching.layer.listen;
server_name _;

# allow to upload big layers
client_max_body_size 0;

# Do some tweaked logging.
access_log /var/log/nginx/access.log tweaked;
set $docker_proxy_request_type "unknown";
Expand All @@ -219,17 +222,6 @@ echo "Docker configured with HTTPS_PROXY=$scheme://$http_host/"
# Docker needs this. Don't ask.
chunked_transfer_encoding on;

# Block POST/PUT/DELETE. Don't use this proxy for pushing.
if ($request_method = POST) {
return 405 "POST method is not allowed";
}
if ($request_method = PUT) {
return 405 "PUT method is not allowed";
}
if ($request_method = DELETE) {
return 405 "DELETE method is not allowed";
}

proxy_read_timeout 900;

# Use cache locking, with a huge timeout, so that multiple Docker clients asking for the same blob at the same time
Expand All @@ -240,6 +232,10 @@ echo "Docker configured with HTTPS_PROXY=$scheme://$http_host/"
# Cache all 200, 206 for 60 days.
proxy_cache_valid 200 206 60d;

proxy_cache_convert_head off;
proxy_cache_methods GET;
proxy_cache_key $scheme$request_method$proxy_host$request_uri;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, this has come up before. it is a breaking change (people with huge 2TB caches will break, due to the cache_key changing, and it will sit there collecting dust, and eventually run out of disk space).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, given that we are only caching GET requests maybe we could leave proxy_cache_key with its default value $scheme$proxy_host$request_uri and drop the change, wdyt?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the big issue is that DockerHub allegedly does not count HEAD requests against its Pull Rate Limit.
@cpuguy83 discovered that Docker Client does not send HEADs, while containerd does. There's patches and discussion upstream in Moby still.
So the way it is now (before this change), all HEADs get upgraded to GETs and it does not matter if the method is included in the cache_key.
I really don't know how this affects pushes, at all -- why did you include this in the patch to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is copied verbatim from #17, no special reason on my side.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without those, the patch then just boils down to removing the 405 blocks from non-GET methods.
those were in place for a reason, although I don't remember why.
maybe since then the underlying reason was solved somewhere else, and it's ok to just remove them, but I doubt it.
anyway, if you boil it down to an ENV var ALLOW_PUSH=true that just removes the blocks it would be okay.
The cache_key stuff I'll handle separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok done, PTAL, i've only left client_max_body_size and proxy_cache_methods set when push is allowed, let me know wdyt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpardini any news about this? let me know if any other changes are required.

# Some extra settings to maximize cache hits and efficiency
proxy_force_ranges on;
proxy_ignore_client_abort on;
Expand Down