You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2021/02/07 16:35:58 UTC
[GitHub] [libcloud] c-w opened a new pull request #1552: Add support for Azure Government to Azure Blobs storage driver
c-w opened a new pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552
## 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:
```py
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 https://github.com/apache/libcloud/issues/1551
### Status
- done, ready for review
### Checklist
- [ ] [Code linting](http://libcloud.readthedocs.org/en/latest/development.html#code-style-guide)
- [x] Documentation **Updated docs**
- [x] [Tests](http://libcloud.readthedocs.org/en/latest/testing.html) **[libcloud-tests passed](https://clewolff.visualstudio.com/libcloud-tests/_build/results?buildId=1589&view=logs&s=ee3800fd-6e81-525f-e564-94108585217d)**
- [x] [ICLA](http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes) **[committer](http://people.apache.org/phonebook.html?uid=clewolff)**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r571941093
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
Review comment:
Some unit tests for this method would be good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] c-w commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r574116998
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
- result = {}
-
- # host argument has precedence
- if not self._host_argument_set:
- result['host'] = '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)
+ # if the user didn't provide a custom host value, assume we're
+ # targeting the default Azure Storage endpoints
+ if self._host is None:
+ return {'host': '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)}
+
+ # connecting to a special storage region like Azure Government or
+ # Azure China requires setting a custom storage endpoint but we
+ # still use the same scheme to identify a specific account as for
+ # the standard storage endpoint
+ try:
+ host_suffix = next(
Review comment:
In that case I'd say to defer the addition of `ex_force_host` for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r571943385
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
- result = {}
-
- # host argument has precedence
- if not self._host_argument_set:
- result['host'] = '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)
+ # if the user didn't provide a custom host value, assume we're
+ # targeting the default Azure Storage endpoints
+ if self._host is None:
+ return {'host': '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)}
+
+ # connecting to a special storage region like Azure Government or
+ # Azure China requires setting a custom storage endpoint but we
+ # still use the same scheme to identify a specific account as for
+ # the standard storage endpoint
+ try:
+ host_suffix = next(
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r574022144
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
- result = {}
-
- # host argument has precedence
- if not self._host_argument_set:
- result['host'] = '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)
+ # if the user didn't provide a custom host value, assume we're
+ # targeting the default Azure Storage endpoints
+ if self._host is None:
+ return {'host': '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)}
+
+ # connecting to a special storage region like Azure Government or
+ # Azure China requires setting a custom storage endpoint but we
+ # still use the same scheme to identify a specific account as for
+ # the standard storage endpoint
+ try:
+ host_suffix = next(
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] c-w commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r573824130
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
Review comment:
Added tests for the Azurite path in f9313bd6 and for the new GovCloud path in 9071060d.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] c-w commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r573830605
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
- result = {}
-
- # host argument has precedence
- if not self._host_argument_set:
- result['host'] = '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)
+ # if the user didn't provide a custom host value, assume we're
+ # targeting the default Azure Storage endpoints
+ if self._host is None:
+ return {'host': '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)}
+
+ # connecting to a special storage region like Azure Government or
+ # Azure China requires setting a custom storage endpoint but we
+ # still use the same scheme to identify a specific account as for
+ # the standard storage endpoint
+ try:
+ host_suffix = next(
Review comment:
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](https://docs.microsoft.com/en-us/azure-stack/user/azure-stack-storage-overview?view=azs-2008). However the API version that libcloud uses (`2018-11-09`) is newer than the [latest supported version](https://docs.microsoft.com/en-us/azure-stack/user/azure-stack-profiles-azure-resource-manager-versions?view=azs-2008#overview-of-the-2019-03-01-hybrid-profile) 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r572157333
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
- result = {}
-
- # host argument has precedence
- if not self._host_argument_set:
- result['host'] = '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)
+ # if the user didn't provide a custom host value, assume we're
+ # targeting the default Azure Storage endpoints
+ if self._host is None:
+ return {'host': '%s.%s' % (self.key, AZURE_STORAGE_HOST_SUFFIX)}
+
+ # connecting to a special storage region like Azure Government or
+ # Azure China requires setting a custom storage endpoint but we
+ # still use the same scheme to identify a specific account as for
+ # the standard storage endpoint
+ try:
+ host_suffix = next(
Review comment:
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] c-w merged pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
c-w merged pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] c-w commented on a change in pull request #1552: Add support for Azure Government to Azure Blobs storage driver
Posted by GitBox <gi...@apache.org>.
c-w commented on a change in pull request #1552:
URL: https://github.com/apache/libcloud/pull/1552#discussion_r573824130
##########
File path: libcloud/storage/drivers/azure_blobs.py
##########
@@ -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):
Review comment:
Added tests for the Azurite path in c5815533 and for the new GovCloud path in 739a6a33.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org