diff --git a/docker/include/proxy_cache.conf b/docker/include/proxy_cache.conf index 6abf8ed..8c75c0e 100644 --- a/docker/include/proxy_cache.conf +++ b/docker/include/proxy_cache.conf @@ -30,11 +30,25 @@ proxy_cache_revalidate on; # Ranged responses must still stay out of the cache: proxy_cache_key has no # Range in it, so caching a 206 would serve those bytes for every other range # of the same object. Unranged GETs (zarr.json, bucket listings) still cache. -proxy_cache_bypass $http_range; +# $cache_bypass (Cache-Control: no-cache) is repeated from proxy_cache_maps.conf +# rather than left at http level: a location-level proxy_cache_bypass replaces +# the http-level one instead of adding to it, so naming only $http_range here +# would silently drop the no-cache policy in every location that includes this +# file. +proxy_cache_bypass $http_range $cache_bypass; proxy_no_cache $http_range; proxy_set_header Range $http_range; proxy_set_header If-Range $http_if_range; +# Same story for the other conditional headers: proxy_cache makes nginx replace +# them with its own revalidation values, so on a cache-bypassed ranged request +# the client's validators never reach x2s3 and it cannot answer the 304 that +# RFC 9110 13.2.2 requires. These variables restore them for ranged requests +# only and leave nginx's own revalidation alone otherwise -- see +# proxy_cache_maps.conf, which defines them and explains why. +proxy_set_header If-None-Match $proxy_if_none_match; +proxy_set_header If-Modified-Since $proxy_if_modified_since; + # Set back a nice HTTP Header to indicate what the cache status was add_header X-Proxy-Cache $upstream_cache_status; diff --git a/docker/include/proxy_cache_maps.conf b/docker/include/proxy_cache_maps.conf new file mode 100644 index 0000000..da917ee --- /dev/null +++ b/docker/include/proxy_cache_maps.conf @@ -0,0 +1,39 @@ +# Cache-related maps, split out because map is http-context only. Included from +# nginx.conf; consumed by proxy_cache.conf. + +# A client asking for a fresh copy with Cache-Control: no-cache should not be +# handed nginx's stored one. Only proxy_cache_bypass is set from this, not +# proxy_no_cache: no-cache means "revalidate before serving", so the response +# should still refresh the cache entry. +map $http_cache_control $cache_bypass { + no-cache 1; +} + +# The remaining maps restore conditional-request headers on ranged requests. +# +# Configuring proxy_cache makes nginx replace the client's If-None-Match and +# If-Modified-Since with its own revalidation values ($upstream_cache_etag and +# $upstream_cache_last_modified). Like the Range stripping described in +# proxy_cache.conf, that is decided at config time, so it applies even to the +# ranged requests proxy_cache_bypass sends straight through -- there the +# client's validators are dropped and x2s3 never sees them, so it cannot answer +# the 304 that RFC 9110 13.2.2 requires when Range and If-None-Match arrive +# together. +# +# Restoring them for ranged requests only is deliberate. On an unranged request +# nginx owns these headers: with proxy_cache_revalidate on, a stale entry is +# revalidated using the ETag nginx itself cached. Forwarding the client's +# validator there would let it answer nginx's question, so a 304 meant for the +# client would mark a stale entry fresh and keep serving its old bytes for +# another proxy_cache_valid period. +# +# The '' branches reproduce nginx's built-in defaults exactly. +map $http_range $proxy_if_none_match { + '' $upstream_cache_etag; + default $http_if_none_match; +} + +map $http_range $proxy_if_modified_since { + '' $upstream_cache_last_modified; + default $http_if_modified_since; +} diff --git a/docker/include/proxy_pass.conf b/docker/include/proxy_pass.conf index 08bdc79..ce3f08e 100644 --- a/docker/include/proxy_pass.conf +++ b/docker/include/proxy_pass.conf @@ -13,9 +13,11 @@ proxy_ignore_headers X-Accel-Expires; proxy_ignore_headers Cache-Control; proxy_ignore_headers Set-Cookie; +# Cache-Control is intentionally NOT hidden here: x2s3 emits it so browsers +# and shared caches can cache responses. proxy_ignore_headers Cache-Control +# above still keeps nginx's own proxy_cache_valid in force regardless. proxy_hide_header Expires; proxy_hide_header X-Accel-Expires; -proxy_hide_header Cache-Control; proxy_hide_header Pragma; # Replace CORS headers diff --git a/docker/nginx.conf b/docker/nginx.conf index 63342c8..b26001d 100644 --- a/docker/nginx.conf +++ b/docker/nginx.conf @@ -11,10 +11,11 @@ events { http { proxy_cache_path /var/cache/nginx keys_zone=mycache:512m max_size=100g levels=1:2 inactive=24h loader_sleep=10ms manager_files=4000 manager_threshold=200m manager_sleep=100ms; - map $http_cache_control $cache_bypass { - no-cache 1; - } - proxy_cache_bypass $cache_bypass; + # map is http-context only, so the maps consumed by proxy_cache.conf have + # to be included out here. The matching proxy_cache_bypass lives in + # proxy_cache.conf: a location-level one replaces an http-level one rather + # than adding to it, so both policies have to be named together there. + include /etc/nginx/conf/proxy_cache_maps.conf; # Increase in-memory buffers (this assumes we have a lot of RAM) proxy_buffer_size 64k; diff --git a/docs/Config.md b/docs/Config.md index e0db539..7cdd671 100644 --- a/docs/Config.md +++ b/docs/Config.md @@ -19,7 +19,6 @@ client_options: max_pool_connections: 50 file: buffer_size: 65536 # 64 KB chunks for streaming - calculate_etags: false ``` ## Targets @@ -41,7 +40,6 @@ Each target may have the following properties: * *file*: Local filesystem targets. Options: * `path`: Path to the root * `buffer_size`: Size of chunks (in bytes) when streaming file content (default: 8192) - * `calculate_etags`: If true, then the etags will be calculated by hashing the content of each file. This is much more expensive and may not be needed for all use cases. ### Botocore Config Options diff --git a/pyproject.toml b/pyproject.toml index 27756c0..b0378aa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "x2s3" -version = "1.4.3" +version = "1.4.4" description = "RESTful web service which makes any storage system X available as an S3-compatible REST API" readme = "README.md" license = { file = "LICENSE" } diff --git a/tests/test_awss3.py b/tests/test_awss3.py index 888212f..ba217e0 100644 --- a/tests/test_awss3.py +++ b/tests/test_awss3.py @@ -160,6 +160,7 @@ def test_head_object(app): with TestClient(app) as client: response = client.head("/janelia-data-examples/jrc_mus_lung_covid.n5/attributes.json") assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" response = client.head("/janelia-data-examples/jrc_mus_lung_covid.n5/") assert response.status_code == 404 @@ -178,6 +179,7 @@ def test_get_object(app): with TestClient(app) as client: response = client.get("/janelia-data-examples/jrc_mus_lung_covid.n5/attributes.json") assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" json_obj = response.json() assert 'n5' in json_obj diff --git a/tests/test_boto.py b/tests/test_boto.py index be3f785..89c87cf 100644 --- a/tests/test_boto.py +++ b/tests/test_boto.py @@ -1,12 +1,17 @@ +import asyncio import time import multiprocessing +from datetime import datetime, timezone +from email.utils import parsedate_to_datetime import boto3 import pytest from pydantic import HttpUrl from x2s3.app import create_app +from x2s3.client_aioboto import AiobotoProxyClient from x2s3.settings import Target, Settings +from x2s3.utils import CACHE_CONTROL_PUBLIC # Set the start method to spawn to avoid pickling issues multiprocessing.set_start_method('spawn', force=True) @@ -165,3 +170,170 @@ def test_get_object_precedence(app, s3_client): assert response['ResponseMetadata']['HTTPStatusCode'] == 200 json_obj = response['Body'].read().decode('utf-8') assert 'n5' in json_obj + + +# The tests below drive AiobotoProxyClient against a stub and need no network. + + +class _StubS3: + """Minimal stand-in for the aiobotocore client, recording its calls.""" + + def __init__(self, get_headers=None, head_response=None, get_error=None): + self.get_headers = get_headers or {} + self.head_response = head_response or {} + self.get_error = get_error + self.calls = [] + + async def get_object(self, **kwargs): + self.calls.append(kwargs) + if self.get_error is not None: + raise self.get_error + return {"ResponseMetadata": {"HTTPHeaders": self.get_headers}, + "Body": None} + + async def head_object(self, **kwargs): + self.calls.append(kwargs) + return self.head_response + + +def call_with_stub(stub, method, *args, **options): + """Await one AiobotoProxyClient method against a stub S3 client. + + The client is built inside the coroutine on purpose: asyncio.Lock() binds + to the running loop on Python 3.9 and asyncio.run() leaves no current loop + behind, so building it outside would break whichever test ran second. + """ + async def run(): + client = AiobotoProxyClient({'target_name': 'test'}, + bucket='test-bucket', **options) + client.client = stub + return await getattr(client, method)(*args) + + return asyncio.run(run()) + + +def test_head_object_last_modified_is_locale_independent(): + # botocore hands head_object a real datetime, and formatting it with + # strftime('%a, %d %b %Y ...') expands %a/%b using the process locale -- + # under e.g. LC_TIME=de_DE that yields '.., 18 Dez ...', which no cache or + # client can parse, silently killing 304 revalidation for S3 targets. No + # non-English locale is installed in CI, so simulate one with a datetime + # whose strftime is locale-poisoned. + class GermanLocaleDatetime(datetime): + def strftime(self, fmt): + return super().strftime(fmt).replace('Dec', 'Dez') + + last_modified = GermanLocaleDatetime(2026, 12, 18, 12, 0, 0, + tzinfo=timezone.utc) + stub = _StubS3(head_response={"ContentLength": 1234, + "LastModified": last_modified}) + + response = call_with_stub(stub, 'head_object', 'some/key.json') + assert parsedate_to_datetime(response.headers['last-modified']) == last_modified + + +def test_get_object_keeps_upstream_cache_control(): + # An object the origin marked uncacheable must not be re-advertised as + # publicly cacheable for an hour: the browser and the shared nginx cache in + # front of x2s3 would both retain content the origin said not to store. + stub = _StubS3(get_headers={"content-length": "10", + "cache-control": "no-store"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json') + assert handle.headers["Cache-Control"] == "no-store" + + +def test_get_object_defaults_cache_control_when_upstream_has_none(): + stub = _StubS3(get_headers={"content-length": "10"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json') + assert handle.headers["Cache-Control"] == CACHE_CONTROL_PUBLIC + + +def test_head_object_keeps_upstream_cache_control(): + stub = _StubS3(head_response={ + "ContentLength": 10, + "LastModified": datetime(2026, 8, 18, 12, 0, 0, tzinfo=timezone.utc), + "CacheControl": "private, max-age=60", + }) + response = call_with_stub(stub, 'head_object', 'some/key.json') + assert response.headers["cache-control"] == "private, max-age=60" + + +NOT_MODIFIED_HEADERS = {"etag": '"upstream-etag"', + "last-modified": "Fri, 26 Jul 2024 13:39:10 GMT"} + + +def _not_modified_error(): + """The ClientError real S3 raises for a conditional GET that matches.""" + from botocore.exceptions import ClientError + return ClientError({"Error": {"Code": "304", "Message": "Not Modified"}, + "ResponseMetadata": {"HTTPStatusCode": 304, + "HTTPHeaders": NOT_MODIFIED_HEADERS}}, + "GetObject") + + +def test_open_object_forwards_conditional_headers_to_s3(): + # Without this the proxy fetches the whole object upstream and throws the + # body away to answer 304, so a client revalidating thousands of cached + # chunks costs nearly as much as never having cached them. + stub = _StubS3(get_headers={"content-length": "10"}) + call_with_stub(stub, 'open_object', 'some/key.json', None, + '"client-etag"', 'Fri, 26 Jul 2024 13:39:10 GMT') + assert stub.calls[0]["IfNoneMatch"] == '"client-etag"' + assert stub.calls[0]["IfModifiedSince"] == parsedate_to_datetime( + "Fri, 26 Jul 2024 13:39:10 GMT") + + +def test_open_object_returns_304_when_s3_says_not_modified(): + stub = _StubS3(get_error=_not_modified_error()) + response = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None) + assert response.status_code == 304 + assert response.headers["last-modified"] == NOT_MODIFIED_HEADERS["last-modified"] + assert response.headers["cache-control"] == CACHE_CONTROL_PUBLIC + + +def test_304_from_s3_carries_etag_only_when_proxied(): + # proxy_etag=False exists to keep upstream ETags off the wire for backends + # whose ETags break the AWS SDK integrity check; a 304 must not leak one. + stub = _StubS3(get_error=_not_modified_error()) + hidden = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None) + assert "etag" not in hidden.headers + + stub = _StubS3(get_error=_not_modified_error()) + shown = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None, proxy_etag=True) + assert shown.headers["etag"] == NOT_MODIFIED_HEADERS["etag"] + + +def test_unparseable_if_modified_since_is_not_forwarded(): + # botocore wants a datetime; handing it a garbage string would raise, so a + # slightly-off client header must not become a 500. + stub = _StubS3(get_headers={"content-length": "10"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json', None, + None, 'not a date') + assert "IfModifiedSince" not in stub.calls[0] + assert handle.status_code == 200 + + +def test_conditional_get_returns_304_against_real_s3(app, s3_client): + # End-to-end through the running proxy: the ETag the client got back must + # be answerable with a 304 rather than a second full body. + bucket = 'janelia-data-examples-with-etag' + key = 'jrc_mus_lung_covid.n5/attributes.json' + etag = s3_client.get_object(Bucket=bucket, Key=key)['ETag'] + + with pytest.raises(s3_client.exceptions.ClientError) as exc_info: + s3_client.get_object(Bucket=bucket, Key=key, IfNoneMatch=etag) + assert exc_info.value.response['ResponseMetadata']['HTTPStatusCode'] == 304 + + +def test_head_and_get_agree_on_caching_headers_over_s3(app, s3_client): + # Same invariant as the file client, across the other pair of code paths. + bucket = 'janelia-data-examples-with-etag' + key = 'jrc_mus_lung_covid.n5/attributes.json' + head = s3_client.head_object(Bucket=bucket, Key=key) + get = s3_client.get_object(Bucket=bucket, Key=key) + assert head['ETag'] == get['ETag'] + assert head['LastModified'] == get['LastModified'] + assert head.get('CacheControl') == get.get('CacheControl') diff --git a/tests/test_file.py b/tests/test_file.py index 5ef3e59..f16f32b 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -5,7 +5,6 @@ from pydantic import HttpUrl from x2s3.app import create_app -from x2s3.client_file import STATIC_ETAG from x2s3.settings import Target, Settings from x2s3.utils import parse_xml @@ -19,14 +18,6 @@ def get_settings(): client='file', options={'path':'.'} ), - Target( - name='local-files-with-etags', - client='file', - options={ - 'path':'.', - 'calculate_etags':'true' - } - ), Target( name='hidden-files', browseable=False, @@ -79,21 +70,8 @@ def test_list_objects(app): for content in contents: etag = content.find('ETag').text assert etag.startswith('"') - assert etag==STATIC_ETAG - - -def test_list_objects_with_etags(app): - with TestClient(app) as client: - bucket_name = 'local-files-with-etags' - response = client.get(f"/{bucket_name}?list-type=2&prefix=tests/") - assert response.status_code == 200 - root = parse_xml(response.text) - assert root.tag == "ListBucketResult" - assert root.find('Name').text == bucket_name - for content in root.findall('Contents'): - etag = content.find('ETag').text - assert etag.startswith('"') - assert etag!=STATIC_ETAG + # mtime-size, not a constant: see make_file_etag + assert etag != '"11111111111111111111111111111111"' def test_list_objects_delimiter(app): @@ -271,3 +249,247 @@ def test_unbrowseable_head(app): assert response.status_code == 200 response = client.head("/local-files/missing") assert response.status_code == 404 + + +def test_file_get_has_caching_headers(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md") + assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'].startswith('"') + + +def test_file_head_has_caching_headers(app): + with TestClient(app) as client: + response = client.head("/local-files/README.md") + assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'] == client.get("/local-files/README.md").headers['etag'] + + +def test_file_last_modified_is_http_date(app): + # The S3 ISO format is for listing XML; a header must be an HTTP-date or + # browsers and shared caches cannot use it. + from email.utils import parsedate_to_datetime + with TestClient(app) as client: + response = client.head("/local-files/README.md") + assert response.headers['last-modified'].endswith("GMT") + assert parsedate_to_datetime(response.headers['last-modified']) is not None + + +def test_file_ranged_response_has_caching_headers(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", headers={"Range": "bytes=0-9"}) + assert response.status_code == 206 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'].startswith('"') + + +def test_listing_keeps_s3_iso_timestamps(app): + # Only the header format changes; the XML body still speaks S3. + with TestClient(app) as client: + response = client.get("/local-files?list-type=2&prefix=tests/&max-keys=1") + root = parse_xml(response.text) + last_modified = root.find('Contents').find('LastModified').text + assert last_modified.endswith("Z") + assert "GMT" not in last_modified + + +def test_get_returns_304_for_matching_etag(app): + with TestClient(app) as client: + first = client.get("/local-files/README.md") + assert first.status_code == 200 + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag']}) + assert second.status_code == 304 + assert second.content == b"" + assert second.headers['cache-control'] == "public, max-age=3600" + + +def test_get_returns_200_for_stale_etag(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"If-None-Match": '"stale-1"'}) + assert response.status_code == 200 + assert response.content + + +def test_get_returns_304_for_if_modified_since(app): + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-Modified-Since": first.headers['last-modified']}) + assert second.status_code == 304 + + +def test_if_none_match_beats_range(app): + # RFC 9110 13.1.3: a matching If-None-Match wins over Range, so this is a + # 304 rather than a 206. + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag'], + "Range": "bytes=0-9"}) + assert second.status_code == 304 + + +def test_head_returns_304_for_matching_etag(app): + with TestClient(app) as client: + first = client.head("/local-files/README.md") + second = client.head("/local-files/README.md", + headers={"If-None-Match": first.headers['etag']}) + assert second.status_code == 304 + + +def test_304_does_not_leak_file_handles(app, monkeypatch): + # The handle is opened before the validator check, so the 304 path has to + # close it explicitly. Neither a ResourceWarning trap nor an open-fd count + # can observe this reliably: CPython's refcounting deallocates the + # underlying file object (running its own closing finalizer) the instant + # the local `handle` variable in get_object_or_denied goes out of scope, + # before either detector gets a chance to look. So this spies directly on + # FileObjectHandle.close() to confirm the 304 path actually calls it. + from x2s3.client_file import FileObjectHandle + + calls = [] + original_close = FileObjectHandle.close + + def spy_close(self): + calls.append(self) + original_close(self) + + monkeypatch.setattr(FileObjectHandle, "close", spy_close) + + with TestClient(app) as client: + # The initial 200 GET streams its own handle closed when the response + # finishes, which also calls close() — so only count closes seen + # during the 304 loop below, not this warm-up call. + etag = client.get("/local-files/README.md").headers['etag'] + before = len(calls) + for _ in range(3): + assert client.get("/local-files/README.md", + headers={"If-None-Match": etag}).status_code == 304 + + assert len(calls) - before == 3 + + +def test_304_not_returned_for_missing_key(app): + with TestClient(app) as client: + response = client.get("/local-files/does-not-exist.txt", + headers={"If-None-Match": "*"}) + assert response.status_code == 404 + + +def test_416_response_is_not_cacheable(app): + # An explicitly cacheable 416 could be replayed by a shared cache for a + # later plain GET (caches key on URI+method, not Range), making the + # object look permanently broken. + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000"}) + assert response.status_code == 416 + assert 'cache-control' not in response.headers + assert 'etag' not in response.headers + + +def test_if_none_match_beats_unsatisfiable_range(app): + # RFC 9110 13.2.2 evaluates If-None-Match before Range, so a matching + # validator yields 304 even when the Range is unsatisfiable. + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag'], + "Range": "bytes=99999999-100000000"}) + assert second.status_code == 304 + + +def test_if_range_stale_with_unsatisfiable_range_returns_full_body(app): + # RFC 9110 13.1.5: a stale If-Range means the Range is ignored entirely, + # including one that would otherwise be unsatisfiable. + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_unsatisfiable_range_with_fresh_if_range_still_416(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-Range": full.headers['etag']}) + assert response.status_code == 416 + + +def test_unsatisfiable_range_with_stale_if_none_match_still_416(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-None-Match": '"stale-1"'}) + assert response.status_code == 416 + + +def test_if_range_matching_etag_returns_ranged_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": full.headers['etag']}) + assert response.status_code == 206 + assert response.content == full.content[:10] + + +def test_if_range_stale_etag_returns_full_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_if_range_without_range_is_ignored(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_if_range_matching_last_modified_returns_ranged_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": full.headers['last-modified']}) + assert response.status_code == 206 + assert response.content == full.content[:10] + + +def test_head_and_get_agree_on_caching_headers(app): + # The validators are built separately in head_object and open_object. If + # they ever drift, a client revalidating against the pair gets a full body + # forever, and nothing else in the suite would notice. + with TestClient(app) as client: + head = client.head("/local-files/README.md") + get = client.get("/local-files/README.md") + for header in ("etag", "last-modified", "cache-control"): + assert head.headers[header] == get.headers[header], header + + +def test_listing_etag_matches_get_etag(app): + # A client that caches an object it found in a listing must be able to + # revalidate with that listing's ETag. A constant ETag guaranteed a miss. + with TestClient(app) as client: + listing = client.get("/local-files?list-type=2&prefix=tests/&max-keys=1") + entry = parse_xml(listing.text).find('Contents') + key, listed = entry.find('Key').text, entry.find('ETag').text + + assert client.get(f"/local-files/{key}").headers['etag'] == listed + assert client.get(f"/local-files/{key}", + headers={"If-None-Match": listed}).status_code == 304 diff --git a/tests/test_nginx_cache_headers.py b/tests/test_nginx_cache_headers.py new file mode 100644 index 0000000..a1650a5 --- /dev/null +++ b/tests/test_nginx_cache_headers.py @@ -0,0 +1,176 @@ +"""Tests for the nginx conditional-header handling in docker/include/. + +nginx replaces the client's If-None-Match/If-Modified-Since with its own +cache-revalidation values whenever proxy_cache is configured, which is decided +at config time. These tests run real nginx in front of an echo upstream and +assert on the headers that actually arrive, since that substitution is +invisible from either end alone. + +Skipped when no nginx binary is installed. +""" + +import json +import shutil +import socket +import subprocess +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from pathlib import Path +from urllib.request import Request, urlopen + +import pytest + +DOCKER_INCLUDE = Path(__file__).resolve().parent.parent / "docker" / "include" +UPSTREAM_ETAG = '"upstream-etag"' +CLIENT_ETAG = '"client-etag"' + +pytestmark = pytest.mark.skipif(shutil.which("nginx") is None, + reason="nginx binary not installed") + + +def _free_port(): + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +class _EchoHandler(BaseHTTPRequestHandler): + """Reflects the request headers it received back as a JSON body.""" + + def do_GET(self): + body = json.dumps(dict(self.headers)).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.send_header("ETag", UPSTREAM_ETAG) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + +@pytest.fixture(scope="module") +def upstream(): + server = ThreadingHTTPServer(("127.0.0.1", 0), _EchoHandler) + threading.Thread(target=server.serve_forever, daemon=True).start() + yield server.server_address[1] + server.shutdown() + + +@pytest.fixture(scope="module") +def nginx(upstream, tmp_path_factory): + root = tmp_path_factory.mktemp("nginx") + port = _free_port() + # proxy_cache_valid is first-match-wins, so this 1s entry must precede the + # 15m one inside proxy_cache.conf for the revalidation test to be quick. + (root / "nginx.conf").write_text(f""" +worker_processes 1; +pid {root}/nginx.pid; +error_log {root}/error.log warn; +events {{ worker_connections 64; }} +http {{ + access_log off; + client_body_temp_path {root}/client_temp; + proxy_temp_path {root}/proxy_temp; + fastcgi_temp_path {root}/fastcgi_temp; + uwsgi_temp_path {root}/uwsgi_temp; + scgi_temp_path {root}/scgi_temp; + proxy_cache_path {root}/cache keys_zone=mycache:10m max_size=32m levels=1:2 inactive=1h; + + include {DOCKER_INCLUDE}/proxy_cache_maps.conf; + + server {{ + listen 127.0.0.1:{port}; + location / {{ + proxy_cache_valid 200 1s; + include {DOCKER_INCLUDE}/proxy_cache.conf; + proxy_pass http://127.0.0.1:{upstream}; + }} + }} +}} +""") + check = subprocess.run(["nginx", "-t", "-c", str(root / "nginx.conf")], + capture_output=True, text=True) + assert check.returncode == 0, check.stderr + subprocess.run(["nginx", "-c", str(root / "nginx.conf")], + capture_output=True, text=True, check=True) + for _ in range(50): + try: + with socket.create_connection(("127.0.0.1", port), timeout=0.1): + break + except OSError: + time.sleep(0.1) + yield port + subprocess.run(["nginx", "-c", str(root / "nginx.conf"), "-s", "quit"], + capture_output=True) + + +def request_through_nginx(port, path, **headers): + """GET through nginx, returning (cache status, headers the upstream saw).""" + request = Request(f"http://127.0.0.1:{port}{path}", headers=headers) + with urlopen(request) as response: + seen = {k.lower(): v for k, v in json.loads(response.read()).items()} + return response.headers.get("X-Proxy-Cache"), seen + + +def upstream_headers(port, path, **headers): + """GET through nginx and return the headers the upstream actually saw.""" + return request_through_nginx(port, path, **headers)[1] + + +def test_ranged_request_forwards_client_if_none_match(nginx): + # Range requests bypass the cache entirely, so nginx is a pass-through and + # the client's validator has to survive the hop or x2s3 can never answer + # the 304 that RFC 9110 13.2.2 requires. + seen = upstream_headers(nginx, "/ranged-inm", Range="bytes=0-9", + **{"If-None-Match": CLIENT_ETAG}) + assert seen.get("if-none-match") == CLIENT_ETAG + assert seen.get("range") == "bytes=0-9" + + +def test_ranged_request_forwards_client_if_modified_since(nginx): + since = "Tue, 18 Aug 2026 12:00:00 GMT" + seen = upstream_headers(nginx, "/ranged-ims", Range="bytes=0-9", + **{"If-Modified-Since": since}) + assert seen.get("if-modified-since") == since + + +def test_cached_request_does_not_leak_client_if_none_match(nginx): + # Unranged requests take part in nginx's cache, where these headers belong + # to nginx's own revalidation. Forwarding the client's validator here would + # let it answer nginx's question and revive a stale cache entry. + seen = upstream_headers(nginx, "/cached-inm", **{"If-None-Match": CLIENT_ETAG}) + assert "if-none-match" not in seen + + +def test_cache_revalidation_uses_nginx_own_validator(nginx): + # The regression guard for the map's '' branch: once the entry goes stale, + # nginx must revalidate with the ETag it cached, not the client's, or a 304 + # meant for the client would mark a stale entry fresh. + # + # Expiry is polled rather than slept through: nginx derives validity from + # the upstream Date header, which is whole-second, so a 1s entry expires + # anywhere in roughly 1-3s. + request_through_nginx(nginx, "/reval") + deadline = time.time() + 15 + while time.time() < deadline: + status, seen = request_through_nginx(nginx, "/reval", + **{"If-None-Match": CLIENT_ETAG}) + if status != "HIT": + assert seen.get("if-none-match") == UPSTREAM_ETAG + return + time.sleep(0.25) + pytest.fail("cache entry never expired, so revalidation was never exercised") + + +def test_no_cache_request_bypasses_the_cache(nginx): + # nginx.conf maps Cache-Control: no-cache to a cache bypass, but a + # location-level proxy_cache_bypass replaces the http-level one rather than + # adding to it, so including proxy_cache.conf silently dropped that policy. + status, _ = request_through_nginx(nginx, "/no-cache-probe") + assert status == "MISS" + status, _ = request_through_nginx(nginx, "/no-cache-probe", + **{"Cache-Control": "no-cache"}) + assert status == "BYPASS" diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py new file mode 100644 index 0000000..7530604 --- /dev/null +++ b/tests/test_utils_caching.py @@ -0,0 +1,159 @@ +from email.utils import parsedate_to_datetime + +from x2s3.utils import ( + CACHE_CONTROL_PUBLIC, + check_not_modified, + format_http_date, + if_range_matches, + make_file_etag, +) + +ETAG = make_file_etag(1755172800.0, 1234) +LAST_MODIFIED = format_http_date(1755172800.0) +RESPONSE_HEADERS = { + "ETag": ETAG, + "Last-Modified": LAST_MODIFIED, + "Cache-Control": CACHE_CONTROL_PUBLIC, + "Content-Length": "1234", + "Content-Type": "application/octet-stream", +} + + +def test_format_http_date_is_rfc7231(): + # Browsers and shared caches can only use Last-Modified if it parses as an + # HTTP-date; the S3 ISO format does not. + assert LAST_MODIFIED.endswith("GMT") + assert parsedate_to_datetime(LAST_MODIFIED) is not None + + +def test_make_file_etag_is_quoted_and_varies(): + assert ETAG.startswith('"') and ETAG.endswith('"') + assert make_file_etag(1755172800.0, 1234) != make_file_etag(1755172800.0, 1235) + assert make_file_etag(1755172800.0, 1234) != make_file_etag(1755172801.0, 1234) + + +def test_no_validators_means_no_304(): + assert check_not_modified({}, RESPONSE_HEADERS) is None + + +def test_matching_if_none_match_returns_304(): + response = check_not_modified({"if-none-match": ETAG}, RESPONSE_HEADERS) + assert response is not None + assert response.status_code == 304 + + +def test_304_carries_validators_but_no_content_headers(): + response = check_not_modified({"if-none-match": ETAG}, RESPONSE_HEADERS) + assert response.headers["etag"] == ETAG + assert response.headers["last-modified"] == LAST_MODIFIED + assert response.headers["cache-control"] == CACHE_CONTROL_PUBLIC + assert "content-length" not in response.headers + assert "content-type" not in response.headers + + +def test_stale_if_none_match_returns_none(): + assert check_not_modified({"if-none-match": '"nope-1"'}, RESPONSE_HEADERS) is None + + +def test_star_if_none_match_returns_304(): + response = check_not_modified({"if-none-match": "*"}, RESPONSE_HEADERS) + assert response is not None + + +def test_weak_and_list_if_none_match_match(): + response = check_not_modified({"if-none-match": f'"other", W/{ETAG}'}, RESPONSE_HEADERS) + assert response is not None + + +def test_if_none_match_takes_precedence_over_if_modified_since(): + # A non-matching ETag means not-modified is false, even though the + # If-Modified-Since alone would have produced a 304. + headers = {"if-none-match": '"nope-1"', "if-modified-since": LAST_MODIFIED} + assert check_not_modified(headers, RESPONSE_HEADERS) is None + + +def test_if_modified_since_at_or_after_mtime_returns_304(): + assert check_not_modified({"if-modified-since": LAST_MODIFIED}, RESPONSE_HEADERS) is not None + later = format_http_date(1755172800.0 + 60) + assert check_not_modified({"if-modified-since": later}, RESPONSE_HEADERS) is not None + + +def test_if_modified_since_before_mtime_returns_none(): + earlier = format_http_date(1755172800.0 - 60) + assert check_not_modified({"if-modified-since": earlier}, RESPONSE_HEADERS) is None + + +def test_unparseable_if_modified_since_returns_none(): + assert check_not_modified({"if-modified-since": "not a date"}, RESPONSE_HEADERS) is None + + +def test_response_header_lookup_is_case_insensitive(): + # Fileglancer passes a plain dict whose keys came from x2s3, so the helper + # cannot rely on Starlette's case-insensitive Headers mapping. + lowered = {k.lower(): v for k, v in RESPONSE_HEADERS.items()} + assert check_not_modified({"if-none-match": ETAG}, lowered) is not None + + +def test_missing_etag_never_matches(): + assert check_not_modified({"if-none-match": ETAG}, {"Cache-Control": CACHE_CONTROL_PUBLIC}) is None + + +def test_naive_if_modified_since_does_not_crash(): + # parsedate_to_datetime returns a naive datetime for a zoneless date, and + # comparing it to our timezone-aware one raises TypeError. + assert check_not_modified({"if-modified-since": "Thu, 14 Aug 2026 12:00:00"}, + RESPONSE_HEADERS) is None + + +def test_make_file_etag_wire_format_is_pinned(): + # Fileglancer's make_etag must produce this byte-for-byte for the same file. + assert make_file_etag(1755172800.0, 1234) == '"1755172800.000000-1234"' + + +def test_if_range_matches_etag(): + assert if_range_matches(ETAG, RESPONSE_HEADERS) is True + + +def test_if_range_matches_last_modified(): + assert if_range_matches(LAST_MODIFIED, RESPONSE_HEADERS) is True + + +def test_if_range_stale_validator_does_not_match(): + assert if_range_matches('"stale-etag"', RESPONSE_HEADERS) is False + + +def test_if_range_weak_etag_never_matches(): + # RFC 9110 13.1.5: a weak validator is never valid in If-Range, even if + # its underlying tag matches -- so this is a plain string comparison, + # not the weak-stripping If-None-Match does. + assert if_range_matches(f"W/{ETAG}", RESPONSE_HEADERS) is False + + +def test_star_if_none_match_matches_even_without_etag(): + # RFC 9110 13.1.2: '*' matches any existing representation, so it must + # yield a 304 even when the response carries no ETag — the default for + # S3 targets, which ship with proxy_etag=False. + response = check_not_modified({"if-none-match": "*"}, + {"Last-Modified": LAST_MODIFIED}) + assert response is not None and response.status_code == 304 + + +def test_etag_containing_a_comma_still_matches(): + # RFC 9110 8.8.3 lets a quoted entity-tag contain a comma, so If-None-Match + # cannot be split on bare commas without cutting one in half. + etag = '"abc,def"' + headers = {"ETag": etag, "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": etag}, headers) is not None + + +def test_comma_etag_in_a_list_still_matches(): + etag = '"abc,def"' + headers = {"ETag": etag, "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": f'"other", {etag}'}, headers) is not None + + +def test_unquoted_etag_still_matches(): + # Guard: a quote-only parser would stop matching backends that emit + # unquoted ETags, which are likelier in the wild than comma-bearing ones. + headers = {"ETag": "abc123", "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": "abc123"}, headers) is not None diff --git a/x2s3/app.py b/x2s3/app.py index e027eef..1503183 100644 --- a/x2s3/app.py +++ b/x2s3/app.py @@ -19,6 +19,7 @@ from x2s3.utils import * from x2s3 import client_registry +from x2s3.client import ObjectHandle from x2s3.settings import get_settings, Target # Use uvloop for better async performance @@ -387,14 +388,88 @@ async def target_dispatcher(request: Request, return get_accessdenied_response() return get_read_access_acl() + def _handle_or_denied(handle): + """Apply the same 404-masking rule to an open_object() result. + Returns a Response to short-circuit with, or None if handle is a + real ObjectHandle that the caller should keep going with. + """ + if not isinstance(handle, ObjectHandle): + if handle.status_code == 404 and not target_config.browseable: + return get_accessdenied_response() + return handle + return None + + async def _open(key, range_header): + """open_object with this request's conditional headers attached.""" + return await client.open_object( + key, range_header, + request.headers.get("if-none-match"), + request.headers.get("if-modified-since")) + async def get_object_or_denied(key): """GetObject with S3-style 404 masking: on unbrowseable buckets a missing key returns 403 AccessDenied so clients can't probe which - keys exist (real S3 does this when s3:ListBucket is denied).""" - response = await client.get_object(key, request.headers.get("range")) - if response.status_code == 404 and not target_config.browseable: - return get_accessdenied_response() - return response + keys exist (real S3 does this when s3:ListBucket is denied). + + Opens the object first so the validator check can see its ETag, + then either answers 304 or streams. The handle is closed on the + 304 path — nothing else will. + + Backends that evaluate the conditional headers themselves return + a 304 straight out of _open with no body transferred; the + check_not_modified call below covers the ones that don't. + """ + handle = await _open(key, request.headers.get("range")) + denied = _handle_or_denied(handle) + if denied is not None and denied.status_code != 416: + return denied + + if_range = request.headers.get("if-range") + if denied is not None: + # 416 from an unsatisfiable Range. RFC 9110 13.2.2 evaluates + # If-None-Match/If-Modified-Since (-> 304) and a stale + # If-Range (-> ignore the Range, serve the full body) before + # Range, so the 416 can't short-circuit them. It also carries + # no validators (see the client 416 branches), so reopen + # without the Range to get some. + if if_range is None \ + and "if-none-match" not in request.headers \ + and "if-modified-since" not in request.headers: + return denied + handle = await _open(key, None) + full_denied = _handle_or_denied(handle) + if full_denied is not None: + return full_denied + if (if_range is None or if_range_matches(if_range, handle.headers)) \ + and check_not_modified(request.headers, handle.headers) is None: + # Validators say the client's copy is stale and the Range + # still applies, so it is still unsatisfiable. + handle.close() + return denied + # Fall through: check_not_modified below answers the 304, or + # a stale If-Range means this full handle streams as a 200. + elif if_range is not None and handle.status_code == 206 \ + and not if_range_matches(if_range, handle.headers): + # RFC 9110 13.1.5: a stale If-Range validator means the Range + # must be ignored and the full representation served instead — + # otherwise a client resuming a partial download against a + # file that's since been overwritten would silently stitch + # bytes from two versions into one corrupt chunk. + handle.close() + handle = await _open(key, None) + denied = _handle_or_denied(handle) + if denied is not None: + return denied + + not_modified = check_not_modified(request.headers, handle.headers) + if not_modified is not None: + handle.close() + return not_modified + try: + return client.stream_object(handle) + except Exception: + handle.close() + raise if list_type: if not target_path: @@ -467,6 +542,10 @@ async def head_object(request: Request, path: str): if response.status_code == 404 and not target_config.browseable: # Mask missing keys on unbrowseable buckets; HEAD carries no body return Response(status_code=403, media_type="application/xml") + if response.status_code == 200: + not_modified = check_not_modified(request.headers, response.headers) + if not_modified is not None: + return not_modified return response except Exception: logger.opt(exception=sys.exc_info()).info("Error requesting head") diff --git a/x2s3/client.py b/x2s3/client.py index fc7b3af..be9cab8 100644 --- a/x2s3/client.py +++ b/x2s3/client.py @@ -32,15 +32,23 @@ async def head_object(self, key: str): https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html """ - async def open_object(self, key: str, range_header: str = None): + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): """ Open an object and return a handle for streaming. This performs the file/storage operation and returns an ObjectHandle containing metadata and a reference to the content, or an error Response. + The conditional headers are passed through so that backends which can + evaluate them remotely may answer 304 without transferring a body. + Doing so is optional: the dispatcher re-checks the validators against + the returned handle, so a client that ignores them is still correct, + just more expensive. + Returns: - ObjectHandle on success, or Response on error + ObjectHandle on success, or Response on error (including 304) """ def stream_object(self, handle: ObjectHandle): @@ -54,14 +62,6 @@ def stream_object(self, handle: ObjectHandle): StreamingResponse that streams the object content """ - async def get_object(self, key: str, range_header: str = None): - """ - Basic interface for AWS S3's GetObject API. - https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html - - This is a convenience method that combines open_object() and stream_object(). - """ - async def list_objects_v2(self, continuation_token: str, delimiter: str, diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index 116a9b8..9d1989d 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -3,6 +3,7 @@ import sys import typing from dataclasses import dataclass +from email.utils import parsedate_to_datetime from typing import Any from typing_extensions import override @@ -32,6 +33,12 @@ def close(self): self._closed = True +def _is_not_modified(e): + """True for the ClientError S3 raises when a conditional GET matches.""" + return (isinstance(e, botocore.exceptions.ClientError) + and e.response.get('ResponseMetadata', {}).get('HTTPStatusCode') == 304) + + def handle_s3_exception(e, key=None): """ Handle various cases of generic errors from the boto AWS API. """ @@ -169,7 +176,11 @@ async def head_object(self, key: str): headers = { "Accept-Ranges": "bytes", "Content-Length": str(s3_res.get("ContentLength")), - "Last-Modified": s3_res.get("LastModified").strftime("%a, %d %b %Y %H:%M:%S GMT"), + # format_http_date, not strftime: %a/%b are locale-dependent + "Last-Modified": format_http_date(s3_res.get("LastModified").timestamp()), + # Our default only applies to objects carrying no policy of + # their own (see open_object) + "Cache-Control": s3_res.get("CacheControl") or CACHE_CONTROL_PUBLIC, } if self.proxy_etag: @@ -184,8 +195,15 @@ async def head_object(self, key: str): @override - async def open_object(self, key: str, range_header: str = None): - """Open an S3 object and return a handle for streaming.""" + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): + """Open an S3 object and return a handle for streaming. + + Conditional headers are forwarded to S3, which evaluates them itself + and answers 304 without sending a body. Without that, revalidating a + cached object means fetching it in full and discarding it. + """ real_key = key if self.bucket_prefix: real_key = os.path.join(self.bucket_prefix, key) if key else self.bucket_prefix @@ -196,6 +214,7 @@ async def open_object(self, key: str, range_header: str = None): headers = { 'Accept-Ranges': "bytes", 'Content-Type': content_type, + 'Cache-Control': CACHE_CONTROL_PUBLIC, } if content_type == 'application/octet-stream': @@ -212,6 +231,17 @@ async def open_object(self, key: str, range_header: str = None): } if range_header: get_object_params["Range"] = range_header + if if_none_match: + get_object_params["IfNoneMatch"] = if_none_match + if if_modified_since: + try: + get_object_params["IfModifiedSince"] = \ + parsedate_to_datetime(if_modified_since) + except (TypeError, ValueError): + # botocore requires a datetime, so a malformed client + # header just skips the upstream condition rather than + # turning into a 500. The dispatcher still checks locally. + pass # Call S3 get_object result = await self.client.get_object(**get_object_params) @@ -233,6 +263,13 @@ async def open_object(self, key: str, range_header: str = None): if "last-modified" in res_headers: headers["Last-Modified"] = res_headers["last-modified"] + if "cache-control" in res_headers: + # The origin's policy wins over our default. Re-advertising a + # no-store or private object as publicly cacheable for an hour + # would let browsers and the shared nginx cache in front of + # x2s3 hold on to content the origin said not to store. + headers["Cache-Control"] = res_headers["cache-control"] + if self.proxy_etag and "etag" in res_headers: headers["ETag"] = res_headers["etag"] @@ -247,8 +284,29 @@ async def open_object(self, key: str, range_header: str = None): ) except Exception as e: + if _is_not_modified(e): + return self._not_modified_response(e, headers) return handle_s3_exception(e, key) + + def _not_modified_response(self, e, headers): + """Build the 304 for a conditional GET that S3 answered as unchanged. + + S3 returns the validators on its 304, so they are echoed back rather + than re-fetched. A 304 must carry no body, which is also why this + cannot go through handle_s3_exception: that renders an XML error. + """ + res_headers = e.response.get('ResponseMetadata', {}).get('HTTPHeaders', {}) + not_modified_headers = { + 'Cache-Control': res_headers.get('cache-control') + or headers['Cache-Control'], + } + if 'last-modified' in res_headers: + not_modified_headers['Last-Modified'] = res_headers['last-modified'] + if self.proxy_etag and 'etag' in res_headers: + not_modified_headers['ETag'] = res_headers['etag'] + return Response(status_code=304, headers=not_modified_headers) + @override def stream_object(self, handle: S3ObjectHandle): """Stream content from an opened S3 object handle.""" @@ -262,20 +320,6 @@ def stream_object(self, handle: S3ObjectHandle): content_length=handle.content_length, ) - @override - async def get_object(self, key: str, range_header: str = None): - """Convenience method that combines open_object() and stream_object().""" - result = await self.open_object(key, range_header) - if isinstance(result, S3ObjectHandle): - try: - return self.stream_object(result) - except Exception: - # Ensure body is closed if stream_object fails - result.close() - raise - return result # Error response - - @override async def list_objects_v2(self, continuation_token: str, diff --git a/x2s3/client_file.py b/x2s3/client_file.py index 50c3859..0ce78b0 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -1,7 +1,6 @@ import os import sys from dataclasses import dataclass -from hashlib import md5 from pathlib import Path from typing import BinaryIO, Optional, Tuple from typing_extensions import override @@ -34,8 +33,6 @@ def close(self): self.file_handle = None -STATIC_ETAG = '"11111111111111111111111111111111"' - def handle_exception(e, key=None): """ Handle various cases of generic errors. """ @@ -150,22 +147,12 @@ def file_iterator(handle: FileObjectHandle, buffer_size: int = DEFAULT_BUFFER_SI handle.close() -# From https://teppen.io/2018/10/23/aws_s3_verify_etags/ -def calc_etag(inputfile, partsize): - md5_digests = [] - with open(inputfile, 'rb') as f: - for chunk in iter(lambda: f.read(partsize), b''): - md5_digests.append(md5(chunk).digest()) - return md5(b''.join(md5_digests)).hexdigest() + '-' + str(len(md5_digests)) - - class FileProxyClient(ProxyClient): def __init__(self, proxy_kwargs, **kwargs): self.proxy_kwargs = proxy_kwargs or {} self.target_name = self.proxy_kwargs['target_name'] self.root_path = str(Path(kwargs['path']).resolve()) - self.calculate_etags = kwargs.get('calculate_etags', False) self.buffer_size = kwargs.get('buffer_size', DEFAULT_BUFFER_SIZE) def _safe_path(self, key: str) -> Optional[str]: @@ -202,7 +189,9 @@ async def head_object(self, key: str): stats = os.stat(path) file_size = stats.st_size headers["Content-Length"] = str(file_size) - headers["Last-Modified"] = format_timestamp_s3(stats.st_mtime) + headers["Last-Modified"] = format_http_date(stats.st_mtime) + headers["ETag"] = make_file_etag(stats.st_mtime, file_size) + headers["Cache-Control"] = CACHE_CONTROL_PUBLIC return Response(headers=headers) except Exception as e: @@ -210,8 +199,15 @@ async def head_object(self, key: str): @override - async def open_object(self, key: str, range_header: str = None): - """Open a file object and return a handle for streaming.""" + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): + """Open a file object and return a handle for streaming. + + The conditional arguments are accepted for interface parity and + ignored: opening a local file is an open plus an fstat, so there is + nothing to save by evaluating them here rather than in the dispatcher. + """ file_handle = None try: path = self._safe_path(key) @@ -233,18 +229,27 @@ async def open_object(self, key: str, range_header: str = None): file_handle = open(path, "rb") stats = os.fstat(file_handle.fileno()) file_size = stats.st_size - headers["Last-Modified"] = format_timestamp_s3(stats.st_mtime) + headers["Last-Modified"] = format_http_date(stats.st_mtime) + headers["ETag"] = make_file_etag(stats.st_mtime, file_size) + headers["Cache-Control"] = CACHE_CONTROL_PUBLIC # Handle range requests if range_header: range_result = parse_range_header(range_header, file_size) if range_result is None: - # Invalid range, return 416 Range Not Satisfiable + # Invalid range, return 416 Range Not Satisfiable. + # Cache-Control/ETag are dropped here (unlike the 200/206 + # branches below): a shared cache keys on URI+method, not + # Range, so an explicitly cacheable 416 could be served + # back for a later plain GET and make the object look + # broken for an hour. file_handle.close() - headers["Content-Range"] = f"bytes */{file_size}" + error_headers = {k: v for k, v in headers.items() + if k not in ("Cache-Control", "ETag")} + error_headers["Content-Range"] = f"bytes */{file_size}" return Response( status_code=416, - headers=headers + headers=error_headers ) start, end = range_result @@ -295,19 +300,6 @@ def stream_object(self, handle: FileObjectHandle): media_type=handle.media_type ) - @override - async def get_object(self, key: str, range_header: str = None): - """Convenience method that combines open_object() and stream_object().""" - result = await self.open_object(key, range_header) - if isinstance(result, FileObjectHandle): - try: - return self.stream_object(result) - except Exception: - # Ensure file is closed if stream_object fails - result.close() - raise - return result # Error response - @override async def list_objects_v2(self, @@ -403,15 +395,13 @@ def walk_path(self, path, continuation_token, delimiter, max_keys): stats = os.stat(file_path) file_size = stats.st_size - etag = STATIC_ETAG - if self.calculate_etags: - # This is VERY slow because it needs to read every file - etag = f'"{calc_etag(file_path, 8388608)}"' - contents.append({ 'Key': key, 'Size': str(file_size), - 'ETag': etag, + # Same validator head_object/open_object return, so a + # client can revalidate what it found here and get a + # 304. Free: the stat above already has both fields. + 'ETag': make_file_etag(stats.st_mtime, file_size), 'LastModified': format_timestamp_s3(stats.st_mtime), 'StorageClass': 'STANDARD' }) diff --git a/x2s3/utils.py b/x2s3/utils.py index c29a83b..8b7dfee 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -3,6 +3,7 @@ import urllib import xml.etree.ElementTree as ET from datetime import datetime, timezone +from email.utils import formatdate, parsedate_to_datetime from mimetypes import guess_type from html import escape @@ -252,3 +253,141 @@ def guess_content_type(filename): return 'text/plain+yaml' else: return 'application/octet-stream' + + +# Zarr chunks are effectively immutable by path, but datasets are sometimes +# overwritten in place, so one hour is the worst-case staleness we accept in +# exchange for stopping sub-second chunk re-fetch storms. No `immutable`. +CACHE_CONTROL_PUBLIC = "public, max-age=3600" + + +def make_file_etag(mtime: float, size: int) -> str: + """Strong ETag for a file, derived from its mtime and size. + + Same scheme as fileglancer's make_etag, so the two repos agree on the + validator for the same file. + + ponytail: mtime granularity is the ceiling — two writes within the + filesystem's mtime resolution that also keep the same size would share an + ETag. Switch to a content hash if that ever matters. + + The "-" separator is load-bearing, not cosmetic: the AWS Java SDK v1 + skips its MD5 integrity check whenever `eTag.contains("-")`, on the + assumption that a hyphen means a multipart-upload ETag (which isn't an + MD5 of the body). That skip is the only reason handing out an ETag here + doesn't reintroduce the "Unable to verify integrity of data download" + failure that tests/java/.../S3v1IntegrityTest.java exists to reproduce, + and that `proxy_etag=False` defaults exist to avoid. Changing the + separator to `_` or `:` would make the SDK run the MD5 check again and + fail it, breaking Fiji/N5 Viewer. + """ + return f'"{mtime:.6f}-{size}"' + + +def format_http_date(timestamp) -> str: + """Format a POSIX timestamp as an RFC 7231 HTTP-date. + + Distinct from format_timestamp_s3, which produces the ISO form S3 uses in + listing XML bodies. Headers must use this one or caches cannot read them. + """ + return formatdate(timestamp, usegmt=True) + + +def _split_etags(if_none_match: str): + """Split an If-None-Match header into its entity-tags. + + Not a plain split(','): a quoted entity-tag may contain a comma of its own + (RFC 9110 8.8.3), and cutting one in half means it can never match, so the + client revalidates forever and always gets a full body back. Tags are kept + quoted because that is how they are compared. + """ + tags, current, quoted = [], [], False + for char in if_none_match: + if char == '"': + quoted = not quoted + current.append(char) + elif char == ',' and not quoted: + tags.append(''.join(current)) + current = [] + else: + current.append(char) + tags.append(''.join(current)) + return [tag.strip() for tag in tags if tag.strip()] + + +def _etag_matches(if_none_match: str, etag: str) -> bool: + """True if any entity-tag in an If-None-Match header matches ours.""" + for candidate in _split_etags(if_none_match): + candidate = candidate.strip() + if candidate == '*': + # '*' matches any existing representation (RFC 9110 13.1.2), + # even one carrying no ETag — the guard below must not run first. + return True + if not etag: + continue + if candidate.startswith('W/'): + candidate = candidate[2:] + if candidate == etag: + return True + return False + + +def if_range_matches(if_range: str, response_headers) -> bool: + """True if an If-Range validator exactly matches ETag or Last-Modified. + + response_headers may be a plain dict with canonical capitalization (see + check_not_modified above), so lookups here are lowercased explicitly. + + Unlike If-None-Match, If-Range carries exactly one validator, never a + comma-separated list, and a weak ETag (W/"...") is never a valid match + (RFC 9110 13.1.5). So this is just two exact string comparisons: against + ETag, then against Last-Modified. + """ + lowered = {k.lower(): v for k, v in response_headers.items()} + etag = lowered.get('etag') + last_modified = lowered.get('last-modified') + if etag is not None and if_range == etag: + return True + if last_modified is not None and if_range == last_modified: + return True + return False + + +def check_not_modified(request_headers, response_headers): + """Return a 304 response if the request's validators match, else None. + + request_headers is a case-insensitive mapping (Starlette Headers). + response_headers may be a plain dict with canonical capitalization, which + is what Fileglancer receives back from its user worker, so lookups here are + lowercased explicitly. + """ + lowered = {k.lower(): v for k, v in response_headers.items()} + etag = lowered.get('etag') + last_modified = lowered.get('last-modified') + + if_none_match = request_headers.get('if-none-match') + if if_none_match is not None: + # If-None-Match wins outright: when it is present and does not match, + # If-Modified-Since must not be consulted (RFC 9110 13.1.3). + if not _etag_matches(if_none_match, etag): + return None + else: + if_modified_since = request_headers.get('if-modified-since') + if not if_modified_since or not last_modified: + return None + try: + since = parsedate_to_datetime(if_modified_since) + modified = parsedate_to_datetime(last_modified) + if since is None or modified is None or modified > since: + return None + except (TypeError, ValueError): + # parsedate_to_datetime returns a NAIVE datetime for a date with no + # zone, which clients do send. Comparing it to our aware one raises + # TypeError, so the comparison has to sit inside the guard or a + # slightly-off header becomes a 500. + return None + + headers = {name: lowered[name.lower()] + for name in ('ETag', 'Last-Modified', 'Cache-Control') + if lowered.get(name.lower())} + return Response(status_code=304, headers=headers)