You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2024/06/11 21:03:03 UTC
[PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
kou opened a new pull request, #42108:
URL: https://github.com/apache/arrow/pull/42108
### Rationale for this change
`BlobSasBuilder::Protocol` is used in `BlobSasBuilder::GenerateSasToken()` and it doesn't have the default value. If we don't specify it explicitly, `GenerateSasToken()` result may be unexpected.
### What changes are included in this PR?
Set `BlobSasBuilder::Protocol` explicitly based on `AzureOptions::blob_storage_scheme`. It's `https` by default but can be `http` when `enable_tls=false` query parameter is specified.
If it's `http`, both of `https` and `http` are accepted as protocol.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2166973326
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 787a6ab32e04fe259504a74c20b9e1135914490f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/26209423923) has more details. It also includes information about 25 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2161600009
Revision: 4226c8cb5410d156a99bbf52965a640f913587f5
Submitted crossbow builds: [ursacomputing/crossbow @ actions-bf085e89e1](https://github.com/ursacomputing/crossbow/branches/all?query=actions-bf085e89e1)
|Task|Status|
|----|------|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/tree/actions-bf085e89e1-github-test-conda-cpp-valgrind)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2161597377
@github-actions crossbow submit test-conda-cpp-valgrind
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2165059741
I merge this as-is.
I'll add `{blob,dfs}_storage_scheme` validation as #42134.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637854351
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
OK. I'll do it as a separated task: GH-42134
Because this is not related to the Valgrind failure.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #42108:
URL: https://github.com/apache/arrow/pull/42108
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637769682
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
> Should we validate the value explicitly? If we should validate the value, we'll do it in `MakeBlobServiceClient()` and `MakeDataLakeServiceClient()` because it can return `Status::Invalid`.
As long as the value is validated somewhere, I'm fine :-)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2161597406
:warning: GitHub issue #42107 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2163909921
> So the `blob_storage_scheme` option was entirely ignored up to now?
No. It's used to create a `BlobServiceClient`:
https://github.com/apache/arrow/blob/512d2458ff777f15354ee2a88bde7723ec3d5035/cpp/src/arrow/filesystem/azurefs.cc#L301-L303
https://github.com/apache/arrow/blob/512d2458ff777f15354ee2a88bde7723ec3d5035/cpp/src/arrow/filesystem/azurefs.cc#L364-L389
> And what about `dfs_storage_scheme`? It seems ignored at well.
It's also used to create a `DataLakeServiceClient`:
https://github.com/apache/arrow/blob/512d2458ff777f15354ee2a88bde7723ec3d5035/cpp/src/arrow/filesystem/azurefs.cc#L305-L307
https://github.com/apache/arrow/blob/512d2458ff777f15354ee2a88bde7723ec3d5035/cpp/src/arrow/filesystem/azurefs.cc#L391-L417
> Do we have other options that are ignored like this?
No, we don't have.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1636079451
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
Can we validate the value is either "http" or "https"? We shouldn't allow other values silently.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637767723
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
Passing an unsupported scheme should be a user error, since filesystem configuration is typically chosen by the user. It doesn't really help the user to allow incorrect values.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637854351
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
OK. I'll do it as a separated task: GH-42134
Because it's not related to the Valgrind failure.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637101545
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
We can validate the value explicitly but other operations such as creating `BlobServiceClient` or `DataLakeServiceClient` will be failed with invalid value before this code.
Should we validate the value explicitly? If we should validate the value, we'll do it in `MakeBlobServiceClient()` and `MakeDataLakeServiceClient()` because it can return `Status::Invalid`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #42108:
URL: https://github.com/apache/arrow/pull/42108#discussion_r1637302122
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -418,6 +418,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
Result<std::string> AzureOptions::GenerateSASToken(
Storage::Sas::BlobSasBuilder* builder, Blobs::BlobServiceClient* client) const {
+ using SasProtocol = Storage::Sas::SasProtocol;
+ builder->Protocol =
+ blob_storage_scheme == "http" ? SasProtocol::HttpsAndHttp : SasProtocol::HttpsOnly;
Review Comment:
Falling back to the secure protocol is not so bad and we ensure it's valid when constructing from URI.
https://github.com/apache/arrow/blob/512d2458ff777f15354ee2a88bde7723ec3d5035/cpp/src/arrow/filesystem/azurefs.cc#L139-L145
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42107: [C++][FS][Azure] Ensure setting BlobSasBuilder::Protocol [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #42108:
URL: https://github.com/apache/arrow/pull/42108#issuecomment-2164923911
> No. It's used to create a `BlobServiceClient`:
Ah, I had misread the code. Sorry!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org