diff --git a/pulpcore/app/util.py b/pulpcore/app/util.py index 76f2a5b47fb..a527938c463 100644 --- a/pulpcore/app/util.py +++ b/pulpcore/app/util.py @@ -17,6 +17,7 @@ from django.conf import settings from django.db import connection from django.db.models import Model, UUIDField +from django.utils.http import parse_http_date from rest_framework.reverse import reverse as drf_reverse from rest_framework.serializers import ValidationError @@ -693,6 +694,22 @@ def normalize_http_status(status): return "" +def check_request_was_modified(request, last_modified): + if not last_modified: + return True + + if_modified_since = request.headers.get("If-Modified-Since") + if not if_modified_since: + return True + + try: + last_modified_ts = parse_http_date(last_modified) + if_modified_ts = parse_http_date(if_modified_since) + return last_modified_ts > if_modified_ts + except (TypeError, ValueError): + return True + + class HashingFileWriter(RawIOBase): """ A file-like object that handles writing data to disk with simultaneous diff --git a/pulpcore/cache/cache.py b/pulpcore/cache/cache.py index 6fcf470e14a..7e0344aa4ab 100644 --- a/pulpcore/cache/cache.py +++ b/pulpcore/cache/cache.py @@ -3,8 +3,8 @@ import time from functools import wraps -from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response, StreamResponse -from aiohttp.web_exceptions import HTTPFound +from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response +from aiohttp.web_exceptions import HTTPFound, HTTPNotModified from django.conf import settings from django.http import FileResponse as ApiFileResponse from django.http import HttpResponse, HttpResponseRedirect @@ -17,6 +17,7 @@ get_async_redis_connection, get_redis_connection, ) +from pulpcore.app.util import check_request_was_modified from pulpcore.metrics import artifacts_size_counter from pulpcore.responses import ArtifactResponse @@ -350,7 +351,7 @@ async def cached_function(*args, **kwargs): await self.auth(request, self, bk) key = self.make_key(request) # Check cache - response = await self.make_response(key, bk) + response = await self.make_response(key, bk, request) if response is None: # Cache miss, create new entry response = await self.make_entry( @@ -369,7 +370,7 @@ def get_request_from_args(self, args): if isinstance(arg, Request): return arg - async def make_response(self, key, base_key): + async def make_response(self, key, base_key, request=None): """Tries to find the cached entry and turn it into a proper response""" entry = await self.get(key, base_key) if not entry: @@ -392,7 +393,14 @@ async def make_response(self, key, base_key): # Bad entry, delete from cache await self.delete(key, base_key) return None - response = self.RESPONSE_TYPES[response_type](**entry) + + headers = entry.get("headers", {}) + if request and not check_request_was_modified( + request, last_modified=headers.get("Last-Modified") + ): + response = HTTPNotModified(headers={"Cache-Control": headers.get("Cache-Control")}) + else: + response = self.RESPONSE_TYPES[response_type](**entry) response.headers.update({"X-PULP-CACHE": "HIT"}) return response @@ -400,13 +408,12 @@ async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT """Gets the response for the request and try to turn it into a cacheable entry""" try: response = await handler(*args, **kwargs) - except (HTTPSuccessful, HTTPFound) as e: + except (HTTPSuccessful, HTTPFound, HTTPNotModified) as e: response = e original_response = response - if isinstance(response, StreamResponse): - if hasattr(response, "future_response"): - response = response.future_response + if hasattr(response, "future_response"): + response = response.future_response entry = {"headers": dict(response.headers), "status": response.status} if expires is not None: diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py index 77c5cfea43e..77bc04496b3 100644 --- a/pulpcore/content/handler.py +++ b/pulpcore/content/handler.py @@ -17,10 +17,12 @@ HTTPFound, HTTPMovedPermanently, HTTPNotFound, + HTTPNotModified, HTTPRequestRangeNotSatisfiable, ) from asgiref.sync import sync_to_async from django.utils import timezone +from django.utils.http import http_date from multidict import CIMultiDict from yarl import URL @@ -56,6 +58,7 @@ ) from pulpcore.app.util import ( # noqa: E402 cache_key, + check_request_was_modified, get_domain, ) from pulpcore.cache import AsyncContentCache # noqa: E402 @@ -67,6 +70,8 @@ log = logging.getLogger(__name__) +EDGE_CACHE_CONTROL = "public, max-age=0, must-revalidate" + class PathNotResolved(HTTPNotFound): """ @@ -524,6 +529,8 @@ def response_headers(path, distribution=None): if content_type: headers["Content-Type"] = content_type + headers["Cache-Control"] = EDGE_CACHE_CONTROL + # Let plugin-Distribution set headers for this path if it wants. if distribution: headers.update(distribution.content_headers_for(path)) @@ -720,6 +727,9 @@ async def _match_and_stream(self, path, request): content_handler_result = await sync_to_async(distro.content_handler)(original_rel_path) if content_handler_result is not None: if isinstance(content_handler_result, ContentArtifact): + await self._add_last_modified_header( + headers, content_artifact=content_handler_result, distribution=distro + ) if content_handler_result.artifact: return await self._serve_content_artifact( content_handler_result, headers, request @@ -755,6 +765,9 @@ async def _match_and_stream(self, path, request): raise HTTPMovedPermanently(f"{request.path}/") original_rel_path = index_path headers = self.response_headers(original_rel_path, distro) + await self._add_last_modified_header( + headers, suggested_last_modified=publication.pulp_created + ) except ObjectDoesNotExist: dir_list, dates, sizes = await self.list_directory(None, publication, rel_path) dir_list.update( @@ -813,6 +826,11 @@ async def _match_and_stream(self, path, request): except ObjectDoesNotExist: pass else: + await self._add_last_modified_header( + headers, + content_artifact=ca, + repository_version=publication.repository_version, + ) if ca.artifact: return await self._serve_content_artifact(ca, headers, request) else: @@ -824,6 +842,9 @@ async def _match_and_stream(self, path, request): if distro.SERVE_FROM_PUBLICATION: ca = await sync_to_async(distro.get_fallback_ca)(original_rel_path) if ca is not None: + await self._add_last_modified_header( + headers, content_artifact=ca, repository_version=repo_version + ) if ca.artifact: return await self._serve_content_artifact(ca, headers, request) else: @@ -871,6 +892,9 @@ async def _match_and_stream(self, path, request): except ObjectDoesNotExist: pass else: + await self._add_last_modified_header( + headers, content_artifact=ca, repository_version=repo_version + ) if ca.artifact: return await self._serve_content_artifact(ca, headers, request) else: @@ -935,6 +959,38 @@ class Error(HTTPError): reason = None raise PathNotResolved(path, reason=reason) + async def _add_last_modified_header( + self, + headers, + suggested_last_modified: datetime | None = None, + content_artifact=None, + repository_version=None, + distribution=None, + ): + """ + Add the last-modified header to the response headers if not already present. + """ + + def _find_repo_add_time(): + if not repository_version: + _, rv, _ = distribution.get_repository_publication_and_version() + else: + rv = repository_version + cpk = content_artifact.content_id + + rc = rv._content_relationships().filter(content_id=cpk).first() + return rc.pulp_created if rc else rv.pulp_created + + if "Last-Modified" not in headers: + last_modified = None + if suggested_last_modified is not None: + last_modified = suggested_last_modified + elif content_artifact and (repository_version or distribution): + last_modified = await sync_to_async(_find_repo_add_time)() + + if last_modified is not None: + headers["Last-Modified"] = http_date(last_modified.timestamp()) + async def _stream_content_artifact(self, request, response, content_artifact): """ Stream and optionally save a ContentArtifact by requesting it using the associated remote. @@ -1174,9 +1230,16 @@ async def _serve_content_artifact(self, content_artifact, headers, request): size = artifact_file.size or "*" raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"}) + response = self._build_response_from_content_artifact(content_artifact, headers, request) + + if not check_request_was_modified(request, last_modified=headers.get("Last-Modified")): + nmod_response = HTTPNotModified(headers={"Cache-Control": EDGE_CACHE_CONTROL}) + if settings.CACHE_ENABLED: + nmod_response.future_response = response + raise nmod_response + artifacts_size_counter.add(content_length) - response = self._build_response_from_content_artifact(content_artifact, headers, request) if isinstance(response, HTTPFound): raise response else: diff --git a/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py new file mode 100644 index 00000000000..f9b0adaf89d --- /dev/null +++ b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py @@ -0,0 +1,186 @@ +"""Tests for If-Modified-Since / 304 Not Modified on the content app. + +When a client already has a copy of a file it can send an If-Modified-Since header +asking the content app to reply "304 Not Modified" (an empty body) instead of +re-sending the whole file. These tests cover that conversation. +""" + +import time +from base64 import b64encode +from urllib.parse import urljoin +from uuid import uuid4 + +import pytest +import requests +from django.utils.http import http_date, parse_http_date + +from pulpcore.client.pulp_file import ( + FileRepositorySyncURL, + PatchedfileFileDistribution, +) +from pulpcore.content.handler import EDGE_CACHE_CONTROL + + +def assert_full_download(response): + """The server sent the whole file, plus the headers a client reuses to revalidate later.""" + assert response.status_code == 200 + assert response.content + assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL + # Last-Modified must be a real date in the past - never missing, epoch, or in the future. + last_modified = response.headers.get("Last-Modified") + assert last_modified + assert 0 < parse_http_date(last_modified) <= time.time() + + +def assert_not_modified(response): + """The server skipped the download: a 304 with an empty body but the caching header intact.""" + assert response.status_code == 304 + assert response.content == b"" + assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL + + +def assert_object_storage_redirect(response): + """On Azure/S3, pulpcore answers with a 302 to the object store, carrying the revalidation + headers - it does not stream the bytes itself.""" + assert response.status_code == 302 + assert response.headers.get("Location") + assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL + # Last-Modified must be a real date in the past - never missing, epoch, or in the future. + last_modified = response.headers.get("Last-Modified") + assert last_modified + assert 0 < parse_http_date(last_modified) <= time.time() + + +@pytest.fixture +def assert_full_response(pulp_settings): + """Return the right assertion for a served (non-304) response on this backend: full bytes on + the filesystem, or a redirect to the object store on Azure/S3.""" + backend = pulp_settings.STORAGES["default"]["BACKEND"] + redirects = ( + backend != "pulpcore.app.models.storage.FileSystem" + and pulp_settings.REDIRECT_TO_OBJECT_STORAGE + ) + return assert_object_storage_redirect if redirects else assert_full_download + + +@pytest.fixture +def distribution( + file_repo_with_auto_publish, + file_remote_factory, + file_bindings, + file_distribution_factory, + monitor_task, + basic_manifest_path, +): + """Sync a small file repo, auto-publish it, and distribute it. Returns the distribution.""" + remote = file_remote_factory(manifest_path=basic_manifest_path, policy="immediate") + body = FileRepositorySyncURL(remote=remote.pulp_href) + monitor_task( + file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task + ) + repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href) + return file_distribution_factory(repository=repo.pulp_href) + + +@pytest.fixture +def distribution_url(distribution, distribution_base_url): + """The externally reachable base URL where `distribution` serves its files.""" + return distribution_base_url(distribution.base_url) + + +@pytest.mark.parallel +def test_a_current_copy_is_not_redownloaded(distribution_url, assert_full_response): + """A client whose copy is already up to date gets a 304 instead of the file.""" + url = urljoin(distribution_url, "1.iso") + + # Download once and remember the date the server reported. + first = requests.get(url, allow_redirects=False) + assert_full_response(first) + served_date = first.headers["Last-Modified"] + + # Asking again with that exact date -> nothing changed -> 304. + reused = requests.get(url, headers={"If-Modified-Since": served_date}, allow_redirects=False) + assert_not_modified(reused) + + # A client claiming an even newer copy than the server's is also told 304. + tomorrow = http_date(time.time() + 3600) + newer = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False) + assert_not_modified(newer) + + +@pytest.mark.parallel +def test_a_stale_copy_gets_the_full_file(distribution_url, assert_full_response): + """A client whose copy predates the file downloads the whole thing again.""" + url = urljoin(distribution_url, "1.iso") + + # "I last saw this at the dawn of time" -> the file is newer -> send it all. + long_ago = http_date(0) + response = requests.get(url, headers={"If-Modified-Since": long_ago}, allow_redirects=False) + assert_full_response(response) + + +@pytest.mark.parallel +def test_authorization_runs_before_revalidation( + distribution, + distribution_url, + assert_full_response, + pulpcore_bindings, + file_bindings, + gen_object_with_cleanup, + monitor_task, +): + """A content guard is checked on every request - even one that would answer 304.""" + url = urljoin(distribution_url, "1.iso") + + # Protect the distribution: callers must send x-header: base64("123456"). + guard = gen_object_with_cleanup( + pulpcore_bindings.ContentguardsHeaderApi, + {"name": str(uuid4()), "header_name": "x-header", "header_value": "123456"}, + ) + body = PatchedfileFileDistribution(content_guard=guard.pulp_href) + monitor_task( + file_bindings.DistributionsFileApi.partial_update(distribution.pulp_href, body).task + ) + + credentials = {"x-header": b64encode(b"123456").decode("ascii")} + tomorrow = http_date(time.time() + 3600) + + # No credentials -> rejected up front, before any 304 revalidation can happen. + denied = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False) + assert denied.status_code == 403 + + # With credentials the normal conversation works: full download, then 304. + authorized = requests.get(url, headers=credentials, allow_redirects=False) + assert_full_response(authorized) + + revalidated = requests.get( + url, + headers={**credentials, "If-Modified-Since": tomorrow}, + allow_redirects=False, + ) + assert_not_modified(revalidated) + + +@pytest.mark.parallel +def test_cache_still_honors_conditional_requests( + distribution_url, assert_full_response, redis_status +): + """A cached response revalidates to 304 but still serves the file to a client without a copy.""" + if not redis_status: + pytest.skip("Could not connect to the Redis server") + + url = urljoin(distribution_url, "1.iso") + + # Warm the cache with a normal download. + assert_full_response(requests.get(url, allow_redirects=False)) + + # A cached response can still answer a conditional request with a 304. + tomorrow = http_date(time.time() + 3600) + revalidated = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False) + assert_not_modified(revalidated) + assert revalidated.headers.get("X-PULP-CACHE") == "HIT" + + # A client without a copy still gets the full file from that same cache entry. + fresh = requests.get(url, allow_redirects=False) + assert_full_response(fresh) + assert fresh.headers.get("X-PULP-CACHE") == "HIT"