This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new b1d9f85802 GH-43197: [C++][AzureFS] Ignore password field in URI
(#44220)
b1d9f85802 is described below
commit b1d9f85802e578709aada3c410adae8d718a6744
Author: Sutou Kouhei <[email protected]>
AuthorDate: Tue Oct 1 08:50:46 2024 +0900
GH-43197: [C++][AzureFS] Ignore password field in URI (#44220)
### Rationale for this change
Other Azure Blob Storage based filesystem API implementations don't use
password field in URI.
We don't use it too for compatibility.
### What changes are included in this PR?
Ignore password field.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
**This PR includes breaking changes to public APIs.**
* GitHub Issue: #43197
Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
cpp/src/arrow/filesystem/azurefs.cc | 52 ++++++++++----------------------
cpp/src/arrow/filesystem/azurefs.h | 10 +++---
cpp/src/arrow/filesystem/azurefs_test.cc | 50 +++++++++++-------------------
3 files changed, 38 insertions(+), 74 deletions(-)
diff --git a/cpp/src/arrow/filesystem/azurefs.cc
b/cpp/src/arrow/filesystem/azurefs.cc
index d407b1654f..6fe1dd3695 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -101,7 +101,6 @@ void AzureOptions::ExtractFromUriSchemeAndHierPart(const
Uri& uri,
}
Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
- const auto account_key = uri.password();
std::optional<CredentialKind> credential_kind;
std::optional<std::string> credential_kind_value;
std::string tenant_id;
@@ -155,10 +154,6 @@ Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
}
if (credential_kind) {
- if (!account_key.empty()) {
- return Status::Invalid("Password must not be specified with
credential_kind=",
- *credential_kind_value);
- }
if (!tenant_id.empty()) {
return Status::Invalid("tenant_id must not be specified with
credential_kind=",
*credential_kind_value);
@@ -190,40 +185,25 @@ Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
break;
}
} else {
- if (!account_key.empty()) {
- // With password
- if (!tenant_id.empty()) {
- return Status::Invalid("tenant_id must not be specified with
password");
- }
- if (!client_id.empty()) {
- return Status::Invalid("client_id must not be specified with
password");
- }
- if (!client_secret.empty()) {
- return Status::Invalid("client_secret must not be specified with
password");
+ if (tenant_id.empty() && client_id.empty() && client_secret.empty()) {
+ // No related parameters
+ if (account_name.empty()) {
+ RETURN_NOT_OK(ConfigureAnonymousCredential());
+ } else {
+ // Default credential
}
- RETURN_NOT_OK(ConfigureAccountKeyCredential(account_key));
} else {
- // Without password
- if (tenant_id.empty() && client_id.empty() && client_secret.empty()) {
- // No related parameters
- if (account_name.empty()) {
- RETURN_NOT_OK(ConfigureAnonymousCredential());
- } else {
- // Default credential
- }
+ // One or more tenant_id, client_id or client_secret are specified
+ if (client_id.empty()) {
+ return Status::Invalid("client_id must be specified");
+ }
+ if (tenant_id.empty() && client_secret.empty()) {
+ RETURN_NOT_OK(ConfigureManagedIdentityCredential(client_id));
+ } else if (!tenant_id.empty() && !client_secret.empty()) {
+ RETURN_NOT_OK(
+ ConfigureClientSecretCredential(tenant_id, client_id,
client_secret));
} else {
- // One or more tenant_id, client_id or client_secret are specified
- if (client_id.empty()) {
- return Status::Invalid("client_id must be specified");
- }
- if (tenant_id.empty() && client_secret.empty()) {
- RETURN_NOT_OK(ConfigureManagedIdentityCredential(client_id));
- } else if (!tenant_id.empty() && !client_secret.empty()) {
- RETURN_NOT_OK(
- ConfigureClientSecretCredential(tenant_id, client_id,
client_secret));
- } else {
- return Status::Invalid("Both of tenant_id and client_secret must be
specified");
- }
+ return Status::Invalid("Both of tenant_id and client_secret must be
specified");
}
}
}
diff --git a/cpp/src/arrow/filesystem/azurefs.h
b/cpp/src/arrow/filesystem/azurefs.h
index ebbe00c4ee..c5e5091256 100644
--- a/cpp/src/arrow/filesystem/azurefs.h
+++ b/cpp/src/arrow/filesystem/azurefs.h
@@ -144,12 +144,10 @@ struct ARROW_EXPORT AzureOptions {
///
/// Supported formats:
///
- /// 1. abfs[s]://[:\<password\>@]\<account\>.blob.core.windows.net
- /// [/\<container\>[/\<path\>]]
- /// 2.
abfs[s]://\<container\>[:\<password\>]\@\<account\>.dfs.core.windows.net[/path]
- /// 3. abfs[s]://[\<account[:\<password\>]@]\<host[.domain]\>[\<:port\>]
- /// [/\<container\>[/path]]
- /// 4. abfs[s]://[\<account[:\<password\>]@]\<container\>[/path]
+ /// 1. abfs[s]://\<account\>.blob.core.windows.net[/\<container\>[/\<path\>]]
+ /// 2. abfs[s]://\<container\>\@\<account\>.dfs.core.windows.net[/path]
+ /// 3.
abfs[s]://[\<account@]\<host[.domain]\>[\<:port\>][/\<container\>[/path]]
+ /// 4. abfs[s]://[\<account@]\<container\>[/path]
///
/// (1) and (2) are compatible with the Azure Data Lake Storage Gen2 URIs
/// [1], (3) is for Azure Blob Storage compatible service including Azurite,
diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc
b/cpp/src/arrow/filesystem/azurefs_test.cc
index a8dc923476..a8087bfc16 100644
--- a/cpp/src/arrow/filesystem/azurefs_test.cc
+++ b/cpp/src/arrow/filesystem/azurefs_test.cc
@@ -562,16 +562,15 @@ class TestAzureOptions : public ::testing::Test {
void TestFromUriAbfs() {
std::string path;
- ASSERT_OK_AND_ASSIGN(
- auto options,
- AzureOptions::FromUri(
- "abfs://account:[email protected]:10000/container/dir/blob",
&path));
+ ASSERT_OK_AND_ASSIGN(auto options,
+ AzureOptions::FromUri(
+
"abfs://[email protected]:10000/container/dir/blob", &path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "https");
ASSERT_EQ(options.dfs_storage_scheme, "https");
- ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kStorageSharedKey);
+ ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}
@@ -579,43 +578,42 @@ class TestAzureOptions : public ::testing::Test {
void TestFromUriAbfss() {
std::string path;
ASSERT_OK_AND_ASSIGN(
- auto options,
- AzureOptions::FromUri(
- "abfss://account:[email protected]:10000/container/dir/blob",
&path));
+ auto options, AzureOptions::FromUri(
+
"abfss://[email protected]:10000/container/dir/blob", &path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "https");
ASSERT_EQ(options.dfs_storage_scheme, "https");
- ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kStorageSharedKey);
+ ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}
void TestFromUriEnableTls() {
std::string path;
- ASSERT_OK_AND_ASSIGN(auto options,
- AzureOptions::FromUri(
-
"abfs://account:[email protected]:10000/container/dir/blob?"
- "enable_tls=false",
- &path));
+ ASSERT_OK_AND_ASSIGN(
+ auto options,
+
AzureOptions::FromUri("abfs://[email protected]:10000/container/dir/blob?"
+ "enable_tls=false",
+ &path));
ASSERT_EQ(options.account_name, "account");
ASSERT_EQ(options.blob_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.dfs_storage_authority, "127.0.0.1:10000");
ASSERT_EQ(options.blob_storage_scheme, "http");
ASSERT_EQ(options.dfs_storage_scheme, "http");
- ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kStorageSharedKey);
+ ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kDefault);
ASSERT_EQ(path, "container/dir/blob");
ASSERT_EQ(options.background_writes, true);
}
void TestFromUriDisableBackgroundWrites() {
std::string path;
- ASSERT_OK_AND_ASSIGN(auto options,
- AzureOptions::FromUri(
-
"abfs://account:[email protected]:10000/container/dir/blob?"
- "background_writes=false",
- &path));
+ ASSERT_OK_AND_ASSIGN(
+ auto options,
+
AzureOptions::FromUri("abfs://[email protected]:10000/container/dir/blob?"
+ "background_writes=false",
+ &path));
ASSERT_EQ(options.background_writes, false);
}
@@ -637,15 +635,6 @@ class TestAzureOptions : public ::testing::Test {
ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kAnonymous);
}
- void TestFromUriCredentialStorageSharedKey() {
- ASSERT_OK_AND_ASSIGN(
- auto options,
- AzureOptions::FromUri(
-
"abfs://:[email protected]/container/dir/blob",
- nullptr));
- ASSERT_EQ(options.credential_kind_,
AzureOptions::CredentialKind::kStorageSharedKey);
- }
-
void TestFromUriCredentialClientSecret() {
ASSERT_OK_AND_ASSIGN(
auto options,
@@ -767,9 +756,6 @@ TEST_F(TestAzureOptions, FromUriDisableBackgroundWrites) {
}
TEST_F(TestAzureOptions, FromUriCredentialDefault) {
TestFromUriCredentialDefault(); }
TEST_F(TestAzureOptions, FromUriCredentialAnonymous) {
TestFromUriCredentialAnonymous(); }
-TEST_F(TestAzureOptions, FromUriCredentialStorageSharedKey) {
- TestFromUriCredentialStorageSharedKey();
-}
TEST_F(TestAzureOptions, FromUriCredentialClientSecret) {
TestFromUriCredentialClientSecret();
}
