Problem
The Paimon storage-option translation introduced by #602 does not follow the Azure configuration contract used by Milvus in two important cases.
1. Incorrect ADLS Gen2 endpoint construction
Milvus normally provides the Azure address as a DNS suffix such as core.windows.net, with the storage account name supplied separately. Paimon currently passes only config.address through BuildEndpointUrl():
|
auto endpoint = [&config]() { |
|
return config.address.empty() ? std::string{} : StorageUri::BuildEndpointUrl(config.address, config.use_ssl); |
|
}; |
and forwards the result directly as azure.endpoint:
|
if (config.cloud_provider == kCloudProviderAzure) { |
|
set("azure.endpoint", endpoint()); |
|
set("azure.account-name", config.access_key_id); |
For the normal configuration this produces:
instead of:
https://<account>.dfs.core.windows.net
The current unit test passes an already-expanded account.dfs.core.windows.net address, so it does not exercise the real Milvus configuration shape:
|
TEST(PaimonStorageOptionsTest, AzureAccountKey) { |
|
ArrowFileSystemConfig config; |
|
config.storage_type = "remote"; |
|
config.cloud_provider = kCloudProviderAzure; |
|
config.address = "account.dfs.core.windows.net"; |
|
config.use_ssl = true; |
|
config.access_key_id = "account"; |
|
config.access_key_value = "account-key"; |
|
|
|
ASSERT_AND_ASSIGN(auto options, ToStorageOptions(config)); |
|
EXPECT_EQ(options.at("azure.endpoint"), "https://account.dfs.core.windows.net"); |
|
EXPECT_EQ(options.at("azure.account-name"), "account"); |
|
EXPECT_EQ(options.at("azure.account-key"), "account-key"); |
2. Azure credential broker is silently ignored
When ArrowFileSystemConfig::IsAzureCredentialBrokerEnabled() is true, the Paimon path neither obtains nor forwards a broker-issued SAS token. It continues through the account-key/service-principal/default-identity logic instead:
|
if (config.cloud_provider == kCloudProviderAzure) { |
|
set("azure.endpoint", endpoint()); |
|
set("azure.account-name", config.access_key_id); |
|
if (config.use_iam) { |
|
auto nonempty_env = [](const char* name) -> const char* { |
|
const auto* value = std::getenv(name); |
|
return value && *value != '\0' ? value : nullptr; |
|
}; |
|
const auto* client_id = nonempty_env("AZURE_CLIENT_ID"); |
|
const auto* client_secret = nonempty_env("AZURE_CLIENT_SECRET"); |
|
const auto* tenant_id = nonempty_env("AZURE_TENANT_ID"); |
|
if (nonempty_env("AZURE_FEDERATED_TOKEN_FILE") && !(client_id && client_secret && tenant_id)) { |
|
return arrow::Status::NotImplemented("Paimon Azure workload identity credentials are not supported"); |
|
} |
|
if (client_id) |
|
set("azure.client-id", client_id); |
|
if (client_secret) |
|
set("azure.client-secret", client_secret); |
|
if (tenant_id) |
|
set("azure.tenant-id", tenant_id); |
|
if (const auto* value = nonempty_env("AZURE_AUTHORITY_HOST")) |
|
set("azure.authority-host", value); |
|
} else { |
|
set("azure.account-key", config.access_key_value); |
|
} |
This can fail authentication or, more seriously, fall through to an unintended default identity/tenant. Broker-issued SAS support already exists for the shared Azure filesystem, Lance, and Iceberg through #599, but Paimon is not connected to it.
Expected behavior
- Construct the ADLS Gen2 endpoint from the account name and Azure suffix for the standard Milvus configuration, while preserving supported explicit/custom and sovereign-cloud endpoints.
- When Azure credential broker mode is enabled:
- integrate the broker-issued SAS credential path; or
- fail explicitly with
NotImplemented until it is supported.
- Never silently fall back to a different credential source when broker mode was requested.
Test coverage
Please add tests for at least:
address = "core.windows.net" plus an account name produces https://<account>.dfs.core.windows.net.
- An explicit/custom or sovereign-cloud endpoint remains correct.
- Broker-enabled configuration either forwards the expected SAS credentials or returns
NotImplemented.
- Broker mode cannot fall through to account-key, service-principal, managed-identity, or anonymous authentication.
References
Problem
The Paimon storage-option translation introduced by #602 does not follow the Azure configuration contract used by Milvus in two important cases.
1. Incorrect ADLS Gen2 endpoint construction
Milvus normally provides the Azure address as a DNS suffix such as
core.windows.net, with the storage account name supplied separately. Paimon currently passes onlyconfig.addressthroughBuildEndpointUrl():milvus-storage/cpp/src/format/paimon/paimon_common.cpp
Lines 28 to 30 in 1fd5edf
and forwards the result directly as
azure.endpoint:milvus-storage/cpp/src/format/paimon/paimon_common.cpp
Lines 60 to 62 in 1fd5edf
For the normal configuration this produces:
instead of:
The current unit test passes an already-expanded
account.dfs.core.windows.netaddress, so it does not exercise the real Milvus configuration shape:milvus-storage/cpp/test/format/paimon/paimon_storage_options_test.cpp
Lines 127 to 139 in 1fd5edf
2. Azure credential broker is silently ignored
When
ArrowFileSystemConfig::IsAzureCredentialBrokerEnabled()is true, the Paimon path neither obtains nor forwards a broker-issued SAS token. It continues through the account-key/service-principal/default-identity logic instead:milvus-storage/cpp/src/format/paimon/paimon_common.cpp
Lines 60 to 84 in 1fd5edf
This can fail authentication or, more seriously, fall through to an unintended default identity/tenant. Broker-issued SAS support already exists for the shared Azure filesystem, Lance, and Iceberg through #599, but Paimon is not connected to it.
Expected behavior
NotImplementeduntil it is supported.Test coverage
Please add tests for at least:
address = "core.windows.net"plus an account name produceshttps://<account>.dfs.core.windows.net.NotImplemented.References