GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri() - #40325

Merged
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri
Mar 11, 2024
Merged

GH-40028: [C++][FS][Azure] Add AzureFileSystem support to FileSystemFromUri()#40325
kou merged 9 commits into
apache:mainfrom
kou:cpp-azurefs-from-uri

Conversation

@kou

@koukou commented Mar 3, 2024

Copy link
Copy Markdown
Member

Rationale for this change

FileSystemFromUri() is a common API to create a file system object. FileSystemFromUri() should be able to create an AzureFileSystem object.

What changes are included in this PR?

Add AzureOptions::FromUri() and use it from FileSystemFromUri().

See the AzureOptions::FromUri()'s docstring about the supported formats.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@kou
kou requested a review from felipecrvMarch 3, 2024 09:34
@github-actions

ghost commented Mar 3, 2024

Copy link
Copy Markdown

⚠️ GitHub issue #40028has been automatically assigned in GitHub to PR creator.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

@Tom-Newton

ghost commented Mar 3, 2024

Copy link
Copy Markdown
Contributor

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

@nosterlu

ghost commented Mar 3, 2024

Copy link
Copy Markdown

@Tom-Newton Are abfs:// and abfss:// natural for AzureFileSystem?

I would say yes. I think originally they came from Hadoop with the extra "s" indicating secure but other filesystem implementations seem to have adopted it and they seem to be used mostly interchangeably.

From the documentation if it helps

The abfs protocol is used as the scheme identifier. If you add an s at the end (abfss) then the ABFS Hadoop client driver will always use Transport Layer Security (TLS) irrespective of the authentication method chosen. If you choose OAuth as your authentication, then the client driver will always use TLS even if you specify abfs instead of abfss because OAuth solely relies on the TLS layer. Finally, if you choose to use the older method of storage account key, then the client driver interprets abfs to mean that you don't want to use TLS.

@kou

ghost commented Mar 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks for the note. I've added the documentation URL as a comment.

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +68 to +69
Result<AzureOptions> AzureOptions::FromUri(const arrow::internal::Uri& uri,
std::string* out_path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR by @bkietz is moving Uri out of internal so we should be careful with the merges.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info!
#39067

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +108 to +110
if (container.empty()) {
return Status::Invalid("Missing container name in Azure Blob File System URI");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need a container name if the filesystem wraps the entire storage account?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need this check. I'll remove this.
(I used GcsOptions::FromUri() as a base implementation and forgot to remove this check.)

Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment on lines +115 to +119
std::unordered_map<std::string, std::string> options_map;
ARROW_ASSIGN_OR_RAISE(const auto options_items, uri.query_items());
for (const auto& kv : options_items) {
options_map.emplace(kv.first, kv.second);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build the map if you're going to iterate over the kv pairs and switch. This is just randomizing the iteration order.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I borrowed this implementation from GcsOptions::FromUri() but I should have noticed this.
(We should remove this conversion in GcsOptions::FromUri() too later.)

options.blob_storage_scheme = kv.second;
} else if (kv.first == "dfs_storage_scheme") {
options.dfs_storage_scheme = kv.second;
} else if (kv.first == "credential_kind") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind_ should be inferred from what you find on the URI without the user having to set both the credential kind and the credentials.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that we should use ConfigureClientSecretCredential() if tenant_id, client_id and client_secret are specified but credential_kind=client_secret isn't specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credential_kind should never be specified and we should validate the URI to keep the invariant that it doesn't configure two different auth methods. And when nothing is provided, we use the default auth chain provided by the SDK.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can we distinguish ConfigureAnonymousCredential(), ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential()? All of them don't require additional information.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter-less auth methods can have dedicated query params for each. These being the valid configurations regarding auth:

  • nothing (use default auth chain)
  • ?anonymous
  • ?use_workload_identity
  • ?account_key=<ACCOUNT_KEY>
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET>
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?anonymous and ?use_workaround_identity are conflicted parameters. (We can't specify both of them at once.) I think that it's better that we use the same parameter name for the type (XXX={anonymous,workload_identity}). If we use it, users can't specify both of them at once. (I know that URI spec accepts XXX=anonymous&XXX=workload_identity.)

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

  • nothing (use default auth chain) -> nothing or ?credential_kind=default
  • ?anonymous -> ?credential_kind=anonymous
  • ?use_workload_identity -> ?credential_kind=workload_identity
  • ?account_key=<ACCOUNT_KEY> -> not changed (?credential_kind=storage_shared_key is invalid)
  • ?tenant_id=<TENANT_ID>&client_id=<CLIENT_ID>&client_secret=<CLIENT_SECRET> -> not changed (?credential_kind=client_secret is invalid)
  • ?client_id=<CLIENT_ID> (client_id alone means managed identity credential) -> not changed (?credential_kind=managed_identity is invalid)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we don't need ?account_key=<ACCOUNT_KEY> because we can get it from the URI's password part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting only (default, ) anonymous and use_workload_identity as valid credential_kind parameter?

Sure. That looks good.

Comment threadcpp/src/arrow/filesystem/azurefs.h Outdated
Comment threadcpp/src/arrow/filesystem/azurefs.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

@felipecrv

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

@kou@Tom-Newton can we use azfs (only https and by default) because this implementation uses both Blobs and File System APIs?

I have quite a strong opinion that we should support the abfss:// and abfs:// Hadoop path formats. As a user I don't want to use different path formats when I use different filesystem implementations.

Fair enough, then what are the semantics of each URI format and how they map to this implementation?

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions cause people running pyarrow on insecure Wi-Fi networks shouldn't have their credentials leaked.

@Tom-Newton

ghost commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

I will start with one rule: both abfss and abfs map to scheme=https in AzureOptions

That is fine with me.

what are the semantics of each URI format and how they map to this implementation?

For Hadoop format URIs I would probably just extract the storage account name. That means there is a lot of redundant information in the URI but I don't think that is really a problem and it gives us compatibility which I think is important.

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 5, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 5, 2024
@felipecrv

ghost commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

If we want to support other URIs formats too that would be useful to some people. Some examples I've seen on other filesystems: https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_urlhttps://github.com/fsspec/adlfs

Thank you @Tom-Newton! This list from the Rust crate defines formats that allow us to express URIs that refer to the entire storage account and not just a specific filesystem. Plus, simple and short URIs that allow us to simply use the default endpoints. cc @kou

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 6, 2024
@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

URI list from https://docs.rs/object_store/latest/object_store/azure/struct.MicrosoftAzureBuilder.html#method.with_url :

  • abfs[s]://<container>/<path> (according to fsspec)
  • abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
  • abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>
  • az://<container>/<path>
  • adl://<container>/<path>
  • azure://<container>/<path>
  • https://<account>.dfs.core.windows.net
  • https://<account>.blob.core.windows.net
  • https://<account>.blob.core.windows.net/<container>
  • https://<account>.dfs.fabric.microsoft.com
  • https://<account>.dfs.fabric.microsoft.com/<container>
  • https://<account>.blob.fabric.microsoft.com
  • https://<account>.blob.fabric.microsoft.com/<container>

We can use abfs/abfss/az/adl/azure schemes with the current FileSystemFromUri() mechanism. But https is a bit tricky. We need to check not only the scheme part but also the host part. This means that we need to put Azure related URI parsing code to filesystem.cc and azurefs.cc. And this isn't suitable for #39067 . #39067 only uses the scheme part to dispatch filesystem implementation.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

@kou let's implement only abfs[s] for now. Not even use the az, adl, azure schemes.

abfs[s]://<container>/<path> (according to [fsspec](https://github.com/fsspec/adlfs))
abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>
abfs[s]://<file_system>@<account_name>.dfs.fabric.microsoft.com/<path>

Supporting https:// would onlye make sense if we were creating an Object Store API, but we are implementing filesystem abstractions on top of object stores accessible via HTTP, so it makes more sense to have them named something that is not http[s].

@kou

ghost commented Mar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

@felipecrv

ghost commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Can we also support one more format for Azurite?

abfs[s]://<host>:<port>/<container>/<path>

If <host> doesn't have . and the <port> part doesn't exist, we will interpret the given URI as abfs[s]://<container>/<path>.

Sure. I think that can work well.

@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

OK. I'll implement the discussed spec.

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]
Added query parameters:
* enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
parameters.
Removed query parameters:
* blob_storage_scheme: Replaced with enable_tls.
* dfs_storage_scheme: Replaced with enable_tls.
Changed query parameters:
* credential_kind: Accepts only "default", "anonymous" and
"workload_identity".
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2024
@kou

ghost commented Mar 7, 2024

Copy link
Copy Markdown
MemberAuthor

Implemented:

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]

Added query parameters:

  • enable_tls: It replaces blob_storage_scheme and dfs_storage_scheme
    parameters.

Removed query parameters:

  • blob_storage_scheme: Replaced with enable_tls.
  • dfs_storage_scheme: Replaced with enable_tls.

Changed query parameters:

  • credential_kind: Accepts only default, anonymous and
    workload_identity.

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 8, 2024
@kou

ghost commented Mar 8, 2024

Copy link
Copy Markdown
MemberAuthor

I'll merge this in the next week if nobody objects it.

@kou
kou merged commit 605f8a7 into apache:mainMar 11, 2024
@kou
kou deleted the cpp-azurefs-from-uri branch March 11, 2024 21:34
@koukou removed the awaiting change review Awaiting change review label Mar 11, 2024
@conbench-apache-arrow

ghost commented Mar 12, 2024

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 605f8a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kou@Tom-Newton@nosterlu@felipecrv