kou commented on code in PR #45021:
URL: https://github.com/apache/arrow/pull/45021#discussion_r1884726261
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -148,8 +148,10 @@ Status AzureOptions::ExtractFromUriQuery(const Uri& uri) {
ARROW_ASSIGN_OR_RAISE(background_writes,
::arrow::internal::ParseBoolean(kv.second));
} else {
- return Status::Invalid(
- "Unexpected query parameter in Azure Blob File System URI: '",
kv.first, "'");
+ // Assume these are part of a SAS token. Its not ideal to make such an
assumption
+ // but given that a SAS token is a complex set of URI parameters, that
could be
+ // tricky to exhaustively list I think its the best option.
+ credential_kind = CredentialKind::kSasToken;
Review Comment:
We don't have the SAS token specification that includes parameter names used
by a SAS token, right?
##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -120,6 +120,7 @@ struct ARROW_EXPORT AzureOptions {
kDefault,
kAnonymous,
kStorageSharedKey,
+ kSasToken,
Review Comment:
We use `CLI` not `Cli` for CLI.
```suggestion
kSASToken,
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -3162,9 +3187,13 @@ class AzureFileSystem::Impl {
return Status::OK();
}
std::string sas_token;
- {
+ if (options_.UsesSasCredential()) {
+ // When authenticated with SAS token GetUrl() automatically provides an
+ // authenticated URL with the SAS token appended.
+ sas_token = "";
Review Comment:
How about changing `GenerateSASToken()` instead? If we do, we don't need to
define `UsersSasCredential()`.
```diff
diff --git a/cpp/src/arrow/filesystem/azurefs.cc
b/cpp/src/arrow/filesystem/azurefs.cc
index 78f4ad1edd..c01ca41aed 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -410,6 +410,10 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient*
client) const {
+ if (credential_kind_ == CredentialKind::kSasToken) {
+ // TODO: Write why we can return "" here
+ return "";
+ }
using SasProtocol = Storage::Sas::SasProtocol;
builder->Protocol =
blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp :
SasProtocol::HttpsOnly;
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -3162,9 +3187,13 @@ class AzureFileSystem::Impl {
return Status::OK();
}
std::string sas_token;
- {
+ if (options_.UsesSasCredential()) {
+ // When authenticated with SAS token GetUrl() automatically provides an
+ // authenticated URL with the SAS token appended.
+ sas_token = "";
+ } else {
Storage::Sas::BlobSasBuilder builder;
- std::chrono::seconds available_period(60);
+ std::chrono::seconds available_period(600);
Review Comment:
Could you add a comment why we use `600` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
