github-actions[bot] commented on code in PR #68540:
URL: https://github.com/apache/doris/pull/68540#discussion_r4248820622
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1162,28 +1207,72 @@ static int alter_s3_storage_vault_by_id(InstanceInfoPB&
instance, std::unique_pt
new_vault.mutable_obj_info()->clear_role_arn();
new_vault.mutable_obj_info()->clear_external_id();
new_vault.mutable_obj_info()->clear_cred_provider_type();
+ new_vault.mutable_obj_info()->clear_credential();
new_vault.mutable_obj_info()->set_ak(new_ak_sk_pair.first);
new_vault.mutable_obj_info()->set_sk(new_ak_sk_pair.second);
new_vault.mutable_obj_info()->mutable_encryption_info()->CopyFrom(encryption_info);
}
- if (obj_info.has_role_arn()) {
- new_vault.mutable_obj_info()->clear_ak();
- new_vault.mutable_obj_info()->clear_sk();
- new_vault.mutable_obj_info()->clear_encryption_info();
+ if (obj_info.has_role_arn() || obj_info.has_cred_provider_type()) {
+ // Only selecting AWS authentication replaces other credentials.
Clearing
+ // an unused role must preserve static keys and native GCP credentials.
+ if (!obj_info.role_arn().empty() || obj_info.has_cred_provider_type())
{
+ new_vault.mutable_obj_info()->clear_ak();
+ new_vault.mutable_obj_info()->clear_sk();
+ new_vault.mutable_obj_info()->clear_encryption_info();
+ new_vault.mutable_obj_info()->clear_credential();
+ }
- new_vault.mutable_obj_info()->set_role_arn(obj_info.role_arn());
-
new_vault.mutable_obj_info()->set_cred_provider_type(get_cred_provider_type(obj_info));
- if (obj_info.has_external_id()) {
+ if (obj_info.has_role_arn()) {
+ if (obj_info.role_arn().empty()) {
+ new_vault.mutable_obj_info()->clear_role_arn();
+ // An external ID has no meaning without a role. Clearing the
role
+ // explicitly clears its dependent value as well.
+ new_vault.mutable_obj_info()->clear_external_id();
+ } else {
+
new_vault.mutable_obj_info()->set_role_arn(obj_info.role_arn());
+ if (!new_vault.obj_info().has_cred_provider_type() &&
+ !obj_info.has_cred_provider_type()) {
+ new_vault.mutable_obj_info()->set_cred_provider_type(
+ CredProviderTypePB::INSTANCE_PROFILE);
+ }
+ }
+ }
+ if (obj_info.has_cred_provider_type()) {
+
new_vault.mutable_obj_info()->set_cred_provider_type(obj_info.cred_provider_type());
+ }
+ }
+
+ // external_id is an independent ALTER field. It may be changed without
+ // requiring FE to resend role_arn and cred_provider_type.
+ if (obj_info.has_external_id()) {
+ if (obj_info.external_id().empty()) {
+ new_vault.mutable_obj_info()->clear_external_id();
+ } else {
new_vault.mutable_obj_info()->set_external_id(obj_info.external_id());
}
}
+ if (has_obj_credential(obj_info)) {
+ if (auto error = apply_obj_credential(obj_info,
new_vault.mutable_obj_info());
+ error.has_value()) {
+ code = MetaServiceCode::INVALID_ARGUMENT;
+ msg = *error;
+ return -1;
+ }
+ }
+
if (obj_info.has_use_path_style()) {
new_vault.mutable_obj_info()->set_use_path_style(obj_info.use_path_style());
}
+ if (auto error = validate_obj_authentication(new_vault.obj_info());
error.has_value()) {
Review Comment:
[P2] Preserve unrelated ALTERs for accepted static-key vaults. A built-in S3
vault provisioned through `CreateInstanceRequest.vault.obj_info` with AK/SK and
an explicit `cred_provider_type=DEFAULT` is accepted by
`create_object_info_with_encrypt`, which encrypts the pair and retains that
field. The object client uses the static pair before checking `provider_type`,
so the vault works. This new complete-state check counts even DEFAULT field
presence as AWS authentication and rejects a `use_path_style`-only ALTER as
conflicting with those stored keys; it fails before applying an unrelated
update although CREATE still accepts the same state. Normalize inactive legacy
provider fields or preserve their static-key semantics, and cover
provisioning/readback followed by a noncredential ALTER.
##########
common/cpp/obj-client/auth/gcp/gcp_token_provider.cpp:
##########
@@ -0,0 +1,102 @@
+// 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.
+
+#include "cpp/obj-client/auth/gcp/gcp_token_provider.h"
+
+#include <glog/logging.h>
+#include <google/cloud/credentials.h>
+#include <google/cloud/internal/unified_rest_credentials.h>
+
+#include <utility>
+
+namespace doris {
+namespace {
+
+constexpr char GCS_READ_WRITE_SCOPE[] =
"https://www.googleapis.com/auth/devstorage.read_write";;
+constexpr char CLOUD_PLATFORM_SCOPE[] =
"https://www.googleapis.com/auth/cloud-platform";;
+
+std::shared_ptr<google::cloud::Credentials> make_credentials(const
GcpCredentialConfig& credential,
+ const
std::string& ca_cert_path,
+ GcpTokenScope
token_scope) {
+ google::cloud::Options options;
+ if (!ca_cert_path.empty()) {
+ options.set<google::cloud::CARootsFilePathOption>(ca_cert_path);
+ }
+
+ // A source credential used for impersonation calls the IAM Credentials API
+ // and therefore needs cloud-platform. The resulting target token uses the
+ // caller-selected scope: object read/write for storage requests, or
+ // cloud-platform when it will call IAM signBlob.
+ const char* requested_scope = token_scope == GcpTokenScope::CloudPlatform
+ ? CLOUD_PLATFORM_SCOPE
+ : GCS_READ_WRITE_SCOPE;
+
options.set<google::cloud::ScopesOption>({credential.impersonation_service_account.empty()
+ ? requested_scope
+ : CLOUD_PLATFORM_SCOPE});
+
+ std::shared_ptr<google::cloud::Credentials> credentials;
+ switch (credential.provider_type) {
+ case GcpCredentialProviderType::Default:
+ credentials = google::cloud::MakeGoogleDefaultCredentials(options);
+ break;
+ case GcpCredentialProviderType::ComputeEngine:
+ credentials = google::cloud::MakeComputeEngineCredentials(options);
+ break;
+ }
+ if (!credential.impersonation_service_account.empty()) {
+ auto impersonation_options = options;
+
impersonation_options.set<google::cloud::ScopesOption>({requested_scope});
+ credentials = google::cloud::MakeImpersonateServiceAccountCredentials(
+ std::move(credentials),
credential.impersonation_service_account,
+ std::move(impersonation_options));
+ }
+ return credentials;
+}
+
+} // namespace
+
+GcpTokenProvider::GcpTokenProvider(const GcpCredentialConfig& credential,
+ const std::string& ca_cert_path,
GcpTokenScope token_scope)
+ : GcpTokenProvider(google::cloud::rest_internal::MapCredentials(
Review Comment:
[P2] Return a client error when ADC construction throws. If a BE's
`GOOGLE_APPLICATION_CREDENTIALS` file contains a JSON object such as
`{"type":false}`, the pinned [ADC
loader](https://github.com/googleapis/google-cloud-cpp/blob/v2.45.0/google/cloud/internal/oauth2_google_credentials.cc#L37-L41)
throws a JSON `type_error` during this eager `MapCredentials` call. The native
client constructor, `S3ClientFactory::_create_s3_client` and
`ObjClientHolder::init/reset` do not catch it; a storage-policy push reaches
them through `update_s3_resource` on `TaskWorkerPool`, whose thread entry has
no catch, so the BE terminates instead of reporting a failed credential/client
update. Convert construction exceptions at the Result/Status boundary and cover
malformed ADC through policy/client construction. The signing-time catch runs
after these providers have already been constructed.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergCatalogFactory.java:
##########
@@ -432,6 +435,36 @@ public static Map<String, String>
buildCatalogProperties(IcebergCatalogPropertie
// s3tables: bespoke instantiation. Preserve the skeleton's
base+impl routing.
break;
}
+ chosenS3.ifPresent(storage ->
storage.toBackendProperties().ifPresent(backend -> {
+ Optional<GcsAuth> auth = GcsAuthResolver.resolve(backend.toMap());
+ if (auth.filter(GcsAuth::isAnonymous).isPresent()) {
+ opts.put(AwsClientProperties.CLIENT_CREDENTIALS_PROVIDER,
+
"software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider");
+ }
+ auth.flatMap(GcsAuth::getNativeCredential).ifPresent(credential ->
{
+ // HadoopCatalog resolves its namespace filesystem
independently of S3FileIO.
+ // Native GCS credentials configure fs.gs.*, so compatibility
warehouse schemes
+ // must select that same filesystem before HadoopCatalog
initializes.
+ String warehouse =
opts.get(CatalogProperties.WAREHOUSE_LOCATION);
+ if (IcebergCatalogProperties.TYPE_HADOOP.equals(flavor) &&
warehouse != null) {
+ if (warehouse.regionMatches(true, 0, "s3://", 0, 5)) {
+ opts.put(CatalogProperties.WAREHOUSE_LOCATION, "gs://"
+ warehouse.substring(5));
+ } else if (warehouse.regionMatches(true, 0, "s3a://", 0,
6)) {
+ opts.put(CatalogProperties.WAREHOUSE_LOCATION, "gs://"
+ warehouse.substring(6));
+ }
+ }
+ putS3FileIODialect(opts, storage);
+ opts.put("provider", "GCP");
+ opts.put(GcpCredential.CREDENTIAL_PROVIDER_TYPE,
credential.getCredentialProviderType().name());
+ putIfNotBlank(opts,
GcpCredential.IMPERSONATION_SERVICE_ACCOUNT,
+ credential.getImpersonationServiceAccount());
+ // Keep explicit FileIO choices. By default, S3 aliases use
the GCP client factory,
+ // while gs/HDFS paths use HadoopFileIO with the native
fs.gs.* configuration
+ // (the plugin does not bundle iceberg-gcp's GCSFileIO).
+ opts.putIfAbsent(CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.io.ResolvingFileIO");
Review Comment:
[P2] Preserve native auth for aliases with explicit HadoopFileIO. For a
native-GCP HMS/REST catalog with
`io-impl=org.apache.iceberg.hadoop.HadoopFileIO` and a private GCS table whose
`metadata_location` uses `s3a://gcp-bucket/...`, this preserves HadoopFileIO
but native storage config contains only `fs.gs.*`. Only a HadoopCatalog
warehouse is rewritten above;
[HadoopFileIO](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java#L84-L95)
passes the table path unchanged to Hadoop, so S3A uses its AWS/default
credentials and endpoint rather than the selected GCP OAuth/impersonation
identity. Metadata reads fail on a VM with no AWS keys although the default
ResolvingFileIO path works. Handle aliases belonging to this GCS binding for
explicit HadoopFileIO, or reject the unsupported combination, and cover that
metadata read; the current explicit-IO test only reads a `file:///` path. This
differs from the existing mixe
d-AWS-table thread: it loses authentication for a GCS table under the
preserved custom FileIO.
--
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]
