From 46bc167863babca4be337af997d8bc0ef617664a Mon Sep 17 00:00:00 2001 From: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:20:42 +0200 Subject: [PATCH] Edge caches can revalidate with If-Modified-Since after content guards run, without downloading the file again. Last-Modified is RepositoryContent.pulp_created for the served version, not disk mtime. Filesystem and ArtifactResponse get Cache-Control: public, max-age=0, must-revalidate; object-storage 302s do not. Redis can 304 from a cached last_modified without rebuilding the body. Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com> --- CHANGES/7929.feature | 1 + pulpcore/app/models/publication.py | 22 +- pulpcore/cache/cache.py | 113 +++++++-- pulpcore/content/handler.py | 213 +++++++++++----- pulpcore/responses.py | 40 ++- pulpcore/tests/unit/content/test_handler.py | 236 +++++++++++++++++- .../unit/models/test_publication_retention.py | 6 + pulpcore/tests/unit/test_cache.py | 192 ++++++++++++++ pulpcore/tests/unit/test_responses.py | 113 +++++++++ 9 files changed, 842 insertions(+), 94 deletions(-) create mode 100644 CHANGES/7929.feature create mode 100644 pulpcore/tests/unit/test_responses.py diff --git a/CHANGES/7929.feature b/CHANGES/7929.feature new file mode 100644 index 00000000000..b2098c9a935 --- /dev/null +++ b/CHANGES/7929.feature @@ -0,0 +1 @@ +Added `Last-Modified` / `If-Modified-Since` (`304 Not Modified`) and `Cache-Control: public, max-age=0, must-revalidate` on content-app artifact responses (filesystem and `ArtifactResponse`; not object-storage 302s) so edge caches can revalidate after ContentGuard without re-fetching the body. diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py index 7de105a50b2..e776bc6e233 100644 --- a/pulpcore/app/models/publication.py +++ b/pulpcore/app/models/publication.py @@ -755,14 +755,26 @@ def get_fallback_ca(self, path): """ Return a ContentArtifact for path from the grace-period publication history, or None. + See :meth:`get_fallback` for the publication that contained the unit. + """ + ca, _publication = self.get_fallback(path) + return ca + + def get_fallback(self, path): + """ + Return ``(ContentArtifact, Publication)`` from grace-period history, or ``(None, None)``. + Iterates DistributedPublication records for this distribution from newest to oldest, trying each publication until the path is found. Handles both pass-through and non-pass-through (PublishedArtifact) publications. - Returns None immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0. + Returns ``(None, None)`` immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0. + The publication is the one that still contains the unit, which may be a superseded + version — callers that need ``RepositoryContent.pulp_created`` must use that publication's + repository version, not the distribution's current one. """ if not retain_distributed_pub_enabled(): - return None + return None, None recent_dp = ( DistributedPublication.get_non_expired() .filter(distribution=self) @@ -778,7 +790,7 @@ def get_fallback_ca(self, path): .first() ) if ca is not None: - return ca + return ca, pub else: pa = ( pub.published_artifact.select_related( @@ -789,8 +801,8 @@ def get_fallback_ca(self, path): .first() ) if pa is not None: - return pa.content_artifact - return None + return pa.content_artifact, pub + return None, None @hook(AFTER_CREATE) @hook( diff --git a/pulpcore/cache/cache.py b/pulpcore/cache/cache.py index 6fcf470e14a..49f42fdf20d 100644 --- a/pulpcore/cache/cache.py +++ b/pulpcore/cache/cache.py @@ -4,10 +4,11 @@ from functools import wraps from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response, StreamResponse -from aiohttp.web_exceptions import HTTPFound +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 +from django.utils.http import parse_http_date_safe from redis import ConnectionError from redis.asyncio import ConnectionError as AConnectionError from rest_framework.request import Request as ApiRequest @@ -18,7 +19,7 @@ get_redis_connection, ) from pulpcore.metrics import artifacts_size_counter -from pulpcore.responses import ArtifactResponse +from pulpcore.responses import ArtifactResponse, PulpFileResponse DEFAULT_EXPIRES_TTL = settings.CACHE_SETTINGS["EXPIRES_TTL"] @@ -306,7 +307,7 @@ class AsyncContentCache(AsyncCache): """Cache object meant to be used for the content app""" RESPONSE_TYPES = { - "FileResponse": FileResponse, + "FileResponse": PulpFileResponse, "ArtifactResponse": ArtifactResponse, "Response": Response, "Redirect": HTTPFound, @@ -349,32 +350,93 @@ async def cached_function(*args, **kwargs): if self.auth: await self.auth(request, self, bk) key = self.make_key(request) + # Check cache - response = await self.make_response(key, bk) - if response is None: - # Cache miss, create new entry - response = await self.make_entry( - key, bk, func, args, kwargs, self.default_expires_ttl + entry = await self.get_entry(key, bk) + if entry is not None: + # Cache hit. Authorization has already run. If the client's If-Modified-Since + # covers the stored last_modified, answer a bodyless 304 without reconstructing + # the full response. Fall back to the header for entries cached before this field. + last_modified = entry.get("last_modified") or entry.get("headers", {}).get( + "Last-Modified" ) - elif size := response.headers.get("X-PULP-ARTIFACT-SIZE"): - artifacts_size_counter.add(size) - + if self._not_modified(request, last_modified): + headers = dict(entry.get("headers") or {}) + headers["X-PULP-CACHE"] = "HIT" + raise self._make_not_modified(headers, last_modified) + response = self.build_response(entry) + if size := response.headers.get("X-PULP-ARTIFACT-SIZE"): + artifacts_size_counter.add(size) + return response + + # Cache miss: build and cache the full response (a 304 is never stored). Still answer + # a matching conditional request with a 304 from the fresh response's Last-Modified, + # but never after a stream has already started writing. + response = await self.make_entry(key, bk, func, args, kwargs, self.default_expires_ttl) + if getattr(response, "prepared", False): + return response + last_modified = response.headers.get("Last-Modified") + if self._not_modified(request, last_modified): + raise self._make_not_modified(response.headers, last_modified) return response return cached_function + @staticmethod + def _not_modified(request, last_modified): + """True when the request's If-Modified-Since covers the given Last-Modified value. + + Ignore If-Modified-Since when If-None-Match is present, or when it is later than + the server clock. + """ + if not last_modified: + return False + if request.headers.get("If-None-Match"): + return False + if_modified_since = parse_http_date_safe(request.headers.get("If-Modified-Since", "")) + if if_modified_since is None or if_modified_since > time.time(): + return False + lm_epoch = parse_http_date_safe(last_modified) + return lm_epoch is not None and lm_epoch <= if_modified_since + + @staticmethod + def _make_not_modified(source_headers, last_modified): + """Build a bodyless 304 echoing Last-Modified and any caching metadata already present.""" + headers = {"Last-Modified": last_modified} + for name in ("Cache-Control", "X-PULP-CACHE"): + if value := source_headers.get(name): + headers[name] = value + return HTTPNotModified(headers=headers) + def get_request_from_args(self, args): """Finds the request object from list of args""" for arg in args: if isinstance(arg, Request): return arg - async def make_response(self, key, base_key): - """Tries to find the cached entry and turn it into a proper response""" + async def get_entry(self, key, base_key): + """Return the cached entry dict for ``key`` (deleting stale/invalid rows), or None.""" entry = await self.get(key, base_key) if not entry: return None entry = json.loads(entry) + response_type = entry.get("type") + # None means "doesn't expire", unset/absent means "already expired". + expires = entry.get("expires", -1) + if (not response_type or response_type not in self.RESPONSE_TYPES) or ( + expires and expires < time.time() + ): + # Bad entry, delete from cache + await self.delete(key, base_key) + return None + return entry + + def build_response(self, entry): + """Turn a cached entry dict into a proper response object (marked as a cache HIT).""" + entry = dict(entry) # do not mutate the caller's dict + entry.pop("expires", None) + entry.pop("last_modified", None) + response_type = entry.pop("type") if binary := entry.pop("body", None): # raw binary data were translated to their hexadecimal representation and saved in @@ -383,23 +445,24 @@ async def make_response(self, key, base_key): # https://docs.aiohttp.org/en/stable/web_reference.html#response entry["body"] = bytes.fromhex(binary) - response_type = entry.pop("type", None) - # None means "doesn't expire", unset means "already expired". - expires = entry.pop("expires", -1) - if (not response_type or response_type not in self.RESPONSE_TYPES) or ( - expires and expires < time.time() - ): - # Bad entry, delete from cache - await self.delete(key, base_key) - return None response = self.RESPONSE_TYPES[response_type](**entry) response.headers.update({"X-PULP-CACHE": "HIT"}) return response + async def make_response(self, key, base_key): + """Tries to find the cached entry and turn it into a proper response""" + entry = await self.get_entry(key, base_key) + if entry is None: + return None + return self.build_response(entry) + async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT_EXPIRES_TTL): """Gets the response for the request and try to turn it into a cacheable entry""" try: response = await handler(*args, **kwargs) + except HTTPNotModified: + # HTTPNotModified is HTTPSuccessful; do not swallow it into a cached entry. + raise except (HTTPSuccessful, HTTPFound) as e: response = e @@ -408,7 +471,13 @@ async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT if hasattr(response, "future_response"): response = response.future_response + if getattr(response, "status", None) == 304: + return original_response + entry = {"headers": dict(response.headers), "status": response.status} + if last_modified := response.headers.get("Last-Modified"): + # Stored alongside headers so a cache hit can 304 without reconstructing the response. + entry["last_modified"] = last_modified if expires is not None: # Redis TTL is not sufficient: https://github.com/pulp/pulpcore/issues/4845 entry["expires"] = expires + time.time() diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py index dcaef910600..1f1cd190587 100644 --- a/pulpcore/content/handler.py +++ b/pulpcore/content/handler.py @@ -10,7 +10,7 @@ import django from aiohttp.client_exceptions import ClientConnectionError, ClientResponseError -from aiohttp.web import FileResponse, HTTPOk, StreamResponse +from aiohttp.web import HTTPOk, StreamResponse from aiohttp.web_exceptions import ( HTTPError, HTTPForbidden, @@ -21,11 +21,12 @@ ) 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 from pulpcore.constants import CHECKPOINT_TS_FORMAT, STORAGE_RESPONSE_MAP -from pulpcore.responses import ArtifactResponse +from pulpcore.responses import ArtifactResponse, PulpFileResponse os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings") django.setup() @@ -524,6 +525,10 @@ def response_headers(path, distribution=None): if content_type: headers["Content-Type"] = content_type + # Tell edge caches to revalidate on every use. Combined with Last-Modified below this + # lets them confirm freshness with a lightweight If-Modified-Since instead of re-fetching. + headers["Cache-Control"] = "public, max-age=0, must-revalidate" + # Let plugin-Distribution set headers for this path if it wants. if distribution: headers.update(distribution.content_headers_for(path)) @@ -720,14 +725,16 @@ 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): - if content_handler_result.artifact: - return await self._serve_content_artifact( - content_handler_result, headers, request - ) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), content_handler_result - ) + ch_repository, ch_repo_version, ch_publication = await sync_to_async( + distro.get_repository_publication_and_version + )() + return await self._serve_ca( + content_handler_result, + headers, + request, + publication=ch_publication, + repository_version=ch_repo_version, + ) else: # the result is a response so just return it return content_handler_result @@ -784,12 +791,7 @@ async def _match_and_stream(self, path, request): except ObjectDoesNotExist: pass else: - if ca.artifact: - return await self._serve_content_artifact(ca, headers, request) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), ca - ) + return await self._serve_ca(ca, headers, request, publication=publication) # pass-through if publication.pass_through: @@ -813,23 +815,13 @@ async def _match_and_stream(self, path, request): except ObjectDoesNotExist: pass else: - if ca.artifact: - return await self._serve_content_artifact(ca, headers, request) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), ca - ) + return await self._serve_ca(ca, headers, request, publication=publication) # Grace-period fallback: serve from a recently-superseded publication if distro.SERVE_FROM_PUBLICATION: - ca = await sync_to_async(distro.get_fallback_ca)(original_rel_path) + ca, fallback_publication = await sync_to_async(distro.get_fallback)(original_rel_path) if ca is not None: - if ca.artifact: - return await self._serve_content_artifact(ca, headers, request) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), ca - ) + return await self._serve_ca(ca, headers, request, publication=fallback_publication) if repo_version and not publication and not distro.SERVE_FROM_PUBLICATION: # Look for index.html or list the directory @@ -871,12 +863,7 @@ async def _match_and_stream(self, path, request): except ObjectDoesNotExist: pass else: - if ca.artifact: - return await self._serve_content_artifact(ca, headers, request) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), ca - ) + return await self._serve_ca(ca, headers, request, repository_version=repo_version) # If we haven't found a match yet, try to use pull-through caching with remote if distro.remote: @@ -893,13 +880,10 @@ async def _match_and_stream(self, path, request): # Try to add content to repository if present & supported if repository and repository.PULL_THROUGH_SUPPORTED: await repository.async_pull_through_add_content(ca) - # Try to stream the ContentArtifact if already created - if ca.artifact: - return await self._serve_content_artifact(ca, headers, request) - else: - return await self._stream_content_artifact( - request, StreamResponse(headers=headers), ca - ) + # Serve the ContentArtifact if already created (streams if not yet saved) + return await self._serve_ca( + ca, headers, request, repository_version=repo_version + ) else: # Try to stream the RemoteArtifact and potentially save it as a new Content unit save_artifact = ( @@ -1090,6 +1074,81 @@ def _save_artifact(self, download_result, remote_artifact, request=None): ret.update({ca.relative_path: ca for ca in cas}) return ret + async def _content_last_modified( + self, content_artifact, *, repository_version=None, publication=None + ): + """ + Return when the content unit was added to the repository being served, or None. + + Uses ``RepositoryContent.pulp_created`` (the time the unit joined the served repository + version), which is the value the content app exposes as ``Last-Modified``. Returns None + when no repository version is available or the unit has no membership row (e.g. publish- + generated metadata), in which case no ``Last-Modified`` header is set. + """ + + def _get(): + repo_version = repository_version + if repo_version is None and publication is not None: + repo_version = publication.repository_version + if repo_version is None: + return None + return ( + repo_version._content_relationships() + .filter(content_id=content_artifact.content_id) + .order_by("-pulp_created") + .values_list("pulp_created", flat=True) + .first() + ) + + return await sync_to_async(_get)() + + @staticmethod + def _last_modified_http_date(last_modified): + """Format a datetime as an HTTP ``Last-Modified`` value, or None.""" + if last_modified is None: + return None + return http_date(last_modified.timestamp()) + + @staticmethod + def _strip_cache_control(headers): + """Drop Cache-Control so a response cannot be stored as a shared public copy.""" + headers.pop("Cache-Control", None) + return headers + + @staticmethod + def _maybe_not_modified(request, headers, last_modified_header, *, raise_304=True): + """Return True when If-Modified-Since covers Last-Modified; optionally raise 304.""" + if not AsyncContentCache._not_modified(request, last_modified_header): + return False + if raise_304: + raise AsyncContentCache._make_not_modified(headers, last_modified_header) + return True + + async def _serve_ca(self, ca, headers, request, *, publication=None, repository_version=None): + """Serve a ContentArtifact, attaching ``Last-Modified`` from pulp_created. + + Looks up when the unit joined the served repository version. Saved artifacts get that + timestamp in ``_serve_content_artifact`` (after the redirect check, so object-storage + 302s stay unmodified). On-demand units without a local artifact 304 before the remote + fetch when If-Modified-Since covers that timestamp. + """ + last_modified = await self._content_last_modified( + ca, publication=publication, repository_version=repository_version + ) + if ca.artifact: + # Last-Modified is applied in `_serve_content_artifact` after the redirect check so + # object-storage 302s do not advertise a Pulp validator they cannot honor. + return await self._serve_content_artifact( + ca, headers, request, last_modified=last_modified + ) + last_modified_header = self._last_modified_http_date(last_modified) + if last_modified_header: + headers["Last-Modified"] = last_modified_header + # 304 before opening the remote, including when the cache is on: streams are not + # stored as cacheable file responses, and a started StreamResponse cannot become 304. + self._maybe_not_modified(request, headers, last_modified_header) + return await self._stream_content_artifact(request, StreamResponse(headers=headers), ca) + def _build_response_from_content_artifact(self, content_artifact, headers, request): """Helper method to build the correct response to serve a ContentArtifact.""" @@ -1118,27 +1177,33 @@ def _build_url(**kwargs): storage = domain.get_storage() headers["X-PULP-ARTIFACT-SIZE"] = str(artifact_file.size) + def _object_storage_redirect(url): + # Presigned Locations must not be stored by shared caches. + return HTTPFound(url, headers=self._strip_cache_control(CIMultiDict(headers))) + if domain.storage_class == "pulpcore.app.models.storage.FileSystem": path = storage.path(artifact_name) if not os.path.exists(path): raise Exception(_("Expected path '{}' is not found").format(path)) - return FileResponse(path, headers=headers) + return PulpFileResponse(path, headers=headers) elif not domain.redirect_to_object_storage: return ArtifactResponse(content_artifact.artifact, headers=headers) elif domain.storage_class in ( "storages.backends.s3boto3.S3Boto3Storage", "storages.backends.s3.S3Storage", ): - return HTTPFound(_build_url(http_method=request.method), headers=headers) + return _object_storage_redirect(_build_url(http_method=request.method)) elif domain.storage_class in ( "storages.backends.azure_storage.AzureStorage", "storages.backends.gcloud.GoogleCloudStorage", ): - return HTTPFound(_build_url(), headers=headers) + return _object_storage_redirect(_build_url()) else: raise NotImplementedError() - async def _serve_content_artifact(self, content_artifact, headers, request): + async def _serve_content_artifact( + self, content_artifact, headers, request, *, last_modified=None + ): """ Handle response for a Content Artifact with the file present. @@ -1150,6 +1215,8 @@ async def _serve_content_artifact(self, content_artifact, headers, request): respond with. headers (dict): A dictionary of response headers. request(aiohttp.web.Request) The request to prepare a response for. + last_modified (datetime): When the content was added to the served repository, used + for the ``Last-Modified`` header and ``If-Modified-Since`` handling. May be None. Raises: [aiohttp.web_exceptions.HTTPFound][]: When we need to redirect to the file @@ -1161,26 +1228,44 @@ async def _serve_content_artifact(self, content_artifact, headers, request): """ artifact_file = content_artifact.artifact.file content_length = artifact_file.size - - try: - range_start, range_stop = request.http_range.start, request.http_range.stop - if range_start or range_stop: - if range_stop and artifact_file.size and range_stop > artifact_file.size: - start = 0 if range_start is None else range_start - content_length = artifact_file.size - start - elif range_stop: - content_length = range_stop - range_start - except ValueError: - size = artifact_file.size or "*" - raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"}) - - artifacts_size_counter.add(content_length) + last_modified_header = self._last_modified_http_date(last_modified) response = self._build_response_from_content_artifact(content_artifact, headers, request) if isinstance(response, HTTPFound): + # Redirect (object-storage) responses are left without a Pulp validator. Presigned + # Locations must not be stored by shared caches. + self._strip_cache_control(response.headers) + artifacts_size_counter.add(content_length) raise response - else: - return response + + if last_modified_header is not None: + response.headers["Last-Modified"] = last_modified_header + + # If-Modified-Since is checked as if Range were not present. A matching + # If-Modified-Since must 304, not 416. When the cache is on, skip the handler 304 so + # Redis can store a 200. + would_304 = self._maybe_not_modified( + request, + response.headers, + response.headers.get("Last-Modified"), + raise_304=not settings.CACHE_ENABLED, + ) + + if not would_304: + try: + range_start, range_stop = request.http_range.start, request.http_range.stop + if range_start or range_stop: + if range_stop and artifact_file.size and range_stop > artifact_file.size: + start = 0 if range_start is None else range_start + content_length = artifact_file.size - start + elif range_stop: + content_length = range_stop - range_start + except ValueError: + size = artifact_file.size or "*" + raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"}) + + artifacts_size_counter.add(content_length) + return response async def _stream_remote_artifact( self, request, response, remote_artifact, save_artifact, repository=None @@ -1212,8 +1297,8 @@ async def _stream_remote_artifact( ) ) - # According to RFC7233 if a server cannot satisfy a Range request, the response needs to - # contain a Content-Range header with an unsatisfied-range value. + # If a Range cannot be satisfied, the response needs a Content-Range header with an + # unsatisfied-range value. try: range_start, range_stop = request.http_range.start, request.http_range.stop size = remote_artifact.size diff --git a/pulpcore/responses.py b/pulpcore/responses.py index 1b1fac62a0d..5e020c6fe02 100644 --- a/pulpcore/responses.py +++ b/pulpcore/responses.py @@ -1,7 +1,7 @@ import asyncio from aiohttp import hdrs -from aiohttp.web import StreamResponse +from aiohttp.web import FileResponse, StreamResponse from aiohttp.web_exceptions import ( HTTPPartialContent, HTTPRequestRangeNotSatisfiable, @@ -9,6 +9,44 @@ from pulpcore.app.models import Artifact +# aiohttp ``@reify`` keys on ``request._cache`` (private, stable through aiohttp 3.10–3.14). +# Only the headers that would 304 against file mtime are suppressed; If-Range / If-Match / +# If-Unmodified-Since stay intact so Range requests cannot return a corrupt 206. +_MTIME_304_REIFY_KEYS = ("if_modified_since", "if_none_match") + + +class PulpFileResponse(FileResponse): + """A FileResponse that lets the content app own the ``Last-Modified`` validator. + + aiohttp's ``FileResponse`` overwrites ``Last-Modified`` with the file's mtime and runs its own + ``If-Modified-Since``/``ETag`` handling against that mtime. The content app instead uses + ``RepositoryContent.pulp_created`` (or omits the header) and answers conditional requests + itself, so this class never advertises filesystem mtime as a validator. + """ + + def _make_response(self, request, accept_encoding): + for key in _MTIME_304_REIFY_KEYS: + request._cache[key] = None + return super()._make_response(request, accept_encoding) + + @property + def last_modified(self): + return FileResponse.last_modified.fget(self) + + @last_modified.setter + def last_modified(self, value): + # Never replace a handler Last-Modified, and never advertise the file mtime. + return + + @property + def etag(self): + return FileResponse.etag.fget(self) + + @etag.setter + def etag(self, value): + # mtime-based ETags would disagree with RepositoryContent.pulp_created as Last-Modified. + return + class ArtifactResponse(StreamResponse): """A response object can be used to send artifacts.""" diff --git a/pulpcore/tests/unit/content/test_handler.py b/pulpcore/tests/unit/content/test_handler.py index 045efdad6cd..bf17b912743 100644 --- a/pulpcore/tests/unit/content/test_handler.py +++ b/pulpcore/tests/unit/content/test_handler.py @@ -1,11 +1,19 @@ import uuid -from datetime import timedelta +from datetime import datetime, timedelta +from datetime import timezone as dt_timezone from unittest.mock import AsyncMock, Mock import pytest import pytest_asyncio -from aiohttp.web_exceptions import HTTPMovedPermanently +from aiohttp.web_exceptions import ( + HTTPFound, + HTTPMovedPermanently, + HTTPNotModified, +) +from asgiref.sync import sync_to_async from django.db import IntegrityError +from django.test import override_settings +from django.utils.http import http_date from django_guid import clear_guid, set_guid from pulpcore.app.models import AppStatus @@ -198,6 +206,18 @@ async def create_distribution(remote, repository=None): ) +async def _add_content_to_new_version(repo, content): + """Add ``content`` to a new complete version of ``repo`` and return that version.""" + repo.CONTENT_TYPES = [Content] + + def _add(): + with repo.new_version() as version: + version.add_content(Content.objects.filter(pk=content.pk)) + return repo.latest_version() + + return await sync_to_async(_add)() + + @pytest.mark.asyncio @pytest.mark.django_db async def test_pull_through_remote_artifact_exists(request123, tmp_path): @@ -586,6 +606,218 @@ def test_render_html_normal_name(): assert 'simple-dir/' in html +_LAST_MODIFIED = datetime(2020, 1, 1, tzinfo=dt_timezone.utc) +_LAST_MODIFIED_HTTP = http_date(_LAST_MODIFIED.timestamp()) +_IF_MODIFIED_SINCE_AFTER = http_date(datetime(2021, 1, 1, tzinfo=dt_timezone.utc).timestamp()) +_IF_MODIFIED_SINCE_BEFORE = http_date(datetime(2019, 1, 1, tzinfo=dt_timezone.utc).timestamp()) +_CACHE_CONTROL = "public, max-age=0, must-revalidate" + + +class _UnsatisfiableRange: + @property + def start(self): + raise ValueError() + + stop = None + + +def _request(*, if_modified_since=None, http_range=None): + return Mock( + method="GET", + http_range=http_range if http_range is not None else Mock(start=None, stop=None), + headers={"If-Modified-Since": if_modified_since} if if_modified_since else {}, + ) + + +def _ca(*, artifact=True): + ca = Mock() + ca.relative_path = "file.iso" + if artifact: + ca.artifact.file.size = 7 + ca.artifact.file.name = "artifacts/obj" + else: + ca.artifact = None + return ca + + +def _handler_with_built_response(monkeypatch, built=None): + handler = Handler() + ca = _ca() + if built is None: + built = Mock(headers={"Cache-Control": _CACHE_CONTROL}, status=200) + monkeypatch.setattr(handler, "_build_response_from_content_artifact", Mock(return_value=built)) + return handler, ca, built + + +def _membership_pulp_created(version, content): + return ( + version._content_relationships() + .filter(content_id=content.pk) + .values_list("pulp_created", flat=True) + .get() + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "if_modified_since, last_modified, cache_enabled, expect_304", + [ + (None, _LAST_MODIFIED, False, False), + (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, False, True), + (_IF_MODIFIED_SINCE_BEFORE, _LAST_MODIFIED, False, False), + (_IF_MODIFIED_SINCE_AFTER, None, False, False), + (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, True, False), + ], + ids=["no-if-modified-since", "fresh", "stale", "no-timestamp", "cache-on"], +) +async def test_serve_content_artifact_if_modified_since( + monkeypatch, if_modified_since, last_modified, cache_enabled, expect_304 +): + """Filesystem responses stamp Last-Modified. + + A matching If-Modified-Since is 304 unless the cache is on. + """ + handler, ca, built = _handler_with_built_response(monkeypatch) + + with override_settings(CACHE_ENABLED=cache_enabled): + if expect_304: + with pytest.raises(HTTPNotModified) as exc: + await handler._serve_content_artifact( + ca, + {}, + _request(if_modified_since=if_modified_since), + last_modified=last_modified, + ) + assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP + assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL + else: + response = await handler._serve_content_artifact( + ca, + {}, + _request(if_modified_since=if_modified_since), + last_modified=last_modified, + ) + assert response is built + if last_modified is None: + assert "Last-Modified" not in response.headers + else: + assert response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP + + +def test_response_headers_sets_cache_control(): + """All content responses instruct edge caches to revalidate on every use.""" + headers = Handler.response_headers("path/to/file.iso") + assert headers["Cache-Control"] == _CACHE_CONTROL + + +@pytest.mark.asyncio +async def test_serve_content_artifact_redirect_is_not_304(monkeypatch): + """Object-storage 302s never get a Pulp Last-Modified. + + A matching If-Modified-Since must not 304. + """ + redirect = HTTPFound( + "http://example.test/redirect", + headers={"Cache-Control": _CACHE_CONTROL}, + ) + handler, ca, _built = _handler_with_built_response(monkeypatch, built=redirect) + + with override_settings(CACHE_ENABLED=False): + with pytest.raises(HTTPFound) as exc: + await handler._serve_content_artifact( + ca, + {}, + _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER), + last_modified=_LAST_MODIFIED, + ) + + assert "Last-Modified" not in exc.value.headers + assert "Cache-Control" not in exc.value.headers + + +@pytest.mark.asyncio +async def test_serve_content_artifact_304_beats_unsatisfiable_range(monkeypatch): + """A matching If-Modified-Since 304s even when Range would otherwise be 416.""" + handler, ca, _built = _handler_with_built_response(monkeypatch) + request = _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER, http_range=_UnsatisfiableRange()) + + with override_settings(CACHE_ENABLED=False): + with pytest.raises(HTTPNotModified) as exc: + await handler._serve_content_artifact(ca, {}, request, last_modified=_LAST_MODIFIED) + + assert exc.value.status == 304 + assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP + + +@pytest.mark.asyncio +async def test_on_demand_conditional_before_stream(monkeypatch): + """On-demand units 304 before the remote fetch; otherwise the stream carries Last-Modified.""" + handler = Handler() + ca = _ca(artifact=False) + monkeypatch.setattr(handler, "_content_last_modified", AsyncMock(return_value=_LAST_MODIFIED)) + handler._stream_content_artifact = AsyncMock(return_value="streamed") + + with pytest.raises(HTTPNotModified) as exc: + await handler._serve_ca( + ca, + {"Cache-Control": _CACHE_CONTROL}, + Mock(headers={"If-Modified-Since": _LAST_MODIFIED_HTTP}), + repository_version="rv", + ) + handler._stream_content_artifact.assert_not_awaited() + assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP + assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL + + result = await handler._serve_ca(ca, {}, Mock(headers={}), repository_version="rv") + assert result == "streamed" + _, stream_response, stream_ca = handler._stream_content_artifact.call_args.args + assert stream_ca is ca + assert stream_response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP + + +@pytest.mark.asyncio +@pytest.mark.django_db +async def test_content_last_modified_from_repository_membership(): + """Last-Modified is RepositoryContent.pulp_created for the served version, else omitted.""" + repo = await create_repository() + content = await create_content() + other = await create_content() + publication = None + try: + ca = await create_content_artifact(content) + handler = Handler() + assert await handler._content_last_modified(ca) is None + + v1 = await _add_content_to_new_version(repo, content) + expected = await sync_to_async(_membership_pulp_created)(v1, content) + assert await handler._content_last_modified(ca, repository_version=v1) == expected + + publication = await sync_to_async(Publication.objects.create)(repository_version=v1) + assert await handler._content_last_modified(ca, publication=publication) == expected + + def _add_other(): + with repo.new_version() as version: + version.add_content(Content.objects.filter(pk=other.pk)) + return repo.latest_version() + + v2 = await sync_to_async(_add_other)() + assert await handler._content_last_modified(ca, repository_version=v2) == expected + + def _remove(): + with repo.new_version() as version: + version.remove_content(Content.objects.filter(pk=content.pk)) + return repo.latest_version() + + v3 = await sync_to_async(_remove)() + assert await handler._content_last_modified(ca, repository_version=v3) is None + finally: + if publication is not None: + await publication.adelete() + await repo.adelete() + await content.adelete() + await other.adelete() + + @pytest.mark.asyncio @pytest.mark.django_db async def test_async_pull_through_add(ca1, monkeypatch, app_status): diff --git a/pulpcore/tests/unit/models/test_publication_retention.py b/pulpcore/tests/unit/models/test_publication_retention.py index f16c9b1e036..75645492cce 100644 --- a/pulpcore/tests/unit/models/test_publication_retention.py +++ b/pulpcore/tests/unit/models/test_publication_retention.py @@ -144,6 +144,9 @@ def test_returns_ca_when_content_in_publication(self, version_with_content, expe pub_with_a = pub_factory(version_with_content, pass_through=True) dist = dist_factory(pub=pub_with_a) assert dist.get_fallback_ca(self.content_path) == expected_ca + ca, publication = dist.get_fallback(self.content_path) + assert ca == expected_ca + assert publication.pk == pub_with_a.pk def test_returns_none_when_content_not_in_publication(self, version_without_content): pub_without_a = pub_factory(version_without_content, pass_through=True) @@ -170,6 +173,9 @@ def test_returns_ca_when_content_only_in_superseded_publication( pub_without_a = pub_factory(version_without_content, pass_through=True) update_dist(dist, pub=pub_without_a) assert dist.get_fallback_ca(self.content_path) == expected_ca + ca, publication = dist.get_fallback(self.content_path) + assert ca == expected_ca + assert publication.pk == pub_with_a.pk def test_returns_none_when_repository_unset(self, version_with_content, expected_ca): repo = version_with_content.repository diff --git a/pulpcore/tests/unit/test_cache.py b/pulpcore/tests/unit/test_cache.py index 6da69732e07..3cb01f9110f 100644 --- a/pulpcore/tests/unit/test_cache.py +++ b/pulpcore/tests/unit/test_cache.py @@ -1,9 +1,17 @@ +import json from time import sleep +from time import time as now +from unittest.mock import AsyncMock, Mock import pytest +from aiohttp.web import Response +from aiohttp.web_exceptions import HTTPNotModified +from django.test import override_settings +from django.utils.http import http_date import pulpcore.app.redis_connection from pulpcore.cache import Cache +from pulpcore.cache.cache import AsyncContentCache @pytest.fixture @@ -107,3 +115,187 @@ def test_clear(pulp_redisdb): cache.redis.flushdb() for key, _, base_key in tuples: assert not cache.exists(key, base_key=base_key) + + +def _request_with_if_modified_since(value): + return Mock(headers={"If-Modified-Since": value} if value else {}) + + +_LM = http_date(1_000_000_000) + + +def test_async_content_cache_not_modified(): + """If-Modified-Since is compared to Last-Modified at second resolution.""" + newer = http_date(1_000_000_060) + older = http_date(999_999_940) + future = http_date(now() + 86400) + inm = Mock(headers={"If-Modified-Since": _LM, "If-None-Match": '"abc"'}) + + assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), _LM) is True + assert AsyncContentCache._not_modified(_request_with_if_modified_since(newer), _LM) is True + assert AsyncContentCache._not_modified(_request_with_if_modified_since(older), _LM) is False + assert AsyncContentCache._not_modified(_request_with_if_modified_since(None), _LM) is False + assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), None) is False + assert AsyncContentCache._not_modified(_request_with_if_modified_since("garbage"), _LM) is False + assert AsyncContentCache._not_modified(inm, _LM) is False + assert AsyncContentCache._not_modified(_request_with_if_modified_since(future), _LM) is False + + +def test_async_content_cache_make_not_modified_echoes_metadata(): + """The 304 carries only validator/caching metadata already present on the source.""" + source = { + "Cache-Control": "public, max-age=0, must-revalidate", + "Content-Length": "1024", + "X-PULP-CACHE": "HIT", + } + + exc = AsyncContentCache._make_not_modified(source, _LM) + + assert isinstance(exc, HTTPNotModified) + assert exc.headers["Last-Modified"] == _LM + assert exc.headers["Cache-Control"] == "public, max-age=0, must-revalidate" + assert exc.headers["X-PULP-CACHE"] == "HIT" + assert "Content-Length" not in exc.headers + + bare = AsyncContentCache._make_not_modified({}, _LM) + assert "X-PULP-CACHE" not in bare.headers + assert "Cache-Control" not in bare.headers + + +def test_async_content_cache_build_response_pops_last_modified(): + """build_response must not pass the stored last_modified field to the response constructor.""" + cache = AsyncContentCache.__new__(AsyncContentCache) + entry = { + "type": "Response", + "status": 200, + "headers": {"Last-Modified": _LM}, + "last_modified": _LM, + "body": b"hello".hex(), + } + + response = cache.build_response(entry) + + assert response.status == 200 + assert response.body == b"hello" + assert response.headers["Last-Modified"] == _LM + assert response.headers["X-PULP-CACHE"] == "HIT" + + +def _entry(*, store_field=True): + entry = { + "type": "Response", + "status": 200, + "headers": { + "Last-Modified": _LM, + "Cache-Control": "public, max-age=0, must-revalidate", + }, + "body": b"payload".hex(), + "expires": None, + } + if store_field: + entry["last_modified"] = _LM + return entry + + +def _cache(): + cache = AsyncContentCache.__new__(AsyncContentCache) + cache.auth = None + cache.default_base_key = "base" + cache.keys = () + cache.default_expires_ttl = 60 + cache.get_request_from_args = lambda args: args[0] + cache.make_key = lambda req: "key" + return cache + + +async def _run_cached(cache, request, handler=None): + if handler is None: + + async def handler(req): + raise AssertionError("handler must not run") + + with override_settings(CACHE_ENABLED=True): + return await AsyncContentCache.__call__(cache, handler)(request) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("store_field", [True, False], ids=["stored-field", "header-fallback"]) +async def test_cache_hit_304_does_not_rebuild_response(store_field): + """A matching If-Modified-Since 304s without reconstructing the cached response.""" + cache = _cache() + cache.get_entry = AsyncMock(return_value=_entry(store_field=store_field)) + cache.build_response = Mock(side_effect=AssertionError("must not reconstruct")) + + with pytest.raises(HTTPNotModified) as exc: + await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM})) + + cache.build_response.assert_not_called() + assert exc.value.headers["Last-Modified"] == _LM + assert exc.value.headers["X-PULP-CACHE"] == "HIT" + + +@pytest.mark.asyncio +async def test_cache_hit_stale_if_modified_since_rebuilds_response(): + """An older If-Modified-Since on a cache hit still reconstructs the full cached response.""" + entry = _entry() + rebuilt = Mock(headers={"X-PULP-ARTIFACT-SIZE": None}) + cache = _cache() + cache.get_entry = AsyncMock(return_value=entry) + cache.build_response = Mock(return_value=rebuilt) + + response = await _run_cached(cache, Mock(headers={"If-Modified-Since": http_date(999_999_000)})) + + cache.build_response.assert_called_once_with(entry) + assert response is rebuilt + + +@pytest.mark.asyncio +async def test_cache_miss_does_not_304_prepared_stream(): + """A live stream that already started writing must not be converted into a 304.""" + stream = Mock(headers={"Last-Modified": _LM}, prepared=True, status=200) + cache = _cache() + cache.get_entry = AsyncMock(return_value=None) + cache.make_entry = AsyncMock(return_value=stream) + + async def handler(req): + raise AssertionError("handler is invoked via make_entry") + + assert await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM}), handler) is stream + + +@pytest.mark.asyncio +async def test_make_entry_does_not_cache_304(): + """HTTPNotModified is HTTPSuccessful but must never be written to Redis.""" + cache = _cache() + cache.set = AsyncMock() + + async def handler(): + raise HTTPNotModified(headers={"Last-Modified": _LM}) + + with pytest.raises(HTTPNotModified): + await cache.make_entry("k", "b", handler, (), {}, 60) + + cache.set.assert_not_called() + + +@pytest.mark.asyncio +async def test_make_entry_stores_last_modified(): + """A 200 with Last-Modified is stored so later cache hits can 304 without rebuilding.""" + captured = {} + cache = _cache() + + async def fake_set(key, value, expires=None, base_key=None): + captured["entry"] = json.loads(value) + + cache.set = fake_set + + async def handler(): + return Response(body=b"hello", headers={"Last-Modified": _LM}) + + result = await cache.make_entry("k", "b", handler, (), {}, 60) + + assert result.headers["Last-Modified"] == _LM + assert result.headers["X-PULP-CACHE"] == "MISS" + assert captured["entry"]["last_modified"] == _LM + assert captured["entry"]["headers"]["Last-Modified"] == _LM + assert captured["entry"]["type"] == "Response" diff --git a/pulpcore/tests/unit/test_responses.py b/pulpcore/tests/unit/test_responses.py new file mode 100644 index 00000000000..2c4d74ecf6e --- /dev/null +++ b/pulpcore/tests/unit/test_responses.py @@ -0,0 +1,113 @@ +import os +from datetime import datetime, timezone + +import pytest +from aiohttp.test_utils import make_mocked_request +from aiohttp.web import FileResponse +from django.utils.http import http_date + +from pulpcore.responses import PulpFileResponse + +# _make_response / _FileResponseResult exist only on aiohttp 3.11+. Lowerbounds installs 3.10. +_SKIP_MAKE_RESPONSE = pytest.mark.skipif( + not hasattr(FileResponse, "_make_response"), + reason="aiohttp FileResponse._make_response requires aiohttp 3.11+", +) + +_PULP_LM = http_date(datetime(2024, 6, 1, tzinfo=timezone.utc).timestamp()) +# 2001-09-09; If-Modified-Since between this and _PULP_LM is the interesting case +_FILE_MTIME = 1_000_000_000 +_IF_MODIFIED_SINCE_AFTER_MTIME = http_date(datetime(2022, 1, 1, tzinfo=timezone.utc).timestamp()) + + +def _artifact(tmp_path): + path = tmp_path / "artifact" + path.write_bytes(b"payload") + os.utime(path, (_FILE_MTIME, _FILE_MTIME)) + return path + + +@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"]) +def test_pulp_file_response_ignores_file_mtime(tmp_path, with_handler_lm): + """aiohttp's file-mtime assignment must not advertise a filesystem Last-Modified.""" + headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None + response = PulpFileResponse(_artifact(tmp_path), headers=headers) + response.last_modified = 2_000_000_000 + if with_handler_lm: + assert response.headers["Last-Modified"] == _PULP_LM + else: + assert "Last-Modified" not in response.headers + + +@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"]) +def test_pulp_file_response_never_emits_mtime_etag(tmp_path, with_handler_lm): + """mtime ETags are not advertised, with or without a Pulp Last-Modified.""" + headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None + response = PulpFileResponse(_artifact(tmp_path), headers=headers) + response.etag = "abc123" + assert "ETag" not in response.headers + + +@_SKIP_MAKE_RESPONSE +@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"]) +def test_pulp_file_response_does_not_304_on_file_mtime(tmp_path, with_handler_lm): + """If-Modified-Since after file mtime must not 304; stock FileResponse would.""" + from aiohttp.web_fileresponse import _FileResponseResult + + path = _artifact(tmp_path) + headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None + request = make_mocked_request( + "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME} + ) + + pulp = PulpFileResponse(str(path), headers=headers) + result, fobj, _st, _enc = pulp._make_response(request, "") + try: + assert result is _FileResponseResult.SEND_FILE + finally: + if fobj: + fobj.close() + if with_handler_lm: + assert pulp.headers["Last-Modified"] == _PULP_LM + else: + assert "Last-Modified" not in pulp.headers + + stock = FileResponse(str(path)) + result, fobj, _st, _enc = stock._make_response( + make_mocked_request( + "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME} + ), + "", + ) + try: + assert result is _FileResponseResult.NOT_MODIFIED + finally: + if fobj: + fobj.close() + + +@_SKIP_MAKE_RESPONSE +def test_pulp_file_response_does_not_blank_if_range(tmp_path): + """If-Range stays available so aiohttp can refuse a stale Range instead of a corrupt 206.""" + from aiohttp.web_fileresponse import _FileResponseResult + + if_range = http_date(_FILE_MTIME) + request = make_mocked_request( + "GET", + "/", + headers={ + "If-Range": if_range, + "Range": "bytes=0-1", + "If-Modified-Since": if_range, + }, + ) + response = PulpFileResponse(str(_artifact(tmp_path)), headers={"Last-Modified": _PULP_LM}) + result, fobj, _st, _enc = response._make_response(request, "") + try: + assert result is _FileResponseResult.SEND_FILE + finally: + if fobj: + fobj.close() + + assert request.if_range is not None + assert request.if_modified_since is None