Make azure sdk stick to winhttp if possible #851
Conversation
phoebusm
force-pushed
the
bugfix/fix_azure_windows_use_libcurl
branch
from
ceefba0 to
55517a9
Compare
phoebusm
force-pushed
the
bugfix/fix_azure_windows_use_libcurl
branch
from
55517a9 to
a32523b
Compare
|
Pending #853 for Azure support on conda build test |
python/arcticdb/arctic.py
Outdated
| @@ -103,6 +103,8 @@ def __init__(self, uri: str, encoding_version: EncodingVersion = DEFAULT_ENCODIN | |||
| | | "/etc/pki/tls/cacert.pem" OpenELEC | | |||
| | | "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" CentOS/RHEL 7 | | |||
| | | "/etc/ssl/cert.pem" Alpine Linux | | |||
| | | WARNING for WINDOWS USER: Not leaving this empty will switch the backend support of Azure SDK from winhttp to libcurl. If ca cert path is needed to be | | |||
There was a problem hiding this comment.
This warning is a bit ungrammatical, and this docstring is getting massive!
But it makes me think, is it ever correct for a Windows user to set CA_cert_path? Shouldn't we leave it to Windows to determine (it doesn't have the Linux "many locations" problem). Should we throw if a Windows user sets this to a non-empty value (and document that)?
I suggest throw if non-empty rather than throw if present so that users can still use the same code across Linux/Windows, just with a bit of config for the cert path location that differs by OS.
There was a problem hiding this comment.
I was torn between should I just simply give exception for it earlier too.
My idea is, there must be a reason for Azure C++ SDK to include libcurl support on Windows. It will be a pity to drop the support on our side. And users need to specifically assign values to CA_cert_path to get themselves into the uncharted territory. As long as warnings are given on the doctoring, users should bear the risk.
I understand my arguments are not strong but I think they are worth being considered.
There was a problem hiding this comment.
One more thing, docstring will be massively reduced once the ca path and directory can be automatically assigned. The commit is coming soon in another branch.
There was a problem hiding this comment.
OK, fair enough. Can I suggest we log a warning on Windows when this is set, then?
What is your comment regarding
using libcurl as backend shows buggy behaviour on ssl ca cert verification. about? What was the buggy behaviour?
It would be good to explain the impact of the change of backend a bit in the log warning.
There was a problem hiding this comment.
Text suggestion for this docstring if we keep it:
WARNING for Windows users: Setting this to anything besides an empty string will switch the backend support of Azure SDK from winhttp to libcurl. <EXPLANATION OF IMPACT>
If CA Cert path needs to be specified, set it in the Windows settings rather than using the option, so that winhttp will be used.
There was a problem hiding this comment.
Could it be that Azure switches to libcurl backend here because it assumes that if CA_Cert_Path is set then it can't be on Windows, or something? It would be good to see if there is anything in the Azure git blame to understand why this happens at all.
There was a problem hiding this comment.
OK, fair enough. Can I suggest we log a warning on Windows when this is set, then?
What is your comment regarding
using libcurl as backend shows buggy behaviour on ssl ca cert verification.about? What was the buggy behaviour?It would be good to explain the impact of the change of backend a bit in the log warning.
Verification fails if SAS Token is used with libcurl on Windows. The problem is gone once switched to winhttp.
I use the word buggy as the fail is weird. You may refer to the corresponding ticket for the details.
There was a problem hiding this comment.
After an offline discussion with @poodlewars, exception will be thrown if CA_cert_path or CA_cert_dir is being set.
Further compacting of the docstring is handled in #867
| def test_azure_sas_token(azure_account_sas_token, azurite_azure_test_connection_setting): | ||
| endpoint, container, credential_name, _, _ = azurite_azure_test_connection_setting | ||
| ac = Arctic( | ||
| f"azure://DefaultEndpointsProtocol=http;SharedAccessSignature={azure_account_sas_token};BlobEndpoint={endpoint}/{credential_name};Container={container}" |
There was a problem hiding this comment.
We should document how to do this in the website
There was a problem hiding this comment.
I am not sure I follow? You mean a little guideline to for the users to setup their own SAS tokens?
There was a problem hiding this comment.
Yeah exactly, a little example in the docs showing how to connect to Azure with SAS
There was a problem hiding this comment.
Maybe the best is by adding the link of below official guidelines for user reference?
Create token:
https://learn.microsoft.com/en-us/azure/storage/common/storage-account-sas-create-python
Connection string:
https://learn.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string#create-a-connection-string-using-a-shared-access-signature
phoebusm
force-pushed
the
bugfix/fix_azure_windows_use_libcurl
branch
3 times, most recently
from
e3bb32b to
9699e41
Compare
-Drop support of libcurl-backed Azure
phoebusm
force-pushed
the
bugfix/fix_azure_windows_use_libcurl
branch
from
9699e41 to
fe3243a
Compare
Reference Issues/PRs
#849
What does this implement/fix? How does it work (high level)? Highlight notable design decisions.
Azure SDK has an undocumented feature, in the situation which if CA path is set, even it is left empty, uses libcurl instead of the default winhttp as backend support on Windows. During the test, using libcurl as backend shows buggy behaviour on ssl ca cert verification.
This fix is to not set
CAINFOif ca path is not specified, to give user choice to use winhttp, to avoid the buggy behaviour.Any other comments?
Checklist
Checklist for code changes...