From 177e3f80e3059ba55855bb0959559d014a068636 Mon Sep 17 00:00:00 2001 From: "Michael R. Crusoe" Date: Thu, 15 Jul 2021 16:49:48 +0200 Subject: [PATCH 1/3] workaround for https://github.com/boto/boto3/issues/125 --- contrib/admin/mypy-with-ignore.py | 3 +- setup.py | 5 +- src/toil/jobStores/abstractJobStore.py | 2 +- src/toil/jobStores/aws/jobStore.py | 16 ++++--- src/toil/jobStores/aws/utils.py | 2 +- src/toil/lib/aws/utils.py | 50 ++++++++++++++++---- src/toil/test/jobStores/jobStoreTest.py | 12 +++-- src/toil/test/lib/aws/__init__.py | 0 src/toil/test/lib/aws/test_s3.py | 62 +++++++++++++++++++++++++ 9 files changed, 127 insertions(+), 25 deletions(-) create mode 100644 src/toil/test/lib/aws/__init__.py create mode 100644 src/toil/test/lib/aws/test_s3.py diff --git a/contrib/admin/mypy-with-ignore.py b/contrib/admin/mypy-with-ignore.py index 451534adaf..cce652d56b 100755 --- a/contrib/admin/mypy-with-ignore.py +++ b/contrib/admin/mypy-with-ignore.py @@ -99,8 +99,7 @@ def main(): 'src/toil/lib/encryption/_dummy.py', 'src/toil/lib/encryption/conftest.py', 'src/toil/lib/encryption/__init__.py', - 'src/toil/lib/aws/utils.py', - 'src/toil/lib/aws/__init__.py' + 'src/toil/lib/aws/__init__.py', ]] filtered_files_to_check = [] diff --git a/setup.py b/setup.py index 9ccd36baf0..e9818de4d5 100644 --- a/setup.py +++ b/setup.py @@ -57,10 +57,13 @@ def run_setup(): addict, pytz, pyyaml, - enlighten] + enlighten, + "typing-extensions ; python_version < '3.8'" + ] aws_reqs = [ boto, boto3, + "boto3-stubs[s3]>=1.17, <2", futures, pycryptodome] cwl_reqs = [ diff --git a/src/toil/jobStores/abstractJobStore.py b/src/toil/jobStores/abstractJobStore.py index 99c49d32cb..879759074f 100644 --- a/src/toil/jobStores/abstractJobStore.py +++ b/src/toil/jobStores/abstractJobStore.py @@ -42,7 +42,7 @@ logger = logging.getLogger(__name__) try: - from botocore.exceptions import ProxyConnectionError # type: ignore + from botocore.exceptions import ProxyConnectionError except ImportError: class ProxyConnectionError(BaseException): # type: ignore pass diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index d42dc7750c..0c111dedbc 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -57,6 +57,7 @@ from toil.jobStores.utils import (ReadablePipe, ReadableTransformingPipe, WritablePipe) +from toil.lib.aws.utils import create_s3_bucket from toil.lib.compatibility import compat_bytes from toil.lib.ec2 import establish_boto3_session from toil.lib.ec2nodes import EC2Regions @@ -744,11 +745,9 @@ def bucket_creation_pending(error): bucketExisted = False logger.debug("Bucket '%s' does not exist.", bucket_name) if create: - logger.debug("Creating bucket '%s'.", bucket_name) - location = region_to_bucket_location(self.region) - bucket = self.s3_resource.create_bucket( - Bucket=bucket_name, - CreateBucketConfiguration={'LocationConstraint': location}) + bucket = create_s3_bucket( + self.s3_resource, bucket_name, self.region + ) # Wait until the bucket exists before checking the region and adding tags bucket.wait_until_exists() @@ -757,7 +756,9 @@ def bucket_creation_pending(error): # produce an S3ResponseError with code # NoSuchBucket. We let that kick us back up to the # main retry loop. - assert self.getBucketRegion(bucket_name) == self.region + assert ( + self.getBucketRegion(bucket_name) == self.region + ), f"bucket_name: {bucket_name}, {self.getBucketRegion(bucket_name)} != {self.region}" owner_tag = os.environ.get('TOIL_OWNER_TAG') if owner_tag: @@ -1637,7 +1638,8 @@ def _delete_domain(self, domain): if not no_such_sdb_domain(e): raise - def _delete_bucket(self, bucket): + @staticmethod + def _delete_bucket(bucket): """ :param bucket: S3.Bucket """ diff --git a/src/toil/jobStores/aws/utils.py b/src/toil/jobStores/aws/utils.py index b90b959655..f58891762c 100644 --- a/src/toil/jobStores/aws/utils.py +++ b/src/toil/jobStores/aws/utils.py @@ -441,4 +441,4 @@ def region_to_bucket_location(region): def bucket_location_to_region(location): - return 'us-east-1' if location == '' else location + return "us-east-1" if location == "" or location is None else location diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index 6bde8a8095..331ecfd553 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -12,22 +12,34 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import sys +from typing import Optional, Union -from typing import Optional +from toil.lib import aws from toil.lib.misc import printq from toil.lib.retry import retry -from toil.lib import aws + +if sys.version_info >= (3, 8): + from typing import Literal +else: + from typing_extensions import Literal try: from boto.exception import BotoServerError + from mypy_boto3_s3 import S3ServiceResource + from mypy_boto3_s3.literals import BucketLocationConstraintType + from mypy_boto3_s3.service_resource import Bucket except ImportError: - BotoServerError = None # AWS/boto extra is not installed + BotoServerError = None # type: ignore + # AWS/boto extra is not installed logger = logging.getLogger(__name__) @retry(errors=[BotoServerError]) -def delete_iam_role(role_name: str, region: Optional[str] = None, quiet: bool = True): +def delete_iam_role( + role_name: str, region: Optional[str] = None, quiet: bool = True +) -> None: from boto.iam.connection import IAMConnection iam_client = aws.client('iam', region_name=region) iam_resource = aws.resource('iam', region_name=region) @@ -47,8 +59,10 @@ def delete_iam_role(role_name: str, region: Optional[str] = None, quiet: bool = @retry(errors=[BotoServerError]) -def delete_iam_instance_profile(instance_profile_name: str, region: Optional[str] = None, quiet: bool = True): - iam_resource = aws.resource('iam', region_name=region) +def delete_iam_instance_profile( + instance_profile_name: str, region: Optional[str] = None, quiet: bool = True +) -> None: + iam_resource = aws.resource("iam", region_name=region) instance_profile = iam_resource.InstanceProfile(instance_profile_name) for role in instance_profile.roles: printq(f'Now dissociating role: {role.name} from instance profile {instance_profile_name}', quiet) @@ -58,14 +72,16 @@ def delete_iam_instance_profile(instance_profile_name: str, region: Optional[str @retry(errors=[BotoServerError]) -def delete_sdb_domain(sdb_domain_name: str, region: Optional[str] = None, quiet: bool = True): - sdb_client = aws.client('sdb', region_name=region) +def delete_sdb_domain( + sdb_domain_name: str, region: Optional[str] = None, quiet: bool = True +) -> None: + sdb_client = aws.client("sdb", region_name=region) sdb_client.delete_domain(DomainName=sdb_domain_name) printq(f'SBD Domain: "{sdb_domain_name}" successfully deleted.', quiet) @retry(errors=[BotoServerError]) -def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True): +def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True) -> None: printq(f'Deleting s3 bucket in region "{region}": {bucket}', quiet) s3_client = aws.client('s3', region_name=region) s3_resource = aws.resource('s3', region_name=region) @@ -78,3 +94,19 @@ def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True): s3_client.delete_object(Bucket=bucket, Key=version['Key'], VersionId=version['VersionId']) s3_resource.Bucket(bucket).delete() printq(f'\n * Deleted s3 bucket successfully: {bucket}\n\n', quiet) + + +def create_s3_bucket( + s3_session: "S3ServiceResource", + bucket_name: str, + region: Union["BucketLocationConstraintType", Literal["us-east-1"]], +) -> "Bucket": + logger.debug("Creating bucket '%s' in region %s.", bucket_name, region) + if region == "us-east-1": # see https://github.com/boto/boto3/issues/125 + bucket = s3_session.create_bucket(Bucket=bucket_name) + else: + bucket = s3_session.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={"LocationConstraint": region}, + ) + return bucket diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index 788539fd0c..296ff92f87 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -41,6 +41,7 @@ NoSuchJobException) from toil.jobStores.fileJobStore import FileJobStore from toil.lib.memoize import memoize +from toil.lib.aws.utils import create_s3_bucket from toil.statsAndLogging import StatsAndLogging from toil.test import (ToilTest, make_tests, @@ -1408,14 +1409,17 @@ def _hashTestFile(self, url: str) -> str: def _createExternalStore(self): """A S3.Bucket instance is returned""" from toil.jobStores.aws.utils import retry_s3 - from toil.jobStores.aws.utils import region_to_bucket_location from toil.jobStores.aws.jobStore import establish_boto3_session - resource = establish_boto3_session().resource('s3', region_name=self.awsRegion()) - bucket = resource.Bucket('import-export-test-%s' % uuid.uuid4()) + + resource = establish_boto3_session().resource( + "s3", region_name=self.awsRegion() + ) + bucket_name = f"import-export-test-{uuid.uuid4()}" + location = self.awsRegion() for attempt in retry_s3(): with attempt: - bucket.create(CreateBucketConfiguration={'LocationConstraint': region_to_bucket_location(self.awsRegion())}) + bucket = create_s3_bucket(resource, bucket_name, location) bucket.wait_until_exists() return bucket diff --git a/src/toil/test/lib/aws/__init__.py b/src/toil/test/lib/aws/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/toil/test/lib/aws/test_s3.py b/src/toil/test/lib/aws/test_s3.py new file mode 100644 index 0000000000..0ad9df52c3 --- /dev/null +++ b/src/toil/test/lib/aws/test_s3.py @@ -0,0 +1,62 @@ +# Copyright (C) 2021 Michael R. Crusoe +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import logging +import os +import uuid +from typing import Optional + +from toil.jobStores.aws.jobStore import AWSJobStore +from toil.lib.aws.utils import create_s3_bucket +from toil.lib.ec2 import establish_boto3_session +from toil.test import ToilTest, needs_aws_s3 + +logger = logging.getLogger(__name__) +logging.basicConfig(level=logging.DEBUG) + + +@needs_aws_s3 +class S3Test(ToilTest): + """Confirm the workarounds for us-east-1.""" + + from mypy_boto3_s3 import S3ServiceResource + from mypy_boto3_s3.service_resource import Bucket + + s3_resource: Optional[S3ServiceResource] + bucket: Optional[Bucket] + + @classmethod + def setUpClass(cls) -> None: + session = establish_boto3_session(region_name="us-east-1") + cls.s3_resource = session.resource("s3", region_name="us-east-1") + cls.bucket = None + + def test_create_bucket(self) -> None: + """Test bucket creation for us-east-1.""" + bucket_name = f"toil-s3test-{uuid.uuid4()}" + assert self.s3_resource + self.bucket = create_s3_bucket(self.s3_resource, bucket_name, "us-east-1") + self.bucket.wait_until_exists() + owner_tag = os.environ.get("TOIL_OWNER_TAG") + if owner_tag: + bucket_tagging = self.s3_resource.BucketTagging(bucket_name) + bucket_tagging.put( + Tagging={"TagSet": [{"Key": "Owner", "Value": owner_tag}]} + ) + self.assertTrue(AWSJobStore.getBucketRegion, "us-east-1") + + @classmethod + def tearDownClass(cls) -> None: + if cls.bucket: + AWSJobStore._delete_bucket(cls.bucket) + super().tearDownClass() From 8716d04daca17fe6b195151b7c1d09958fb2b692 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 19 Jul 2021 10:31:12 -0400 Subject: [PATCH 2/3] Add a docstring to the new function --- src/toil/lib/aws/utils.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index 331ecfd553..fd1881d62b 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -101,6 +101,14 @@ def create_s3_bucket( bucket_name: str, region: Union["BucketLocationConstraintType", Literal["us-east-1"]], ) -> "Bucket": + """ + Create an AWS S3 bucket, using the given Boto3 S3 session, with the + given name, in the given region. + + Supports the us-east-1 region, where bucket creation is special. + + *ALL* S3 bucket creation should use this function. + """ logger.debug("Creating bucket '%s' in region %s.", bucket_name, region) if region == "us-east-1": # see https://github.com/boto/boto3/issues/125 bucket = s3_session.create_bucket(Bucket=bucket_name) From 570cfb3725fe130d1be61e23988b5104e1e1275a Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 19 Jul 2021 10:38:18 -0400 Subject: [PATCH 3/3] Add the new lib/aws tests to a CI job so they run --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b1a71bdd97..8afe998ae1 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -271,6 +271,7 @@ py37_main: - make test tests=src/toil/test/src - make test tests=src/toil/test/utils - make test tests=src/toil/test/lib/test_ec2.py + - make test tests=src/toil/test/lib/aws - make test tests=src/toil/test/lib/test_conversions.py py37_appliance_build: