From d638e9497158ec1fbe3265ba9ec7b77193214f05 Mon Sep 17 00:00:00 2001
From: Alexander Solovyov <alexander@solovyov.net>
Date: Wed, 12 Jun 2024 20:22:04 +0300
Subject: [PATCH] certain settings should be decrypted for cache config
migration to run (#44048)
---
.../migrations/001_update_migrations.yaml | 59 +++++++++++------
.../fill_cache_config_root_fix.my.sql | 10 +++
src/metabase/db/custom_migrations.clj | 7 ++
test/metabase/db/custom_migrations_test.clj | 18 +++++
test/metabase/db/schema_migrations_test.clj | 66 ++++++++++++++-----
5 files changed, 126 insertions(+), 34 deletions(-)
create mode 100644 resources/migrations/custom_sql/fill_cache_config_root_fix.my.sql
diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml
index 288686686da..72095939137 100644
--- a/resources/migrations/001_update_migrations.yaml
+++ b/resources/migrations/001_update_migrations.yaml
@@ -7226,25 +7226,6 @@ databaseChangeLog:
nullable: false
defaultValue: false
- - changeSet:
- id: v50.2024-04-12T12:33:09
- author: piranha
- comment: 'Copy old cache configurations to cache_config table'
- changes:
- - sqlFile:
- dbms: postgresql
- path: custom_sql/fill_cache_config.pg.sql
- relativeToChangelogFile: true
- - sqlFile:
- dbms: mysql,mariadb
- path: custom_sql/fill_cache_config.my.sql
- relativeToChangelogFile: true
- - sqlFile:
- dbms: h2
- path: custom_sql/fill_cache_config.h2.sql
- relativeToChangelogFile: true
- rollback: # no rollback necessary, we're not removing the columns yet
-
- changeSet:
id: v50.2024-04-15T16:30:35
author: qnkhuat
@@ -7872,6 +7853,46 @@ databaseChangeLog:
relativeToChangelogFile: true
rollback: # not needed, columns will be deleted
+ - changeSet:
+ id: v50.2024-06-12T12:33:07
+ author: piranha
+ comment: 'Decrypt some settings so the next migration runs well'
+ changes:
+ - customChange:
+ class: "metabase.db.custom_migrations.DecryptCacheSettings"
+ rollback: # no rollback necessary, they'll be encrypted if you downgrade and touch them
+
+ - changeSet:
+ # this had id `v50.2024-04-12T12:33:09` before #44048
+ id: v50.2024-06-12T12:33:08
+ author: piranha
+ comment: 'Copy old cache configurations to cache_config table'
+ changes:
+ - sqlFile:
+ dbms: postgresql
+ path: custom_sql/fill_cache_config.pg.sql
+ relativeToChangelogFile: true
+ - sqlFile:
+ dbms: mysql,mariadb
+ path: custom_sql/fill_cache_config.my.sql
+ relativeToChangelogFile: true
+ - sqlFile:
+ dbms: h2
+ path: custom_sql/fill_cache_config.h2.sql
+ relativeToChangelogFile: true
+ rollback: # no rollback necessary, we're not removing the columns yet
+
+ - changeSet:
+ id: v50.2024-06-12T12:33:09
+ author: piranha
+ comment: 'Fix root cache config for mysql if it is wrong'
+ dbms: mysql,mariadb
+ changes:
+ - sqlFile:
+ path: custom_sql/fill_cache_config_root_fix.my.sql
+ relativeToChangelogFile: true
+ rollback: # no rollback necessary here too
+
- changeSet:
id: v51.2024-05-13T15:30:57
author: metamben
diff --git a/resources/migrations/custom_sql/fill_cache_config_root_fix.my.sql b/resources/migrations/custom_sql/fill_cache_config_root_fix.my.sql
new file mode 100644
index 00000000000..1ff4aa66937
--- /dev/null
+++ b/resources/migrations/custom_sql/fill_cache_config_root_fix.my.sql
@@ -0,0 +1,10 @@
+UPDATE cache_config
+ SET config = json_object(
+ 'multiplier', coalesce((select cast(`value` as unsigned) from setting where `key` = 'query-caching-ttl-ratio'), 10),
+ 'min_duration_ms', coalesce((select cast(`value` as unsigned) from setting where `key` = 'query-caching-min-ttl'), 60000)
+ )
+ WHERE model = 'root' AND
+ model_id = 0 AND
+ strategy = 'ttl' AND
+ (json_extract(config, '$.multiplier') = 0 OR
+ json_extract(config, '$.min_duration_ms') = 0);
diff --git a/src/metabase/db/custom_migrations.clj b/src/metabase/db/custom_migrations.clj
index bcc8da7b9b2..a47e23ca5ce 100644
--- a/src/metabase/db/custom_migrations.clj
+++ b/src/metabase/db/custom_migrations.clj
@@ -1458,3 +1458,10 @@
(->> settings
(filter :value)
(t2/insert! :setting)))))
+
+(define-migration DecryptCacheSettings
+ (let [decrypt! (fn [k]
+ (t2/update! :setting :key k {:value (raw-setting-value k)}))]
+ (run! decrypt! ["query-caching-ttl-ratio"
+ "query-caching-min-ttl"
+ "enable-query-caching"])))
diff --git a/test/metabase/db/custom_migrations_test.clj b/test/metabase/db/custom_migrations_test.clj
index 5413b62371a..dc57827f179 100644
--- a/test/metabase/db/custom_migrations_test.clj
+++ b/test/metabase/db/custom_migrations_test.clj
@@ -1995,3 +1995,21 @@
:date_joined :%now})
(migrate!)
(is (false? (sample-content-created?)))))))
+
+(deftest decrypt-cache-settings-test
+ (impl/test-migrations "v50.2024-06-12T12:33:07" [migrate!]
+ (encryption-test/with-secret-key "whateverwhatever"
+ (t2/insert! :setting [{:key "enable-query-caching", :value (encryption/maybe-encrypt "true")}
+ {:key "query-caching-ttl-ratio", :value (encryption/maybe-encrypt "100")}
+ {:key "query-caching-min-ttl", :value (encryption/maybe-encrypt "123")}]))
+
+ (testing "Values were indeed encrypted"
+ (is (not= "true" (t2/select-one-fn :value :setting :key "enable-query-caching"))))
+
+ (encryption-test/with-secret-key "whateverwhatever"
+ (migrate!))
+
+ (testing "But not anymore"
+ (is (= "true" (t2/select-one-fn :value :setting :key "enable-query-caching")))
+ (is (= "100" (t2/select-one-fn :value :setting :key "query-caching-ttl-ratio")))
+ (is (= "123" (t2/select-one-fn :value :setting :key "query-caching-min-ttl"))))))
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index ddbc1a5b489..2da8e2a16b8 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -33,6 +33,8 @@
[metabase.models.collection :as collection]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
+ [metabase.util.encryption :as encryption]
+ [metabase.util.encryption-test :as encryption-test]
[toucan2.core :as t2]))
(set! *warn-on-reflection* true)
@@ -1494,10 +1496,13 @@
(deftest cache-config-migration-test
(testing "Caching config is correctly copied over"
- (impl/test-migrations "v50.2024-04-12T12:33:09" [migrate!]
- (mdb.query/update-or-insert! :model/Setting {:key "enable-query-caching"} (constantly {:value "true"}))
- (mdb.query/update-or-insert! :model/Setting {:key "query-caching-ttl-ratio"} (constantly {:value "100"}))
- (mdb.query/update-or-insert! :model/Setting {:key "query-caching-min-ttl"} (constantly {:value "123"}))
+ (impl/test-migrations ["v50.2024-06-12T12:33:07"] [migrate!]
+ ;; this peculiar setup is to reproduce #44012, `enable-query-caching` should be unencrypted for the condition
+ ;; to hit it
+ (t2/insert! :setting [{:key "enable-query-caching", :value (encryption/maybe-encrypt "true")}])
+ (encryption-test/with-secret-key "whateverwhatever"
+ (t2/insert! :setting [{:key "query-caching-ttl-ratio", :value (encryption/maybe-encrypt "100")}
+ {:key "query-caching-min-ttl", :value (encryption/maybe-encrypt "123")}]))
(let [user (create-raw-user! (mt/random-email))
db (t2/insert-returning-pk! :metabase_database (-> (mt/with-temp-defaults Database)
@@ -1521,31 +1526,34 @@
:database_id db
:created_at :%now
:updated_at :%now})]
- (migrate!)
+
+ (encryption-test/with-secret-key "whateverwhatever"
+ (migrate! :up))
(is (=? [{:model "root"
:model_id 0
- :strategy :ttl
+ :strategy "ttl"
:config {:multiplier 100
:min_duration_ms 123}}
{:model "database"
:model_id db
- :strategy :duration
+ :strategy "duration"
:config {:duration 10 :unit "hours"}}
{:model "dashboard"
:model_id dash
- :strategy :duration
+ :strategy "duration"
:config {:duration 20 :unit "hours"}}
{:model "question"
:model_id card
- :strategy :duration
+ :strategy "duration"
:config {:duration 30 :unit "hours"}}]
- (t2/select :model/CacheConfig))))))
+ (->> (t2/select :cache_config)
+ (mapv #(update % :config json/decode true))))))))
(testing "And not copied if caching is disabled"
- (impl/test-migrations "v50.2024-04-12T12:33:09" [migrate!]
- (mdb.query/update-or-insert! :model/Setting {:key "enable-query-caching"} (constantly {:value "false"}))
- (mdb.query/update-or-insert! :model/Setting {:key "query-caching-ttl-ratio"} (constantly {:value "100"}))
- (mdb.query/update-or-insert! :model/Setting {:key "query-caching-min-ttl"} (constantly {:value "123"}))
+ (impl/test-migrations ["v50.2024-04-12T12:33:07"] [migrate!]
+ (t2/insert! :setting [{:key "enable-query-caching", :value (encryption/maybe-encrypt "false")}
+ {:key "query-caching-ttl-ratio", :value (encryption/maybe-encrypt "100")}
+ {:key "query-caching-min-ttl", :value (encryption/maybe-encrypt "123")}])
;; this one to have custom configuration to check they are not copied over
(t2/insert-returning-pk! :metabase_database (-> (mt/with-temp-defaults Database)
@@ -1554,7 +1562,35 @@
(assoc :cache_ttl 10)))
(migrate!)
(is (= []
- (t2/select :model/CacheConfig))))))
+ (t2/select :cache_config))))))
+
+(deftest cache-config-mysql-update-test
+ (when (= (mdb/db-type) :mysql)
+ (testing "Root cache config for mysql is updated with correct values"
+ (encryption-test/with-secret-key "whateverwhatever"
+ (impl/test-migrations ["v50.2024-06-12T12:33:07"] [migrate!]
+ (t2/insert! :setting [{:key "enable-query-caching", :value (encryption/maybe-encrypt "true")}
+ {:key "query-caching-ttl-ratio", :value (encryption/maybe-encrypt "100")}
+ {:key "query-caching-min-ttl", :value (encryption/maybe-encrypt "123")}])
+
+ ;; the idea here is that `v50.2024-04-12T12:33:09` during execution with partially encrypted data (see
+ ;; `cache-config-migration-test`) instead of throwing an error just silently put zeros in config. If config
+ ;; contains zeros, we assume human did not touch it yet and update with existing (decrypted thanks to
+ ;; `v50.2024-06-12T12:33:07`) settings
+ (t2/insert! :cache_config {:model "root"
+ :model_id 0
+ :strategy "ttl"
+ :config (json/encode {:multiplier 0
+ :min_duration_ms 0})})
+ (migrate!)
+
+ (is (=? {:model "root"
+ :model_id 0
+ :strategy "ttl"
+ :config {:multiplier 100
+ :min_duration_ms 123}}
+ (-> (t2/select-one :cache_config)
+ (update :config json/decode true)))))))))
(deftest split-data-permissions-migration-test
(testing "View Data and Create Query permissions are created correctly based on existing data permissions"
--
GitLab