From dc505a6b98ede3211f5e9b1ec4601ad5c6cf0372 Mon Sep 17 00:00:00 2001
From: Lei Zhang <27994433+SWJTU-ZhangLei@users.noreply.github.com>
Date: Fri, 8 Nov 2024 12:14:42 +0800
Subject: [PATCH] [fix](cloud) Add check for alter storage vault type (#43352)

* Add check to avoid alter hdfs property for s3 storage vault or alter
s3 propery for hdfs storage vault
---
 .../meta-service/meta_service_resource.cpp    | 17 ++++
 .../alter/test_alter_vault_type.groovy        | 78 +++++++++++++++++++
 .../vault_p0/create/test_create_vault.groovy  |  2 +-
 3 files changed, 96 insertions(+), 1 deletion(-)
 create mode 100644 regression-test/suites/vault_p0/alter/test_alter_vault_type.groovy

diff --git a/cloud/src/meta-service/meta_service_resource.cpp b/cloud/src/meta-service/meta_service_resource.cpp
index 1f99974360045e..5d4a4d6922771b 100644
--- a/cloud/src/meta-service/meta_service_resource.cpp
+++ b/cloud/src/meta-service/meta_service_resource.cpp
@@ -570,6 +570,15 @@ static int alter_hdfs_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Tr
     }
     StorageVaultPB new_vault;
     new_vault.ParseFromString(val);
+
+    if (!new_vault.has_hdfs_info()) {
+        code = MetaServiceCode::INVALID_ARGUMENT;
+        std::stringstream ss;
+        ss << name << " is not hdfs storage vault";
+        msg = ss.str();
+        return -1;
+    }
+
     auto origin_vault_info = new_vault.DebugString();
     if (vault.has_alter_name()) {
         if (!is_valid_storage_vault_name(vault.alter_name())) {
@@ -671,6 +680,14 @@ static int alter_s3_storage_vault(InstanceInfoPB& instance, std::unique_ptr<Tran
     }
     StorageVaultPB new_vault;
     new_vault.ParseFromString(val);
+    if (!new_vault.has_obj_info()) {
+        code = MetaServiceCode::INVALID_ARGUMENT;
+        std::stringstream ss;
+        ss << name << " is not s3 storage vault";
+        msg = ss.str();
+        return -1;
+    }
+
     if (vault.has_alter_name()) {
         if (!is_valid_storage_vault_name(vault.alter_name())) {
             code = MetaServiceCode::INVALID_ARGUMENT;
diff --git a/regression-test/suites/vault_p0/alter/test_alter_vault_type.groovy b/regression-test/suites/vault_p0/alter/test_alter_vault_type.groovy
new file mode 100644
index 00000000000000..1a5181d6dcfedd
--- /dev/null
+++ b/regression-test/suites/vault_p0/alter/test_alter_vault_type.groovy
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_alter_vault_type", "nonConcurrent") {
+    def suiteName = name;
+    if (!isCloudMode()) {
+        logger.info("skip ${name} case, because not cloud mode")
+        return
+    }
+
+    if (!enableStoragevault()) {
+        logger.info("skip ${name} case, because storage vault not enabled")
+        return
+    }
+
+    def hdfsVaultName = suiteName + "_HDFS"
+
+    sql """
+        CREATE STORAGE VAULT IF NOT EXISTS ${hdfsVaultName}
+        PROPERTIES (
+            "type"="HDFS",
+            "fs.defaultFS"="${getHmsHdfsFs()}",
+            "path_prefix" = "${hdfsVaultName}",
+            "hadoop.username" = "hadoop"
+        );
+    """
+
+    expectExceptionLike({
+        sql """
+            ALTER STORAGE VAULT ${hdfsVaultName}
+            PROPERTIES (
+                "type"="s3",
+                "s3.access_key" = "new_ak"
+            );
+        """
+    }, "is not s3 storage vault")
+
+
+    def s3VaultName = suiteName + "_S3"
+    sql """
+        CREATE STORAGE VAULT IF NOT EXISTS ${s3VaultName}
+        PROPERTIES (
+            "type"="S3",
+            "s3.endpoint"="${getS3Endpoint()}",
+            "s3.region" = "${getS3Region()}",
+            "s3.access_key" = "${getS3AK()}",
+            "s3.secret_key" = "${getS3SK()}",
+            "s3.root.path" = "${s3VaultName}",
+            "s3.bucket" = "${getS3BucketName()}",
+            "s3.external_endpoint" = "",
+            "provider" = "${getS3Provider()}"
+        );
+    """
+
+    expectExceptionLike({
+        sql """
+            ALTER STORAGE VAULT ${s3VaultName}
+            PROPERTIES (
+                "type"="hdfs",
+                "hadoop.username" = "hdfs"
+            );
+        """
+    }, "is not hdfs storage vault")
+}
\ No newline at end of file
diff --git a/regression-test/suites/vault_p0/create/test_create_vault.groovy b/regression-test/suites/vault_p0/create/test_create_vault.groovy
index bf6ddc756dfac8..13c9f8d8283a4a 100644
--- a/regression-test/suites/vault_p0/create/test_create_vault.groovy
+++ b/regression-test/suites/vault_p0/create/test_create_vault.groovy
@@ -171,7 +171,7 @@ suite("test_create_vault", "nonConcurrent") {
 
     expectExceptionLike({
         sql """
-            CREATE STORAGE VAULT IF NOT EXISTS built_in_storage_vault
+            CREATE STORAGE VAULT built_in_storage_vault
             PROPERTIES (
                 "type"="S3",
                 "s3.endpoint"="${getS3Endpoint()}",