Skip to content

feat(nginx): cache by blob sha instead of uri #34

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

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Conversation

ChandonPierre
Copy link

From upstream PR: rpardini#107

If images sourced from separate registries share the same layer, do not cache them twice

Tested with ghcr.io/goauthentik/server:2023.10.6 registry.gitlab.com/coreweave/utility-images/authentik:66c08674 from the below dockerfile

Almost all layers were cache HITs when pulling the second image, versus MISS in the previous implementation

# Use the base image from ghcr.io/goauthentik/server with tag 2023.10.6
FROM ghcr.io/goauthentik/server:2023.10.6

# Download the LDAP models.py file and add it to /authentik/sources/ldap/models.py
ADD https://raw.githubusercontent.com/goauthentik/authentik/c0b7d32b365520425e24d8a22940525d5327b205/authentik/sources/ldap/models.py /authentik/sources/ldap/models.py

USER root

RUN chmod 644 /authentik/sources/ldap/models.py

USER 1000

@ChandonPierre ChandonPierre requested a review from robbat2 January 19, 2024 04:35
@robbat2
Copy link

robbat2 commented Jan 19, 2024

how does this prevent a cache collision between HEAD/GET/OPTIONS on the same URLs? Esp when the conditionals for ALLOW_PUSH etc are false, so proxy_cache_methods is not set.

I think $request_method should have been included in the cache_key, so that HEAD is also cached, distinctly from GET requests.

If you've tested that case, sure, let's ship it, otherwise please verify that first.
As a plus, it will also mean that HEAD is cached and doesn't hit ratelimits

@ChandonPierre
Copy link
Author

how does this prevent a cache collision between HEAD/GET/OPTIONS on the same URLs? Esp when the conditionals for ALLOW_PUSH etc are false, so proxy_cache_methods is not set.

I think $request_method should have been included in the cache_key, so that HEAD is also cached, distinctly from GET requests.

If you've tested that case, sure, let's ship it, otherwise please verify that first. As a plus, it will also mean that HEAD is cached and doesn't hit ratelimits

Good catch; that's likely the reason for rpardini#89

I'll pull in something similar and add to this PR

@ChandonPierre ChandonPierre merged commit 2236393 into coreweave Jan 31, 2024
@ChandonPierre ChandonPierre deleted the blob branch April 7, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants