Skip to content
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

Set cache control header on API responses #4005

Closed
sarayourfriend opened this issue Apr 2, 2024 · 2 comments
Closed

Set cache control header on API responses #4005

sarayourfriend opened this issue Apr 2, 2024 · 2 comments
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API 💬 talk: discussion Open for discussions and feedback

Comments

@sarayourfriend
Copy link
Collaborator

sarayourfriend commented Apr 2, 2024

Problem

We do not set cache-control headers on our API responses at the origin, and instead rely on Cloudflare to set these.

While we can continue configuring specific cache-control settings, Cloudflare is able to interpret cache control headers from upstream responses and use those settings for edge TTL. That would simplify our Cloudflare configuration by removing the details of individual endpoints and allow that to live instead in the API code, where it probably belongs.

This wouldn't preclude us adding additional cache settings in Cloudflare if we ever wanted to.

Description

Add the cache_control decorator to the API views, with TTL as listed. The TTLs are taken from our Cloudflare configuration and for this issue we'll take for granted that they are correct, and defer modifications to these TTLs to a future issue if needed.

  • Stats endpoint: 259200 (3 days)
  • Thumbnail endpoint: 31536000 (1 year)
  • Waveform endpoint: 31536000 (1 year)
  • Default for all other endpoints: 2592000 (1 month)

Lastly, explicitly exclude the auth_tokens routes by adding never_cache. This again reduces configuration necessary in Cloudflare and moves it upstream into our code.

Additional context

This is a supplement to https://github.com/WordPress/openverse-infrastructure/issues/777, removing the need to move many of the rules defined in our Cloudflare configuration to the the openverse.org zone. As such, I'm setting it to high priority and into the todo list and starting work on it ASAP as part of the openverse.org migration project. This doesn't block that issue because, as I've written in the issue, we'll just ignore the .engineering caching rules and add a single rule to instruct Cloudflare to use the upstream header.

@sarayourfriend sarayourfriend added 🟧 priority: high Stalls work on the project or its dependents 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🧱 stack: api Related to the Django API labels Apr 2, 2024
@sarayourfriend sarayourfriend self-assigned this Apr 2, 2024
@sarayourfriend
Copy link
Collaborator Author

This will require upgrading to Django 5, due to the need for async support on cache_control decorator; or to backport support for async support in that decorator from Django 5 into 4.2.

@sarayourfriend sarayourfriend added the 💬 talk: discussion Open for discussions and feedback label Apr 3, 2024
@sarayourfriend
Copy link
Collaborator Author

I'm going to close this issue as won't do, at least for now. It's out of scope of the original project and potentially way way more complicated than I originally imagined. Thanks @obulat for pointing out some the issues in the linked PR. Closing the PR as well.

@sarayourfriend sarayourfriend closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API 💬 talk: discussion Open for discussions and feedback
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant