Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix](storage vault) Fix missing use_path_style when create storage vault #45155

Merged
merged 2 commits into from
Dec 17, 2024
Merged

[fix](storage vault) Fix missing use_path_style when create storage vault #45155

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96b948c
[fix](storage vault) Fix missing use_path_style when create storage v…
gavinchou Dec 7, 2024
dbe00c2
Update fe/fe-core/src/main/java/org/apache/doris/datasource/property/…
gavinchou Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions cloud/src/meta-service/meta_service_resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,7 @@ struct ObjectStorageDesc {
std::string& endpoint;
std::string& external_endpoint;
std::string& region;
bool& use_path_style;
};

static int extract_object_storage_info(const AlterObjStoreInfoRequest* request,
Expand All @@ -765,7 +766,7 @@ static int extract_object_storage_info(const AlterObjStoreInfoRequest* request,
msg = "s3 obj info err " + proto_to_json(*request);
return -1;
}
auto& [ak, sk, bucket, prefix, endpoint, external_endpoint, region] = obj_desc;
auto& [ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style] = obj_desc;
const auto& obj = request->has_obj() ? request->obj() : request->vault().obj_info();
// Prepare data
if (!obj.has_ak() || !obj.has_sk()) {
Expand All @@ -791,6 +792,7 @@ static int extract_object_storage_info(const AlterObjStoreInfoRequest* request,
endpoint = obj.has_endpoint() ? obj.endpoint() : "";
external_endpoint = obj.has_external_endpoint() ? obj.external_endpoint() : "";
region = obj.has_region() ? obj.region() : "";
use_path_style = obj.use_path_style();
// obj size > 1k, refuse
if (obj.ByteSizeLong() > 1024) {
code = MetaServiceCode::INVALID_ARGUMENT;
Expand All @@ -800,13 +802,13 @@ static int extract_object_storage_info(const AlterObjStoreInfoRequest* request,
return 0;
}

static ObjectStoreInfoPB object_info_pb_factory(ObjectStorageDesc& obj_info,
static ObjectStoreInfoPB object_info_pb_factory(ObjectStorageDesc& obj_desc,
const ObjectStoreInfoPB& obj,
InstanceInfoPB& instance,
EncryptionInfoPB& encryption_info,
AkSkPair& cipher_ak_sk_pair) {
ObjectStoreInfoPB last_item;
auto& [ak, sk, bucket, prefix, endpoint, external_endpoint, region] = obj_info;
auto& [ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style] = obj_desc;
auto now_time = std::chrono::system_clock::now();
uint64_t time =
std::chrono::duration_cast<std::chrono::seconds>(now_time.time_since_epoch()).count();
Expand All @@ -828,6 +830,7 @@ static ObjectStoreInfoPB object_info_pb_factory(ObjectStorageDesc& obj_info,
last_item.set_region(region);
last_item.set_provider(obj.provider());
last_item.set_sse_enabled(instance.sse_enabled());
last_item.set_use_path_style(use_path_style);
return last_item;
}

Expand All @@ -836,14 +839,15 @@ void MetaServiceImpl::alter_storage_vault(google::protobuf::RpcController* contr
AlterObjStoreInfoResponse* response,
::google::protobuf::Closure* done) {
std::string ak, sk, bucket, prefix, endpoint, external_endpoint, region;
bool use_path_style;
EncryptionInfoPB encryption_info;
AkSkPair cipher_ak_sk_pair;
RPC_PREPROCESS(alter_storage_vault);
switch (request->op()) {
case AlterObjStoreInfoRequest::ADD_S3_VAULT:
case AlterObjStoreInfoRequest::DROP_S3_VAULT: {
auto tmp_desc =
ObjectStorageDesc {ak, sk, bucket, prefix, endpoint, external_endpoint, region};
auto tmp_desc = ObjectStorageDesc {
ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style};
if (0 != extract_object_storage_info(request, code, msg, tmp_desc, encryption_info,
cipher_ak_sk_pair)) {
return;
Expand Down Expand Up @@ -982,8 +986,8 @@ void MetaServiceImpl::alter_storage_vault(google::protobuf::RpcController* contr
}
}
// calc id
auto tmp_tuple =
ObjectStorageDesc {ak, sk, bucket, prefix, endpoint, external_endpoint, region};
auto tmp_tuple = ObjectStorageDesc {
ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style};
ObjectStoreInfoPB last_item = object_info_pb_factory(tmp_tuple, obj, instance,
encryption_info, cipher_ak_sk_pair);
if (instance.storage_vault_names().end() !=
Expand Down Expand Up @@ -1126,15 +1130,16 @@ void MetaServiceImpl::alter_obj_store_info(google::protobuf::RpcController* cont
AlterObjStoreInfoResponse* response,
::google::protobuf::Closure* done) {
std::string ak, sk, bucket, prefix, endpoint, external_endpoint, region;
bool use_path_style;
EncryptionInfoPB encryption_info;
AkSkPair cipher_ak_sk_pair;
RPC_PREPROCESS(alter_obj_store_info);
switch (request->op()) {
case AlterObjStoreInfoRequest::ADD_OBJ_INFO:
case AlterObjStoreInfoRequest::LEGACY_UPDATE_AK_SK:
case AlterObjStoreInfoRequest::UPDATE_AK_SK: {
auto tmp_desc =
ObjectStorageDesc {ak, sk, bucket, prefix, endpoint, external_endpoint, region};
auto tmp_desc = ObjectStorageDesc {
ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style};
if (0 != extract_object_storage_info(request, code, msg, tmp_desc, encryption_info,
cipher_ak_sk_pair)) {
return;
Expand Down Expand Up @@ -1273,8 +1278,8 @@ void MetaServiceImpl::alter_obj_store_info(google::protobuf::RpcController* cont
}
}
// calc id
auto tmp_tuple =
ObjectStorageDesc {ak, sk, bucket, prefix, endpoint, external_endpoint, region};
auto tmp_tuple = ObjectStorageDesc {
ak, sk, bucket, prefix, endpoint, external_endpoint, region, use_path_style};
ObjectStoreInfoPB last_item = object_info_pb_factory(tmp_tuple, obj, instance,
encryption_info, cipher_ak_sk_pair);
instance.add_obj_info()->CopyFrom(last_item);
Expand Down
4 changes: 4 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/catalog/StorageVault.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.doris.common.AnalysisException;
import org.apache.doris.common.DdlException;
import org.apache.doris.common.UserException;
import org.apache.doris.datasource.property.PropertyConverter;
import org.apache.doris.qe.ShowResultSetMetaData;

import com.google.common.base.Strings;
Expand Down Expand Up @@ -144,6 +145,9 @@ public void setId(String id) {
vault.modifyProperties(stmt.getProperties());
break;
case S3:
if (!stmt.getProperties().containsKey(PropertyConverter.USE_PATH_STYLE)) {
stmt.getProperties().put(PropertyConverter.USE_PATH_STYLE, "true");
}
CreateResourceStmt resourceStmt =
new CreateResourceStmt(false, ifNotExists, name, stmt.getProperties());
resourceStmt.analyzeResourceType();
Expand Down
3 changes: 3 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/S3Properties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,9 @@ public static Cloud.ObjectStoreInfoPB.Builder getObjStoreInfoPB(Map<String, Stri
|| value.equalsIgnoreCase("false"),
"Invalid use_path_style value: %s only 'true' or 'false' is acceptable", value);
builder.setUsePathStyle(value.equalsIgnoreCase("true"));
} else {
// use path style
builder.setUsePathStyle(true);
gavinchou marked this conversation as resolved.
Show resolved Hide resolved
}
return builder;
}
Expand Down
48 changes: 48 additions & 0 deletions regression-test/suites/vault_p0/create/load.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,54 @@ suite("create_storage_vault", "nonConcurrent") {
);
"""

// test path style
sql """
CREATE STORAGE VAULT IF NOT EXISTS create_s3_vault1
PROPERTIES (
"type"="S3",
"s3.endpoint"="${getS3Endpoint()}",
"s3.region" = "${getS3Region()}",
"s3.access_key" = "${getS3AK()}",
"s3.secret_key" = "${getS3SK()}",
"s3.root.path" = "test_create_s3_vault1",
"s3.bucket" = "${getS3BucketName()}",
"s3.external_endpoint" = "",
"use_path_style" = "true",
"provider" = "${getS3Provider()}"
);
"""

// test path style
sql """
CREATE STORAGE VAULT IF NOT EXISTS create_s3_vault2
PROPERTIES (
"type"="S3",
"s3.endpoint"="${getS3Endpoint()}",
"s3.region" = "${getS3Region()}",
"s3.access_key" = "${getS3AK()}",
"s3.secret_key" = "${getS3SK()}",
"s3.root.path" = "test_create_s3_vault2",
"s3.bucket" = "${getS3BucketName()}",
"s3.external_endpoint" = "",
"use_path_style" = "false",
"provider" = "${getS3Provider()}"
);
"""

var result = """show storage vault"""
assertTrue(result.size() >= 3);
for (int i = 0; i < result.size(); ++i) {
if (result[i][0].equals("create_s3_vault")) {
assertTrue(result[i][2].contains("use_path_style: true"));
}
if (result[i][0].equals("create_s3_vault1")) {
assertTrue(result[i][2].contains("use_path_style: true"));
}
if (result[i][0].equals("create_s3_vault2")) {
assertTrue(result[i][2].contains("use_path_style: false"));
}
}

expectExceptionLike({
sql """
CREATE STORAGE VAULT create_s3_vault
Expand Down
Loading