github-actions[bot] commented on code in PR #68540:
URL: https://github.com/apache/doris/pull/68540#discussion_r4228073250
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java:
##########
@@ -218,72 +237,121 @@ protected static void pingS3(String bucketName, String
rootPath, Map<String, Str
LOG.info("success to ping s3");
}
+ private static void normalizeProperties(Map<String, String> properties,
String provider) {
+ // Validate the raw aliases before a preferred value can hide
conflicting credentials.
+ GcsAuthResolver.resolve(properties);
+ if (StringUtils.isBlank(provider) &&
GcsAuthResolver.guessIsGcs(properties)) {
+ provider = "GCP";
+ properties.put("provider", provider);
+ }
+ if (StringUtils.isBlank(provider)) {
+ return;
+ }
+ switch (provider.toUpperCase(Locale.ROOT)) {
+ case "GCP":
+ // Normalize before validation, policy checks and persistence,
so FE connector
+ // binding and Resource/Vault protocol builders consume the
same values.
+ GCS_PROPERTY_ALIASES.forEach((alias, key) -> {
+ String value = properties.remove(alias);
+ // Match connector binding: nonblank gs.* values take
precedence.
+ if (StringUtils.isNotBlank(value)) {
+ properties.put(key, value);
+ }
+ });
+ break;
+ default:
+ break;
+ }
+ }
+
@Override
- public void modifyProperties(Map<String, String> properties) throws
DdlException {
+ public synchronized void modifyProperties(Map<String, String>
newProperties) throws DdlException {
+ // Serialize the snapshot, validation and publication. A lock only
around publication
+ // would allow a concurrent ALTER to replace a successful update with
an older snapshot.
+ Map<String, String> properties = new HashMap<>(newProperties);
+ String provider =
StringUtils.defaultIfEmpty(properties.get("provider"),
+ this.properties.get("provider"));
+ if (StringUtils.isBlank(provider)) {
+ Map<String, String> selectionProperties = new
HashMap<>(this.properties);
+ selectionProperties.putAll(properties);
+ if (GcsAuthResolver.guessIsGcs(selectionProperties)) {
+ provider = "GCP";
+ properties.put("provider", provider);
+ }
+ }
+ // Preserve AWS_* ALTER compatibility before merging with stored
canonical properties.
+ S3ResourceCompat.convertToStdProperties(properties);
+ // Resolve aliases separately so this ALTER wins over persisted values
regardless
+ // of their spelling. Within each map, nonblank gs.* values still take
precedence.
+ normalizeProperties(properties, provider);
+ Map<String, String> effectiveProperties = new
HashMap<>(this.properties);
+ normalizeProperties(effectiveProperties, provider);
+ S3ResourceCompat.convertToStdProperties(effectiveProperties);
+ for (Map.Entry<String, String> update : properties.entrySet()) {
+ // Match persistence: empty updates are ignored, except when
clearing a session token.
+ replaceIfEffectiveValue(effectiveProperties, update.getKey(),
update.getValue());
Review Comment:
[P2] Honor an empty GCP impersonation account on ALTER RESOURCE. For a
native GCP resource configured with `gs.credential_provider_type=DEFAULT` and
`gs.impersonation_service_account=o...@project.iam.gserviceaccount.com`, an
`ALTER RESOURCE` setting `gs.impersonation_service_account=""` reaches this
loop, but `replaceIfEffectiveValue` ignores the empty value. Validation and
ping use the retained old account, the ALTER succeeds, and subsequent
storage-policy Thrift still impersonates it. Treat an explicitly empty account
as a clear before validation and publication, and cover the ALTER/readback path.
--
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]
