-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[data] handle azure UC user delegation SAS #59393
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
Conversation
Handle azure_user_delegation_sas responses from Databricks UC credential vending and surface keys in error messages for easier troubleshooting. Signed-off-by: soffer-anyscale <stephen.offer@anyscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for Azure Unity Catalog user delegation SAS tokens, which is a valuable enhancement for Azure users. The implementation correctly extracts the SAS token and storage account information from the Databricks UC response. Additionally, the improvement to the credential error message is a great addition for debuggability. I have one minor suggestion to improve code consistency.
Raise when azure_user_delegation_sas lacks a token and set storage account env vars with direct assignment. Signed-off-by: soffer-anyscale <stephen.offer@anyscale.com>
| azure.get("sas_token") | ||
| or azure.get("sas") | ||
| or azure.get("token") | ||
| or azure.get("sasToken") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this varying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support multiple versions of Azure UC, which apparently had changed the response key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add release test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| azure.get("sas_token") | ||
| or azure.get("sas") | ||
| or azure.get("token") | ||
| or azure.get("sasToken") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add release test for this
| azure.get("sas_token") | ||
| or azure.get("sas") | ||
| or azure.get("token") | ||
| or azure.get("sasToken") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Description Add support for Databricks UC Azure responses that return `azure_user_delegation_sas`, extracting the SAS token and storage account for Ray Data reads. Improve the credential error message to list available keys, aiding troubleshooting when UC returns unexpected fields. ## Related issues Related to Azure Databricks Unity Catalog credential vending; no tracked GitHub issue provided. ## Additional information Uses Databricks UC credential vending docs for Azure: https://docs.databricks.com/en/data-governance/unity-catalog/credential-vending.html No API changes; behavior now matches AWS/Azure parity for `ray.data.read_unity_catalog`. --------- Signed-off-by: soffer-anyscale <stephen.offer@anyscale.com>
Description
Add support for Databricks UC Azure responses that return
azure_user_delegation_sas, extracting the SAS token and storage account for Ray Data reads.Improve the credential error message to list available keys, aiding troubleshooting when UC returns unexpected fields.
Related issues
Related to Azure Databricks Unity Catalog credential vending; no tracked GitHub issue provided.
Additional information
Uses Databricks UC credential vending docs for Azure: https://docs.databricks.com/en/data-governance/unity-catalog/credential-vending.html
No API changes; behavior now matches AWS/Azure parity for
ray.data.read_unity_catalog.