Repository: iterative/dvc

Issue:
fs: don't automatically create buckets
~~We currently do that in azure https://github.com/iterative/dvc/blob/master/dvc/remote/azure.py#L90 .~~

Related https://github.com/iterative/dvc.org/issues/415

Discussion:

Patch:
diff --git a/dvc/fs/azure.py b/dvc/fs/azure.py
index fb7132e48..eaeffb52e 100644
--- a/dvc/fs/azure.py
+++ b/dvc/fs/azure.py
@@ -84,14 +84,12 @@ class AzureFileSystem(ObjectFSWrapper):
 
         url = config.get("url")
         self.path_info = self.PATH_CLS(url)
-        self.bucket = self.path_info.bucket
 
-        if not self.bucket:
+        if not self.path_info.bucket:
             container = self._az_config.get("storage", "container_name", None)
             url = f"azure://{container}"
 
         self.path_info = self.PATH_CLS(url)
-        self.bucket = self.path_info.bucket
 
     def _prepare_credentials(self, **config):
         from azure.identity.aio import DefaultAzureCredential
@@ -117,8 +115,21 @@ class AzureFileSystem(ObjectFSWrapper):
         login_info["client_id"] = config.get("client_id")
         login_info["client_secret"] = config.get("client_secret")
 
-        if not any(
+        if not (login_info["account_name"] or login_info["connection_string"]):
+            raise AzureAuthError(
+                "Authentication to Azure Blob Storage requires either "
+                "account_name or connection_string.\nLearn more about "
+                "configuration settings at "
+                + format_link("https://man.dvc.org/remote/modify")
+            )
+
+        any_secondary = any(
             value for key, value in login_info.items() if key != "account_name"
+        )
+        if (
+            login_info["account_name"]
+            and not any_secondary
+            and not config.get("allow_anonymous_login")
         ):
             login_info["credential"] = DefaultAzureCredential(
                 exclude_interactive_browser_credential=False
@@ -167,10 +178,6 @@ class AzureFileSystem(ObjectFSWrapper):
         try:
             with _temp_event_loop():
                 file_system = AzureBlobFileSystem(**self.fs_args)
-                if self.bucket not in [
-                    container.rstrip("/") for container in file_system.ls("/")
-                ]:
-                    file_system.mkdir(self.bucket)
         except (ValueError, AzureError) as e:
             raise AzureAuthError(
                 f"Authentication to Azure Blob Storage via {self.login_method}"
diff --git a/dvc/fs/oss.py b/dvc/fs/oss.py
index 20f4fde92..b92a87714 100644
--- a/dvc/fs/oss.py
+++ b/dvc/fs/oss.py
@@ -45,7 +45,6 @@ class OSSFileSystem(BaseFileSystem):  # pylint:disable=abstract-method
         url = config.get("url")
         self.path_info = self.PATH_CLS(url) if url else None
 
-        self.bucket = self.path_info.bucket
         self.endpoint = config.get("oss_endpoint") or os.getenv("OSS_ENDPOINT")
 
         self.key_id = (
@@ -68,25 +67,17 @@ class OSSFileSystem(BaseFileSystem):  # pylint:disable=abstract-method
         logger.debug(f"key id: {self.key_id}")
         logger.debug(f"key secret: {self.key_secret}")
 
-        auth = oss2.Auth(self.key_id, self.key_secret)
-        bucket = oss2.Bucket(auth, self.endpoint, self.bucket)
-
-        # Ensure bucket exists
-        try:
-            bucket.get_bucket_info()
-        except oss2.exceptions.NoSuchBucket:
-            bucket.create_bucket(
-                oss2.BUCKET_ACL_PUBLIC_READ,
-                oss2.models.BucketCreateConfig(
-                    oss2.BUCKET_STORAGE_CLASS_STANDARD
-                ),
-            )
-        return bucket
+        return oss2.Auth(self.key_id, self.key_secret)
 
-    def _generate_download_url(self, path_info, expires=3600):
-        assert path_info.bucket == self.bucket
+    def _get_bucket(self, bucket):
+        import oss2
 
-        return self.oss_service.sign_url("GET", path_info.path, expires)
+        return oss2.Bucket(self.oss_service, self.endpoint, bucket)
+
+    def _generate_download_url(self, path_info, expires=3600):
+        return self._get_bucket(path_info.bucket).sign_url(
+            "GET", path_info.path, expires
+        )
 
     def exists(self, path_info) -> bool:
         paths = self._list_paths(path_info)
@@ -96,7 +87,7 @@ class OSSFileSystem(BaseFileSystem):  # pylint:disable=abstract-method
         import oss2
 
         for blob in oss2.ObjectIterator(
-            self.oss_service, prefix=path_info.path
+            self._get_bucket(path_info.bucket), prefix=path_info.path
         ):
             yield blob.key
 
@@ -114,16 +105,17 @@ class OSSFileSystem(BaseFileSystem):  # pylint:disable=abstract-method
             raise NotImplementedError
 
         logger.debug(f"Removing oss://{path_info}")
-        self.oss_service.delete_object(path_info.path)
+        self._get_bucket(path_info.bucket).delete_object(path_info.path)
 
     def _upload_fobj(self, fobj, to_info):
-        self.oss_service.put_object(to_info.path, fobj)
+        self._get_bucket(to_info.bucket).put_object(to_info.path, fobj)
 
     def _upload(
         self, from_file, to_info, name=None, no_progress_bar=False, **_kwargs
     ):
         with Tqdm(desc=name, disable=no_progress_bar, bytes=True) as pbar:
-            self.oss_service.put_object_from_file(
+            bucket = self._get_bucket(to_info.bucket)
+            bucket.put_object_from_file(
                 to_info.path, from_file, progress_callback=pbar.update_to
             )
 
@@ -133,8 +125,9 @@ class OSSFileSystem(BaseFileSystem):  # pylint:disable=abstract-method
         with Tqdm(desc=name, disable=no_progress_bar, bytes=True) as pbar:
             import oss2
 
+            bucket = self._get_bucket(from_info.bucket)
             oss2.resumable_download(
-                self.oss_service,
+                bucket,
                 from_info.path,
                 to_file,
                 progress_callback=pbar.update_to,
diff --git a/setup.py b/setup.py
index 86f08da6f..ee74e6ae6 100644
--- a/setup.py
+++ b/setup.py
@@ -73,7 +73,7 @@ install_requires = [
     "flufl.lock>=3.2,<4",
     "win-unicode-console>=0.5; sys_platform == 'win32'",
     "pywin32>=225; sys_platform == 'win32'",
-    "networkx>=2.1",
+    "networkx~=2.5",
     "psutil>=5.8.0",
     "pydot>=1.2.4",
     "speedcopy>=2.0.1; python_version < '3.8' and sys_platform == 'win32'",


Test patch:
diff --git a/tests/remotes/azure.py b/tests/remotes/azure.py
index 8a4c4a32d..b2a7996e7 100644
--- a/tests/remotes/azure.py
+++ b/tests/remotes/azure.py
@@ -28,19 +28,12 @@ class Azure(Base, CloudURLInfo):
     @cached_property
     def service_client(self):
         # pylint: disable=no-name-in-module
-        from azure.core.exceptions import ResourceNotFoundError
         from azure.storage.blob import BlobServiceClient
 
         service_client = BlobServiceClient.from_connection_string(
             self.CONNECTION_STRING
         )
 
-        container_client = service_client.get_container_client(self.bucket)
-        try:  # verify that container exists
-            container_client.get_container_properties()
-        except ResourceNotFoundError:
-            container_client.create_container()
-
         return service_client
 
     @property
@@ -98,7 +91,16 @@ def azure_server(test_config, docker_compose, docker_services):
 
 @pytest.fixture
 def azure(azure_server):
+    from azure.core.exceptions import ResourceNotFoundError
+
     url = f"azure://{TEST_AZURE_CONTAINER}/{uuid.uuid4()}"
     ret = Azure(url)
     ret.config = {"url": url, "connection_string": azure_server}
+
+    container = ret.service_client.get_container_client(TEST_AZURE_CONTAINER)
+    try:  # verify that container exists
+        container.get_container_properties()
+    except ResourceNotFoundError:
+        container.create_container()
+
     return ret
diff --git a/tests/remotes/oss.py b/tests/remotes/oss.py
index e84f3948c..42ee82f85 100644
--- a/tests/remotes/oss.py
+++ b/tests/remotes/oss.py
@@ -64,6 +64,8 @@ def oss_server(test_config, docker_compose, docker_services):
 
 @pytest.fixture
 def oss(oss_server):
+    import oss2
+
     url = OSS.get_url()
     ret = OSS(url)
     ret.config = {
@@ -72,6 +74,19 @@ def oss(oss_server):
         "oss_key_secret": EMULATOR_OSS_ACCESS_KEY_SECRET,
         "oss_endpoint": oss_server,
     }
+
+    auth = oss2.Auth(
+        EMULATOR_OSS_ACCESS_KEY_ID, EMULATOR_OSS_ACCESS_KEY_SECRET
+    )
+    bucket = oss2.Bucket(auth, oss_server, TEST_OSS_REPO_BUCKET)
+    try:
+        bucket.get_bucket_info()
+    except oss2.exceptions.NoSuchBucket:
+        bucket.create_bucket(
+            oss2.BUCKET_ACL_PUBLIC_READ,
+            oss2.models.BucketCreateConfig(oss2.BUCKET_STORAGE_CLASS_STANDARD),
+        )
+
     return ret
 
 
diff --git a/tests/unit/fs/test_azure.py b/tests/unit/fs/test_azure.py
index cd79c898f..cf74a32d9 100644
--- a/tests/unit/fs/test_azure.py
+++ b/tests/unit/fs/test_azure.py
@@ -3,7 +3,7 @@ from concurrent.futures import ThreadPoolExecutor
 
 import pytest
 
-from dvc.fs.azure import AzureFileSystem, _temp_event_loop
+from dvc.fs.azure import AzureAuthError, AzureFileSystem, _temp_event_loop
 from dvc.path_info import PathInfo
 
 container_name = "container-name"
@@ -67,3 +67,66 @@ def test_temp_event_loop():
 
         future = executor.submit(wrapped_procedure)
         assert future.result() == "yeey"
+
+
+def test_azure_login_methods():
+    def get_login_method(config):
+        fs = AzureFileSystem(**config)
+        # pylint: disable=pointless-statement
+        return fs.login_method
+
+    with pytest.raises(AzureAuthError):
+        get_login_method({})
+
+    assert (
+        get_login_method({"connection_string": "test"}) == "connection string"
+    )
+    assert get_login_method({"account_name": "test"}).startswith(
+        "default credentials"
+    )
+    assert (
+        get_login_method(
+            {"account_name": "test", "allow_anonymous_login": True}
+        )
+        == "anonymous login"
+    )
+
+    with pytest.raises(AzureAuthError):
+        get_login_method(
+            {"tenant_id": "test", "client_id": "test", "client_secret": "test"}
+        )
+
+    assert (
+        get_login_method(
+            {
+                "account_name": "test",
+                "tenant_id": "test",
+                "client_id": "test",
+                "client_secret": "test",
+            }
+        )
+        == "AD service principal"
+    )
+
+    assert (
+        get_login_method({"account_name": "test", "account_key": "test"})
+        == "account key"
+    )
+    assert (
+        get_login_method({"account_name": "test", "sas_token": "test"})
+        == "SAS token"
+    )
+    assert (
+        get_login_method(
+            {
+                "connection_string": "test",
+                "account_name": "test",
+                "sas_token": "test",
+            }
+        )
+        == "connection string"
+    )
+    assert (
+        get_login_method({"connection_string": "test", "sas_token": "test"})
+        == "connection string"
+    )