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

Fix azure pre-signed url for blob from different account #6594

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Sep 13, 2023

Fix #6595

@nopcoder nopcoder added bug Something isn't working azure Issues regarding azure block adapter and support labels Sep 13, 2023
@nopcoder nopcoder self-assigned this Sep 13, 2023
@nopcoder nopcoder added the include-changelog PR description should be included in next release changelog label Sep 13, 2023
Copy link
Contributor

@lynnro314 lynnro314 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Cool, thanks! I know very little about our Azure internals, @guy-har probably best look at that when he gets back. I am concerned that "adls" doesn't appear in this PR. So instead I'll just ask...

How was this tested?

@nopcoder
Copy link
Contributor Author

Manual test and pending @lynnro314 test and verification for the specific data that the bug was found.

In order to reproduce you need to use azure and import data from different account.

The implementation have two implementation to handle (different API with Azure SDK):

  1. The blob on the same account and you have access key configured.
  2. The blob on another account which means you have service principle configured.

In the first one the API returns the URL after we provide the account/container.
In the second we request for credentials for the second account to get sign query parameters for our url and our code format the URL. The code used the object absolute path, and extracted the domain and use it for the signed URL.

This fix make sure we format the object location based azure default endpoint.

We should verify that the import works with the blob endpoint also for adls container and remove the support of import from this endpoint.
This code was added when import had to understand which algorithm to work based on the listing behaviour - which I don't think it is the case anymore (need to verify it).

So import with blob domain should work, and this fix should help existing data.

@nopcoder
Copy link
Contributor Author

@lynnro314 ping or merge after verification on your environment.

@nopcoder nopcoder merged commit aa755c0 into master Sep 14, 2023
@nopcoder nopcoder deleted the fix/azure-signed-blob branch September 14, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure Issues regarding azure block adapter and support bug Something isn't working include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Pre-sign urls in Azure return urls with adls subdomain after import
3 participants