launchpad-reviewers team mailing list archive

[Colin Watson <mp+455045@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lp-archive:always-set-cache-control into lp-archive:main.

Commit message:
Always set Cache-Control header on redirect responses

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lp-archive/+git/lp-archive/+merge/455045

It's much easier to fix our nginx configuration to copy the Cache-Control header over to the followed-redirect response if we ensure that the redirect always has a Cache-Control header rather than relying on the one returned by the librarian.

Thanks to Clinton Fung for help puzzling this out.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-archive:always-set-cache-control into lp-archive:main.

diff --git a/lp_archive/archive.py b/lp_archive/archive.py
index 3e2e50f..f948c8f 100644
--- a/lp_archive/archive.py
+++ b/lp_archive/archive.py
@@ -76,7 +76,10 @@ def check_auth(archive: str) -> None:
 
 
 def get_extra_headers(path: str, live_at: datetime | None) -> dict[str, str]:
-    headers = {}
+    # It's safe to default to long caching even for files in private
+    # archives, since we always set "Vary: Authorization" (see add_headers
+    # below).
+    cache_control = "max-age=31536000"
     # Non-content-addressed files under dists/ are mutable and may change on
     # subsequent publisher runs, so if we aren't addressing a snapshot then
     # we need to tell caches to give them a relatively short expiry time.
@@ -84,8 +87,8 @@ def get_extra_headers(path: str, live_at: datetime | None) -> dict[str, str]:
     if live_at is None:
         path_parts = PurePath(path).parts
         if path_parts[0] == "dists" and path_parts[2:][-3:-2] != ("by-hash",):
-            headers["Cache-Control"] = "max-age=1800, proxy-revalidate"
-    return headers
+            cache_control = "max-age=1800, proxy-revalidate"
+    return {"Cache-Control": cache_control}
 
 
 def translate(
diff --git a/tests/test_archive.py b/tests/test_archive.py
index 11481f7..a40cc6b 100644
--- a/tests/test_archive.py
+++ b/tests/test_archive.py
@@ -306,32 +306,33 @@ def test_translate_cache_control_dists_not_found(client, archive_proxy):
 
 
 def test_translate_cache_control_dists_at_timestamp(client, archive_proxy):
-    # Non-content-addressed requests to a snapshot do not get a
-    # Cache-Control header, even if they're under dists/.
+    # Non-content-addressed requests to a snapshot get a Cache-Control
+    # header with a long expiry time, even if they're under dists/.
     response = client.get(
         "/ubuntu/20220101T120000Z/dists/focal/InRelease",
         headers=[("Host", "snapshot.ubuntu.test")],
     )
     assert response.status_code == 307
-    assert "Cache-Control" not in response.headers
+    assert response.headers["Cache-Control"] == "max-age=31536000"
 
 
 def test_translate_cache_control_dists_by_hash(client, archive_proxy):
-    # Content-addressed requests under dists/ do not get a Cache-Control
-    # header.
+    # Content-addressed requests under dists/ get a Cache-Control header
+    # with a long expiry time.
     response = client.get(
         f"/ubuntu/dists/focal/by-hash/SHA256/{'0' * 64}",
         headers=[("Host", "snapshot.ubuntu.test")],
     )
     assert response.status_code == 307
-    assert "Cache-Control" not in response.headers
+    assert response.headers["Cache-Control"] == "max-age=31536000"
 
 
 def test_translate_cache_control_not_dists(client, archive_proxy):
-    # Requests not under dists/ do not get a Cache-Control header.
+    # Requests not under dists/ get a Cache-Control header with a long
+    # expiry time.
     response = client.get(
         "/ubuntu/pool/main/h/hello/hello_1.0-1.deb",
         headers=[("Host", "snapshot.ubuntu.test")],
     )
     assert response.status_code == 307
-    assert "Cache-Control" not in response.headers
+    assert response.headers["Cache-Control"] == "max-age=31536000"