fix(apps): refuse a compose edit that shrinks or drops a volume (#292)

API_CHANGELOG.md

@@ -36,6 +36,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Changed
 
+- **A managed app's persistent volume can grow but can no longer shrink, be removed or be renamed** (issue #292) — enforced when an app is updated through the admin API; `PATCH /api/admin/v1/apps/{id}` returns `400` when the incoming compose lowers a `volumes[].size`, or drops a `(service, volume name)` pair the stored compose declares. Kubernetes permits PVC expansion only, and the operator re-reads the catalog compose on every reconcile pass, so one shrinking edit puts every existing deployment of that app into a permanent `422 Forbidden: field can not be less than status.capacity` loop that the operator cannot walk back. Removals and renames fail differently and just as permanently: server-side apply cannot prune an object it merely stops applying, so the old PVC survives with the customer's data in it, unmounted and no longer counted against the app's storage — a rename is a remove plus an add, and orphans it the same way. Create is unaffected (there is no stored row to compare against), as is the operator, which keeps reconciling a row stored before this rule.
+
 - **A managed-app service addressed as `name:port` by another service must declare `ports:`** (issue #281) — enforced when an app is created or updated through the admin API; `POST`/`PATCH /api/admin/v1/apps` returns `400` for a compose that points at a peer with no ports. The operator creates a Kubernetes Service — the only source of an in-namespace DNS name — only for a service with declared ports, so `postgres://buzz:…@db:5432/buzz` against a `db` that declares none is a hostname that does not resolve. The Buzz catalog app reached production this way and failed on its first connection with `Name or service not known`; the same defect was latent in `route96`, whose `db` is addressed at `db:3306` and declared no ports. Both documented composes now carry internal port blocks (`expose: none`, so nothing becomes publicly reachable).
 
   Authoring-time only, like the `${…}` rule beside it: an app definition stored before this keeps reconciling unchanged and is caught the next time an admin edits it. The live catalog rows need the same edit, which no migration performs — and because the operator re-reads the app's compose on every pass, updating the row repairs an already-deployed instance without re-ordering it.

docs/managed-app-examples.md

@@ -217,6 +217,16 @@ starts the container as whatever non-root UID the image names — so the app com
 up and fails on its first write, on a fresh volume, every time. `user: root` is
 a valid answer: a root process can write to a root-owned volume.
 
+**A volume's `size` can be raised later but never lowered, and a volume can
+never be removed or renamed** (issue #292) — app update refuses all three,
+matched on `(service, volume name)` against the stored compose. Kubernetes
+permits PVC expansion only, so a smaller size makes every existing deployment
+of the app fail to reconcile with a `422` until the size is put back; and
+because the operator cannot prune a PVC it merely stops applying, dropping or
+renaming a volume leaves the old one behind holding the customer's data,
+unmounted. Pick names and sizes on the assumption that the first deployment
+freezes both.
+
 `user` also accepts a **numeric UID** (e.g. `user: "1000"`), which is required
 when the image's Dockerfile sets `USER` to a *name* rather than a number (e.g.
 `USER nonroot`). The kubelet enforces `runAsNonRoot` by reading the image

lnvps_api_admin/src/admin/apps.rs

@@ -204,6 +204,28 @@ fn validate_category(category: String) -> Result<String, lnvps_api_common::ApiEr
     Ok(category.to_string())
 }
 
+/// Refuse an edit that shrinks, drops or renames a persistent volume (#292).
+///
+/// Update only: there is no stored row to compare against on create, and an
+/// app's first compose declares whatever it likes.
+///
+/// A stored compose that no longer parses is skipped rather than fatal —
+/// nothing can be compared against it, and refusing the edit would leave an
+/// admin unable to repair the very row that is broken.
+fn validate_volume_changes(
+    stored: &str,
+    incoming: &str,
+) -> Result<(), lnvps_api_common::ApiError> {
+    let Ok(stored) = lnvps_compose::Compose::parse(stored) else {
+        return Ok(());
+    };
+    let incoming = lnvps_compose::Compose::parse(incoming)
+        .map_err(|e| lnvps_api_common::ApiError::new(format!("invalid compose: {e}")))?;
+    incoming
+        .validate_volume_changes(&stored)
+        .map_err(|e| lnvps_api_common::ApiError::new(format!("invalid compose: {e}")))
+}
+
 /// Parse the compose and compute the app's resource footprint (already
 /// validated by `validate_app_fields`).
 fn compose_footprint(
@@ -328,6 +350,7 @@ async fn admin_update_app(
 ) -> ApiResult<AdminAppInfo> {
     auth.require_permission(AdminResource::App, AdminAction::Update)?;
     let mut app = this.db.get_app(id).await?;
+    let stored_compose = app.compose.clone();
 
     if let Some(name) = req.name {
         app.name = name.trim().to_string();
@@ -380,6 +403,7 @@ async fn admin_update_app(
     }
 
     validate_app_fields(&app.name, &app.display_name, &app.compose, &app.currency)?;
+    validate_volume_changes(&stored_compose, &app.compose)?;
     // Resolve before any write, so an unknown slug leaves the app untouched
     // rather than half-applying the rest of the patch.
     let tag_ids = match &req.tags {

lnvps_compose/src/lib.rs

@@ -751,6 +751,56 @@ impl Compose {
         Ok(())
     }
 
+    /// Check this compose against the one it would replace: a persistent
+    /// volume may grow, but it may not shrink, vanish or be renamed.
+    ///
+    /// An authoring rule, checked at admission only, like
+    /// [`Self::validate_declarations`]: a row that already violates it has to
+    /// keep reconciling rather than disappear.
+    ///
+    /// Volumes are matched on `(service, volume name)` — the PVC identity the
+    /// operator builds — and compared in bytes, so a change of unit alone
+    /// passes. An unparseable *previous* document is the caller's problem, not
+    /// this function's.
+    pub fn validate_volume_changes(&self, previous: &Compose) -> Result<()> {
+        for (sname, prev_svc) in &previous.services {
+            for prev in &prev_svc.volumes {
+                let prev_bytes = parse_bytes(&prev.size).map_err(|e| {
+                    anyhow!("stored service '{sname}': volume '{}': {e}", prev.name)
+                })?;
+                let current = self
+                    .services
+                    .get(sname)
+                    .and_then(|s| s.volumes.iter().find(|v| v.name == prev.name));
+                let Some(current) = current else {
+                    bail!(
+                        "service '{sname}': volume '{}' is missing — the operator cannot prune \
+                         a PVC it stops applying, so it would survive unmounted with the \
+                         customer's data in it. A rename is a remove plus an add and orphans it \
+                         the same way.",
+                        prev.name
+                    );
+                };
+                let bytes = parse_bytes(&current.size)
+                    .map_err(|e| anyhow!("service '{sname}': volume '{}': {e}", current.name))?;
+                if bytes < prev_bytes {
+                    bail!(
+                        "service '{sname}': volume '{}' shrinks from {} to {} — Kubernetes permits \
+                         PVC expansion only, so every existing deployment of this app would fail \
+                         to reconcile with a 422 until the size is put back. The floor checked \
+                         here is this row's stored size; the real floor is whatever capacity the \
+                         PVCs were provisioned at, which can be larger if the row was lowered \
+                         before this rule existed.",
+                        current.name,
+                        prev.size.trim(),
+                        current.size.trim()
+                    );
+                }
+            }
+        }
+        Ok(())
+    }
+
     /// Validate structural + policy rules. Enforced at parse time so the
     /// operator never tries to render an unsafe or malformed app.
     ///
@@ -2239,6 +2289,88 @@ config:
         assert!(Compose::parse(&svc("")).is_ok());
     }
 
+    /// A stored persistent volume may grow, but it may not shrink, vanish or
+    /// be renamed (#292) — each of those is unrecoverable once a deployment
+    /// exists.
+    #[test]
+    fn volumes_may_grow_but_not_shrink_or_vanish() {
+        let app = |vols: &str| {
+            Compose::parse(&format!(
+                "services:\n  db:\n    image: x\n    user: \"1000\"\n    volumes:\n{vols}"
+            ))
+            .unwrap()
+        };
+        let stored = app("      - { name: data, path: /data, size: 5Gi }\n");
+
+        // Same size, and a larger one, both pass.
+        assert!(
+            app("      - { name: data, path: /data, size: 5Gi }\n")
+                .validate_volume_changes(&stored)
+                .is_ok()
+        );
+        assert!(
+            app("      - { name: data, path: /data, size: 20Gi }\n")
+                .validate_volume_changes(&stored)
+                .is_ok()
+        );
+        // Compared in bytes, so a change of unit alone is a no-op.
+        assert!(
+            app("      - { name: data, path: /data, size: 5120Mi }\n")
+                .validate_volume_changes(&stored)
+                .is_ok()
+        );
+        // A new volume beside the old one is fine: nothing existing moves.
+        assert!(
+            app(
+                "      - { name: data, path: /data, size: 5Gi }\n      \
+                 - { name: logs, path: /logs, size: 1Gi }\n"
+            )
+            .validate_volume_changes(&stored)
+            .is_ok()
+        );
+
+        // Shrink: the 422 loop.
+        let err = app("      - { name: data, path: /data, size: 1Gi }\n")
+            .validate_volume_changes(&stored)
+            .unwrap_err()
+            .to_string();
+        assert!(err.contains("shrinks from 5Gi to 1Gi"), "{err}");
+
+        // Removal: the PVC survives unmounted, holding the customer's data.
+        let err = Compose::parse("services:\n  db:\n    image: x\n")
+            .unwrap()
+            .validate_volume_changes(&stored)
+            .unwrap_err()
+            .to_string();
+        assert!(err.contains("volume 'data' is missing"), "{err}");
+
+        // Rename is a remove plus an add, and orphans the old PVC the same way.
+        let err = app("      - { name: store, path: /data, size: 5Gi }\n")
+            .validate_volume_changes(&stored)
+            .unwrap_err()
+            .to_string();
+        assert!(err.contains("volume 'data' is missing"), "{err}");
+
+        // Volumes are keyed by (service, name): moving one to another service
+        // is a different PVC, so it counts as dropping the original.
+        let moved = Compose::parse(
+            "services:\n  cache:\n    image: x\n    user: \"1000\"\n    volumes:\n      \
+             - { name: data, path: /data, size: 5Gi }\n",
+        )
+        .unwrap();
+        assert!(moved.validate_volume_changes(&stored).is_err());
+
+        // Authoring-time only: `validate()` still accepts the shrunk document,
+        // so a row stored before this rule keeps reconciling.
+        assert!(
+            Compose::parse(
+                "services:\n  db:\n    image: x\n    user: \"1000\"\n    volumes:\n      \
+                 - { name: data, path: /data, size: 1Gi }\n"
+            )
+            .is_ok()
+        );
+    }
+
     #[test]
     fn rejects_empty_and_bad_refs() {
         assert!(Compose::parse("services: {}\n").is_err());

lnvps_e2e/src/admin_api.rs

@@ -1179,6 +1179,93 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::OK);
     }
 
+    /// A catalog edit may grow a persistent volume but not shrink, drop or
+    /// rename one (#292). Kubernetes permits PVC expansion only, and the
+    /// operator cannot prune a PVC it stops applying, so each of those puts
+    /// every existing deployment of the app somewhere the API cannot walk back.
+    #[tokio::test]
+    async fn test_admin_app_compose_volume_may_not_shrink() {
+        let client = setup().await;
+        let suffix = nostr::Keys::generate().public_key().to_hex()[..8].to_string();
+        let slug = format!("e2e-vol-shrink-{suffix}");
+
+        let compose = |vol: &str| {
+            format!(
+                "services:\n  db:\n    image: example/db:latest\n    user: \"1000\"\n    \
+                 volumes:\n      - {vol}\n"
+            )
+        };
+        let patch = |vol: &str| serde_json::json!({ "compose": compose(vol) });
+
+        let resp = client
+            .post_auth(
+                "/api/admin/v1/apps",
+                &serde_json::json!({
+                    "name": slug,
+                    "display_name": "Volumes Only Grow",
+                    "category": "Nostr relay",
+                    "compose": compose("{ name: data, path: /data, size: 5Gi }"),
+                    "amount": 1000,
+                    "currency": "usd",
+                    "interval_amount": 1,
+                    "interval_type": "month",
+                    "setup_amount": 0
+                }),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+        let created: Value = serde_json::from_str(&resp.text().await.unwrap()).unwrap();
+        let app_id = created["data"]["id"].as_u64().expect("app id");
+        let path = format!("/api/admin/v1/apps/{app_id}");
+
+        let resp = client
+            .patch_auth(&path, &patch("{ name: data, path: /data, size: 1Gi }"))
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+        let err = resp.text().await.unwrap();
+        assert!(err.contains("shrinks from 5Gi to 1Gi"), "{err}");
+
+        // Dropping the volume, and renaming it, are refused the same way: the
+        // old PVC survives with the customer's data in it either way.
+        let resp = client
+            .patch_auth(
+                &path,
+                &serde_json::json!({
+                    "compose": "services:\n  db:\n    image: example/db:latest\n"
+                }),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+        let err = resp.text().await.unwrap();
+        assert!(err.contains("volume 'data' is missing"), "{err}");
+
+        let resp = client
+            .patch_auth(&path, &patch("{ name: store, path: /data, size: 5Gi }"))
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        // Growing is the whole point of allowing the edit at all...
+        let resp = client
+            .patch_auth(&path, &patch("{ name: data, path: /data, size: 20Gi }"))
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+
+        // ...and the new size is the floor from then on.
+        let resp = client
+            .patch_auth(&path, &patch("{ name: data, path: /data, size: 5Gi }"))
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        let resp = client.delete_auth(&path).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+    }
+
     /// A generated secret can declare its byte length (issue #243), and an
     /// unusable length is refused when the app is created or updated rather
     /// than becoming an app that deploys and crash-loops on its own key.