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

Add support for Azure Government to Azure Blobs storage driver #1552

Merged
merged 5 commits into from Feb 10, 2021

Conversation

c-w
Copy link
Member

@c-w c-w commented Feb 7, 2021

Add support for Azure Government to Azure Blobs storage driver

Description

This pull request adds support to the Azure Blobs storage driver for deployments of Azure Storage that use custom hostnames such as Azure Government, Azure China, and Azure Private Link. To target these deployments, the driver can be initialized as follows:

from libcloud.storage.types import Provider
from libcloud.storage.providers import get_driver

cls = get_driver(Provider.AZURE_BLOBS)

host = 'privatelink.blob.core.windows.net'  # for Azure Private Link
host = 'blob.core.chinacloudapi.cn'  # for Azure China
host = 'blob.core.usgovcloudapi.net'  # for Azure Government

driver = cls(key='your storage account name',
             secret='your access key',
             host=host)

The change maintains backwards compatibility with the previous approach of using a custom host argument to target Azurite or IoT Edge Storage by explicitly checking for the Azure China, Azure Government, and Azure Private Link well-known endpoint constants.

Resolves #1551

Status

  • done, ready for review

Checklist

Note that this change also enables using the driver with Azure China and
Azure Private Link.
@@ -217,15 +221,34 @@ def __init__(self, key, secret=None, secure=True, host=None, port=None,
port=port, **kwargs)

def _ex_connection_class_kwargs(self):
Copy link
Member

Choose a reason for hiding this comment

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

Some unit tests for this method would be good.

Copy link
Member Author

@c-w c-w Feb 10, 2021

Choose a reason for hiding this comment

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

Added tests for the Azurite path in c581553 and for the new GovCloud path in 739a6a3.

# still use the same scheme to identify a specific account as for
# the standard storage endpoint
try:
host_suffix = next(
Copy link
Member

Choose a reason for hiding this comment

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

Is there a situation where we may want to allow end user to specify full host as-is - aka so we don't add any prefix to it, but user already includes that in the host argument?

I think this would give users more flexibility in some situations.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify - if a new endpoint ever gets added or similar, this would require use to update the code to support it.

If we allowed user to specify a full arbitrary host, they may be able to use that instead (until a new version is released / similar).

For us to be able to support that, we would likely need to add new argument to the constructor - e.g. ex_use_host_as_is / ex_dont_prefix_host (or some better name, can't come up with anything better atm).

Copy link
Member Author

Choose a reason for hiding this comment

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

The only current Azure Storage version that I'm aware of which wouldn't be covered by the approach proposed in this PR is Azure Stack Hub Storage. However the API version that libcloud uses (2018-11-09) is newer than the latest supported version of Azure Stack (2017-11-09) so libcloud doesn't support it anyways.

I get the appeal of an ex_force_host or similar argument, but I wonder if the simplicity of this explicit approach will be more user-friendly as it doesn't introduce an argument that's specific to just this driver and instead manages the complexity internally. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I didn't really mean it as an alternative, but in addition to your proposed changes (so the interface for the common case would still be the same, but in case there is a need to use a fully custom host, there is an easy way to do it).

I also don't think that's a blocker for this PR so if you think it's a good idea, feel free to open a new PR with that change in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case I'd say to defer the addition of ex_force_host for now.

@c-w c-w force-pushed the storage-azureblobs-fix-1551-govcloud branch from f9313bd to b16eb28 Compare February 10, 2021 15:41
@c-w c-w requested a review from Kami February 10, 2021 15:50
Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

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

LGTM.

Once both of the PRs are merged you can also work on a new minor release with those two changes, if you wish.

@c-w c-w merged commit 86f5c1c into trunk Feb 10, 2021
@c-w c-w deleted the storage-azureblobs-fix-1551-govcloud branch February 10, 2021 22:17
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 15, 2021
…e#1552)

* Fix typo in documentation

* Add support for blob storage in Azure Government

Note that this change also enables using the driver with Azure China and
Azure Private Link.

* Add unit tests for GovCloud host formatting

* Add unit tests for Azurite host formatting

* Allow conn_kwargs to override host
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 19, 2021
…e#1552)

* Fix typo in documentation

* Add support for blob storage in Azure Government

Note that this change also enables using the driver with Azure China and
Azure Private Link.

* Add unit tests for GovCloud host formatting

* Add unit tests for Azurite host formatting

* Allow conn_kwargs to override host
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure blobs - setting AzureBlobsStorageDriver host does not work for non-emulated requests
2 participants