From e97a8e7e935a75fa77f1fc28099d9c16607a9a2f Mon Sep 17 00:00:00 2001
From: Dennis Schridde <63082+devurandom@users.noreply.github.com>
Date: Wed, 7 Aug 2024 16:52:48 +0200
Subject: [PATCH] Delete database using config file (#46437)
== Goal ==
Currently the config file (`MB_CONFIG_FILE_PATH`) only allows adding or
changing databases, but not removing them. Since connected databases
can be managed as a service by us, it becomes necessary to delete any
information Metabase has about them, including any stored credentials.
Since this is a destructive operation with no way to revert (without
restoring from a backup), we raise the bar for accidental deletion and
require not just a (boolean) flag, but a magic string to be set.
== How to test ==
1. Configure a database as described in https://www.metabase.com/docs/latest/configuring-metabase/config-file#databases.
2. Start your Metabase instance.
3. Verify the database shows up in the "admin" section and it works.
Create a dashboard and questions using the database.
4. Add a `delete: "DELETE_WITH_DEPENDENTS:${name}"` line to your
database configuration section.
5. Restart your Metabase instance.
6. Verify the database no longer shows up in the "admin" section.
Notice that questions using it are gone, too.
References: https://github.com/metabase/harbormaster/issues/5173
---
.../advanced_config/file/databases.clj | 35 +++++++++++------
.../advanced_config/file/databases_test.clj | 38 ++++++++++++++++++-
2 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/advanced_config/file/databases.clj b/enterprise/backend/src/metabase_enterprise/advanced_config/file/databases.clj
index 3bd01e3cc5a..7d82de30786 100644
--- a/enterprise/backend/src/metabase_enterprise/advanced_config/file/databases.clj
+++ b/enterprise/backend/src/metabase_enterprise/advanced_config/file/databases.clj
@@ -37,18 +37,31 @@
(defn- init-from-config-file!
[database]
- ;; assert that we are able to connect to this Database. Otherwise, throw an Exception.
- (driver.u/can-connect-with-details? (keyword (:engine database)) (:details database) :throw-exceptions)
- (if-let [existing-database-id (t2/select-one-pk Database :engine (:engine database), :name (:name database))]
+ (if (contains? database :delete)
+ ;; Databases can be managed as a service by us. When the service is canceled, we need to delete any information
+ ;; Metabase has about them, including any stored credentials. This is a config file flag instead of a CLI command,
+ ;; so we can ensure the database stays deleted even after restoring backups.
+ (let [magic-request (format "DELETE_WITH_DEPENDENTS:%s" (:name database))]
+ (log/info (u/format-color :blue "Deleting databases via the config file is an internal feature subject to breaking changes."))
+ (when (not= magic-request (:delete database))
+ (throw (ex-info (format "To delete database %s set `delete` to %s" (pr-str (:name database)) (pr-str magic-request))
+ {:database-name (:name database)})))
+ (when-let [existing-database-id (t2/select-one-pk Database :engine (:engine database), :name (:name database))]
+ (log/info (u/format-color :blue "Deleting Database %s %s" (:engine database) (pr-str (:name database))))
+ (t2/delete! Database existing-database-id)))
(do
- (log/info (u/format-color :blue "Updating Database %s %s" (:engine database) (pr-str (:name database))))
- (t2/update! Database existing-database-id database))
- (do
- (log/info (u/format-color :green "Creating new %s Database %s" (:engine database) (pr-str (:name database))))
- (let [db (first (t2/insert-returning-instances! Database database))]
- (if (config-from-file-sync-databases)
- ((requiring-resolve 'metabase.sync/sync-database!) db)
- (log/info "Sync on database creation when initializing from file is disabled. Skipping sync."))))))
+ ;; assert that we are able to connect to this Database. Otherwise, throw an Exception.
+ (driver.u/can-connect-with-details? (keyword (:engine database)) (:details database) :throw-exceptions)
+ (if-let [existing-database-id (t2/select-one-pk Database :engine (:engine database), :name (:name database))]
+ (do
+ (log/info (u/format-color :blue "Updating Database %s %s" (:engine database) (pr-str (:name database))))
+ (t2/update! Database existing-database-id database))
+ (do
+ (log/info (u/format-color :green "Creating new %s Database %s" (:engine database) (pr-str (:name database))))
+ (let [db (first (t2/insert-returning-instances! Database database))]
+ (if (config-from-file-sync-databases)
+ ((requiring-resolve 'metabase.sync/sync-database!) db)
+ (log/info "Sync on database creation when initializing from file is disabled. Skipping sync."))))))))
(defmethod advanced-config.file.i/initialize-section! :databases
[_section-name databases]
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj
index aa799209201..cf1bc718a83 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_config/file/databases_test.clj
@@ -7,7 +7,9 @@
[metabase.models :refer [Database Table]]
[metabase.test :as mt]
[metabase.util :as u]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2])
+ (:import
+ (clojure.lang ExceptionInfo)))
(use-fixtures :each (fn [thunk]
(binding [advanced-config.file/*supported-versions* {:min 1, :max 1}
@@ -80,3 +82,37 @@
(is (zero? (t2/count Table :db_id (u/the-id db))))))))
(finally
(t2/delete! Database :name test-db-name))))))
+
+(deftest delete-test
+ (testing "We should be able to delete Databases from the config file if we pass the confirmation string"
+ (mt/with-temp [Database _ {:name test-db-name
+ :engine "h2"}]
+ (try
+ (binding [advanced-config.file/*config* {:version 1
+ :config {:settings {:config-from-file-sync-databases false}
+ :databases [{:name test-db-name
+ :engine "h2"
+ :details {}
+ :delete (format "DELETE_WITH_DEPENDENTS:%s" test-db-name)}]}}]
+ (is (= :ok
+ (advanced-config.file/initialize!)))
+ (is (not (t2/exists? Database :name test-db-name))))
+ (finally
+ (t2/delete! Database :name test-db-name)))))
+ (testing "We should not delete Databases from the config file if the confirmation string mismatches"
+ (mt/with-temp [Database _ {:name test-db-name
+ :engine "h2"}]
+ (try
+ (binding [advanced-config.file/*config* {:version 1
+ :config {:settings {:config-from-file-sync-databases false}
+ :databases [{:name test-db-name
+ :engine "h2"
+ :details {}
+ :delete "DELETE_WITH_DEPENDENTS:copy-paste-mistake"}]}}]
+ (is (thrown-with-msg?
+ ExceptionInfo
+ (re-pattern (format "To delete database \"%s\" set `delete` to \"DELETE_WITH_DEPENDENTS:%s\"" test-db-name test-db-name))
+ (advanced-config.file/initialize!)))
+ (is (t2/exists? Database :name test-db-name)))
+ (finally
+ (t2/delete! Database :name test-db-name))))))
--
GitLab