github-actions[bot] commented on code in PR #68540:
URL: https://github.com/apache/doris/pull/68540#discussion_r4119930347
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/storage/ObjectInfoAdapter.java:
##########
@@ -74,8 +74,17 @@ public static StorageAdapter toStorageAdapter(ObjectInfo
obj) {
case OSS:
return StorageAdapter.ofProvider("OSS",
buildS3CompatibleProps(obj));
case S3:
- case GCP:
return StorageAdapter.ofProvider("S3",
buildS3CompatibleProps(obj));
+ case GCP:
+ Map<String, String> gcpProperties =
buildS3CompatibleProps(obj);
+ gcpProperties.put("provider", "GCP");
Review Comment:
[P1] Pass the native stage credential into COPY INTO scan properties. The
new adapter forwards gs.credential_provider_type and
gs.impersonation_service_account for FE filesystem calls, but
CopyIntoInfo.validateStagePB builds brokerProperties manually from AK/SK,
endpoint, bucket, prefix and provider, omitting both fields. Even after
get_stage preserves the credential, the BrokerDesc sent to BE resolves GCP as
DEFAULT ADC, so COMPUTE_ENGINE or impersonated COPY reads use the wrong
identity. Please carry these fields into that broker map and test the BE scan
properties.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -781,14 +782,23 @@ static void create_object_info_with_encrypt(const
InstanceInfoPB& instance, Obje
std::string external_endpoint = obj->has_external_endpoint() ?
obj->external_endpoint() : "";
std::string region = obj->has_region() ? obj->region() : "";
- if (obj->has_role_arn()) {
- if (obj->role_arn().empty() || !obj->has_cred_provider_type() ||
!obj->has_provider() ||
+ if (has_obj_credential(*obj)) {
Review Comment:
[P1] Preserve the GCP credential when returning an internal stage. This
branch now accepts and stores a native GCP credential for instance object
storage, but get_stage rebuilds the INTERNAL StagePB.obj_info field by field at
4199-4217 without old_obj.credential. A COMPUTE_ENGINE or impersonated instance
is then returned to FE as provider=GCP with no credential, so stage operations
silently use DEFAULT ADC. Please copy the credential envelope into the stage
response and cover the get_stage path in a test.
##########
regression-test/suites/object_storage_iam_p0/test_alter_resource_with_role.groovy:
##########
@@ -0,0 +1,234 @@
+// 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.
+
+import groovy.json.JsonSlurper
+import org.apache.doris.regression.util.ObjectStorageIamTestUtils
+
+suite("test_alter_resource_with_role") {
+ if (!ObjectStorageIamTestUtils.isProvider(context.config.otherConfigs,
"AWS")) {
+ logger.info("skip ${name} because objectStorageIamProvider is not AWS")
+ return
+ }
+ def config =
ObjectStorageIamTestUtils.getConfig(context.config.otherConfigs)
+
+
+ if (isCloudMode()) {
+ logger.info("skip ${name} case, because it is cloud mode")
+ return
+ }
+
+ def tableName = "test_alter_resource_with_role"
+ def randomStr = UUID.randomUUID().toString().replace("-", "")
+ def resourceName = "alter_resource_${randomStr}"
+ def policyName = "alter_policy_${randomStr}"
+
+ def awsEndpoint = config.endpoint
+ def region = config.region
+ def bucket = config.bucket
+ def roleArn = config.roleArn
+ def externalId = config.externalId ?: ""
+ def prefix = config.prefix
+
+ def awsAccessKey = context.config.awsAccessKey
+ def awsSecretKey = context.config.awsSecretKey
+
+ sql """
+ CREATE RESOURCE IF NOT EXISTS "${resourceName}"
+ PROPERTIES(
+ "type"="s3",
+ "AWS_ENDPOINT" = "${awsEndpoint}",
+ "AWS_REGION" = "${region}",
+ "AWS_BUCKET" = "${bucket}",
+ "AWS_ROOT_PATH" =
"${prefix}/test_alter_resource_with_role/${randomStr}",
+ "AWS_ACCESS_KEY" = "error_ak",
+ "AWS_SECRET_KEY" = "error_sk",
+ "s3_validity_check" = "false"
+ );
+ """
+
+ sql """
+ CREATE STORAGE POLICY IF NOT EXISTS ${policyName}
+ PROPERTIES(
+ "storage_resource" = "${resourceName}",
+ "cooldown_ttl" = "1"
+ )
+ """
+
+ sql """
+ DROP TABLE IF EXISTS ${tableName} FORCE;
+ """
+
+ sql """
+ CREATE TABLE ${tableName}
+ (
+ siteid INT DEFAULT '10',
+ citycode SMALLINT NOT NULL,
+ username VARCHAR(32) DEFAULT '',
+ pv BIGINT SUM DEFAULT '0'
+ )
+ AGGREGATE KEY(siteid, citycode, username)
+ DISTRIBUTED BY HASH(siteid) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "storage_policy" = "${policyName}"
+ )
+ """
+
+ sql """insert into ${tableName}(siteid, citycode, username, pv) values (1,
1, "xxx", 1),
+ (2, 2, "yyy", 2),
+ (3, 3, "zzz", 3)
+ """
+
+ def result = sql """ SHOW RESOURCES WHERE NAME = "${resourceName}"; """
+ log.info("result:${result}")
+ assertTrue(!result.toString().contains(roleArn))
+ assertTrue(!result.toString().contains(externalId));
Review Comment:
[P2] Handle an omitted external ID in this new AWS suite.
objectStorageIamAwsExternalId defaults to empty and getAwsAuthCases explicitly
omits it when empty, but externalId becomes "" here and
result.toString().contains("") is always true. This assertion fails before the
ALTER, and the same assertion at line 203 fails too. Please guard the absence
checks when externalId is nonempty.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -781,14 +782,23 @@ static void create_object_info_with_encrypt(const
InstanceInfoPB& instance, Obje
std::string external_endpoint = obj->has_external_endpoint() ?
obj->external_endpoint() : "";
std::string region = obj->has_region() ? obj->region() : "";
- if (obj->has_role_arn()) {
- if (obj->role_arn().empty() || !obj->has_cred_provider_type() ||
!obj->has_provider() ||
+ if (has_obj_credential(*obj)) {
+ if (auto error = validate_and_normalize_obj_credential(obj);
error.has_value()) {
Review Comment:
[P1] Clear native GCP auth when legacy instance keys replace it. After this
branch stores a GCP credential in instance.obj_info,
ALTER_OBJ_INFO/LEGACY_UPDATE_AK_SK and update_ak_sk can set new AK/SK on that
same object without clearing credential. BE and recycler still select the
retained GCP OAuth client, so the update reports success while the new keys are
unused; FE may also reject the mixed credential state. Please clear the native
envelope on a key transition or reject the update, and test this sequence.
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Syncer.groovy:
##########
@@ -1027,25 +1027,15 @@ class Syncer {
"""
}
- void createS3RepositoryWithRole(String name, boolean readOnly = false) {
- String roleArn = suite.context.config.awsRoleArn
- String externalId = suite.context.config.awsExternalId
- String endpoint = suite.context.config.awsEndpoint
- String region = suite.context.config.awsRegion
- String bucket = suite.context.config.awsBucket
- String prefix = suite.context.config.awsPrefix
-
+ void createObjectStorageIamRepository(String name, Map iamConfig, Map
authCase, boolean readOnly = false) {
Review Comment:
[P2] Keep the existing AWS IAM backup/restore suite callable. This replaces
createS3RepositoryWithRole, but
aws_iam_role_p0/test_backup_restore_with_role.groovy:41 still calls
syncer.createS3RepositoryWithRole(repoName) when awsRoleArn is configured. That
suite now fails with MissingMethodException before exercising backup/restore.
Please retain a delegating helper or migrate the old suite to
createObjectStorageIamRepository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
