Add `DisableOldSecretCreation` feature flag for disabling old secret creation (#46165)

7 files changed, 265 insertions, 0 deletions

diff --git a/ydb/core/kqp/host/kqp_gateway_proxy.cpp b/ydb/core/kqp/host/kqp_gateway_proxy.cpp
index 46a28ac2fe7..78670a49611 100644
--- a/ydb/core/kqp/host/kqp_gateway_proxy.cpp
+++ b/ydb/core/kqp/host/kqp_gateway_proxy.cpp
@@ -19,6 +19,7 @@
 #include <ydb/core/ydb_convert/ydb_convert.h>
 #include <ydb/library/formats/arrow/protos/accessor.pb.h>
 #include <ydb/services/metadata/abstract/kqp_common.h>
+#include <ydb/services/metadata/manager/abstract.h>
 
 #include <util/generic/overloaded.h>
 
@@ -2302,6 +2303,10 @@ public:
     TFuture<TGenericResult> UpsertObject(const TString& cluster, const TUpsertObjectSettings& settings) override {
         CHECK_PREPARED_DDL(UpsertObject);
 
+        if (const auto rejected = CheckOldSecretCreationDisabled(settings.GetTypeId())) {
+            return MakeFuture(*rejected);
+        }
+
         if (IsPrepare()) {
             return MakeFuture(PrepareObjectOperation(cluster, settings, &NMetadata::NModifications::IOperationsManager::PrepareUpsertObjectSchemeOperation));
         } else {
@@ -2312,6 +2317,10 @@ public:
     TFuture<TGenericResult> CreateObject(const TString& cluster, const TCreateObjectSettings& settings) override {
         CHECK_PREPARED_DDL(CreateObject);
 
+        if (const auto rejected = CheckOldSecretCreationDisabled(settings.GetTypeId())) {
+            return MakeFuture(*rejected);
+        }
+
         if (IsPrepare()) {
             return MakeFuture(PrepareObjectOperation(cluster, settings, &NMetadata::NModifications::IOperationsManager::PrepareCreateObjectSchemeOperation));
         } else {
@@ -3799,6 +3808,18 @@ public:
     }
 
 private:
+    TMaybe<TGenericResult> CheckOldSecretCreationDisabled(const TString& typeId) const {
+        if (!AppData()->FeatureFlags.GetDisableOldSecretCreation() || to_lower(typeId) != "secret") {
+            return Nothing();
+        }
+        TGenericResult errResult;
+        const auto status = NYql::YqlStatusFromYdbStatus(Ydb::StatusIds::BAD_REQUEST);
+        errResult.SetStatus(status);
+        errResult.AddIssue(NYql::TIssue(NMetadata::NModifications::GetOldSecretCreationDisabledMessage())
+            .SetCode(status, NYql::TSeverityIds::S_ERROR));
+        return errResult;
+    }
+
     bool IsPrepare() const {
         if (!SessionCtx) {
             return false;
diff --git a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
index e40dba8c8dd..f636d97bc2b 100644
--- a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
+++ b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
@@ -14942,6 +14942,86 @@ END DO)",
         }
     }
 
+    Y_UNIT_TEST(OldSecretsCreationDisabled) {
+        NKikimrConfig::TFeatureFlags featureFlags;
+        featureFlags.SetEnableSchemaSecrets(true);
+        featureFlags.SetDisableOldSecretCreation(true);
+        const auto settings = TKikimrSettings()
+            .SetFeatureFlags(featureFlags);
+        TKikimrRunner kikimr(settings);
+        auto db = kikimr.GetTableClient();
+        auto session = db.CreateSession().GetValueSync().GetSession();
+
+        // new schema secrets
+        { // create
+            static const auto query = R"sql(
+                CREATE SECRET `/Root/secret-name-1` WITH (value = "secret-value");
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
+        { // alter
+            static const auto query = R"sql(
+                ALTER SECRET `/Root/secret-name-1` WITH (value = "secret-value");
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
+        { // drop
+            static const auto query = R"sql(
+                DROP SECRET `/Root/secret-name-1`;
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
+
+        // old secrets
+        { // create
+            static const auto query = R"sql(
+                CREATE OBJECT SecretName (TYPE SECRET) WITH value="secret-value";
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::BAD_REQUEST, result.GetIssues().ToString());
+
+            UNIT_ASSERT_STRING_CONTAINS_C(
+                result.GetIssues().ToString(),
+                "Old secrets creation syntax is disabled now. Please use the new one",
+                result.GetIssues().ToString());
+        }
+        { // upsert
+            static const auto query = R"sql(
+                UPSERT OBJECT SecretName (TYPE SECRET) WITH value="secret-value";
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::BAD_REQUEST, result.GetIssues().ToString());
+
+            UNIT_ASSERT_STRING_CONTAINS_C(
+                result.GetIssues().ToString(),
+                "Old secrets creation syntax is disabled now. Please use the new one",
+                result.GetIssues().ToString());
+        }
+        { // alter
+            static const auto query = R"sql(
+                ALTER OBJECT SecretName (TYPE SECRET) SET value="secret-value";
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::GENERIC_ERROR, result.GetIssues().ToString());
+
+            UNIT_ASSERT_STRING_CONTAINS_C(
+                result.GetIssues().ToString(),
+                "preparation problem: secret SecretName not found for alter",
+                result.GetIssues().ToString());
+        }
+        { // drop
+            static const auto query = R"sql(
+                DROP OBJECT SecretName (TYPE SECRET);
+            )sql";
+            const auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            // old secrets pretend that removing non existent secret is fine and succeeded
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
+    }
+
     Y_UNIT_TEST(SimpleTruncateTableFullPathTableClient) {
         TestTruncateTable("`/Root/TestTable`", false);
     }
diff --git a/ydb/core/protos/feature_flags.proto b/ydb/core/protos/feature_flags.proto
index 31feffa7158..ceaf661280c 100644
--- a/ydb/core/protos/feature_flags.proto
+++ b/ydb/core/protos/feature_flags.proto
@@ -339,4 +339,5 @@ message TFeatureFlags {
     optional bool EnableNodeBrokerLongLease = 292 [default = false];
     optional bool EnableExportInParquet = 293 [default = false];
     optional bool EnableCutHistory = 294 [default = false];
+    optional bool DisableOldSecretCreation = 295 [default = false];
 }
diff --git a/ydb/services/metadata/manager/abstract.cpp b/ydb/services/metadata/manager/abstract.cpp
index 8111e7edcfc..6f39345cb4f 100644
--- a/ydb/services/metadata/manager/abstract.cpp
+++ b/ydb/services/metadata/manager/abstract.cpp
@@ -1,8 +1,28 @@
 #include "abstract.h"
+#include <ydb/core/base/appdata.h>
 #include <ydb/services/metadata/service.h>
 
 namespace NKikimr::NMetadata::NModifications {
 
+namespace {
+
+IOperationsManager::TYqlConclusionStatus OldSecretCreationDisabledStatus() {
+    return IOperationsManager::TYqlConclusionStatus::Fail(
+        NYql::TIssuesIds::KIKIMR_BAD_REQUEST,
+        GetOldSecretCreationDisabledMessage());
+}
+
+bool IsOldSecretType(const TString& typeId) {
+    return to_lower(typeId) == "secret";
+}
+
+} // namespace
+
+const TString& GetOldSecretCreationDisabledMessage() {
+    static const TString message("Old secrets creation syntax is disabled now. Please use the new one");
+    return message;
+}
+
 TTableSchema::TTableSchema(const THashMap<ui32, TSysTables::TTableColumnInfo>& description) {
     std::map<TString, Ydb::Column> columns;
     std::map<ui32, Ydb::Column> pkColumns;
@@ -82,6 +102,9 @@ IOperationsManager::TYqlConclusionStatus IOperationsManager::PrepareUpsertObject
     if (!NMetadata::NProvider::TServiceOperator::IsEnabled()) {
         return TYqlConclusionStatus::Fail("metadata provider service is disabled");
     }
+    if (AppData()->FeatureFlags.GetDisableOldSecretCreation() && IsOldSecretType(settings.GetTypeId())) {
+        return OldSecretCreationDisabledStatus();
+    }
     TInternalModificationContext internalContext(context);
     internalContext.SetActivityType(EActivityType::Upsert);
     return DoPrepare(schemeOperation, settings, manager, internalContext);
@@ -93,6 +116,9 @@ IOperationsManager::TYqlConclusionStatus IOperationsManager::PrepareCreateObject
     if (!NMetadata::NProvider::TServiceOperator::IsEnabled()) {
         return TYqlConclusionStatus::Fail("metadata provider service is disabled");
     }
+    if (AppData()->FeatureFlags.GetDisableOldSecretCreation() && IsOldSecretType(settings.GetTypeId())) {
+        return OldSecretCreationDisabledStatus();
+    }
     TInternalModificationContext internalContext(context);
     internalContext.SetActivityType(EActivityType::Create);
     return DoPrepare(schemeOperation, settings, manager, internalContext);
diff --git a/ydb/services/metadata/manager/abstract.h b/ydb/services/metadata/manager/abstract.h
index efd012b29ad..e37d035a85c 100644
--- a/ydb/services/metadata/manager/abstract.h
+++ b/ydb/services/metadata/manager/abstract.h
@@ -23,6 +23,8 @@ namespace NKikimr::NMetadata::NModifications {
 
 using TOperationParsingResult = TConclusion<NInternal::TTableRecord>;
 
+const TString& GetOldSecretCreationDisabledMessage();
+
 class TAlterOperationContext {
 private:
     YDB_READONLY_DEF(TString, SessionId);
diff --git a/ydb/tests/functional/secrets/test_old_secrets_usage.py b/ydb/tests/functional/secrets/test_old_secrets_usage.py
new file mode 100644
index 00000000000..e9622c85163
--- /dev/null
+++ b/ydb/tests/functional/secrets/test_old_secrets_usage.py
@@ -0,0 +1,134 @@
+# -*- coding: utf-8 -*-
+import logging
+
+import pytest
+
+from ydb.tests.library.common.wait_for import wait_for
+from ydb.tests.library.harness.kikimr_runner import KiKiMR
+from ydb.tests.library.harness.kikimr_config import KikimrConfigGenerator
+from ydb.tests.oss.ydb_sdk_import import ydb
+
+logger = logging.getLogger(__name__)
+
+DATABASE = "/Root"
+
+
+class Utils:
+    def __init__(self):
+        self.config = KikimrConfigGenerator(use_in_memory_pdisks=False)
+        self.config.yaml_config["feature_flags"]["enable_external_data_sources"] = True
+
+        self.cluster = KiKiMR(self.config)
+        self.cluster.start()
+
+        self.driver = None
+        self.session_pool = None
+        self._start_client()
+
+    def _start_client(self):
+        self.driver = ydb.Driver(endpoint=self.cluster.nodes[1].endpoint, database=DATABASE)
+        self.driver.wait(5, fail_fast=True)
+        self.session_pool = ydb.SessionPool(self.driver)
+
+    def _stop_client(self):
+        if self.session_pool is not None:
+            self.session_pool.stop()
+            self.session_pool = None
+        if self.driver is not None:
+            self.driver.stop()
+            self.driver = None
+
+    def stop(self):
+        self._stop_client()
+        self.cluster.stop()
+
+    def restart_cluster(self, disable_old_secret_creation, enable_schema_secrets=True):
+        self._stop_client()
+        self.config.yaml_config["feature_flags"]["disable_old_secret_creation"] = disable_old_secret_creation
+        self.config.yaml_config["feature_flags"]["enable_schema_secrets"] = enable_schema_secrets
+        self.cluster.update_configurator_and_restart(self.config)
+        self._start_client_after_restart()
+
+    def _start_client_after_restart(self):
+        driver_holder = {}
+
+        def driver_ready():
+            driver = ydb.Driver(endpoint=self.cluster.nodes[1].endpoint, database=DATABASE)
+            try:
+                driver.wait(timeout=5, fail_fast=True)
+                driver_holder["driver"] = driver
+                return True
+            except Exception as exc:
+                logger.info("Driver not ready yet after cluster restart: %s", exc)
+                driver.stop()
+                return False
+
+        if not wait_for(driver_ready, timeout_seconds=120, step_seconds=1):
+            raise AssertionError("Driver didn't become ready after cluster restart")
+
+        self.driver = driver_holder["driver"]
+        self.session_pool = ydb.SessionPool(self.driver)
+
+    def create_old_secret(self, secret_name, value):
+        with self.session_pool.checkout() as session:
+            session.execute_scheme(f"CREATE OBJECT {secret_name} (TYPE SECRET) WITH value='{value}';")
+
+    def upsert_old_secret(self, secret_name, value):
+        with self.session_pool.checkout() as session:
+            session.execute_scheme(f"UPSERT OBJECT {secret_name} (TYPE SECRET) WITH value='{value}';")
+
+    def alter_old_secret(self, secret_name, value):
+        with self.session_pool.checkout() as session:
+            session.execute_scheme(f"ALTER OBJECT {secret_name} (TYPE SECRET) SET value='{value}';")
+
+    def create_eds(self, eds_name, secret_name):
+        with self.session_pool.checkout() as session:
+            query = f"""
+                CREATE EXTERNAL DATA SOURCE `{eds_name}` WITH (
+                    SOURCE_TYPE="ObjectStorage",
+                    LOCATION="my-bucket",
+                    AUTH_METHOD="SERVICE_ACCOUNT",
+                    SERVICE_ACCOUNT_ID="mysa",
+                    SERVICE_ACCOUNT_SECRET_NAME="{secret_name}"
+                );"""
+            session.execute_scheme(query)
+
+    def create_schema_secret(self, secret_name, value):
+        with self.session_pool.checkout() as session:
+            session.execute_scheme(f"CREATE SECRET {secret_name} WITH (value='{value}');")
+
+
[email protected]
+def old_secrets_utils():
+    utils = Utils()
+    yield utils
+    utils.stop()
+
+
+def test_create_eds_with_old_secret_after_disabling_old_secret_creation(old_secrets_utils):
+    # can create old secrets by default
+    old_secrets_utils.create_old_secret("OldSecret", value="")
+
+    # can use old secrets by default
+    old_secrets_utils.create_eds("eds-before-restart", "OldSecret")
+
+    old_secrets_utils.restart_cluster(disable_old_secret_creation=True)
+
+    # can create schema secrets with old secrets disabled
+    old_secrets_utils.create_schema_secret("NewSecret", value="")
+
+    # can use old secrets with old secrets disabled
+    old_secrets_utils.create_eds("eds-after-restart", "OldSecret")
+
+    # can alter old secrets with old secrets disabled
+    old_secrets_utils.alter_old_secret("OldSecret", "NewValue")
+
+    # can not create old secrets with old secrets disabled
+    with pytest.raises(Exception) as exc_info:
+        old_secrets_utils.create_old_secret("NewOldSecret", value="")
+    assert "Old secrets creation syntax is disabled now. Please use the new one" in str(exc_info.value)
+
+    # can not upsert old secrets with old secrets disabled
+    with pytest.raises(Exception) as exc_info:
+        old_secrets_utils.upsert_old_secret("NewOldSecretUpsert", value="")
+    assert "Old secrets creation syntax is disabled now. Please use the new one" in str(exc_info.value)
diff --git a/ydb/tests/functional/secrets/ya.make b/ydb/tests/functional/secrets/ya.make
index 70a80be37dd..9facf4e8846 100644
--- a/ydb/tests/functional/secrets/ya.make
+++ b/ydb/tests/functional/secrets/ya.make
@@ -9,6 +9,7 @@ INCLUDE(${ARCADIA_ROOT}/ydb/tests/harness_dep.inc)
 
 TEST_SRCS(
     conftest.py
+    test_old_secrets_usage.py
     test_secrets.py
     test_secrets_usage.py
     test_secrets_monitoring.py