From 062da748d08b679e1e21ed95ef1908cf3bc440b7 Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 14:46:53 -0700 Subject: [PATCH 01/11] Simplify create s3 bucket. --- src/toil/jobStores/aws/jobStore.py | 141 ++++++++------------ src/toil/lib/aws/s3.py | 90 +++++++++++++ src/toil/provisioners/aws/awsProvisioner.py | 17 ++- src/toil/test/jobStores/jobStoreTest.py | 20 ++- src/toil/test/lib/aws/test_iam.py | 16 +-- src/toil/test/lib/aws/test_s3.py | 18 +-- src/toil/test/server/serverTest.py | 6 +- 7 files changed, 178 insertions(+), 130 deletions(-) create mode 100644 src/toil/lib/aws/s3.py diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index b468c85d18..1d7922229c 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -13,7 +13,6 @@ # limitations under the License. import hashlib import itertools -import json import logging import os import pickle @@ -21,8 +20,6 @@ import reprlib import stat import time -import urllib.error -import urllib.request import uuid from contextlib import contextmanager from io import BytesIO @@ -57,10 +54,8 @@ ReadableTransformingPipe, WritablePipe) from toil.lib.aws.session import establish_boto3_session -from toil.lib.aws.utils import (create_s3_bucket, - enable_public_objects, - flatten_tags, - get_bucket_region, +from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.utils import (get_bucket_region, get_object_for_url, list_objects_for_url, retry_s3, @@ -721,85 +716,63 @@ def bucket_retry_predicate(error): return False bucketExisted = True - for attempt in retry_s3(predicate=bucket_retry_predicate): - with attempt: - try: - # the head_bucket() call makes sure that the bucket exists and the user can access it - self.s3_client.head_bucket(Bucket=bucket_name) - - bucket = self.s3_resource.Bucket(bucket_name) - except ClientError as e: - error_http_status = get_error_status(e) - if error_http_status == 404: - bucketExisted = False - logger.debug("Bucket '%s' does not exist.", bucket_name) - if create: - 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() - - # It is possible for create_bucket to return but - # for an immediate request for the bucket region to - # produce an S3ResponseError with code - # NoSuchBucket. We let that kick us back up to the - # main retry loop. - assert ( - get_bucket_region(bucket_name) == self.region - ), f"bucket_name: {bucket_name}, {get_bucket_region(bucket_name)} != {self.region}" - - tags = build_tag_dict_from_env() - - if tags: - flat_tags = flatten_tags(tags) - bucket_tagging = self.s3_resource.BucketTagging(bucket_name) - bucket_tagging.put(Tagging={'TagSet': flat_tags}) - - # Configure bucket so that we can make objects in - # it public, which was the historical default. - enable_public_objects(bucket_name) - elif block: - raise - else: - return None - elif error_http_status == 301: - # This is raised if the user attempts to get a bucket in a region outside - # the specified one, if the specified one is not `us-east-1`. The us-east-1 - # server allows a user to use buckets from any region. - raise BucketLocationConflictException(get_bucket_region(bucket_name)) - else: - raise - else: - bucketRegion = get_bucket_region(bucket_name) - if bucketRegion != self.region: - raise BucketLocationConflictException(bucketRegion) - - if versioning and not bucketExisted: - # only call this method on bucket creation - bucket.Versioning().enable() - # Now wait until versioning is actually on. Some uploads - # would come back with no versions; maybe they were - # happening too fast and this setting isn't sufficiently - # consistent? - time.sleep(1) - while not self._getBucketVersioning(bucket_name): - logger.warning(f"Waiting for versioning activation on bucket '{bucket_name}'...") - time.sleep(1) - elif check_versioning_consistency: - # now test for versioning consistency - # we should never see any of these errors since 'versioning' should always be true - bucket_versioning = self._getBucketVersioning(bucket_name) - if bucket_versioning != versioning: - assert False, 'Cannot modify versioning on existing bucket' - elif bucket_versioning is None: - assert False, 'Cannot use a bucket with versioning suspended' - if bucketExisted: - logger.debug(f"Using pre-existing job store bucket '{bucket_name}'.") + try: + # the head_bucket() call makes sure that the bucket exists and the user can access it + self.s3_client.head_bucket(Bucket=bucket_name) + bucket = self.s3_resource.Bucket(bucket_name) + except ClientError as e: + error_http_status = get_error_status(e) + if error_http_status == 404: + bucketExisted = False + logger.debug("Bucket '%s' does not exist.", bucket_name) + if create: + bucket = create_s3_bucket( + s3_resource=self.s3_resource, + bucket_name=bucket_name, + region=self.region, + tags=build_tag_dict_from_env() + ) + elif block: + raise else: - logger.debug(f"Created new job store bucket '{bucket_name}' with versioning state {versioning}.") + return None + elif error_http_status == 301: + # This is raised if the user attempts to get a bucket in a region outside + # the specified one, if the specified one is not `us-east-1`. The us-east-1 + # server allows a user to use buckets from any region. + raise BucketLocationConflictException(get_bucket_region(bucket_name)) + else: + raise + else: + bucketRegion = get_bucket_region(bucket_name) + if bucketRegion != self.region: + raise BucketLocationConflictException(bucketRegion) + + if versioning and not bucketExisted: + # only call this method on bucket creation + bucket.Versioning().enable() + # Now wait until versioning is actually on. Some uploads + # would come back with no versions; maybe they were + # happening too fast and this setting isn't sufficiently + # consistent? + time.sleep(1) + while not self._getBucketVersioning(bucket_name): + logger.warning(f"Waiting for versioning activation on bucket '{bucket_name}'...") + time.sleep(1) + elif check_versioning_consistency: + # now test for versioning consistency + # we should never see any of these errors since 'versioning' should always be true + bucket_versioning = self._getBucketVersioning(bucket_name) + if bucket_versioning != versioning: + assert False, 'Cannot modify versioning on existing bucket' + elif bucket_versioning is None: + assert False, 'Cannot use a bucket with versioning suspended' + if bucketExisted: + logger.debug(f"Using pre-existing job store bucket '{bucket_name}'.") + else: + logger.debug(f"Created new job store bucket '{bucket_name}' with versioning state {versioning}.") - return bucket + return bucket def _bindDomain(self, domain_name, create=False, block=True): """ diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py new file mode 100644 index 0000000000..35331a4147 --- /dev/null +++ b/src/toil/lib/aws/s3.py @@ -0,0 +1,90 @@ +# Copyright (C) 2015-2023 Regents of the University of California +# +# 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 sys + +from typing import (Dict, + Optional, + Union) + +from toil.lib.retry import retry +from toil.lib.aws.utils import (enable_public_objects, + flatten_tags, + get_bucket_region) + +if sys.version_info >= (3, 8): + from typing import Literal +else: + from typing_extensions import Literal + +try: + from boto.exception import BotoServerError, S3ResponseError + from botocore.exceptions import ClientError + from mypy_boto3_iam import IAMClient, IAMServiceResource + from mypy_boto3_s3 import S3Client, S3ServiceResource + from mypy_boto3_s3.literals import BucketLocationConstraintType + from mypy_boto3_s3.service_resource import Bucket, Object + from mypy_boto3_sdb import SimpleDBClient +except ImportError: + BotoServerError = Exception # type: ignore + S3ResponseError = Exception # type: ignore + ClientError = Exception # type: ignore + # AWS/boto extra is not installed + + +logger = logging.getLogger(__name__) + + +@retry(errors=[BotoServerError, S3ResponseError, ClientError]) +def create_s3_bucket( + s3_resource: "S3ServiceResource", + bucket_name: str, + region: Union["BucketLocationConstraintType", Literal["us-east-1"]], + tags: Optional[Dict[str]], + public: bool = True +) -> "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_resource.create_bucket(Bucket=bucket_name) + else: + bucket = s3_resource.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={"LocationConstraint": region}, + ) + # wait until the bucket exists before checking the region and adding tags + bucket.wait_until_exists() + + # do we really this? do we need to doubt AWS creating a bucket in the correct region? + check_region = get_bucket_region(bucket_name) + if region != check_region: + raise RuntimeError(f"s3 bucket does not match region: {bucket_name}, {region} != {check_region}") + + if tags: + bucket_tagging = s3_resource.BucketTagging(bucket_name) + bucket_tagging.put(Tagging={'TagSet': flatten_tags(tags)}) + + # Configure bucket so that we can make objects in + # it public, which was the historical default. + if public: + enable_public_objects(bucket_name) + + return bucket diff --git a/src/toil/provisioners/aws/awsProvisioner.py b/src/toil/provisioners/aws/awsProvisioner.py index 820558de45..a8669765d6 100644 --- a/src/toil/provisioners/aws/awsProvisioner.py +++ b/src/toil/provisioners/aws/awsProvisioner.py @@ -44,13 +44,13 @@ from boto.utils import get_instance_metadata from botocore.exceptions import ClientError -from toil.lib.aws import zone_to_region +from toil.lib.aws import zone_to_region, build_tag_dict_from_env from toil.lib.aws.ami import get_flatcar_ami from toil.lib.aws.iam import (CLUSTER_LAUNCHING_PERMISSIONS, get_policy_permissions, policy_permissions_allow) from toil.lib.aws.session import AWSConnectionManager -from toil.lib.aws.utils import create_s3_bucket +from toil.lib.aws.s3 import create_s3_bucket from toil.lib.conversions import human2bytes from toil.lib.ec2 import (a_short_time, create_auto_scaling_group, @@ -259,14 +259,13 @@ def _write_file_to_cloud(self, key: str, contents: bytes) -> str: bucket = s3.Bucket(bucket_name) except ClientError as err: if get_error_status(err) == 404: - bucket = create_s3_bucket(s3, bucket_name=bucket_name, region=self._region) - bucket.wait_until_exists() + bucket = create_s3_bucket( + s3_resource=s3, + bucket_name=bucket_name, + region=self._region, + tags=build_tag_dict_from_env() + ) bucket.Versioning().enable() - - owner_tag = os.environ.get('TOIL_OWNER_TAG') - if owner_tag: - bucket_tagging = s3.BucketTagging(bucket_name) - bucket_tagging.put(Tagging={'TagSet': [{'Key': 'Owner', 'Value': owner_tag}]}) else: raise diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index 618f6fcbcc..7023b1204f 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -40,7 +40,8 @@ from toil.jobStores.abstractJobStore import (NoSuchFileException, NoSuchJobException) from toil.jobStores.fileJobStore import FileJobStore -from toil.lib.aws.utils import create_s3_bucket, get_object_for_url +from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.utils import get_object_for_url from toil.lib.memoize import memoize from toil.lib.retry import retry from toil.statsAndLogging import StatsAndLogging @@ -1468,19 +1469,14 @@ def _hashTestFile(self, url: str) -> str: def _createExternalStore(self): """A S3.Bucket instance is returned""" from toil.jobStores.aws.jobStore import establish_boto3_session - from toil.lib.aws.utils import retry_s3 - resource = establish_boto3_session().resource( - "s3", region_name=self.awsRegion() + resource = establish_boto3_session().resource("s3", region_name=self.awsRegion()) + + return create_s3_bucket( + s3_resource=resource, + bucket_name=f"import-export-test-{uuid.uuid4()}", + region=self.awsRegion() ) - bucket_name = f"import-export-test-{uuid.uuid4()}" - location = self.awsRegion() - - for attempt in retry_s3(): - with attempt: - bucket = create_s3_bucket(resource, bucket_name, location) - bucket.wait_until_exists() - return bucket def _cleanUpExternalStore(self, bucket): bucket.objects.all().delete() diff --git a/src/toil/test/lib/aws/test_iam.py b/src/toil/test/lib/aws/test_iam.py index f61c076ca5..fe6d6ae54d 100644 --- a/src/toil/test/lib/aws/test_iam.py +++ b/src/toil/test/lib/aws/test_iam.py @@ -12,24 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -import os -import uuid -from typing import Optional - -import pytest - -from toil.jobStores.aws.jobStore import AWSJobStore -from toil.lib.aws import iam -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 - -import moto import boto3 import json from moto import mock_iam +from toil.lib.aws import iam +from toil.test import ToilTest + logger = logging.getLogger(__name__) logging.basicConfig(level=logging.DEBUG) diff --git a/src/toil/test/lib/aws/test_s3.py b/src/toil/test/lib/aws/test_s3.py index 131e2c3cbc..c00ded0968 100644 --- a/src/toil/test/lib/aws/test_s3.py +++ b/src/toil/test/lib/aws/test_s3.py @@ -17,8 +17,10 @@ from typing import Optional from toil.jobStores.aws.jobStore import AWSJobStore +from toil.lib.aws import build_tag_dict_from_env from toil.lib.aws.session import establish_boto3_session -from toil.lib.aws.utils import create_s3_bucket, get_bucket_region +from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.utils import get_bucket_region from toil.test import ToilTest, needs_aws_s3 logger = logging.getLogger(__name__) @@ -46,14 +48,12 @@ def test_create_bucket(self) -> None: """Test bucket creation for us-east-1.""" bucket_name = f"toil-s3test-{uuid.uuid4()}" assert self.s3_resource - S3Test.bucket = create_s3_bucket(self.s3_resource, bucket_name, "us-east-1") - S3Test.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}]} - ) + S3Test.bucket = create_s3_bucket( + s3_resource=self.s3_resource, + bucket_name=bucket_name, + region="us-east-1", + tags=build_tag_dict_from_env() + ) self.assertEqual(get_bucket_region(bucket_name), "us-east-1") # Make sure all the bucket location getting strategies work on a bucket we created diff --git a/src/toil/test/server/serverTest.py b/src/toil/test/server/serverTest.py index 6de0458b91..25af1cad1c 100644 --- a/src/toil/test/server/serverTest.py +++ b/src/toil/test/server/serverTest.py @@ -33,6 +33,7 @@ # extra wasn't installed. We'll then skip them all. pass +from toil.lib.aws import build_tag_dict_from_env from toil.test import ToilTest, needs_aws_s3, needs_celery_broker, needs_server logger = logging.getLogger(__name__) @@ -206,14 +207,13 @@ def setUpClass(cls) -> None: super().setUpClass() from toil.lib.aws import get_current_aws_region, session - from toil.lib.aws.utils import create_s3_bucket + from toil.lib.aws.s3 import create_s3_bucket cls.region = get_current_aws_region() cls.s3_resource = session.resource("s3", region_name=cls.region) cls.bucket_name = f"toil-test-{uuid.uuid4()}" - cls.bucket = create_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region) - cls.bucket.wait_until_exists() + cls.bucket = create_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region, tags=build_tag_dict_from_env()) @classmethod def tearDownClass(cls) -> None: From 114268172231d400b83f6b9315434a3c37c86e0f Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 14:53:30 -0700 Subject: [PATCH 02/11] Remove old function. --- src/toil/lib/aws/utils.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index ec6e7843db..8b27ada9dc 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -195,29 +195,6 @@ def delete_s3_bucket( printq(f'\n * S3 bucket no longer exists: {bucket}\n\n', quiet) -def create_s3_bucket( - s3_resource: "S3ServiceResource", - 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_resource.create_bucket(Bucket=bucket_name) - else: - bucket = s3_resource.create_bucket( - Bucket=bucket_name, - CreateBucketConfiguration={"LocationConstraint": region}, - ) - return bucket - @retry(errors=[ClientError]) def enable_public_objects(bucket_name: str) -> None: """ From 25ab299b4515601a925246a0b5b18f4f9971b51f Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 16:13:08 -0700 Subject: [PATCH 03/11] Resolve one more mk bucket location. --- src/toil/test/jobStores/jobStoreTest.py | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index 7023b1204f..8b790083c7 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -40,6 +40,7 @@ from toil.jobStores.abstractJobStore import (NoSuchFileException, NoSuchJobException) from toil.jobStores.fileJobStore import FileJobStore +from toil.lib.aws import build_tag_dict_from_env from toil.lib.aws.s3 import create_s3_bucket from toil.lib.aws.utils import get_object_for_url from toil.lib.memoize import memoize @@ -1325,22 +1326,16 @@ def testSDBDomainsDeletedOnFailedJobstoreBucketCreation(self): # both the default, and a non-default server. testJobStoreUUID = str(uuid.uuid4()) # Create the bucket at the external region - bucketName = 'domain-test-' + testJobStoreUUID + '--files' + bucket_name = 'domain-test-' + testJobStoreUUID + '--files' client = establish_boto3_session().client('s3', region_name=externalAWSLocation) resource = establish_boto3_session().resource('s3', region_name=externalAWSLocation) - for attempt in retry_s3(delays=(2, 5, 10, 30, 60), timeout=600): - with attempt: - # Create the bucket at the home region - client.create_bucket(Bucket=bucketName, - CreateBucketConfiguration={'LocationConstraint': externalAWSLocation}) - - owner_tag = os.environ.get('TOIL_OWNER_TAG') - if owner_tag: - for attempt in retry_s3(delays=(1, 1, 2, 4, 8, 16), timeout=33): - with attempt: - bucket_tagging = resource.BucketTagging(bucketName) - bucket_tagging.put(Tagging={'TagSet': [{'Key': 'Owner', 'Value': owner_tag}]}) + create_s3_bucket( + s3_resource=resource, + bucket_name=bucket_name, + region=externalAWSLocation, + tags=build_tag_dict_from_env() + ) options = Job.Runner.getDefaultOptions('aws:' + testRegion + ':domain-test-' + testJobStoreUUID) options.logLevel = 'DEBUG' @@ -1366,7 +1361,7 @@ def testSDBDomainsDeletedOnFailedJobstoreBucketCreation(self): try: for attempt in retry_s3(): with attempt: - client.delete_bucket(Bucket=bucketName) + client.delete_bucket(Bucket=bucket_name) except ClientError as e: # The actual HTTP code of the error is in status. if e.response.get('ResponseMetadata', {}).get('HTTPStatusCode') == 404: From 5bf34bcb86d0d65a0584c12160c34da4bdb1445b Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 17:08:30 -0700 Subject: [PATCH 04/11] Simplify tagging. --- src/toil/common.py | 2 +- src/toil/jobStores/aws/jobStore.py | 8 +-- src/toil/lib/aws/__init__.py | 26 ++++---- src/toil/lib/aws/s3.py | 23 +++---- src/toil/provisioners/aws/awsProvisioner.py | 9 +-- src/toil/test/jobStores/jobStoreTest.py | 8 +-- src/toil/test/lib/aws/test_s3.py | 9 +-- src/toil/test/lib/aws/test_utils.py | 67 --------------------- src/toil/test/server/serverTest.py | 4 +- src/toil/utils/toilLaunchCluster.py | 5 +- 10 files changed, 30 insertions(+), 131 deletions(-) delete mode 100644 src/toil/test/lib/aws/test_utils.py diff --git a/src/toil/common.py b/src/toil/common.py index 0faedf4ca3..5fd02dee9e 100644 --- a/src/toil/common.py +++ b/src/toil/common.py @@ -69,7 +69,7 @@ QueueSizeMessage, gen_message_bus_path) from toil.fileStores import FileID -from toil.lib.aws import zone_to_region, build_tag_dict_from_env +from toil.lib.aws import zone_to_region from toil.lib.compatibility import deprecated from toil.lib.conversions import bytes2human, human2bytes from toil.lib.io import try_path diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index 1d7922229c..cd941dbd60 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -32,7 +32,6 @@ from botocore.exceptions import ClientError import toil.lib.encryption as encryption -from toil.lib.aws import build_tag_dict_from_env from toil.fileStores import FileID from toil.jobStores.abstractJobStore import (AbstractJobStore, ConcurrentFileModificationException, @@ -726,12 +725,7 @@ def bucket_retry_predicate(error): bucketExisted = False logger.debug("Bucket '%s' does not exist.", bucket_name) if create: - bucket = create_s3_bucket( - s3_resource=self.s3_resource, - bucket_name=bucket_name, - region=self.region, - tags=build_tag_dict_from_env() - ) + bucket = create_s3_bucket(self.s3_resource, bucket_name, self.region) elif block: raise else: diff --git a/src/toil/lib/aws/__init__.py b/src/toil/lib/aws/__init__.py index 3b1d493ddd..c1eee5dfbb 100644 --- a/src/toil/lib/aws/__init__.py +++ b/src/toil/lib/aws/__init__.py @@ -168,6 +168,7 @@ def file_begins_with(path, prefix): except (URLError, socket.timeout, HTTPException): return False + def running_on_ecs() -> bool: """ Return True if we are currently running on Amazon ECS, and false otherwise. @@ -175,22 +176,17 @@ def running_on_ecs() -> bool: # We only care about relatively current ECS return 'ECS_CONTAINER_METADATA_URI_V4' in os.environ -def build_tag_dict_from_env(environment: MutableMapping[str, str] = os.environ) -> Dict[str, str]: - tags = dict() - owner_tag = environment.get('TOIL_OWNER_TAG') + +def tags_from_env() -> Dict[str, str]: + try: + tags = json.loads(os.environ.get('TOIL_AWS_TAGS', '{}')) + except json.decoder.JSONDecodeError: + logger.error('TOIL_AWS_TAGS must be in JSON format: {"key" : "value", ...}') + exit(1) + + # TODO: Remove TOIL_OWNER_TAG and only use TOIL_AWS_TAGS . + owner_tag = os.environ.get('TOIL_OWNER_TAG') if owner_tag: tags.update({'Owner': owner_tag}) - user_tags = environment.get('TOIL_AWS_TAGS') - if user_tags: - try: - json_user_tags = json.loads(user_tags) - if isinstance(json_user_tags, dict): - tags.update(json.loads(user_tags)) - else: - logger.error('TOIL_AWS_TAGS must be in JSON format: {"key" : "value", ...}') - exit(1) - except json.decoder.JSONDecodeError: - logger.error('TOIL_AWS_TAGS must be in JSON format: {"key" : "value", ...}') - exit(1) return tags diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index 35331a4147..9772f31729 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -19,9 +19,8 @@ Union) from toil.lib.retry import retry -from toil.lib.aws.utils import (enable_public_objects, - flatten_tags, - get_bucket_region) +from toil.lib.aws import tags_from_env +from toil.lib.aws.utils import enable_public_objects, flatten_tags if sys.version_info >= (3, 8): from typing import Literal @@ -51,7 +50,7 @@ def create_s3_bucket( s3_resource: "S3ServiceResource", bucket_name: str, region: Union["BucketLocationConstraintType", Literal["us-east-1"]], - tags: Optional[Dict[str]], + tags: Optional[Dict[str]] = None, public: bool = True ) -> "Bucket": """ @@ -70,20 +69,14 @@ def create_s3_bucket( Bucket=bucket_name, CreateBucketConfiguration={"LocationConstraint": region}, ) - # wait until the bucket exists before checking the region and adding tags + # wait until the bucket exists before adding tags bucket.wait_until_exists() - # do we really this? do we need to doubt AWS creating a bucket in the correct region? - check_region = get_bucket_region(bucket_name) - if region != check_region: - raise RuntimeError(f"s3 bucket does not match region: {bucket_name}, {region} != {check_region}") + tags = tags_from_env() if tags is None else tags + bucket_tagging = s3_resource.BucketTagging(bucket_name) + bucket_tagging.put(Tagging={'TagSet': flatten_tags(tags)}) - if tags: - bucket_tagging = s3_resource.BucketTagging(bucket_name) - bucket_tagging.put(Tagging={'TagSet': flatten_tags(tags)}) - - # Configure bucket so that we can make objects in - # it public, which was the historical default. + # enabling public objects is the historical default if public: enable_public_objects(bucket_name) diff --git a/src/toil/provisioners/aws/awsProvisioner.py b/src/toil/provisioners/aws/awsProvisioner.py index a8669765d6..46fadb80a3 100644 --- a/src/toil/provisioners/aws/awsProvisioner.py +++ b/src/toil/provisioners/aws/awsProvisioner.py @@ -44,7 +44,7 @@ from boto.utils import get_instance_metadata from botocore.exceptions import ClientError -from toil.lib.aws import zone_to_region, build_tag_dict_from_env +from toil.lib.aws import zone_to_region from toil.lib.aws.ami import get_flatcar_ami from toil.lib.aws.iam import (CLUSTER_LAUNCHING_PERMISSIONS, get_policy_permissions, @@ -259,12 +259,7 @@ def _write_file_to_cloud(self, key: str, contents: bytes) -> str: bucket = s3.Bucket(bucket_name) except ClientError as err: if get_error_status(err) == 404: - bucket = create_s3_bucket( - s3_resource=s3, - bucket_name=bucket_name, - region=self._region, - tags=build_tag_dict_from_env() - ) + bucket = create_s3_bucket(s3, bucket_name, self._region) bucket.Versioning().enable() else: raise diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index 8b790083c7..f8399ed72b 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -40,7 +40,6 @@ from toil.jobStores.abstractJobStore import (NoSuchFileException, NoSuchJobException) from toil.jobStores.fileJobStore import FileJobStore -from toil.lib.aws import build_tag_dict_from_env from toil.lib.aws.s3 import create_s3_bucket from toil.lib.aws.utils import get_object_for_url from toil.lib.memoize import memoize @@ -1330,12 +1329,7 @@ def testSDBDomainsDeletedOnFailedJobstoreBucketCreation(self): client = establish_boto3_session().client('s3', region_name=externalAWSLocation) resource = establish_boto3_session().resource('s3', region_name=externalAWSLocation) - create_s3_bucket( - s3_resource=resource, - bucket_name=bucket_name, - region=externalAWSLocation, - tags=build_tag_dict_from_env() - ) + create_s3_bucket(resource, bucket_name, externalAWSLocation) options = Job.Runner.getDefaultOptions('aws:' + testRegion + ':domain-test-' + testJobStoreUUID) options.logLevel = 'DEBUG' diff --git a/src/toil/test/lib/aws/test_s3.py b/src/toil/test/lib/aws/test_s3.py index c00ded0968..d922fa35f2 100644 --- a/src/toil/test/lib/aws/test_s3.py +++ b/src/toil/test/lib/aws/test_s3.py @@ -17,7 +17,6 @@ from typing import Optional from toil.jobStores.aws.jobStore import AWSJobStore -from toil.lib.aws import build_tag_dict_from_env from toil.lib.aws.session import establish_boto3_session from toil.lib.aws.s3 import create_s3_bucket from toil.lib.aws.utils import get_bucket_region @@ -47,13 +46,7 @@ def setUpClass(cls) -> 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 - S3Test.bucket = create_s3_bucket( - s3_resource=self.s3_resource, - bucket_name=bucket_name, - region="us-east-1", - tags=build_tag_dict_from_env() - ) + S3Test.bucket = create_s3_bucket(self.s3_resource, bucket_name, "us-east-1") self.assertEqual(get_bucket_region(bucket_name), "us-east-1") # Make sure all the bucket location getting strategies work on a bucket we created diff --git a/src/toil/test/lib/aws/test_utils.py b/src/toil/test/lib/aws/test_utils.py deleted file mode 100644 index 3de3862a94..0000000000 --- a/src/toil/test/lib/aws/test_utils.py +++ /dev/null @@ -1,67 +0,0 @@ -# Copyright (C) 2015-2021 Regents of the University of California -# -# 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 - -import pytest - -from toil.lib.aws import build_tag_dict_from_env -from toil.test import ToilTest - -logger = logging.getLogger(__name__) -logging.basicConfig(level=logging.DEBUG) - -class TagGenerationTest(ToilTest): - """ - Test for tag generation from environment variables - """ - def test_build_tag(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = "😀" - environment["TOIL_AWS_TAGS"] = None - tag_dict = build_tag_dict_from_env(environment) - assert(tag_dict == {'Owner': '😀'}) - - def test_empty_aws_tags(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "{}" - tag_dict = build_tag_dict_from_env(environment) - assert (tag_dict == dict()) - - def test_incorrect_json_object(self): - with pytest.raises(SystemExit): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "231" - tag_dict = build_tag_dict_from_env(environment) - - def test_incorrect_json_emoji(self): - with pytest.raises(SystemExit): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "😀" - tag_dict = build_tag_dict_from_env(environment) - - def test_build_tag_with_tags(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = "😀" - environment["TOIL_AWS_TAGS"] = '{"1": "2", " ":")"}' - tag_dict = build_tag_dict_from_env(environment) - assert(tag_dict == {'Owner': '😀', '1': '2', ' ': ')'}) - - - diff --git a/src/toil/test/server/serverTest.py b/src/toil/test/server/serverTest.py index 25af1cad1c..5a0f221c7f 100644 --- a/src/toil/test/server/serverTest.py +++ b/src/toil/test/server/serverTest.py @@ -33,7 +33,6 @@ # extra wasn't installed. We'll then skip them all. pass -from toil.lib.aws import build_tag_dict_from_env from toil.test import ToilTest, needs_aws_s3, needs_celery_broker, needs_server logger = logging.getLogger(__name__) @@ -213,7 +212,7 @@ def setUpClass(cls) -> None: cls.s3_resource = session.resource("s3", region_name=cls.region) cls.bucket_name = f"toil-test-{uuid.uuid4()}" - cls.bucket = create_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region, tags=build_tag_dict_from_env()) + cls.bucket = create_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region) @classmethod def tearDownClass(cls) -> None: @@ -222,6 +221,7 @@ def tearDownClass(cls) -> None: delete_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region) super().tearDownClass() + class AWSStateStoreTest(hidden.AbstractStateStoreTest, BucketUsingTest): """ Test AWS-based state storage. diff --git a/src/toil/utils/toilLaunchCluster.py b/src/toil/utils/toilLaunchCluster.py index 27bea8ad77..712d8528a1 100644 --- a/src/toil/utils/toilLaunchCluster.py +++ b/src/toil/utils/toilLaunchCluster.py @@ -18,8 +18,9 @@ from typing import Dict, List, Tuple, Union from toil import applianceSelf + +from toil.lib.aws import tags_from_env from toil.common import parser_with_common_options -from toil.lib.aws import build_tag_dict_from_env from toil.provisioners import (check_valid_node_types, cluster_factory, parse_node_types) @@ -117,7 +118,7 @@ def main() -> None: options = parser.parse_args() set_logging_from_options(options) - tags = create_tags_dict(options.tags) if options.tags else build_tag_dict_from_env() + tags = create_tags_dict(options.tags) if options.tags else tags_from_env() # Get worker node types worker_node_types = parse_node_types(options.nodeTypes) From cd47aa147796b96bb0fa51f743d83189534ada12 Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 22:00:56 -0700 Subject: [PATCH 05/11] Typing. --- src/toil/lib/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index 9772f31729..a7d32c335c 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -50,7 +50,7 @@ def create_s3_bucket( s3_resource: "S3ServiceResource", bucket_name: str, region: Union["BucketLocationConstraintType", Literal["us-east-1"]], - tags: Optional[Dict[str]] = None, + tags: Optional[Dict[str, str]] = None, public: bool = True ) -> "Bucket": """ From b39d561a3b6bbcc911e95c4912365397619c58dc Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 16 Aug 2023 22:24:15 -0700 Subject: [PATCH 06/11] Typing. --- src/toil/lib/aws/s3.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index a7d32c335c..87364c54c8 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -14,7 +14,8 @@ import logging import sys -from typing import (Dict, +from typing import (Any, + Dict, Optional, Union) @@ -74,7 +75,7 @@ def create_s3_bucket( tags = tags_from_env() if tags is None else tags bucket_tagging = s3_resource.BucketTagging(bucket_name) - bucket_tagging.put(Tagging={'TagSet': flatten_tags(tags)}) + bucket_tagging.put(Tagging={'TagSet': flatten_tags(tags)}) # type: ignore # enabling public objects is the historical default if public: From 5b25a35a59821b56b54dcf7f26be6a2fc9d2cc7f Mon Sep 17 00:00:00 2001 From: Lon Blauvelt Date: Thu, 31 Aug 2023 07:10:13 -0700 Subject: [PATCH 07/11] Simplify s3 functions: delete_s3_bucket. (#4567) * Simplify delete bucket function. * Don't lie in progress message --------- Co-authored-by: Adam Novak --- contrib/admin/cleanup_aws_resources.py | 3 +- src/toil/jobStores/aws/jobStore.py | 29 ++--------- src/toil/lib/aws/s3.py | 64 ++++++++++++++++++++++++- src/toil/lib/aws/utils.py | 29 ----------- src/toil/test/jobStores/jobStoreTest.py | 15 ++---- src/toil/test/lib/aws/test_s3.py | 4 +- src/toil/test/server/serverTest.py | 4 +- 7 files changed, 74 insertions(+), 74 deletions(-) diff --git a/contrib/admin/cleanup_aws_resources.py b/contrib/admin/cleanup_aws_resources.py index 709e78670a..b741568657 100755 --- a/contrib/admin/cleanup_aws_resources.py +++ b/contrib/admin/cleanup_aws_resources.py @@ -22,7 +22,8 @@ from src.toil.lib import aws from src.toil.lib.aws import session -from src.toil.lib.aws.utils import delete_iam_role, delete_iam_instance_profile, delete_s3_bucket, delete_sdb_domain +from src.toil.lib.aws.utils import delete_iam_role, delete_iam_instance_profile, delete_sdb_domain +from src.toil.lib.aws.s3 import delete_s3_bucket from src.toil.lib.generatedEC2Lists import regionDict # put us-west-2 first as our default test region; that way anything with a universal region shows there diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index cd941dbd60..11f1a8acfc 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -30,6 +30,7 @@ import boto.sdb from boto.exception import SDBResponseError from botocore.exceptions import ClientError +from mypy_boto3_s3.service_resource import Bucket import toil.lib.encryption as encryption from toil.fileStores import FileID @@ -53,7 +54,7 @@ ReadableTransformingPipe, WritablePipe) from toil.lib.aws.session import establish_boto3_session -from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.s3 import create_s3_bucket, delete_s3_bucket from toil.lib.aws.utils import (get_bucket_region, get_object_for_url, list_objects_for_url, @@ -1585,7 +1586,7 @@ def destroy(self): # TODO: Add other failure cases to be ignored here. self._registered = None if self.filesBucket is not None: - self._delete_bucket(self.filesBucket) + delete_s3_bucket(s3_resource=s3_boto3_resource, bucket_name=self.filesBucket) self.filesBucket = None for name in 'filesDomain', 'jobsDomain': domain = getattr(self, name) @@ -1603,30 +1604,6 @@ def _delete_domain(self, domain): if not no_such_sdb_domain(e): raise - @staticmethod - def _delete_bucket(bucket): - """ - :param bucket: S3.Bucket - """ - for attempt in retry_s3(): - with attempt: - try: - uploads = s3_boto3_client.list_multipart_uploads(Bucket=bucket.name).get('Uploads') - if uploads: - for u in uploads: - s3_boto3_client.abort_multipart_upload(Bucket=bucket.name, - Key=u["Key"], - UploadId=u["UploadId"]) - - bucket.objects.all().delete() - bucket.object_versions.delete() - bucket.delete() - except s3_boto3_client.exceptions.NoSuchBucket: - pass - except ClientError as e: - if get_error_status(e) != 404: - raise - aRepr = reprlib.Repr() aRepr.maxstring = 38 # so UUIDs don't get truncated (36 for UUID plus 2 for quotes) diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index 87364c54c8..3df32cccca 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -16,10 +16,13 @@ from typing import (Any, Dict, + List, Optional, - Union) + Union, + cast) -from toil.lib.retry import retry +from toil.lib.retry import retry, get_error_status +from toil.lib.misc import printq from toil.lib.aws import tags_from_env from toil.lib.aws.utils import enable_public_objects, flatten_tags @@ -82,3 +85,60 @@ def create_s3_bucket( enable_public_objects(bucket_name) return bucket + + +@retry(errors=[BotoServerError, S3ResponseError, ClientError]) +def delete_s3_bucket( + s3_resource: "S3ServiceResource", + bucket_name: str, + quiet: bool = True +) -> None: + """ + Delete the bucket with 'bucket_name'. + + Note: 'quiet' is False when used for a clean up utility script (contrib/admin/cleanup_aws_resources.py) + that prints progress rather than logging. Logging should be used for all other internal Toil usage. + """ + logger.debug("Deleting bucket '%s'.", bucket_name) + printq(f'\n * Deleting s3 bucket: {bucket_name}\n\n', quiet) + + s3_client = s3_resource.meta.client + + try: + for u in s3_client.list_multipart_uploads(Bucket=bucket_name).get('Uploads', []): + s3_client.abort_multipart_upload( + Bucket=bucket_name, + Key=u["Key"], + UploadId=u["UploadId"] + ) + + paginator = s3_client.get_paginator('list_object_versions') + for response in paginator.paginate(Bucket=bucket_name): + # Versions and delete markers can both go in here to be deleted. + # They both have Key and VersionId, but there's no shared base type + # defined for them in the stubs to express that. See + # . So we + # have to do gymnastics to get them into the same list. + to_delete: List[Dict[str, Any]] = cast(List[Dict[str, Any]], response.get('Versions', [])) + \ + cast(List[Dict[str, Any]], response.get('DeleteMarkers', [])) + for entry in to_delete: + printq(f" Deleting {entry['Key']} version {entry['VersionId']}", quiet) + s3_client.delete_object( + Bucket=bucket_name, + Key=entry['Key'], + VersionId=entry['VersionId'] + ) + bucket = s3_resource.Bucket(bucket_name) + bucket.objects.all().delete() + bucket.object_versions.delete() + bucket.delete() + printq(f'\n * Deleted s3 bucket successfully: {bucket_name}\n\n', quiet) + logger.debug("Deleted s3 bucket successfully '%s'.", bucket_name) + except s3_client.exceptions.NoSuchBucket: + printq(f'\n * S3 bucket no longer exists: {bucket_name}\n\n', quiet) + logger.debug("S3 bucket no longer exists '%s'.", bucket_name) + except ClientError as e: + if get_error_status(e) != 404: + raise + printq(f'\n * S3 bucket no longer exists: {bucket_name}\n\n', quiet) + logger.debug("S3 bucket no longer exists '%s'.", bucket_name) diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index 8b27ada9dc..03f93b7645 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -165,35 +165,6 @@ def retry_s3(delays: Iterable[float] = DEFAULT_DELAYS, timeout: float = DEFAULT_ """ return old_retry(delays=delays, timeout=timeout, predicate=predicate) -@retry(errors=[BotoServerError]) -def delete_s3_bucket( - s3_resource: "S3ServiceResource", - bucket: str, - quiet: bool = True -) -> None: - """ - Delete the given S3 bucket. - """ - printq(f'Deleting s3 bucket: {bucket}', quiet) - - paginator = s3_resource.meta.client.get_paginator('list_object_versions') - try: - for response in paginator.paginate(Bucket=bucket): - # Versions and delete markers can both go in here to be deleted. - # They both have Key and VersionId, but there's no shared base type - # defined for them in the stubs to express that. See - # . So we - # have to do gymnastics to get them into the same list. - to_delete: List[Dict[str, Any]] = cast(List[Dict[str, Any]], response.get('Versions', [])) + \ - cast(List[Dict[str, Any]], response.get('DeleteMarkers', [])) - for entry in to_delete: - printq(f" Deleting {entry['Key']} version {entry['VersionId']}", quiet) - s3_resource.meta.client.delete_object(Bucket=bucket, Key=entry['Key'], VersionId=entry['VersionId']) - s3_resource.Bucket(bucket).delete() - printq(f'\n * Deleted s3 bucket successfully: {bucket}\n\n', quiet) - except s3_resource.meta.client.exceptions.NoSuchBucket: - printq(f'\n * S3 bucket no longer exists: {bucket}\n\n', quiet) - @retry(errors=[ClientError]) def enable_public_objects(bucket_name: str) -> None: diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py index f8399ed72b..2ecf707828 100644 --- a/src/toil/test/jobStores/jobStoreTest.py +++ b/src/toil/test/jobStores/jobStoreTest.py @@ -40,7 +40,7 @@ from toil.jobStores.abstractJobStore import (NoSuchFileException, NoSuchJobException) from toil.jobStores.fileJobStore import FileJobStore -from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.s3 import create_s3_bucket, delete_s3_bucket from toil.lib.aws.utils import get_object_for_url from toil.lib.memoize import memoize from toil.lib.retry import retry @@ -1352,17 +1352,8 @@ def testSDBDomainsDeletedOnFailedJobstoreBucketCreation(self): else: self.fail() finally: - try: - for attempt in retry_s3(): - with attempt: - client.delete_bucket(Bucket=bucket_name) - except ClientError as e: - # The actual HTTP code of the error is in status. - if e.response.get('ResponseMetadata', {}).get('HTTPStatusCode') == 404: - # The bucket doesn't exist; maybe a failed delete actually succeeded. - pass - else: - raise + delete_s3_bucket(s3_resource=resource, bucket_name=bucket_name) + @slow def testInlinedFiles(self): diff --git a/src/toil/test/lib/aws/test_s3.py b/src/toil/test/lib/aws/test_s3.py index d922fa35f2..1d0dd90d89 100644 --- a/src/toil/test/lib/aws/test_s3.py +++ b/src/toil/test/lib/aws/test_s3.py @@ -18,7 +18,7 @@ from toil.jobStores.aws.jobStore import AWSJobStore from toil.lib.aws.session import establish_boto3_session -from toil.lib.aws.s3 import create_s3_bucket +from toil.lib.aws.s3 import create_s3_bucket, delete_s3_bucket from toil.lib.aws.utils import get_bucket_region from toil.test import ToilTest, needs_aws_s3 @@ -66,5 +66,5 @@ def test_get_bucket_location_public_bucket(self) -> None: @classmethod def tearDownClass(cls) -> None: if cls.bucket: - AWSJobStore._delete_bucket(cls.bucket) + delete_s3_bucket(cls.bucket) super().tearDownClass() diff --git a/src/toil/test/server/serverTest.py b/src/toil/test/server/serverTest.py index 5a0f221c7f..f957441c2e 100644 --- a/src/toil/test/server/serverTest.py +++ b/src/toil/test/server/serverTest.py @@ -216,9 +216,9 @@ def setUpClass(cls) -> None: @classmethod def tearDownClass(cls) -> None: - from toil.lib.aws.utils import delete_s3_bucket + from toil.lib.aws.s3 import delete_s3_bucket if cls.bucket_name: - delete_s3_bucket(cls.s3_resource, cls.bucket_name, cls.region) + delete_s3_bucket(cls.s3_resource, cls.bucket_name) super().tearDownClass() From 2bb90920a7aad7190fa3acada9515b5dd7072d02 Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 1 May 2024 11:11:35 -0700 Subject: [PATCH 08/11] Missing import. --- src/toil/jobStores/aws/jobStore.py | 1 + src/toil/lib/aws/utils.py | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index 9cd0cef992..fa18709626 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -31,6 +31,7 @@ from mypy_boto3_sdb import SimpleDBClient from mypy_boto3_sdb.type_defs import ReplaceableItemTypeDef, ReplaceableAttributeTypeDef, SelectResultTypeDef, ItemTypeDef, AttributeTypeDef, DeletableItemTypeDef, UpdateConditionTypeDef +from toil.lib.aws.utils import flatten_tags import toil.lib.encryption as encryption from toil.fileStores import FileID from toil.job import Job, JobDescription diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index aa391c6bbe..81145bc4bd 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -382,9 +382,7 @@ def list_objects_for_url(url: ParseResult) -> List[str]: def flatten_tags(tags: Dict[str, str]) -> List[Dict[str, str]]: - """ - Convert tags from a key to value dict into a list of 'Key': xxx, 'Value': xxx dicts. - """ + """Convert tags from a key to value dict into a list of 'Key': xxx, 'Value': xxx dicts.""" return [{'Key': k, 'Value': v} for k, v in tags.items()] From cb9e47444050e3b56a0aba4d5c2d189e39292b7f Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Wed, 1 May 2024 22:02:02 -0700 Subject: [PATCH 09/11] Another missing import. --- src/toil/jobStores/aws/jobStore.py | 2 +- src/toil/lib/aws/s3.py | 2 +- src/toil/lib/aws/utils.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index fa18709626..4bc8325af8 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -31,7 +31,7 @@ from mypy_boto3_sdb import SimpleDBClient from mypy_boto3_sdb.type_defs import ReplaceableItemTypeDef, ReplaceableAttributeTypeDef, SelectResultTypeDef, ItemTypeDef, AttributeTypeDef, DeletableItemTypeDef, UpdateConditionTypeDef -from toil.lib.aws.utils import flatten_tags +from toil.lib.aws.utils import flatten_tags, enable_public_objects import toil.lib.encryption as encryption from toil.fileStores import FileID from toil.job import Job, JobDescription diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index cd93eb4bb2..10889c0139 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -23,7 +23,7 @@ from toil.lib.retry import retry, get_error_status from toil.lib.misc import printq -from toil.lib.aws import tags_from_env +from . import tags_from_env from toil.lib.aws.utils import enable_public_objects, flatten_tags if sys.version_info >= (3, 8): diff --git a/src/toil/lib/aws/utils.py b/src/toil/lib/aws/utils.py index 81145bc4bd..c5d65ba483 100644 --- a/src/toil/lib/aws/utils.py +++ b/src/toil/lib/aws/utils.py @@ -28,7 +28,7 @@ from urllib.parse import ParseResult from mypy_boto3_sdb.type_defs import AttributeTypeDef -from toil.lib.aws import session, AWSRegionName, AWSServerErrors +from . import session, AWSRegionName, AWSServerErrors from toil.lib.misc import printq from toil.lib.retry import (DEFAULT_DELAYS, DEFAULT_TIMEOUT, From 53dcd4263ffb9f88cc562cb71f32519438b27017 Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Thu, 6 Jun 2024 11:34:23 -0700 Subject: [PATCH 10/11] Remove cruft. --- src/toil/utils/toilLaunchCluster.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/toil/utils/toilLaunchCluster.py b/src/toil/utils/toilLaunchCluster.py index 6032e341a3..80d9cd6fbf 100644 --- a/src/toil/utils/toilLaunchCluster.py +++ b/src/toil/utils/toilLaunchCluster.py @@ -21,12 +21,6 @@ from toil.lib.aws import tags_from_env from toil.common import parser_with_common_options - -try: - from toil.lib.aws import build_tag_dict_from_env -except ModuleNotFoundError: - build_tag_dict_from_env: Dict[str, str] = lambda _: {} # type: ignore[no-redef] - from toil.lib.conversions import opt_strtobool from toil.provisioners import (check_valid_node_types, cluster_factory, From 21bbf5e77e1e103f7f503caca24f9c287b116d2d Mon Sep 17 00:00:00 2001 From: DailyDreaming Date: Thu, 27 Jun 2024 19:45:52 -0700 Subject: [PATCH 11/11] Patch test_utils.py. --- src/toil/test/lib/aws/test_utils.py | 43 ++++++++++++----------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/toil/test/lib/aws/test_utils.py b/src/toil/test/lib/aws/test_utils.py index 6370e14347..31ecaba54c 100644 --- a/src/toil/test/lib/aws/test_utils.py +++ b/src/toil/test/lib/aws/test_utils.py @@ -1,59 +1,50 @@ # Copyright (C) 2015-2021 Regents of the University of California # -# Licensed under the Apache License, Version 2.0 (the "License"); +# 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, +# 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 pytest +from unittest import mock + from toil.lib.aws import tags_from_env from toil.test import ToilTest logger = logging.getLogger(__name__) logging.basicConfig(level=logging.DEBUG) - class TagGenerationTest(ToilTest): """Test for tag generation from environment variables.""" + + @mock.patch.dict(os.environ, {'TOIL_OWNER_TAG': '😀', + 'TOIL_AWS_TAGS': '{}'}) def test_build_tag(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = "😀" - environment["TOIL_AWS_TAGS"] = None - tag_dict = tags_from_env(environment) + tag_dict = tags_from_env() assert tag_dict == {'Owner': '😀'} + @mock.patch.dict(os.environ, {'TOIL_AWS_TAGS': '{}'}) def test_empty_aws_tags(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "{}" - tag_dict = tags_from_env(environment) + tag_dict = tags_from_env() assert tag_dict == dict() - def test_incorrect_json_object(self): - with pytest.raises(SystemExit): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "231" - tags_from_env(environment) - + @mock.patch.dict(os.environ, {'TOIL_AWS_TAGS': '😀'}) def test_incorrect_json_emoji(self): with pytest.raises(SystemExit): - environment = dict() - environment["TOIL_OWNER_TAG"] = None - environment["TOIL_AWS_TAGS"] = "😀" - tags_from_env(environment) + tags_from_env() + @mock.patch.dict(os.environ, {'TOIL_OWNER_TAG': '😀', + 'TOIL_AWS_TAGS': '{"1": "2", " ":")"}'}) def test_build_tag_with_tags(self): - environment = dict() - environment["TOIL_OWNER_TAG"] = "😀" - environment["TOIL_AWS_TAGS"] = '{"1": "2", " ":")"}' - tag_dict = tags_from_env(environment) + tag_dict = tags_from_env() assert tag_dict == {'Owner': '😀', '1': '2', ' ': ')'}