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.

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

feat: Make automatic use of Azure storage account keys opt-in #20652

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

nameexhaustion
Copy link
Collaborator

@nameexhaustion nameexhaustion commented Jan 10, 2025

ref #20634

  • Fixes a behavior where storage account keys retrieved from Azure CLI would in some cases override user-specified configurations
  • Disables retrieving and using storage account keys from Azure CLI by default, this is now only done if POLARS_AUTO_USE_AZURE_STORAGE_ACCOUNT_KEY=1 is in the environment (note that azure.identity must also be installed as well, so that CredentialProviderAzure gets instantiated)

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Jan 10, 2025
@nameexhaustion nameexhaustion changed the title feat: Make automatic authentication using Azure storage account keys is opt-in feat: Make automatic authentication using Azure storage account keys opt-in Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.02%. Comparing base (dd9a180) to head (46f6001).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
py-polars/polars/io/cloud/credential_provider.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #20652      +/-   ##
==========================================
+ Coverage   79.00%   79.02%   +0.02%     
==========================================
  Files        1559     1559              
  Lines      221216   221151      -65     
  Branches     2529     2530       +1     
==========================================
- Hits       174761   174758       -3     
+ Misses      45876    45814      -62     
  Partials      579      579              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nameexhaustion nameexhaustion marked this pull request as ready for review January 10, 2025 08:10
@nameexhaustion nameexhaustion marked this pull request as draft January 10, 2025 08:10
@nameexhaustion nameexhaustion marked this pull request as ready for review January 10, 2025 08:11
@nameexhaustion nameexhaustion changed the title feat: Make automatic authentication using Azure storage account keys opt-in feat: Make automatic use of Azure storage account keys opt-in Jan 10, 2025
eprintln!("[CloudOptions::build_azure]: Permission check OK");
}
return Ok(store);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Authentication will now prioritize Entra ID (via the Python-side DefaultAzureCredential if azure.identity is installed). Using the storage account key will only happen if we get a permission error (above).

@nameexhaustion nameexhaustion changed the title feat: Make automatic use of Azure storage account keys opt-in feat: Remove automatic use of Azure storage account keys Jan 13, 2025
@nameexhaustion nameexhaustion changed the title feat: Remove automatic use of Azure storage account keys feat: Make automatic use of Azure storage account keys opt-in Jan 13, 2025
@nameexhaustion
Copy link
Collaborator Author

nameexhaustion commented Jan 13, 2025

PR has been updated to remove the fallback mechanism, I don't have any ideas for a good way to determine whether the Azure object store has sufficient permissions - according to #20634 (comment), attempting to fetch a random file path is not good enough as there can be file-level RBAC configurations on the bucket.

Instead, if POLARS_AUTO_USE_AZURE_STORAGE_ACCOUNT_KEY=1 is set, the Python-side Azure credential provider will prioritize using storage account keys if they can be found from the Azure CLI. Otherwise, credential resolution is left to DefaultAzureCredential.

@nameexhaustion nameexhaustion marked this pull request as ready for review January 13, 2025 07:12
@ritchie46 ritchie46 merged commit 8dcaec2 into pola-rs:main Jan 13, 2025
32 checks passed
@c-peters c-peters added the accepted Ready for implementation label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants