Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minimum required curl version should be updated. #4792

Closed
teo-tsirpanis opened this issue Jul 14, 2023 · 1 comment · Fixed by #4821
Closed

Minimum required curl version should be updated. #4792

teo-tsirpanis opened this issue Jul 14, 2023 · 1 comment · Fixed by #4821
Assignees
@RickWinter
Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team

Comments

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Jul 14, 2023 •
edited
Loading

We specify the minimum supported version of curl as 7.44 in

azure-sdk-for-cpp/sdk/core/azure-core/CMakeLists.txt

Line 31 in 6c616bd

set(CURL_MIN_REQUIRED_VERSION 7.44)
but in

azure-sdk-for-cpp/sdk/core/azure-core/src/http/curl/curl.cpp

Line 2318 in 6c616bd

if (!SetLibcurlOption(m_handle, CURLOPT_CAINFO_BLOB, &rootCertBlob, &result))
we use CURLOPT_CAINFO_BLOB which is not available until 7.77.

The minimum version should be updated and tests should use it.
@github-actions github-actions bot added Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-triage Workflow: This issue needs the team to triage. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jul 14, 2023
@RickWinter
Copy link
Member

Thanks for pointing this out. We will investigate.

//cc: @LarryOsterman

@RickWinter RickWinter added bug This issue requires a change to an existing behavior in the product in order to be resolved. and removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jul 14, 2023
@RickWinter RickWinter removed the needs-team-triage Workflow: This issue needs the team to triage. label Jul 14, 2023
@github-actions github-actions bot added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jul 14, 2023
@RickWinter RickWinter mentioned this issue Jul 14, 2023
Closed
2 tasks
@antkmsft antkmsft mentioned this issue Jul 20, 2023
Merged
@RickWinter RickWinter linked a pull request Jul 28, 2023 that will close this issue
Support root cert validation on CURL #4821
Merged
@RickWinter RickWinter mentioned this issue Jul 28, 2023
Merged
azure-sdk added a commit to azure-sdk/vcpkg that referenced this issue Aug 4, 2023
@azure-sdk
[azure-core-cpp] Update to 1.10.2
bcfb7f6 
## 1.10.2 (2023-08-04)

### Bugs Fixed

- [[microsoft#4792]](Azure/azure-sdk-for-cpp#4792) Only support CURL's root cert validation when CURL version is >= 7.77.0.
JavierMatosD pushed a commit to microsoft/vcpkg that referenced this issue Aug 7, 2023
@azure-sdk
[azure-core-cpp] Update to 1.10.2 (#32971)
754105a 
## 1.10.2 (2023-08-04)

### Bugs Fixed

- [[#4792]](Azure/azure-sdk-for-cpp#4792) Only support CURL's root cert validation when CURL version is >= 7.77.0.
@Tom-Newton Tom-Newton mentioned this issue Aug 26, 2023
Merged
kou added a commit to apache/arrow that referenced this issue Aug 30, 2023
@Tom-Newton @kou @pitrou
GH-29847: [C++] Build with Azure SDK for C++ (#36835)
3cb4481 
### Rationale for this change
We want to use the Azure SDK for C++ to read/write to Azure blob storage. Obviously this is pretty important for building an `AzureFileSystem`. 

### What changes are included in this PR?
Builds the the relevant parts of the azure SDK as a cmake external project. Adds a couple of simple tests that just assert that the Azure SDK is working and a couple of lines in `AzureFileSystem` to initialise the blob storage client to ensure the build is working correctly in all environments.

I started with the build setup from #12914 but I did make few changes.

1. Although its atypical for this project we chose to switch from cmake's `ExternalProject` to `FetchContent`. `FetchContent` is recomended by the Azure docs https://github.com/Azure/azure-sdk-for-cpp#cmake-project--fetch-content. It also solves a few problems including: automatically linking system curl and ssl instead of bootstrapping vcpkg and installing curl and ssl from there. 
2. Only build one version of the Azure SDK for C++ because it contains all the components. Previously we were unnecessarily building 5 different versions of the whole thing on top of each other. This created race conditions for which version each component came from.
3. We are using `azure-core_1.10.2` which is a very recent version. There are a couple of important reasons for this 1.  [an important managed identity fix](Azure/azure-sdk-for-cpp#4723), 2. [fixed support for curl versions < 7.71.0](Azure/azure-sdk-for-cpp#4792).

There will be follow up PRs to enable Azure in the manylinux builds. We need to update `vcpkg` first so we can get a version of the Azure SDK which contains [an important managed identity fix](Azure/azure-sdk-for-cpp#4723).

### Are these changes tested?
Yes. There is a simple test that just runs the Azure client against azurite. Additionally just initialising the client in `AzureFileSystem` goes a long way towards ensuring the build is working. 

### Are there any user-facing changes?
No
* Closes: #29847

Lead-authored-by: Thomas Newton <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: shefali singh <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@RickWinter RickWinter

Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support root cert validation on CURL RickWinter/azure-sdk-for-cpp
3 participants
@RickWinter @LarryOsterman @teo-tsirpanis