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

[azure] Use AZURE_CUSTOM_DOMAIN only for retrieving blob URLs, and use storage URL for other operations #1176

Merged
merged 8 commits into from Sep 27, 2022

Conversation

JeffreyCA
Copy link
Contributor

@JeffreyCA JeffreyCA commented Sep 22, 2022

Fixes #1116, #1097

This restores the previous behaviour that was in 1.11 (with old Azure backend) so that the custom domain is used only for retrieving the blob URLs. All other operations like uploads go through the actual storage account URL. I updated the docs to clarify this too.

In 1.12, we switched to the new storage SDK and also changed it so that all storage operations go through the custom domain instead (not sure if intentional or not). Doing so uncovered several upstream issues that affect storage operations when connecting directly to the custom domain. More details in #1116.

I also made a small change to simplify the exists() method a bit.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 22, 2022

If I understand correctly - which I may not, please put me right

  • there are bugs such that using custom domains / content delivery networks don't work properly
  • this MR works around that by bypassing those things and going direct to the storage account

Isn't that basically the wrong thing to do, albeit driven by the very pragmatic wish to have something that actually works?

If so it seems unsatisfactory to leave this project committed to doing the wrong thing: and to accept configuration making it look as though it does one thing when in fact it does another. Sooner or later there'll be bug reports saying "I configured use of a custom domain / content delivery network, but that is not being respected"...

Perhaps it would be better simply to note the upstream bugs in the docs somewhere, and recommend that people don't configure things that way until they are fixed?

@JeffreyCA JeffreyCA marked this pull request as draft September 22, 2022 19:53
@JeffreyCA
Copy link
Contributor Author

JeffreyCA commented Sep 22, 2022

This PR basically reverts the behaviour to what it was like in v1.11 and earlier: using two service clients, a regular client that uses the storage endpoint and a "custom" client that uses the custom domain (if it exists, and falls back to storage endpoint otherwise). The custom client is only used to retrieve the blob URL, and the regular client is used for all other operations, which to me makes more sense than what the current behaviour is.

For reference this is what we were doing in the old version:

def url(self, name, expire=None):
name = self._get_valid_path(name)
if expire is None:
expire = self.expiration_secs
make_blob_url_kwargs = {}
if expire:
sas_token = self.custom_service.generate_blob_shared_access_signature(
self.azure_container, name, permission=BlobPermissions.READ, expiry=self._expire_at(expire))
make_blob_url_kwargs['sas_token'] = sas_token
return self.custom_service.make_blob_url(
container_name=self.azure_container,
blob_name=filepath_to_uri(name),
protocol=self.azure_protocol,
**make_blob_url_kwargs)

If we keep the current implementation, there aren't any good workarounds for people who want to use Azure CDNs. Using Microsoft CDNs just doesn't work at all, and for Akamai CDNs you'd have to set AZURE_OVERWRITE_FILES to True so that it doesn't send a particular HTTP header.

I wouldn't really say doing it the old way is wrong -using the custom domain only for retrieving blob URLs is consistent with how the other storage backends work in django-storages.

@JeffreyCA JeffreyCA marked this pull request as ready for review September 22, 2022 22:37
@jschneier
Copy link
Owner

Happy to merge this as-is. Can you explain how you tested this change? I don't think the unit tests properly cover everything unfortunately.

@JeffreyCA
Copy link
Contributor Author

Thanks for reviewing, I added a few more tests.

I used Fiddler to verify that the storage endpoint is used for uploads and other operations, and that the custom domain is used for generating the file URLs. I tested with all the different types of Azure CDNs and did not get any auth errors. Also verified the user delegation logic still works using an Azure VM w/ managed identity.

@jschneier jschneier merged commit f0df471 into jschneier:master Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure - Authentication error when AZURE_CUSTOM_DOMAIN set to Azure CDN
3 participants