From 8ec44fd4aa7af36cdc26eafdb876aab4b8370b59 Mon Sep 17 00:00:00 2001 From: Evelyn Chen Date: Sat, 29 Aug 2026 23:37:18 +0000 Subject: [PATCH] fix(site-publish): retain immutable history across deploys Authored-By: @architect --- README.md | 14 ++-- scripts/deploy.py | 160 ++++++++++++++++++++++++-------------- templates/ingress.yaml.j2 | 7 -- tests/test_contract.py | 68 +++++++++++++--- 4 files changed, 165 insertions(+), 84 deletions(-) diff --git a/README.md b/README.md index 9f0ca29..c1cdb64 100644 --- a/README.md +++ b/README.md @@ -133,14 +133,12 @@ any route's mutable objects change. Mutable default and override partitions receive their final cache policy before the matching prefix-scoped stale deletion, so publication never exposes a provisional cache policy or a pointer to a missing immutable target. -Generated Ingress annotations retain each bucket's prior route and immutable paths. When a move -places that retired prefix inside the new sync scope, only its declared -immutable subtrees are excluded; a current-file collision fails publication. -The bucket-keyed history rejects a protected-to-public transition even when the -artifact is renamed; publishing that artifact publicly requires a new bucket. -Legacy single-surface is public for this downgrade check. Removing or renaming -an immutable rule preserves its prior URLs; current mutable content at one of -those paths is rejected instead of replacing it. +An append-only `site-publish-history.json` beside each generated site retains every bucket's access +class and absolute immutable prefixes, including removed routes and rules. When a route move places +a retired prefix inside the new sync scope, that subtree is excluded; a current-file collision +fails publication. Protected access remains sticky across artifact renames and legacy mode, so +publishing the same artifact publicly requires a new bucket. Decommissioning removes the live +application and manifests while retaining this history because its Garage bucket is not purged. Artifact input directories must be pairwise disjoint after filesystem resolution. Publication stops before build or upload if one contains another or diff --git a/scripts/deploy.py b/scripts/deploy.py index b298349..4900210 100644 --- a/scripts/deploy.py +++ b/scripts/deploy.py @@ -8,12 +8,10 @@ import os import re import shutil import subprocess -from pathlib import Path +from pathlib import Path, PurePosixPath from urllib.error import HTTPError, URLError from urllib.request import Request, urlopen -import yaml - from utils import ( NAMESPACE, clone_apps, @@ -30,6 +28,7 @@ from utils import ( GARAGE_ADMIN_ENDPOINT = os.environ.get( "GARAGE_ADMIN_ENDPOINT", "http://garage.storage.svc:3903" ) +HISTORY_FILE = "site-publish-history.json" def validate_artifact_output(site_dir, artifact): @@ -179,35 +178,38 @@ def publish_route_immutables(artifact, route, site_dir, credential_env_names=Non def retired_immutable_filters(artifact, route, html_dir, previous_contract): - """Protect immutable keys only when an old route falls inside the new scope.""" + """Protect every historical immutable prefix inside the current sync scope.""" if not previous_contract: return [] current_prefix = route["path"].strip("/") - previous_prefix = previous_contract["path"].strip("/") - if current_prefix: - marker = f"{current_prefix}/" - if previous_prefix == current_prefix: - previous_prefix = "" - elif not previous_prefix.startswith(marker): - return [] - else: - previous_prefix = previous_prefix[len(marker):] filters = [] - current_immutable = { - rule["path"] for rule in artifact["cache_rules"] if _is_immutable(rule) - } - for immutable_path in previous_contract["immutable_paths"]: - retired_path = "/".join(part for part in (previous_prefix, immutable_path) if part) - collision = html_dir / retired_path - if (immutable_path not in current_immutable and collision.exists() + current_immutable = set(immutable_prefixes(artifact, route)) + for immutable_prefix in previous_contract["immutable_prefixes"]: + if current_prefix: + marker = f"{current_prefix}/" + if not immutable_prefix.startswith(marker): + continue + relative_path = immutable_prefix[len(marker):] + else: + relative_path = immutable_prefix + collision = html_dir / relative_path + if (immutable_prefix not in current_immutable and collision.exists() and any(path.is_file() for path in collision.rglob("*"))): raise RuntimeError( - f"current artifact collides with retired immutable partition: {retired_path}" + f"current artifact collides with retired immutable partition: {relative_path}" ) - filters.extend(("--exclude", f"{retired_path}/*")) + filters.extend(("--exclude", f"{relative_path}/*")) return filters +def immutable_prefixes(artifact, route): + route_prefix = route["path"].strip("/") + return sorted({ + "/".join(part for part in (route_prefix, rule["path"]) if part) + for rule in artifact["cache_rules"] if _is_immutable(rule) + }) + + def s3_sync(artifact, route, site_dir, credential_env_names=None, previous_contract=None): endpoint = artifact["s3_endpoint"] html_dir = site_dir / artifact["build_dir"] @@ -317,13 +319,7 @@ def render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg): for route in cfg["routes"]: artifact = artifact_by_name[route["artifact"]] resource_name = k8s_name(site_name) if cfg["compatibility"] else f"{k8s_name(site_name)}-{route['name']}" - immutable_paths = [ - rule["path"] for rule in artifact["cache_rules"] if _is_immutable(rule) - ] - routes.append({ - **route, "resource_name": resource_name, "artifact_config": artifact, - "immutable_paths_json": json.dumps(immutable_paths, separators=(",", ":")), - }) + routes.append({**route, "resource_name": resource_name, "artifact_config": artifact}) template_vars = { "site": site_name, "site_k8s": k8s_name(site_name), @@ -337,40 +333,84 @@ def render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg): def previous_route_contracts(app_dir): - """Read bucket-keyed route history from generated Ingresses.""" - contracts = {} - manifests = app_dir / "manifests" - if not manifests.exists(): - return contracts - for path in sorted(manifests.glob("ingress*.yaml")): - document = yaml.safe_load(path.read_text()) or {} - annotations = document.get("metadata", {}).get("annotations", {}) - artifact = annotations.get("site-publish.fritzlab.net/artifact") - access = annotations.get("site-publish.fritzlab.net/access") - bucket = annotations.get("site-publish.fritzlab.net/bucket") - immutable_paths_json = annotations.get("site-publish.fritzlab.net/immutable-paths") - route_path = annotations.get("site-publish.fritzlab.net/route-path") - values = (artifact, access, bucket, immutable_paths_json, route_path) - if all(value is None for value in values): - continue - if (not all(isinstance(value, str) for value in values) - or access not in {"public", "protected"} or not route_path.startswith("/")): + """Read the append-only bucket history kept beside generated manifests.""" + path = app_dir / HISTORY_FILE + if not path.exists(): + return {} + try: + document = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError) as exc: + raise RuntimeError(f"invalid site-publish route history in {path}") from exc + if (not isinstance(document, dict) or set(document) != {"schemaVersion", "buckets"} + or document["schemaVersion"] != 1 or not isinstance(document["buckets"], dict)): + raise RuntimeError(f"invalid site-publish route history in {path}") + for bucket, contract in document["buckets"].items(): + if (not isinstance(bucket, str) or not isinstance(contract, dict) + or set(contract) != {"access", "artifact", "immutablePrefixes", "routePath"} + or contract["access"] not in {"legacy", "protected", "public"} + or not isinstance(contract["artifact"], str) + or not isinstance(contract["routePath"], str) + or not contract["routePath"].startswith("/") + or not isinstance(contract["immutablePrefixes"], list) + or any(not _valid_immutable_prefix(value) + for value in contract["immutablePrefixes"]) + or len(contract["immutablePrefixes"]) != len(set(contract["immutablePrefixes"]))): raise RuntimeError(f"invalid site-publish route history in {path}") - try: - immutable_paths = json.loads(immutable_paths_json) - except json.JSONDecodeError as exc: - raise RuntimeError(f"invalid site-publish route history in {path}") from exc - if not isinstance(immutable_paths, list) or any(not isinstance(item, str) for item in immutable_paths): - raise RuntimeError(f"invalid site-publish route history in {path}") - if bucket in contracts: - raise RuntimeError(f"duplicate site-publish route history for bucket {bucket}") + return { + bucket: { + "access": contract["access"], + "artifact": contract["artifact"], + "immutable_prefixes": sorted(contract["immutablePrefixes"]), + "path": contract["routePath"], + } + for bucket, contract in document["buckets"].items() + } + + +def next_route_contracts(cfg, previous_contracts): + """Carry protected access and immutable prefixes forward for every known bucket.""" + contracts = json.loads(json.dumps(previous_contracts)) + artifacts = {artifact["name"]: artifact for artifact in cfg["artifacts"]} + for route in cfg["routes"]: + artifact = artifacts[route["artifact"]] + bucket = artifact["bucket"] + previous = previous_contracts.get(bucket) + access = "protected" if ( + route["access"] == "protected" or previous and previous["access"] == "protected" + ) else route["access"] contracts[bucket] = { - "path": route_path, "access": access, "artifact": artifact, - "immutable_paths": immutable_paths, + "access": access, + "artifact": route["artifact"], + "immutable_prefixes": sorted(set( + (previous or {}).get("immutable_prefixes", []) + immutable_prefixes(artifact, route) + )), + "path": route["path"], } return contracts +def write_route_contracts(app_dir, contracts): + app_dir.mkdir(parents=True, exist_ok=True) + document = { + "schemaVersion": 1, + "buckets": { + bucket: { + "access": contract["access"], + "artifact": contract["artifact"], + "immutablePrefixes": contract["immutable_prefixes"], + "routePath": contract["path"], + } + for bucket, contract in sorted(contracts.items()) + }, + } + (app_dir / HISTORY_FILE).write_text(f"{json.dumps(document, indent=2, sort_keys=True)}\n") + + +def _valid_immutable_prefix(value): + return (isinstance(value, str) and value and not value.startswith("/") + and not value.endswith("/") and ".." not in PurePosixPath(value).parts) + + def validate_route_migrations(cfg, previous_contracts): artifacts = {artifact["name"]: artifact for artifact in cfg["artifacts"]} for route in cfg["routes"]: @@ -413,6 +453,7 @@ def deploy_static(site_name, site_dir, action_dir, token, cfg): if cfg["compatibility"]: ensure_bucket_aliases(site_name, cfg["aliases"], os.environ.get("GARAGE_ADMIN_TOKEN")) + write_route_contracts(app_dir, next_route_contracts(cfg, previous_contracts)) render_site_manifests(site_name, action_dir, app_dir, manifests_dir, cfg) commit_and_push(apps_dir, f"Deploy {site_name}", token) @@ -425,7 +466,12 @@ def decommission(site_name, token, buckets=None): if not site_path.exists(): print(f"No manifests for {site_name} — nothing to remove") return + history_path = site_path / HISTORY_FILE + history = history_path.read_bytes() if history_path.exists() else None shutil.rmtree(site_path) + if history is not None: + site_path.mkdir(parents=True) + (site_path / HISTORY_FILE).write_bytes(history) commit_and_push(apps_dir, f"Decommission {site_name}", token) for bucket in buckets or [site_name]: print(f"Bucket {bucket} and its objects are NOT purged automatically.") diff --git a/templates/ingress.yaml.j2 b/templates/ingress.yaml.j2 index daa1c07..30ab0b9 100644 --- a/templates/ingress.yaml.j2 +++ b/templates/ingress.yaml.j2 @@ -4,13 +4,6 @@ metadata: name: {{ route.resource_name }} namespace: {{ namespace }} annotations: -{%- if not compatibility %} - site-publish.fritzlab.net/artifact: {{ route.artifact | tojson }} - site-publish.fritzlab.net/access: {{ route.access | tojson }} - site-publish.fritzlab.net/bucket: {{ route.artifact_config.bucket | tojson }} - site-publish.fritzlab.net/immutable-paths: {{ route.immutable_paths_json | tojson }} - site-publish.fritzlab.net/route-path: {{ route.path | tojson }} -{%- endif %} traefik.ingress.kubernetes.io/router.middlewares: https-redirect@file,retry-upstream@file{% if route.access_middleware %},{{ route.access_middleware }}@file{% endif %}{% for m in route.middlewares %},{{ m }}@file{% endfor %} spec: ingressClassName: traefik diff --git a/tests/test_contract.py b/tests/test_contract.py index b202901..8fbcb97 100644 --- a/tests/test_contract.py +++ b/tests/test_contract.py @@ -273,16 +273,17 @@ class GenerationTests(unittest.TestCase): self.assertNotIn("passhostheader", files["manifests/ingress-portal.yaml"]) self.assertIn("baseline-dist.web.sjc001.fritzlab.net", files["manifests/service-distributions.yaml"]) - def test_yaml_ambiguous_artifact_name_stays_a_string_annotation(self): + def test_history_keeps_yaml_ambiguous_artifact_names_as_strings(self): raw = fixture("split-site.yaml") raw["artifacts"][0]["name"] = "yes" raw["routes"][0]["artifact"] = "yes" - tmp, _, files = self.render(raw) - self.addCleanup(tmp.cleanup) - ingress = yaml.safe_load(files["manifests/ingress-portal.yaml"]) - self.assertEqual( - "yes", ingress["metadata"]["annotations"]["site-publish.fritzlab.net/artifact"], - ) + cfg = normalize_site_config(raw, "baseline.fritzlab.net") + contracts = deploy.next_route_contracts(cfg, {}) + with tempfile.TemporaryDirectory() as tmp: + app_dir = Path(tmp) + deploy.write_route_contracts(app_dir, contracts) + restored = deploy.previous_route_contracts(app_dir) + self.assertEqual("yes", restored["baseline-portal"]["artifact"]) def test_generation_is_deterministic_when_input_lists_are_reversed(self): raw = fixture("split-site.yaml") @@ -473,7 +474,7 @@ class PublishingTests(unittest.TestCase): ): deploy.s3_sync(artifact, route, root, previous_contract={ "path": "/foo", "access": "public", "artifact": "distributions", - "immutable_paths": ["releases"], + "immutable_prefixes": ["foo/releases"], }) rendered = [" ".join(command) for command in commands] self.assertTrue(all("foo/releases/*" in command for command in rendered[:2])) @@ -484,7 +485,7 @@ class PublishingTests(unittest.TestCase): previous = { "baseline-dist": { "path": "/dist", "access": "protected", "artifact": "old-name", - "immutable_paths": ["releases"], + "immutable_prefixes": ["dist/releases"], } } with self.assertRaisesRegex(RuntimeError, "cannot become public while reusing protected"): @@ -504,7 +505,7 @@ class PublishingTests(unittest.TestCase): previous = { "baseline.fritzlab.net": { "path": "/portal", "access": "protected", "artifact": "portal", - "immutable_paths": [], + "immutable_prefixes": [], } } with self.assertRaisesRegex(RuntimeError, "cannot become public while reusing protected"): @@ -521,17 +522,60 @@ class PublishingTests(unittest.TestCase): html = Path(tmp) filters = deploy.retired_immutable_filters(artifact, route, html, { "path": "/dist", "access": "public", "artifact": "distributions", - "immutable_paths": ["releases"], + "immutable_prefixes": ["dist/releases"], }) self.assertEqual(["--exclude", "releases/*"], filters) + previous = { + "baseline-dist": { + "path": "/dist", "access": "public", "artifact": "distributions", + "immutable_prefixes": ["dist/releases"], + } + } + next_contracts = deploy.next_route_contracts(cfg, previous) + self.assertEqual( + ["dist/releases"], next_contracts["baseline-dist"]["immutable_prefixes"], + ) + self.assertEqual( + ["--exclude", "releases/*"], + deploy.retired_immutable_filters( + artifact, route, html, next_contracts["baseline-dist"], + ), + ) (html / "releases").mkdir() (html / "releases" / "replacement.js").write_text("mutable") with self.assertRaisesRegex(RuntimeError, "collides with retired immutable"): deploy.retired_immutable_filters(artifact, route, html, { "path": "/dist", "access": "public", "artifact": "distributions", - "immutable_paths": ["releases"], + "immutable_prefixes": ["dist/releases"], }) + def test_removed_route_history_remains_append_only(self): + cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net") + previous = { + "retired-bucket": { + "path": "/retired", "access": "protected", "artifact": "retired", + "immutable_prefixes": ["retired/releases"], + } + } + contracts = deploy.next_route_contracts(cfg, previous) + self.assertEqual(previous["retired-bucket"], contracts["retired-bucket"]) + + def test_decommission_preserves_history_for_an_unpurged_bucket(self): + with tempfile.TemporaryDirectory() as tmp: + apps = Path(tmp) + site = apps / "sjc001/websites/baseline" + site.mkdir(parents=True) + history = b'{"schemaVersion":1,"buckets":{}}\n' + (site / deploy.HISTORY_FILE).write_bytes(history) + (site / "app.yaml").write_text("live\n") + with patch.object(deploy, "clone_apps", return_value=apps), patch.object( + deploy, "commit_and_push" + ) as commit: + deploy.decommission("baseline", "token", ["baseline-dist"]) + self.assertEqual(history, (site / deploy.HISTORY_FILE).read_bytes()) + self.assertFalse((site / "app.yaml").exists()) + commit.assert_called_once_with(apps, "Decommission baseline", "token") + def test_later_route_immutable_failure_stops_all_mutable_publication(self): cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net") with tempfile.TemporaryDirectory() as tmp: