From 0d855b34366a7f3b726df7009ba85536af1f7f43 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 10 Sep 2026 19:27:47 +0000 Subject: [PATCH] :sparkles: Add per-bucket S3 target routing to asset storage Introduce optional named S3 targets and route internal semantic buckets (for example tempfile) to them, keeping the fs/s3 backend choice and the storage_object.backend value unchanged. Targets and routes are read from an EDN file referenced by PENPOT_OBJECTS_STORAGE_S3_ROUTES_FILE. The chosen target id is stored in the object metadata as :storage-target and resolved on reads, URL signing, deduplication, individual deletes and both GC tasks. The implicit :default target is built from PENPOT_OBJECTS_STORAGE_S3_*, so behavior is unchanged when the file is absent. Bulk deletion now carries the target, and one S3 client/presigner pair is shared per distinct region/endpoint. Closes #11630 AI-assisted-by: deepseek-flash --- .serena/memories/backend/storage.md | 19 +- backend/src/app/config.clj | 1 + backend/src/app/main.clj | 10 +- backend/src/app/storage.clj | 27 ++- backend/src/app/storage/config.clj | 140 ++++++++++++ backend/src/app/storage/fs.clj | 2 +- backend/src/app/storage/gc_deleted.clj | 21 +- backend/src/app/storage/impl.clj | 10 +- backend/src/app/storage/pending_gc.clj | 7 +- backend/src/app/storage/s3.clj | 151 +++++++++--- .../backend_tests/storage_config_test.clj | 81 +++++++ backend/test/backend_tests/storage_test.clj | 215 +++++++++++++++++- docker/images/docker-compose.yaml | 5 + docs/technical-guide/configuration.md | 38 ++++ .../developer/subsystems/assets-storage.md | 16 +- 15 files changed, 678 insertions(+), 65 deletions(-) create mode 100644 backend/src/app/storage/config.clj create mode 100644 backend/test/backend_tests/storage_config_test.clj diff --git a/.serena/memories/backend/storage.md b/.serena/memories/backend/storage.md index 101c77702a..49bc3a46b3 100644 --- a/.serena/memories/backend/storage.md +++ b/.serena/memories/backend/storage.md @@ -8,13 +8,26 @@ - The backend stores the binary content. - Supported backends are `:fs` and `:s3`. - FS uses one root directory and a UUID-derived path. -- S3 uses one configured bucket and an optional prefix. +- S3 uses a default configured bucket and an optional prefix, plus optional named targets. - A Penpot bucket is metadata. It is not an S3 bucket or a filesystem directory. - FS and S3 use the same UUID-derived object path. The bucket does not change the path. - `PENPOT_OBJECTS_STORAGE_*` configures the current object backend. - Deprecated asset-storage config keys remain supported for migration. - Database rows keep the backend name. Keep the legacy `:assets-fs` and `:assets-s3` aliases. +## S3 Targets and Routing + +- The `:s3` backend keeps `storage_object.backend = 's3'`; routing lives inside the backend. +- Routing maps a Penpot semantic bucket to a named target (own bucket, optional prefix/region/endpoint). +- Targets are declared in an EDN file referenced by `PENPOT_OBJECTS_STORAGE_S3_ROUTES_FILE` (`app.storage.config/load`). +- Schema: `{:targets { {:bucket ... :prefix? ... :region? ... :endpoint? ...}} :routes {"" }}`. +- The reserved `:default` target is implicit and built from `PENPOT_OBJECTS_STORAGE_S3_*`; declared targets inherit missing region/endpoint/prefix from it. +- Without a routes file, `::sto/bucket->target` is nil and every object uses `:default` (unchanged behavior). +- The chosen target id is stored in object metadata as `:storage-target` (plain string) by `put-object!`. +- `app.storage.s3/resolve-target` reads the object metadata; missing/unknown ids fall back to `:default`. +- One S3 client/presigner is built per distinct `[region endpoint]` and shared by targets. +- Target ids are logged in metadata, so they must stay stable; removing one makes its old rows unreadable/unGC-able. + ## Object Lifecycle - `put-object!` creates the database row before it writes backend content. @@ -64,7 +77,7 @@ Since `put-object!` uses backend-specific operations (`impl/resolve-backend` + ` ## Deduplication - Deduplication requires `::sto/deduplicate?`, a content hash, and bucket metadata. -- The lookup matches hash, bucket, backend, and `deleted_at IS NULL`. +- The lookup matches hash, bucket, backend, storage target (`:storage-target`, coalesced to `default`), and `deleted_at IS NULL`. - The lookup only considers rows with `status='valid'`; pending rows are invisible. - A hit whose blob is missing is repaired in place: the same row/id is kept, and `put-object!` rewrites the blob under that id. This heals all existing @@ -93,6 +106,8 @@ Since `put-object!` uses backend-specific operations (`impl/resolve-backend` + ` - The valid bucket set lives in `app.storage/valid-buckets`. - `file-media-object` is the default bucket for old rows without bucket metadata. +- Under `:s3`, any valid bucket may be routed to a named target; unrouted buckets use `:default`. +- GC resolves the target from `metadata.:storage-target` for deleted and pending rows. - Do not assign a new bucket without adding its access and cleanup behavior. - The touched-object collector raises an internal error for an unknown bucket. - It supports `file-media-object`, `team-font-variant`, `file-object-thumbnail`, `file-thumbnail`, `profile`, `file-data`, `tempfile`, and `organization`. diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index d338bb4813..c12e27f57f 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -293,6 +293,7 @@ [:objects-storage-s3-bucket {:optional true} :string] [:objects-storage-s3-region {:optional true} :keyword] [:objects-storage-s3-endpoint {:optional true} ::sm/uri] + [:objects-storage-s3-routes-file {:optional true} :string] ;; SSRF protection [:ssrf-allowed-hosts {:optional true} [::sm/set :string]] diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index d1ca759df9..e814dbe514 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -34,6 +34,7 @@ [app.setup :as-alias setup] [app.srepl :as-alias srepl] [app.storage :as-alias sto] + [app.storage.config :as sto.config] [app.storage.fs :as-alias sto.fs] [app.storage.gc-deleted :as-alias sto.gc-deleted] [app.storage.gc-touched :as-alias sto.gc-touched] @@ -148,6 +149,11 @@ ::mdef/labels [] ::mdef/type :histogram}}) +(def ^:private storage-routing + "Optional S3 storage targets and per semantic-bucket routing. Loaded once + from the external routes file. Empty when the feature is not configured." + (sto.config/load)) + (def system-config {::db/pool {::db/uri (cf/get :database-uri) @@ -522,7 +528,8 @@ ;; explicit migration because the database objects/rows will ;; still reference the old names). :assets-s3 (ig/ref :app.storage.s3/backend) - :assets-fs (ig/ref :app.storage.fs/backend)}} + :assets-fs (ig/ref :app.storage.fs/backend)} + ::sto/bucket->target (:routes storage-routing)} :app.storage.s3/backend {::sto.s3/region (or (cf/get :storage-assets-s3-region) @@ -533,6 +540,7 @@ (cf/get :objects-storage-s3-bucket)) ::sto.s3/io-threads (or (cf/get :storage-assets-s3-io-threads) (cf/get :objects-storage-s3-io-threads)) + ::sto.s3/targets (:targets storage-routing) ::wrk/netty-io-executor (ig/ref ::wrk/netty-io-executor)} diff --git a/backend/src/app/storage.clj b/backend/src/app/storage.clj index 0f35b0a54a..8f8f2b83a0 100644 --- a/backend/src/app/storage.clj +++ b/backend/src/app/storage.clj @@ -70,6 +70,7 @@ [:map {:title "storage"} [::backends schema:backends] [::backend [:enum :s3 :fs]] + [::bucket->target {:optional true} [:map-of :string :keyword]] ::db/pool]) (def valid-storage? @@ -112,17 +113,18 @@ params)) (defn- get-database-object-by-hash - [connectable backend bucket hash] + [connectable backend bucket target hash] (let [sql (str "select * from storage_object " " where (metadata->>'~:hash') = ? " " and (metadata->>'~:bucket') = ? " + " and coalesce(metadata->>'~:storage-target', 'default') = ? " " and backend = ?" " and deleted_at is null" " and status = 'valid'" " limit 1")] ;; NOTE: metadata is left encoded; row->storage-object is ;; responsible for decoding it. - (db/exec-one! connectable [sql hash bucket (name backend)]))) + (db/exec-one! connectable [sql hash bucket target (name backend)]))) (defn- promote-object! [storage object] @@ -185,6 +187,13 @@ (let [ds (db/get-connectable storage)] (get-database-object ds id))) +(defn- resolve-target-id + "Returns the storage target id for the given semantic bucket, or nil when + the routing does not apply (non-S3 backends)." + [storage bucket] + (when (= :s3 (::backend storage)) + (get (::bucket->target storage) bucket :default))) + (defn put-object! "Creates a new object with the provided content." [{:keys [::backend ::db/pool] :as storage} @@ -193,9 +202,16 @@ (assert (impl/content? content) "expected an instance of content") (let [id (or (::id params) (uuid/random)) - mdata (cond-> (get-metadata params) + base-mdata (get-metadata params) + bucket (:bucket base-mdata) + target (resolve-target-id storage bucket) + target-str (or (some-> target name) "default") + mdata (cond-> base-mdata (satisfies? impl/IContentHash content) - (assoc :hash (impl/get-hash content))) + (assoc :hash (impl/get-hash content)) + + (some? target) + (assoc :storage-target target-str)) touched-at (if touch (or touched-at (ct/now)) @@ -214,10 +230,11 @@ (not= tempfile-bucket (:bucket mdata))) (get-database-object-by-hash pool backend (:bucket mdata) + target-str (:hash mdata)))] ;; PHASE 2: an existing reference is found: reuse or repair it. - (if (impl/exists-object? backend' hit) + (if (impl/exists-object? backend' (row->storage-object hit)) ;; PHASE 2a: healthy reference. Optionally refresh touched_at ;; and reuse the object as it is. diff --git a/backend/src/app/storage/config.clj b/backend/src/app/storage/config.clj new file mode 100644 index 0000000000..b6e77e05f6 --- /dev/null +++ b/backend/src/app/storage/config.clj @@ -0,0 +1,140 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns app.storage.config + "Configuration of the optional S3 storage targets and their per + semantic-bucket routing. + + The routing is read from an external EDN file referenced by the + `PENPOT_OBJECTS_STORAGE_S3_ROUTES_FILE` environment variable. The file + declares additional S3 targets and, optionally, a map from Penpot + semantic bucket to target id: + + {:targets + {:temp {:bucket \"penpot-temp\"} + :cold {:bucket \"penpot-cold\" :region :us-east-1}} + :routes + {\"tempfile\" :temp + \"file-data\" :temp}} + + The implicit `:default` target always exists and is built from the + existing `PENPOT_OBJECTS_STORAGE_S3_*` configuration, so the feature is + fully backward compatible when the file is absent." + (:refer-clojure :exclude [load]) + (:require + [app.common.exceptions :as ex] + [app.common.schema :as sm] + [app.common.uri :as u] + [app.config :as cf] + [app.storage :as sto] + [clojure.edn :as edn] + [clojure.java.io :as io])) + +(def ^:private schema:target + [:map {:title "s3-target"} + [:bucket :string] + [:region {:optional true} :keyword] + [:endpoint {:optional true} [:or :string ::sm/uri]] + [:prefix {:optional true} :string]]) + +(def ^:private schema:file + [:map {:title "storage-routes"} + [:targets [:map-of :keyword schema:target]] + [:routes {:optional true} [:map-of :string :keyword]]]) + +(def ^:private valid-file? + (sm/validator schema:file)) + +(def ^:private explain-file + (sm/explainer schema:file)) + +(defn- read-file + [path] + (try + (-> (io/file path) slurp (edn/read-string)) + (catch Exception cause + (ex/raise :type :validation + :code :invalid-storage-routes-file + :hint "unable to read storage routes file" + :path (str path) + :cause cause)))) + +(defn- assert-backend-s3! + "The routing only makes sense on top of the S3 backend." + [] + (let [backend (or (sto/get-legacy-backend) + (cf/get :objects-storage-backend) + :fs)] + (when-not (= :s3 backend) + (ex/raise :type :validation + :code :invalid-storage-routes-backend + :hint "storage routes file requires the :s3 backend" + :backend (keyword backend))))) + +(defn- validate! + [data path] + (when-not (valid-file? data) + (ex/raise :type :validation + :code :invalid-storage-routes + :hint "invalid storage routes file" + :path (str path) + :explain (explain-file data))) + + (let [targets (:targets data) + routes (:routes data)] + + (when (contains? targets :default) + (ex/raise :type :validation + :code :reserved-storage-target + :hint "`:default` is a reserved storage target id" + :path (str path))) + + (doseq [[bucket target] routes] + (when-not (contains? sto/valid-buckets bucket) + (ex/raise :type :validation + :code :invalid-storage-routes-bucket + :hint "unknown semantic bucket in storage routes" + :bucket bucket + :path (str path))) + + (when-not (contains? targets target) + (ex/raise :type :validation + :code :unknown-storage-target + :hint "storage route points to an undeclared target" + :bucket bucket + :target target + :path (str path)))) + + data)) + +(defn- normalize-target + [target] + (cond-> target + (string? (:endpoint target)) + (update :endpoint u/uri))) + +(defn- normalize-targets + [targets] + (persistent! + (reduce-kv (fn [acc id target] + (assoc! acc id (normalize-target target))) + (transient {}) + targets))) + +(defn load + "Reads and validates the optional S3 routing file. + + Returns a map with `:targets` (id -> target definition) and `:routes` + (semantic bucket -> target id), or `{:targets nil :routes nil}` when the + configuration key is unset." + [] + (if-let [path (not-empty (cf/get :objects-storage-s3-routes-file))] + (let [data (-> (read-file path) (validate! path))] + (assert-backend-s3!) + {:targets (normalize-targets (:targets data)) + :routes (:routes data)}) + {:targets nil + :routes nil})) diff --git a/backend/src/app/storage/fs.clj b/backend/src/app/storage/fs.clj index cf07128579..f26bf99ce3 100644 --- a/backend/src/app/storage/fs.clj +++ b/backend/src/app/storage/fs.clj @@ -141,7 +141,7 @@ (Files/deleteIfExists ^Path path))) (defmethod impl/del-objects-in-bulk :fs - [backend ids] + [backend _target ids] (assert (valid-backend? backend) "expected a valid backend instance") (let [base (fs/path (::directory backend))] (reduce (fn [fail-ids id] diff --git a/backend/src/app/storage/gc_deleted.clj b/backend/src/app/storage/gc_deleted.clj index ab49030041..3b3b28de34 100644 --- a/backend/src/app/storage/gc_deleted.clj +++ b/backend/src/app/storage/gc_deleted.clj @@ -87,11 +87,11 @@ Returns the number of successfully deleted objects, or 0 if no rows could be locked." - [conn storage backend-id ids] + [conn storage backend-id target ids] (if-let [locked-ids (lock-ids conn ids)] (let [fail-ids (try (-> (impl/resolve-backend storage backend-id) - (impl/del-objects-in-bulk locked-ids)) + (impl/del-objects-in-bulk target locked-ids)) (catch Throwable cause (l/err :hint "error on physical deletion, will retry" :ids locked-ids @@ -118,12 +118,15 @@ (count ok-ids)) 0)) -(defn- group-by-backend +(defn- group-by-route [items] - (d/group-by (comp keyword :backend) :id #{} items)) + (d/group-by (fn [item] + [(keyword (:backend item)) (:target item)]) + :id #{} items)) (def ^:private sql:get-deleted-chunk - "SELECT id, backend + "SELECT id, backend, + coalesce(metadata->>'~:storage-target', 'default') as target FROM storage_object WHERE deleted_at IS NOT NULL AND deleted_at <= ? @@ -144,11 +147,11 @@ (fn [{:keys [::db/conn ::sto/storage]}] (let [chunk (get-deleted-chunk conn chunk-size)] (when (seq chunk) - (let [by-backend (group-by-backend chunk)] - (reduce-kv (fn [acc backend-id ids] - (+ acc (process-chunk conn storage backend-id ids))) + (let [by-route (group-by-route chunk)] + (reduce-kv (fn [acc [backend-id target] ids] + (+ acc (process-chunk conn storage backend-id target ids))) 0 - by-backend))))))] + by-route))))))] (if deleted (recur (+ total deleted)) total)))) diff --git a/backend/src/app/storage/impl.clj b/backend/src/app/storage/impl.clj index 76fafe2cac..2d3c8569fd 100644 --- a/backend/src/app/storage/impl.clj +++ b/backend/src/app/storage/impl.clj @@ -72,12 +72,14 @@ :context cfg)) (defmulti del-objects-in-bulk - "Delete multiple objects in bulk. Returns #{fail-ids} — the set of ids - whose blob deletion failed. Empty set = all succeeded." - (fn [cfg _] (::sto/type cfg))) + "Delete multiple objects in bulk. `target` is an optional backend-specific + destination id (used by the S3 storage targets); backends that do not route + ignore it. Returns #{fail-ids} — the set of ids whose blob deletion failed. + Empty set = all succeeded." + (fn [cfg _ _] (::sto/type cfg))) (defmethod del-objects-in-bulk :default - [cfg _] + [cfg _ _] (ex/raise :type :internal :code :invalid-storage-backend :context cfg)) diff --git a/backend/src/app/storage/pending_gc.clj b/backend/src/app/storage/pending_gc.clj index df161f40c9..31d0402606 100644 --- a/backend/src/app/storage/pending_gc.clj +++ b/backend/src/app/storage/pending_gc.clj @@ -20,7 +20,8 @@ [integrant.core :as ig])) (def ^:private sql:get-pending-sobjects - "SELECT id, backend + "SELECT id, backend, + coalesce(metadata->>'~:storage-target', 'default') as target FROM storage_object WHERE status = 'pending' AND created_at <= now() - interval '24 hours' @@ -56,10 +57,10 @@ "Best-effort removal of the orphaned blobs. Runs after the pending rows have been committed so a failure here never blocks their reclamation." [storage rows] - (doseq [{:keys [id backend]} rows] + (doseq [{:keys [id backend target]} rows] (try (-> (impl/resolve-backend storage (keyword backend)) - (impl/del-object {:id id})) + (impl/del-object (with-meta {:id id} {:storage-target target}))) (catch Throwable cause (l/err :hint "error deleting orphaned pending blob" :id (str id) diff --git a/backend/src/app/storage/s3.clj b/backend/src/app/storage/s3.clj index 6cf97321f1..d8d6eb5f80 100644 --- a/backend/src/app/storage/s3.clj +++ b/backend/src/app/storage/s3.clj @@ -88,13 +88,21 @@ ;; --- BACKEND INIT +(def ^:private schema:target + [:map {:title "s3-target"} + [:bucket ::sm/text] + [:region {:optional true} :keyword] + [:endpoint {:optional true} ::sm/uri] + [:prefix {:optional true} ::sm/text]]) + (def ^:private schema:config [:map {:title "s3-backend-config"} ::wrk/netty-io-executor [::region {:optional true} :keyword] [::bucket {:optional true} ::sm/text] [::prefix {:optional true} ::sm/text] - [::endpoint {:optional true} ::sm/uri]]) + [::endpoint {:optional true} ::sm/uri] + [::targets {:optional true} [:map-of :keyword schema:target]]]) (defmethod ig/expand-key ::backend [k v] @@ -104,42 +112,114 @@ [_ params] (assert (sm/check schema:config params))) +(defn- build-client-pair + [{:keys [::wrk/netty-io-executor]} region endpoint] + (let [params {::region region + ::endpoint endpoint + ::wrk/netty-io-executor netty-io-executor} + client (build-s3-client params) + presigner (build-s3-presigner params)] + {:client @client + :presigner presigner + :close-fn #(.close ^java.lang.AutoCloseable client)})) + +(defn- build-targets + "Resolves the implicit `:default` target plus the declared targets, sharing + one S3 client/presigner pair per distinct `[region endpoint]`." + [{:keys [::region ::endpoint ::bucket ::prefix ::targets] :as params}] + (let [defs (merge {:default {:region region :endpoint endpoint + :bucket bucket :prefix prefix}} + (into {} + (map (fn [[id target]] + [id {:region (or (:region target) region) + :endpoint (or (:endpoint target) endpoint) + :bucket (:bucket target) + :prefix (or (:prefix target) prefix)}])) + targets)) + result (reduce-kv + (fn [acc id {:keys [region endpoint bucket prefix]}] + (let [k [region endpoint] + pair (or (get-in acc [:pairs k]) + (build-client-pair params region endpoint)) + acc (cond-> acc + (nil? (get-in acc [:pairs k])) + (-> (assoc-in [:pairs k] pair) + (update :close-fns conj (:close-fn pair))))] + (assoc-in acc [:targets id] + {::client (:client pair) + ::presigner (:presigner pair) + ::bucket bucket + ::prefix prefix}))) + {:pairs {} :targets {} :close-fns []} + defs)] + (select-keys result [:targets :close-fns]))) + (defmethod ig/init-key ::backend [_ params] (when (and (contains? params ::region) (contains? params ::bucket)) - (let [client (build-s3-client params) - presigner (build-s3-presigner params)] + (let [{:keys [targets close-fns]} (build-targets params)] (assoc params ::sto/type :s3 ::counter (AtomicLong. 0) - ::client @client - ::presigner presigner - ::close-fn #(.close ^java.lang.AutoCloseable client))))) + ::default-target :default + ::targets targets + ::close-fns (vec close-fns))))) (defmethod ig/resolve-key ::backend [_ params] - (dissoc params ::close-fn)) + (dissoc params ::close-fns)) (defmethod ig/halt-key! ::backend - [_ {:keys [::close-fn]}] - (when (fn? close-fn) - (close-fn))) + [_ {:keys [::close-fns]}] + (doseq [f close-fns] + (when (fn? f) + (f)))) (def ^:private schema:backend [:map {:title "s3-backend"} - ;; [::region :keyword] - ;; [::bucket ::sm/text] - [::client [:fn #(instance? S3AsyncClient %)]] - [::presigner [:fn #(instance? S3Presigner %)]] - [::prefix {:optional true} ::sm/text] - #_[::sto/type [:= :s3]]]) + [::default-target :keyword] + [::targets + [:map-of :keyword + [:map + [::client [:fn #(instance? S3AsyncClient %)]] + [::presigner [:fn #(instance? S3Presigner %)]] + [::bucket ::sm/text] + [::prefix {:optional true} ::sm/text]]]]]) (sm/register! ::backend schema:backend) (def ^:private valid-backend? (sm/validator schema:backend)) +;; --- TARGET RESOLUTION + +(defn- target-id + "Returns the configured target id stored on the object, or the default + target name when the object predates the routing feature." + [backend object] + (or (:storage-target object) + (some-> (meta object) :storage-target) + (name (::default-target backend)))) + +(defn- resolve-target-by-id + [backend target-id] + (let [tid (cond + (nil? target-id) (::default-target backend) + (keyword? target-id) target-id + :else (keyword target-id))] + (or (get (::targets backend) tid) + (get (::targets backend) (::default-target backend)) + (ex/raise :type :internal + :code :invalid-storage-target + :hint "storage target not configured" + :target target-id + :available (vec (keys (::targets backend))))))) + +(defn- resolve-target + [backend object] + (resolve-target-by-id backend (target-id backend object))) + ;; --- API IMPL (defmethod impl/put-object :s3 @@ -210,13 +290,14 @@ true))) (defmethod impl/del-objects-in-bulk :s3 - [backend ids] + [backend target ids] (assert (valid-backend? backend) "expected a valid backend instance") - (let [key->id (into {} (map (fn [id] - [(str (::prefix backend) (impl/id->path id)) id])) + (let [target (resolve-target-by-id backend target) + key->id (into {} (map (fn [id] + [(str (::prefix target) (impl/id->path id)) id])) ids) result (try - (p/await! (del-object-in-bulk backend ids)) + (p/await! (del-object-in-bulk target ids)) (catch Throwable cause (l/err :hint "error on s3 bulk deletion" :ids ids @@ -320,8 +401,9 @@ ^Subscriber subscriber)))))) (defn- put-object - [{:keys [::client ::bucket ::prefix ::counter]} {:keys [id] :as object} content] - (let [path (dm/str prefix (impl/id->path id)) + [{:keys [::counter] :as backend} {:keys [id] :as object} content] + (let [{:keys [::client ::bucket ::prefix]} (resolve-target backend object) + path (dm/str prefix (impl/id->path id)) mdata (meta object) mtype (:content-type mdata "application/octet-stream") rbody (make-request-body counter content) @@ -344,8 +426,9 @@ (proxy-super close)))) (defn- get-object-data - [{:keys [::client ::bucket ::prefix]} {:keys [id size]}] - (let [gor (.. (GetObjectRequest/builder) + [backend {:keys [id size] :as object}] + (let [{:keys [::client ::bucket ::prefix]} (resolve-target backend object) + gor (.. (GetObjectRequest/builder) (bucket bucket) (key (str prefix (impl/id->path id))) (build))] @@ -369,16 +452,18 @@ (p/fmap #(.asInputStream ^ResponseBytes %))))))) (defn- head-object - [{:keys [::client ::bucket ::prefix]} {:keys [id]}] - (let [hor (.. (HeadObjectRequest/builder) + [backend {:keys [id] :as object}] + (let [{:keys [::client ::bucket ::prefix]} (resolve-target backend object) + hor (.. (HeadObjectRequest/builder) (bucket bucket) (key (str prefix (impl/id->path id))) (build))] (.headObject ^S3AsyncClient client ^HeadObjectRequest hor))) (defn- get-object-bytes - [{:keys [::client ::bucket ::prefix]} {:keys [id]}] - (let [gor (.. (GetObjectRequest/builder) + [backend {:keys [id] :as object}] + (let [{:keys [::client ::bucket ::prefix]} (resolve-target backend object) + gor (.. (GetObjectRequest/builder) (bucket bucket) (key (str prefix (impl/id->path id))) (build)) @@ -392,7 +477,7 @@ (ct/duration {:minutes 10})) (defn- get-object-url - [{:keys [::presigner ::bucket ::prefix]} {:keys [id]} + [backend {:keys [id] :as object} {:keys [max-age content-disposition] :or {max-age default-max-age}}] (assert (ct/duration? max-age) "expected valid duration instance") @@ -400,7 +485,8 @@ ;; object store sets that header on the response the client fetches after ;; following the redirect. It is only set when asked for, so urls for ;; objects served inline stay byte identical to before. - (let [gorb (.. (GetObjectRequest/builder) + (let [{:keys [::presigner ::bucket ::prefix]} (resolve-target backend object) + gorb (.. (GetObjectRequest/builder) (bucket bucket) (key (dm/str prefix (impl/id->path id)))) gorb (cond-> gorb @@ -415,8 +501,9 @@ (u/uri (str (.url ^PresignedGetObjectRequest pgor))))) (defn- del-object - [{:keys [::bucket ::client ::prefix]} {:keys [id] :as obj}] - (let [dor (.. (DeleteObjectRequest/builder) + [backend {:keys [id] :as object}] + (let [{:keys [::bucket ::client ::prefix]} (resolve-target backend object) + dor (.. (DeleteObjectRequest/builder) (bucket bucket) (key (dm/str prefix (impl/id->path id))) (build))] diff --git a/backend/test/backend_tests/storage_config_test.clj b/backend/test/backend_tests/storage_config_test.clj new file mode 100644 index 0000000000..7d140c77b7 --- /dev/null +++ b/backend/test/backend_tests/storage_config_test.clj @@ -0,0 +1,81 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns backend-tests.storage-config-test + (:require + [app.common.uri :as u] + [app.config :as cf] + [app.storage.config :as sto.config] + [clojure.java.io :as io] + [clojure.test :as t])) + +(defn- write-routes! + [content] + (let [file (java.io.File/createTempFile "storage-routes" ".edn")] + (spit file content) + file)) + +(defn- delete! + [file] + (io/delete-file file true)) + +(defn- load-with + [config] + (binding [cf/config (merge cf/config + {:objects-storage-backend :s3} + config)] + (sto.config/load))) + +(defn- load-error + "Runs `load` against a routes file with `content` and returns the raised + throwable, or nil when it succeeds." + [content config] + (let [file (write-routes! content)] + (try + (try + (load-with (assoc config :objects-storage-s3-routes-file (.getAbsolutePath file))) + nil + (catch Throwable cause cause)) + (finally + (delete! file))))) + +(t/deftest returns-empty-when-unset + (t/is (= {:targets nil :routes nil} + (load-with {:objects-storage-s3-routes-file nil})))) + +(t/deftest loads-and-normalizes-routes + (let [file (write-routes! + "{:targets {:temp {:bucket \"penpot-temp\" :endpoint \"https://s3.example.com\"}} :routes {\"tempfile\" :temp}}")] + (try + (let [result (load-with {:objects-storage-s3-routes-file (.getAbsolutePath file)})] + (t/is (= #{:temp} (set (keys (:targets result))))) + (t/is (= "penpot-temp" (get-in result [:targets :temp :bucket]))) + (t/is (u/uri? (get-in result [:targets :temp :endpoint]))) + (t/is (= {"tempfile" :temp} (:routes result)))) + (finally + (delete! file))))) + +(t/deftest rejects-reserved-default-target + (t/is (some? (load-error "{:targets {:default {:bucket \"x\"}}}" + {})))) + +(t/deftest rejects-unknown-route-target + (t/is (some? (load-error + "{:targets {:temp {:bucket \"x\"}} :routes {\"tempfile\" :missing}}" + {})))) + +(t/deftest rejects-invalid-semantic-bucket + (t/is (some? (load-error + "{:targets {:temp {:bucket \"x\"}} :routes {\"not-a-bucket\" :temp}}" + {})))) + +(t/deftest rejects-unreadable-file + (t/is (some? (load-error "{:targets" {})))) + +(t/deftest rejects-routing-when-backend-is-not-s3 + (t/is (some? (load-error + "{:targets {:temp {:bucket \"x\"}} :routes {\"tempfile\" :temp}}" + {:objects-storage-backend :fs})))) diff --git a/backend/test/backend_tests/storage_test.clj b/backend/test/backend_tests/storage_test.clj index cc34773e67..c4b7a91588 100644 --- a/backend/test/backend_tests/storage_test.clj +++ b/backend/test/backend_tests/storage_test.clj @@ -13,13 +13,16 @@ [app.rpc :as-alias rpc] [app.storage :as sto] [app.storage.fs :as-alias sto.fs] + [app.storage.gc-deleted :as sto.gc-deleted] [app.storage.impl :as impl] + [app.storage.pending-gc :as sto.pending-gc] [app.storage.s3 :as-alias sto.s3] [backend-tests.helpers :as th] [clojure.test :as t] [cuerdas.core :as str] [datoteka.fs :as fs] [datoteka.io :as io] + [integrant.core :as ig] [mockery.core :refer [with-mocks]] [promesa.core :as p]) (:import @@ -41,6 +44,17 @@ [storage] (assoc storage ::sto/backend :fs)) +(declare fake-s3-backend-with-targets) + +(defn configure-s3-storage + "Returns a storage configured with the fake S3 backend and the given + semantic-bucket -> target routing." + [bucket->target] + (-> (:app.storage/storage th/*system*) + (assoc ::sto/backend :s3) + (assoc-in [::sto/backends :s3] (fake-s3-backend-with-targets)) + (assoc ::sto/bucket->target bucket->target))) + (t/deftest put-and-retrieve-object (let [storage (-> (:app.storage/storage th/*system*) (configure-storage-backend)) @@ -708,7 +722,7 @@ {:id (:id object)}) (with-mocks [_mock {:target 'app.storage.impl/del-objects-in-bulk - :return (fn [_ ids] (set ids))}] + :return (fn [_ _ ids] (set ids))}] (let [res (th/run-task! :storage-gc-deleted {})] (t/is (= 0 (:deleted res))))) @@ -832,9 +846,82 @@ (defn- fake-s3-backend [] - {::sto/type :s3 - ::sto.s3/client (reify S3AsyncClient) - ::sto.s3/presigner (reify S3Presigner)}) + {::sto/type :s3 + ::sto.s3/default-target :default + ::sto.s3/targets + {:default {::sto.s3/client (reify S3AsyncClient) + ::sto.s3/presigner (reify S3Presigner) + ::sto.s3/bucket "test-bucket" + ::sto.s3/prefix nil}}}) + +(defn- fake-s3-target + [bucket prefix] + {::sto.s3/client (reify S3AsyncClient) + ::sto.s3/presigner (reify S3Presigner) + ::sto.s3/bucket bucket + ::sto.s3/prefix prefix}) + +(defn- fake-s3-backend-with-targets + [] + (assoc (fake-s3-backend) + ::sto.s3/targets + {:default (fake-s3-target "default" nil) + :temp (fake-s3-target "temp" "tmp/")})) + +(t/deftest s3-bulk-delete-resolves-target + (let [backend (fake-s3-backend-with-targets) + captured (atom nil)] + (with-mocks [_mock {:target 'app.storage.s3/del-object-in-bulk + :return (fn [target _ids] + (reset! captured target) + (p/resolved nil))}] + (t/is (= #{} (impl/del-objects-in-bulk backend :temp #{(uuid/next)}))) + (t/is (= "temp" (::sto.s3/bucket @captured))) + (t/is (= "tmp/" (::sto.s3/prefix @captured)))))) + +(t/deftest s3-bulk-delete-falls-back-to-default-target + (let [backend (fake-s3-backend-with-targets) + captured (atom nil)] + (with-mocks [_mock {:target 'app.storage.s3/del-object-in-bulk + :return (fn [target _ids] + (reset! captured target) + (p/resolved nil))}] + (impl/del-objects-in-bulk backend :missing #{(uuid/next)}) + (t/is (= "default" (::sto.s3/bucket @captured)))))) + +(t/deftest s3-build-targets-shares-clients-and-closes-them + (let [clients (atom []) + close-count (atom 0) + mk-client (fn [_] + (let [c (reify + clojure.lang.IDeref + (deref [_] (Object.)) + java.lang.AutoCloseable + (close [_] (swap! close-count inc)))] + (swap! clients conj c) + c))] + (with-mocks [_c {:target 'app.storage.s3/build-s3-client + :return mk-client} + _p {:target 'app.storage.s3/build-s3-presigner + :return (fn [_] (reify S3Presigner))}] + (let [backend (ig/init-key :app.storage.s3/backend + {:app.storage.s3/region :eu-central-1 + :app.storage.s3/bucket "main" + :app.worker/netty-io-executor :executor + :app.storage.s3/targets + {:same {:bucket "same"} + :other {:bucket "other" :region :us-east-1}}})] + ;; one client pair per distinct [region endpoint] + (t/is (= 2 (count @clients))) + ;; same region reuses the default client + (t/is (= (get-in backend [:app.storage.s3/targets :default :app.storage.s3/client]) + (get-in backend [:app.storage.s3/targets :same :app.storage.s3/client]))) + ;; different region gets its own client + (t/is (not= (get-in backend [:app.storage.s3/targets :default :app.storage.s3/client]) + (get-in backend [:app.storage.s3/targets :other :app.storage.s3/client]))) + + (ig/halt-key! :app.storage.s3/backend backend) + (t/is (= 2 @close-count)))))) (t/deftest s3-exists-object-returns-true-on-found (with-mocks [mock {:target 'app.storage.s3/head-object @@ -874,3 +961,123 @@ (t/is (= "boom" (ex-message (ex-cause ex))))) ;; one initial attempt plus max-retries (t/is (= 4 (:call-count @mock))))) + +;; --- Storage target metadata / dedup + +(t/deftest put-object-routes-bucket-to-storage-target + (let [storage (configure-s3-storage {"tempfile" :temp})] + (with-mocks [_mock {:target 'app.storage.impl/put-object + :return (fn [_ object _] object)}] + (let [object (sto/put-object! storage {::sto/content (sto/content "content") + :bucket "tempfile" + :content-type "text/plain"}) + row (th/db-exec-one! + ["select backend, metadata->>'~:storage-target' as target + from storage_object where id = ?" (:id object)])] + (t/is (= "s3" (:backend row))) + (t/is (= "temp" (:target row))))))) + +(t/deftest put-object-falls-back-to-default-storage-target + (let [storage (configure-s3-storage {"tempfile" :temp})] + (with-mocks [_mock {:target 'app.storage.impl/put-object + :return (fn [_ object _] object)}] + (let [object (sto/put-object! storage {::sto/content (sto/content "content") + :bucket "file-media-object" + :content-type "text/plain"}) + row (th/db-exec-one! + ["select metadata->>'~:storage-target' as target + from storage_object where id = ?" (:id object)])] + (t/is (= "default" (:target row))))))) + +(t/deftest put-object-fs-does-not-set-storage-target + (let [storage (configure-storage-backend (:app.storage/storage th/*system*)) + object (sto/put-object! storage {::sto/content (sto/content "content") + :bucket "file-media-object" + :content-type "text/plain"}) + row (th/db-exec-one! + ["select metadata->>'~:storage-target' as target + from storage_object where id = ?" (:id object)])] + (t/is (nil? (:target row))))) + +(t/deftest dedup-is-isolated-per-storage-target + (let [storage (configure-s3-storage {"file-data" :temp}) + content (-> (sto/content "content") + (sto/wrap-with-hash "same-hash"))] + (with-mocks [_mock {:target 'app.storage.impl/put-object + :return (fn [_ object _] object)}] + (let [object1 (sto/put-object! storage {::sto/content content + ::sto/deduplicate? true + :bucket "file-data" + :content-type "text/plain"}) + object2 (sto/put-object! storage {::sto/content content + ::sto/deduplicate? true + :bucket "file-media-object" + :content-type "text/plain"})] + (t/is (not= (:id object1) (:id object2))))))) + +(t/deftest dedup-hit-carries-storage-target-metadata + (let [storage (configure-s3-storage {"file-data" :temp}) + content (-> (sto/content "content") + (sto/wrap-with-hash "same-hash")) + captured (atom nil)] + (with-mocks [_p {:target 'app.storage.impl/put-object + :return (fn [_ object _] object)} + _e {:target 'app.storage.impl/exists-object? + :return (fn [_ object] + (reset! captured object) + true)}] + (sto/put-object! storage {::sto/content content + ::sto/deduplicate? true + :bucket "file-data" + :content-type "text/plain"}) + (sto/put-object! storage {::sto/content content + ::sto/deduplicate? true + :bucket "file-data" + :content-type "text/plain"}) + (t/is (some? @captured)) + (t/is (= "temp" (:storage-target (meta @captured))))))) + +;; --- GC target routing + +(defn- storage-with-s3-targets + [] + (assoc (:app.storage/storage th/*system*) + ::sto/backends + {:s3 (fake-s3-backend-with-targets)})) + +(t/deftest gc-deleted-deletes-from-routed-target + (let [storage (storage-with-s3-targets) + cfg {::db/pool th/*pool* ::sto/storage storage} + id (uuid/next) + captured (atom nil)] + (th/db-exec! ["insert into storage_object (id, size, backend, metadata, deleted_at, status) + values (?, 1, 's3', ?, ?, 'valid')" + id + (db/tjson {:bucket "file-data" :storage-target "temp"}) + (ct/in-past {:minutes 1})]) + (with-mocks [_mock {:target 'app.storage.impl/del-objects-in-bulk + :return (fn [_ target _ids] + (reset! captured target) + #{})}] + (t/is (= 1 (#'sto.gc-deleted/clean-deleted! cfg))) + (t/is (= "temp" @captured))))) + +(t/deftest pending-gc-deletes-from-routed-target + (let [storage (storage-with-s3-targets) + id (uuid/next) + captured (atom nil)] + (th/db-exec! ["insert into storage_object (id, size, backend, metadata, created_at, status) + values (?, 1, 's3', ?, ?, 'pending')" + id + (db/tjson {:storage-target "temp"}) + (ct/in-past {:days 2})]) + (let [rows (th/db-exec! + ["select id, backend, + coalesce(metadata->>'~:storage-target','default') as target + from storage_object where id = ?" id])] + (with-mocks [_mock {:target 'app.storage.impl/del-object + :return (fn [_ object] + (reset! captured object) + nil)}] + (#'sto.pending-gc/delete-blobs! storage rows) + (t/is (= "temp" (:storage-target (meta @captured)))))))) diff --git a/docker/images/docker-compose.yaml b/docker/images/docker-compose.yaml index 63e92d9916..1d039ae8a8 100644 --- a/docker/images/docker-compose.yaml +++ b/docker/images/docker-compose.yaml @@ -157,6 +157,11 @@ services: # PENPOT_OBJECTS_STORAGE_S3_ENDPOINT: # PENPOT_OBJECTS_STORAGE_S3_BUCKET: + ## Optional: route specific internal semantic buckets (for example + ## tempfile) to additional S3 targets. The file is EDN; see + ## docs/technical-guide/configuration.md for the format. + # PENPOT_OBJECTS_STORAGE_S3_ROUTES_FILE: /opt/data/storage-routes.edn + ## Telemetry. When enabled, a periodical process will send anonymous data about this ## instance. Telemetry data will enable us to learn how the application is used, ## based on real scenarios. If you want to help us, please leave it enabled. You can diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 76426c22e5..6ba59cb95e 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -549,6 +549,44 @@ PENPOT_OBJECTS_STORAGE_S3_ENDPOINT: These settings are equally useful if you have a Minio storage system.

+#### S3 targets and per-bucket routing + +__Since version 2.19.0__ + +By default the S3 backend stores every object in the configured bucket. You can +route specific internal semantic buckets (for example `tempfile` or `file-data`) +to additional S3 targets. Each target has its own bucket and, optionally, its +own key prefix, region and endpoint. + +Targets and routing are declared in an external EDN file referenced by the +`PENPOT_OBJECTS_STORAGE_S3_ROUTES_FILE` environment variable: + +```clojure +{:targets + {:temp {:bucket "penpot-temp"} + :cold {:bucket "penpot-cold" + :region :us-east-1 + :endpoint "https://s3.us-east-1.amazonaws.com" + :prefix "objects/"}} + :routes + {"tempfile" :temp + "file-data" :temp}} +``` + +- `:targets` declares the named destinations. `:bucket` is required; `:prefix`, + `:region` and `:endpoint` are optional and inherit the values from the default + target (`PENPOT_OBJECTS_STORAGE_S3_*`) when omitted. +- `:routes` maps a Penpot semantic bucket to a target id. Semantic buckets that + are not listed use the default target (the configured + `PENPOT_OBJECTS_STORAGE_S3_BUCKET`). +- The `:default` target id is reserved and implicit; do not declare it. +- The file is optional and only applies to the `s3` backend. Without it, all + objects use the default bucket and the behavior is unchanged. + +The target id is stored in the object metadata, so it must stay stable. If you +remove or rename a target id, objects already written with it can no longer be +read or deleted. + ### File Data Storage __Since version 2.11.0__ diff --git a/docs/technical-guide/developer/subsystems/assets-storage.md b/docs/technical-guide/developer/subsystems/assets-storage.md index 7108670c5a..6ef91ab8a0 100644 --- a/docs/technical-guide/developer/subsystems/assets-storage.md +++ b/docs/technical-guide/developer/subsystems/assets-storage.md @@ -16,11 +16,12 @@ that may be used for any kind of user uploaded files. Currently: There is an abstract interface and several implementations (or **backends**), depending on where the objects are actually stored: - * :assets-fs stores ojects in the file system, under a given base path. - * :assets-s3 stores them in any cloud storage with an AWS-S3 compatible + * :fs stores objects in the file system, under a given base path. + * :s3 stores them in any cloud storage with an AWS-S3 compatible interface. - * :assets-db stores them inside the PostgreSQL database, in a special table - with a binary column. + +Legacy rows may still reference the deprecated :assets-fs and +:assets-s3 names, which alias the current backends. ## Storage API @@ -83,6 +84,13 @@ The storage module may use the bucket (hardcoded) to make special treatment to object, such as storing in a different path, or guessing how to know if an object is referenced from other place. +When the :s3 backend is used, a semantic bucket may also be routed to a +named S3 **target** (a different bucket and/or key prefix). The chosen target id +is stored in the object metadata (:storage-target) and resolved again on +reads, URL signing, deduplication and garbage collection. Objects without a +stored target use the default target. See the configuration guide for the +routing file format. + ## Sharing and deleting objects To save storage space, duplicated objects wre shared. So, if for example