From 8a1a2426d5da4bde74175ae024c39f1afb88a988 Mon Sep 17 00:00:00 2001 From: tegwick Date: Tue, 8 Sep 2026 08:56:59 +0200 Subject: [PATCH] Service fixes found by the first real deployment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects the test suite could not have caught, because each needed a real cluster, a mounted secret, or a live PostgreSQL. env.py read database_url rather than resolved_database_url. `configured` was true because a file was set, and the field it then read was empty — so Alembic received an empty URL and the migration could never run in the cluster. The value is also now escaped for ConfigParser interpolation, since a `%` in a generated password would otherwise raise at credential rotation, which is the worst time to find out. SET ROLE opened an implicit transaction that Alembic then nested inside rather than owning, so it never committed and leaving the connection block rolled everything back. Alembic logged "Running upgrade" for every revision against a database that stayed empty. SET ROLE is session-scoped, so committing immediately ends the implicit transaction without discarding the role. A missing optional publish-token file was treated as a hard failure. The absence is the documented read-only posture — the secret is mounted optional and deliberately not issued — so treating it as a fault turned an intended state into a 500 rather than the 503 that explains it. `required` now separates the two cases: a missing database URL still fails loudly, because there the silence would hide a real fault. Migrations also assume the durable owner role rather than creating objects as the leased migration login, per the rapp-postgres database-owner boundary. The role name is validated against an identifier pattern because SET ROLE cannot be parameterised. Service tests 36 -> 47. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bjefh8NUiEiahN4JLwoSKM Assistant: claude-code Assistant-Model: opus Assistant-Process: 388925@bnt-lap001 Assistant-Session: 3507023f-e0fd-4a1e-9d90-a0d4217d1502 --- service/README.md | 23 ++++++ service/migrations/env.py | 16 +++- service/pyproject.toml | 2 +- .../src/canned_prompts_service/__init__.py | 2 +- service/src/canned_prompts_service/db.py | 37 +++++++++ .../src/canned_prompts_service/settings.py | 16 +++- service/tests/test_health.py | 15 ++++ service/tests/test_schema.py | 82 +++++++++++++++++++ 8 files changed, 186 insertions(+), 7 deletions(-) diff --git a/service/README.md b/service/README.md index 073244a..667bc00 100644 --- a/service/README.md +++ b/service/README.md @@ -152,6 +152,29 @@ properties of the deployment rather than of this process, and belong to `--expect-migration` matters: without it the check can only confirm the schema is stamped at all, and it says so rather than implying it verified the head. +## Deployed + +Running on railiance01 since 2026-09-08 via `rapp-canned-prompts`, image tag +0.1.4, schema at alembic `0002`, read-only by design. + +Four defects surfaced during that first rollout that the test suite could not +have caught, because each needed a real cluster, a mounted secret, or a live +PostgreSQL: + +- `env.py` read `database_url` rather than `resolved_database_url`, so the + migration could not run where the credential is a file; +- `SET ROLE` opened an implicit transaction that Alembic then **nested inside + rather than owning**, so every revision logged as applied and was rolled + back — success reported against an empty database; +- a missing *optional* publish-token file was treated as a hard failure, so the + documented read-only posture returned 500 instead of an explanatory 503; +- and separately in the rapp, an egress NetworkPolicy that did not select the + migration Job at all. + +Each now has a regression test. The transaction one is the most worth knowing: +touching an Alembic connection before Alembic does changes who owns the +transaction, and the failure is silent in both directions. + ## Status `CANP-WP-0006` is complete: skeleton and health surface, tenant-keyed schema diff --git a/service/migrations/env.py b/service/migrations/env.py index c336c13..02d021c 100644 --- a/service/migrations/env.py +++ b/service/migrations/env.py @@ -9,7 +9,7 @@ from __future__ import annotations from alembic import context from sqlalchemy import engine_from_config, pool -from canned_prompts_service.db import Base +from canned_prompts_service.db import Base, assume_owner_role from canned_prompts_service.settings import get_settings from canned_prompts_service import models # noqa: F401 — registers the tables @@ -18,7 +18,15 @@ target_metadata = Base.metadata settings = get_settings() if settings.configured: - config.set_main_option("sqlalchemy.url", settings.database_url) + # resolved_database_url, not database_url: in the cluster the credential + # arrives as a mounted file and the plain field is empty. + # + # The value is escaped because set_main_option interpolates through + # ConfigParser, so a `%` in a generated password would otherwise raise — + # at credential rotation, which is the worst time to discover it. + config.set_main_option( + "sqlalchemy.url", settings.resolved_database_url.replace("%", "%%") + ) def run_migrations_offline() -> None: @@ -38,9 +46,13 @@ def run_migrations_online() -> None: poolclass=pool.NullPool, ) with connectable.connect() as connection: + assume_owner_role(connection) context.configure(connection=connection, target_metadata=target_metadata) with context.begin_transaction(): context.run_migrations() + # Belt and braces: if anything else ever touches this connection before + # Alembic, the same nesting trap reappears silently. + connection.commit() if context.is_offline_mode(): diff --git a/service/pyproject.toml b/service/pyproject.toml index 18ba2fa..589d3fe 100644 --- a/service/pyproject.toml +++ b/service/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "canned-prompts-service" -version = "0.1.0" +version = "0.1.4" description = "Hosted registry and index service for Canned Prompt Format packages" requires-python = ">=3.12" dependencies = [ diff --git a/service/src/canned_prompts_service/__init__.py b/service/src/canned_prompts_service/__init__.py index 3aa13a4..3870b88 100644 --- a/service/src/canned_prompts_service/__init__.py +++ b/service/src/canned_prompts_service/__init__.py @@ -8,4 +8,4 @@ boundary holds, so rendering stays deterministic and a `derive` default remains a declaration the service does not satisfy. """ -__version__ = "0.1.0" +__version__ = "0.1.4" diff --git a/service/src/canned_prompts_service/db.py b/service/src/canned_prompts_service/db.py index 9f8cede..1482320 100644 --- a/service/src/canned_prompts_service/db.py +++ b/service/src/canned_prompts_service/db.py @@ -8,6 +8,9 @@ from __future__ import annotations from dataclasses import dataclass +import os +import re + from sqlalchemy import create_engine, text from sqlalchemy.engine import Engine from sqlalchemy.orm import DeclarativeBase, Session, sessionmaker @@ -62,3 +65,37 @@ def check_readiness(engine: Engine | None) -> Readiness: if revision is None: return Readiness(False, "schema not migrated") return Readiness(True, "ok", revision) + + +# PostgreSQL identifiers only. The role name is interpolated into SET ROLE, +# which cannot be parameterised, so it is validated rather than trusted. +ROLE_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def assume_owner_role(connection, role: str | None = None) -> str | None: + """Create objects as the durable owner role, not the leased migration login. + + Migration credentials are short-lived leases. An object left owned by a + login that is later revoked has to be normalized by the platform afterwards, + so the migration authenticates as its own role and immediately assumes the + owner. Required by the rapp-postgres database-owner boundary. + """ + role = (role if role is not None else os.environ.get("CANNED_PROMPTS_MIGRATION_ROLE", "")).strip() + if not role: + return None + if not ROLE_RE.match(role): + raise ValueError(f"invalid migration role name: {role!r}") + connection.execute(text(f'SET ROLE "{role}"')) + # Commit immediately, and not as a formality. + # + # Executing anything on the connection opens an implicit transaction under + # SQLAlchemy 2.0. Alembic's own begin_transaction() then nests inside it + # instead of owning it, so it never commits — and leaving the connection + # block rolls the migration back. Alembic logs "Running upgrade" for every + # revision and the database ends up empty, which is a genuinely confusing + # way to fail. + # + # SET ROLE is session-scoped, so committing here ends the implicit + # transaction without discarding the role. + connection.commit() + return role diff --git a/service/src/canned_prompts_service/settings.py b/service/src/canned_prompts_service/settings.py index d374293..baad976 100644 --- a/service/src/canned_prompts_service/settings.py +++ b/service/src/canned_prompts_service/settings.py @@ -33,11 +33,21 @@ class Settings(BaseSettings): publish_token_file: str = "" - def _read_secret(self, path: str) -> str: + def _read_secret(self, path: str, *, required: bool = True) -> str: + """Read a mounted secret. + + `required` separates two cases that look identical on disk. A missing + database URL is a misconfiguration and must fail loudly. A missing + publish token is the *documented* read-only posture — the secret is + mounted `optional: true` and is deliberately not issued — so treating + its absence as a fault turns an intended state into a 500. + """ try: return Path(path).read_text(encoding="utf-8").strip() except OSError as exc: - raise RuntimeError(f"cannot read secret file {path}: {exc}") from exc + if required: + raise RuntimeError(f"cannot read secret file {path}: {exc}") from exc + return "" @property def resolved_database_url(self) -> str: @@ -51,7 +61,7 @@ class Settings(BaseSettings): @property def resolved_publish_token(self) -> str: if self.publish_token_file: - return self._read_secret(self.publish_token_file) + return self._read_secret(self.publish_token_file, required=False) return self.publish_token @property diff --git a/service/tests/test_health.py b/service/tests/test_health.py index 7bb197c..3ef50d5 100644 --- a/service/tests/test_health.py +++ b/service/tests/test_health.py @@ -123,3 +123,18 @@ def test_unreadable_secret_file_fails_loudly(tmp_path: Path) -> None: settings = Settings(database_url_file=str(tmp_path / "missing")) with pytest.raises(RuntimeError, match="cannot read secret file"): _ = settings.resolved_database_url + + +def test_absent_publish_token_file_means_read_only_not_broken(tmp_path: Path) -> None: + """The token secret is mounted optional and deliberately not issued. Its + absence is the documented read-only posture, not a fault — treating it as + one returned 500 from /packages instead of a 503 explaining why.""" + settings = Settings(publish_token_file=str(tmp_path / "absent")) + assert settings.resolved_publish_token == "" + + +def test_absent_database_file_still_fails_loudly(tmp_path: Path) -> None: + """The database URL is required; silence there would hide a real fault.""" + settings = Settings(database_url_file=str(tmp_path / "absent")) + with pytest.raises(RuntimeError, match="cannot read secret file"): + _ = settings.resolved_database_url diff --git a/service/tests/test_schema.py b/service/tests/test_schema.py index 2873cc0..3992c21 100644 --- a/service/tests/test_schema.py +++ b/service/tests/test_schema.py @@ -23,3 +23,85 @@ def test_index_entry_records_arrival_not_authorship() -> None: columns = models.IndexEntry.__table__.c assert "included_at" in columns and "last_seen_at" in columns assert "source" in columns and "method" in columns + + +# --- migration role assumption (rapp-postgres boundary requirement) --- + +import pytest + +from canned_prompts_service import db as service_db + + +class FakeConnection: + def __init__(self) -> None: + self.statements: list[str] = [] + self.commits = 0 + + def execute(self, statement) -> None: + self.statements.append(str(statement)) + + def commit(self) -> None: + self.commits += 1 + + +def test_no_role_assumed_when_unset(monkeypatch) -> None: + monkeypatch.delenv("CANNED_PROMPTS_MIGRATION_ROLE", raising=False) + conn = FakeConnection() + service_db.assume_owner_role(conn) + assert conn.statements == [] + + +def test_owner_role_is_assumed(monkeypatch) -> None: + """Objects must end up owned by the durable role, not a leased login.""" + monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", "canned_prompts_owner") + conn = FakeConnection() + service_db.assume_owner_role(conn) + assert conn.statements == ['SET ROLE "canned_prompts_owner"'] + + +@pytest.mark.parametrize("bad", ['x"; drop schema public; --', "role-with-dash", "1role", ""]) +def test_role_name_is_validated_not_trusted(monkeypatch, bad) -> None: + """SET ROLE cannot be parameterised, so the name is validated.""" + monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", bad) + conn = FakeConnection() + if bad == "": + service_db.assume_owner_role(conn) + assert conn.statements == [] + else: + with pytest.raises(ValueError, match="invalid migration role"): + service_db.assume_owner_role(conn) + + +def test_configured_but_env_field_empty_is_the_cluster_case() -> None: + """The bug that broke the first migration: `configured` was true because a + file was set, while the plain field it then read was empty.""" + from canned_prompts_service.settings import Settings + import tempfile, os + + with tempfile.NamedTemporaryFile("w", suffix=".url", delete=False) as f: + f.write("postgresql+psycopg://u:p%40x@h/db") + path = f.name + settings = Settings(database_url_file=path) + assert settings.configured is True + assert settings.database_url == "" + assert settings.resolved_database_url.startswith("postgresql+psycopg://") + # A percent in the password must survive ConfigParser interpolation. + assert settings.resolved_database_url.replace("%", "%%").count("%%") == 1 + os.unlink(path) + + +def test_set_role_commits_so_alembic_owns_its_transaction(monkeypatch) -> None: + """Regression: SET ROLE opened an implicit transaction that Alembic then + nested inside rather than owning, so every migration was rolled back while + logging as applied.""" + monkeypatch.setenv("CANNED_PROMPTS_MIGRATION_ROLE", "canned_prompts_owner") + conn = FakeConnection() + service_db.assume_owner_role(conn) + assert conn.commits == 1, "SET ROLE must not leave an open transaction" + + +def test_no_commit_when_no_role_is_assumed(monkeypatch) -> None: + monkeypatch.delenv("CANNED_PROMPTS_MIGRATION_ROLE", raising=False) + conn = FakeConnection() + service_db.assume_owner_role(conn) + assert conn.commits == 0