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