From 126373508854ed86f0f3e7366c5bbe5e3ef5f590 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 21 Jun 2021 16:27:30 -0400 Subject: [PATCH 01/37] Added methods for cf and s3 files and init UI --- samcli/cli/cli_config_file.py | 13 ++- samcli/cli/command.py | 1 + samcli/commands/delete/__init__.py | 6 ++ samcli/commands/delete/command.py | 88 +++++++++++++++++ samcli/commands/delete/delete_context.py | 117 +++++++++++++++++++++++ samcli/commands/delete/exceptions.py | 14 +++ samcli/lib/delete/__init__.py | 0 samcli/lib/delete/cf_utils.py | 105 ++++++++++++++++++++ samcli/lib/delete/utils.py | 16 ++++ samcli/lib/package/s3_uploader.py | 23 +++++ 10 files changed, 379 insertions(+), 4 deletions(-) create mode 100644 samcli/commands/delete/__init__.py create mode 100644 samcli/commands/delete/command.py create mode 100644 samcli/commands/delete/delete_context.py create mode 100644 samcli/commands/delete/exceptions.py create mode 100644 samcli/lib/delete/__init__.py create mode 100644 samcli/lib/delete/cf_utils.py create mode 100644 samcli/lib/delete/utils.py diff --git a/samcli/cli/cli_config_file.py b/samcli/cli/cli_config_file.py index 67e214e122c..9e2b4aa0209 100644 --- a/samcli/cli/cli_config_file.py +++ b/samcli/cli/cli_config_file.py @@ -27,12 +27,14 @@ class TomlProvider: A parser for toml configuration files """ - def __init__(self, section=None): + def __init__(self, section=None, cmd_names=None): """ The constructor for TomlProvider class :param section: section defined in the configuration file nested within `cmd` + :param cmd_names: cmd_name defined in the configuration file """ self.section = section + self.cmd_names = cmd_names def __call__(self, config_path, config_env, cmd_names): """ @@ -67,18 +69,21 @@ def __call__(self, config_path, config_env, cmd_names): LOG.debug("Config file '%s' does not exist", samconfig.path()) return resolved_config + if not self.cmd_names: + self.cmd_names = cmd_names + try: LOG.debug( "Loading configuration values from [%s.%s.%s] (env.command_name.section) in config file at '%s'...", config_env, - cmd_names, + self.cmd_names, self.section, samconfig.path(), ) # NOTE(TheSriram): change from tomlkit table type to normal dictionary, # so that click defaults work out of the box. - resolved_config = dict(samconfig.get_all(cmd_names, self.section, env=config_env).items()) + resolved_config = dict(samconfig.get_all(self.cmd_names, self.section, env=config_env).items()) LOG.debug("Configuration values successfully loaded.") LOG.debug("Configuration values are: %s", resolved_config) @@ -87,7 +92,7 @@ def __call__(self, config_path, config_env, cmd_names): "Error reading configuration from [%s.%s.%s] (env.command_name.section) " "in configuration file at '%s' with : %s", config_env, - cmd_names, + self.cmd_names, self.section, samconfig.path(), str(ex), diff --git a/samcli/cli/command.py b/samcli/cli/command.py index 384529f78ba..c329345f14e 100644 --- a/samcli/cli/command.py +++ b/samcli/cli/command.py @@ -19,6 +19,7 @@ "samcli.commands.local.local", "samcli.commands.package", "samcli.commands.deploy", + "samcli.commands.delete", "samcli.commands.logs", "samcli.commands.publish", # We intentionally do not expose the `bootstrap` command for now. We might open it up later diff --git a/samcli/commands/delete/__init__.py b/samcli/commands/delete/__init__.py new file mode 100644 index 00000000000..ea5b0202d29 --- /dev/null +++ b/samcli/commands/delete/__init__.py @@ -0,0 +1,6 @@ +""" +`sam delete` command +""" + +# Expose the cli object here +from .command import cli # noqa diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py new file mode 100644 index 00000000000..bdc201aef4a --- /dev/null +++ b/samcli/commands/delete/command.py @@ -0,0 +1,88 @@ +# """ +# CLI command for "delete" command +# """ + +import logging + +import click +from samcli.cli.cli_config_file import TomlProvider, configuration_option +from samcli.cli.main import aws_creds_options, common_options, pass_context, print_cmdline_args + +from samcli.lib.utils.version_checker import check_newer_version + +SHORT_HELP = "Delete an AWS SAM application." + +HELP_TEXT = """The sam delete command deletes a Cloudformation Stack and deletes all your resources which were created. + +\b +e.g. sam delete --stack-name sam-app --region us-east-1 + +\b +""" + +CONFIG_SECTION = "parameters" +CONFIG_COMMAND = "deploy" +LOG = logging.getLogger(__name__) + + +@click.command( + "delete", + short_help=SHORT_HELP, + context_settings={"ignore_unknown_options": False, "allow_interspersed_args": True, "allow_extra_args": True}, + help=HELP_TEXT, +) +@configuration_option(provider=TomlProvider(section=CONFIG_SECTION, cmd_names=[CONFIG_COMMAND])) +@click.option( + "--stack-name", + required=False, + help="The name of the AWS CloudFormation stack you want to delete. ", +) +@click.option( + "--s3-bucket", + required=False, + help="The name of the S3 bucket where this command delets your " "CloudFormation artifacts.", +) +@click.option( + "--s3-prefix", + required=False, + help="A prefix name that the command uses to delete the " + "artifacts' that were deployed to the S3 bucket. " + "The prefix name is a path name (folder name) for the S3 bucket.", +) +@aws_creds_options +@common_options +@pass_context +@check_newer_version +@print_cmdline_args +def cli( + ctx, + stack_name, + s3_bucket, + s3_prefix, + config_file, + config_env, +): + """ + `sam delete` command entry point + """ + + # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing + do_cli(stack_name, ctx.region, ctx.profile, s3_bucket, s3_prefix) # pragma: no cover + + +def do_cli( + stack_name, + region, + profile, + s3_bucket, + s3_prefix +): + """ + Implementation of the ``cli`` method + """ + from samcli.commands.delete.delete_context import DeleteContext + + with DeleteContext( + stack_name=stack_name, region=region, s3_bucket=s3_bucket, s3_prefix=s3_prefix, profile=profile + ) as delete_context: + delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py new file mode 100644 index 00000000000..d34d096c0ca --- /dev/null +++ b/samcli/commands/delete/delete_context.py @@ -0,0 +1,117 @@ +import boto3 + +import click +from click import confirm +from click import prompt + +from samcli.lib.utils.botoconfig import get_boto_config_with_user_agent +from samcli.lib.delete.cf_utils import CfUtils +from samcli.lib.delete.utils import get_cf_template_name +from samcli.lib.package.s3_uploader import S3Uploader +from samcli.yamlhelper import yaml_parse +# from samcli.lib.package.artifact_exporter import Template +# from samcli.lib.package.ecr_uploader import ECRUploader +# from samcli.lib.package.uploaders import Uploaders +import docker + +class DeleteContext: + def __init__(self, stack_name, region, s3_bucket, s3_prefix, profile): + self.stack_name = stack_name + self.region = region + self.profile = profile + self.s3_bucket = s3_bucket + self.s3_prefix = s3_prefix + self.cf_utils = None + self.start_bold = "\033[1m" + self.end_bold = "\033[0m" + self.s3_uploader = None + # self.uploaders = None + self.cf_template_file_name = None + self.delete_artifacts_folder = None + self.delete_cf_template_file = None + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + def run(self): + # print("Stack Name:", self.stack_name) + # print(self.s3_bucket) + # print(self.s3_prefix) + if not self.stack_name: + self.stack_name = prompt( + f"\t{self.start_bold}Enter stack name you want to delete{self.end_bold}", type=click.STRING + ) + + delete_stack = confirm( + f"\t{self.start_bold}Are you sure you want to delete the stack {self.stack_name}?{self.end_bold}", + default=False, + ) + # Fetch the template using the stack-name + if delete_stack: + boto_config = get_boto_config_with_user_agent() + + # Define cf_client based on the region as different regions can have same stack-names + cloudformation_client = boto3.client( + "cloudformation", region_name=self.region if self.region else None, config=boto_config + ) + + s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) + ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + + self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) + + # docker_client = docker.from_env() + # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + + self.cf_utils = CfUtils(cloudformation_client) + + is_deployed = self.cf_utils.has_stack(self.stack_name) + + if is_deployed: + template_str = self.cf_utils.get_stack_template(self.stack_name, "Original") + + template_dict = yaml_parse(template_str) + + if self.s3_bucket and self.s3_prefix: + self.delete_artifacts_folder = confirm( + f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + default=False, + ) + if not self.delete_artifacts_folder: + self.cf_template_file_name = get_cf_template_name(template_str, "template") + delete_cf_template_file = confirm( + f"\t{self.start_bold}Do you want to delete the template file {self.cf_template_file_name} in S3?{self.end_bold}", + default=False, + ) + + click.echo("\n") + # Delete the primary stack + self.cf_utils.delete_stack(self.stack_name) + + click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) + + # Delete the artifacts + # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) + # template = Template(None, None, self.uploaders, None) + # template.delete(template_dict) + + # Delete the template file using template_str + if self.delete_cf_template_file: + self.s3_uploader.delete_artifact(cf_template_file_name) + + # Delete the folder of artifacts if s3_bucket and s3_prefix provided + elif self.delete_artifacts_folder: + prefix_files = s3_client.list_objects_v2(Bucket=self.s3_bucket, Prefix=self.s3_prefix) + self.s3_uploader.delete_artifact(None, prefix_files) + + # Delete the ECR companion stack + + if self.cf_template_file_name: + click.echo("- deleting template file {0}".format(cf_template_file)) + click.echo("\n") + click.echo("delete complete") + else: + click.echo("Error: The input stack {0} does not exist on Cloudformation".format(self.stack_name)) diff --git a/samcli/commands/delete/exceptions.py b/samcli/commands/delete/exceptions.py new file mode 100644 index 00000000000..82c56b6bb67 --- /dev/null +++ b/samcli/commands/delete/exceptions.py @@ -0,0 +1,14 @@ +""" +Exceptions that are raised by sam delete +""" +from samcli.commands.exceptions import UserException + + +class DeleteFailedError(UserException): + def __init__(self, stack_name, msg): + self.stack_name = stack_name + self.msg = msg + + message_fmt = "Failed to delete the stack: {stack_name}, {msg}" + + super().__init__(message=message_fmt.format(stack_name=self.stack_name, msg=msg)) diff --git a/samcli/lib/delete/__init__.py b/samcli/lib/delete/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py new file mode 100644 index 00000000000..b8bccdc651e --- /dev/null +++ b/samcli/lib/delete/cf_utils.py @@ -0,0 +1,105 @@ +""" +Delete Cloudformation stacks and s3 files +""" + +import botocore +import logging + +from samcli.commands.delete.exceptions import DeleteFailedError + +LOG = logging.getLogger(__name__) + + +class CfUtils: + def __init__(self, cloudformation_client): + self._client = cloudformation_client + + def has_stack(self, stack_name): + """ + Checks if a CloudFormation stack with given name exists + + :param stack_name: Name or ID of the stack + :return: True if stack exists. False otherwise + """ + try: + resp = self._client.describe_stacks(StackName=stack_name) + if not resp["Stacks"]: + return False + + stack = resp["Stacks"][0] + return stack["StackStatus"] != "REVIEW_IN_PROGRESS" + + except botocore.exceptions.ClientError as e: + # If a stack does not exist, describe_stacks will throw an + # exception. Unfortunately we don't have a better way than parsing + # the exception msg to understand the nature of this exception. + + if "Stack with id {0} does not exist".format(stack_name) in str(e): + LOG.debug("Stack with id %s does not exist", stack_name) + return False + except botocore.exceptions.BotoCoreError as e: + # If there are credentials, environment errors, + # catch that and throw a deploy failed error. + + LOG.debug("Botocore Exception : %s", str(e)) + raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e + + except Exception as e: + # We don't know anything about this exception. Don't handle + LOG.debug("Unable to get stack details.", exc_info=e) + raise e + + def get_stack_template(self, stack_name, stage): + try: + resp = self._client.get_template(StackName=stack_name, TemplateStage=stage) + if not resp["TemplateBody"]: + return "" + + return resp["TemplateBody"] + + except botocore.exceptions.ClientError as e: + # If a stack does not exist, get_stack_template will throw an + # exception. Unfortunately we don't have a better way than parsing + # the exception msg to understand the nature of this exception. + + if "Stack with id {0} does not exist".format(stack_name) in str(e): + LOG.debug("Stack with id %s does not exist", stack_name) + return "" + except botocore.exceptions.BotoCoreError as e: + # If there are credentials, environment errors, + # catch that and throw a deploy failed error. + + LOG.debug("Botocore Exception : %s", str(e)) + raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e + + except Exception as e: + # We don't know anything about this exception. Don't handle + LOG.debug("Unable to get stack details.", exc_info=e) + raise e + + def delete_stack(self, stack_name): + try: + resp = self._client.delete_stack(StackName=stack_name) + + return resp + + except botocore.exceptions.ClientError as e: + # If a stack does not exist, describe_stacks will throw an + # exception. Unfortunately we don't have a better way than parsing + # the exception msg to understand the nature of this exception. + + if "Stack with id {0} does not exist".format(stack_name) in str(e): + LOG.debug("Stack with id %s does not exist", stack_name) + return False + except botocore.exceptions.BotoCoreError as e: + # If there are credentials, environment errors, + # catch that and throw a deploy failed error. + + LOG.debug("Botocore Exception : %s", str(e)) + raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e + + except Exception as e: + # We don't know anything about this exception. Don't handle + LOG.debug("Unable to get stack details.", exc_info=e) + raise e + diff --git a/samcli/lib/delete/utils.py b/samcli/lib/delete/utils.py new file mode 100644 index 00000000000..280d24e462b --- /dev/null +++ b/samcli/lib/delete/utils.py @@ -0,0 +1,16 @@ +""" +Utilities for Delete +""" + +from samcli.lib.utils.hash import file_checksum +from samcli.lib.package.artifact_exporter import mktempfile + +def get_cf_template_name(self, template_str, extension): + with mktempfile() as temp_file: + temp_file.write(template_str) + temp_file.flush() + + filemd5 = file_checksum(temp_file.name) + remote_path = filemd5 + "." + extension + + return remote_path \ No newline at end of file diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 4a64a983d07..08bbf4db233 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -144,6 +144,29 @@ def upload_with_dedup( return self.upload(file_name, remote_path) + def delete_artifact(self, file_name: str, prefix_files=None): + + try: + if not self.bucket_name: + raise BucketNotSpecifiedError() + + remote_path = file_name + if self.prefix: + if remote_path: + remote_path = "{0}/{1}".format(self.prefix, file_name) + print("- deleting", remote_path) + self.s3.delete_object(Bucket=self.bucket_name, Key=remote_path) + elif prefix_files: + for obj in prefix_files["Contents"]: + print("- deleting", obj["Key"]) + self.s3.delete_object(Bucket=self.bucket_name, Key=obj["Key"]) + + except botocore.exceptions.ClientError as ex: + error_code = ex.response["Error"]["Code"] + if error_code == "NoSuchBucket": + raise NoSuchBucketError(bucket_name=self.bucket_name) from ex + raise ex + def file_exists(self, remote_path: str) -> bool: """ Check if the file we are trying to upload already exists in S3 From ba47369e90cf966e95f799e4153571e4321e449f Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 23 Jun 2021 13:36:39 -0400 Subject: [PATCH 02/37] Added unit tests for utils methods and s3_uploader --- samcli/commands/delete/command.py | 15 ++--- samcli/commands/delete/delete_context.py | 46 ++++++++------ samcli/lib/delete/cf_utils.py | 56 ++++++++--------- samcli/lib/delete/utils.py | 4 +- samcli/lib/package/s3_uploader.py | 38 ++++++++---- tests/unit/lib/delete/__init__.py | 0 tests/unit/lib/delete/test_cf_utils.py | 71 ++++++++++++++++++++++ tests/unit/lib/package/test_s3_uploader.py | 45 ++++++++++++++ 8 files changed, 204 insertions(+), 71 deletions(-) create mode 100644 tests/unit/lib/delete/__init__.py create mode 100644 tests/unit/lib/delete/test_cf_utils.py diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index bdc201aef4a..13483b40e18 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -1,6 +1,6 @@ -# """ -# CLI command for "delete" command -# """ +""" +CLI command for "delete" command +""" import logging @@ -70,18 +70,13 @@ def cli( do_cli(stack_name, ctx.region, ctx.profile, s3_bucket, s3_prefix) # pragma: no cover -def do_cli( - stack_name, - region, - profile, - s3_bucket, - s3_prefix -): +def do_cli(stack_name, region, profile, s3_bucket, s3_prefix): """ Implementation of the ``cli`` method """ from samcli.commands.delete.delete_context import DeleteContext + # ctx = click.get_current_context() #This is here if s3_bucket and s3_prefix options are not used with DeleteContext( stack_name=stack_name, region=region, s3_bucket=s3_bucket, s3_prefix=s3_prefix, profile=profile ) as delete_context: diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index d34d096c0ca..7e4999442fb 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -1,5 +1,9 @@ -import boto3 +""" +Delete a SAM stack +""" +import boto3 +import docker import click from click import confirm from click import prompt @@ -9,10 +13,12 @@ from samcli.lib.delete.utils import get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader from samcli.yamlhelper import yaml_parse + +# Intentionally commented # from samcli.lib.package.artifact_exporter import Template # from samcli.lib.package.ecr_uploader import ECRUploader # from samcli.lib.package.uploaders import Uploaders -import docker + class DeleteContext: def __init__(self, stack_name, region, s3_bucket, s3_prefix, profile): @@ -37,20 +43,24 @@ def __exit__(self, *args): pass def run(self): - # print("Stack Name:", self.stack_name) - # print(self.s3_bucket) - # print(self.s3_prefix) + """ + Delete the stack based on the argument provided by customers and samconfig.toml. + """ if not self.stack_name: self.stack_name = prompt( f"\t{self.start_bold}Enter stack name you want to delete{self.end_bold}", type=click.STRING ) + if not self.region: + self.region = prompt( + f"\t{self.start_bold}Enter region you want to delete from{self.end_bold}", type=click.STRING + ) delete_stack = confirm( f"\t{self.start_bold}Are you sure you want to delete the stack {self.stack_name}?{self.end_bold}", default=False, ) # Fetch the template using the stack-name - if delete_stack: + if delete_stack and self.region: boto_config = get_boto_config_with_user_agent() # Define cf_client based on the region as different regions can have same stack-names @@ -63,8 +73,8 @@ def run(self): self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - # docker_client = docker.from_env() - # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + docker_client = docker.from_env() + ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) self.cf_utils = CfUtils(cloudformation_client) @@ -77,14 +87,16 @@ def run(self): if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", - default=False, + f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} \ + in S3 which contains the artifacts?{self.end_bold}", + default=False, ) if not self.delete_artifacts_folder: self.cf_template_file_name = get_cf_template_name(template_str, "template") delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file {self.cf_template_file_name} in S3?{self.end_bold}", - default=False, + f"\t{self.start_bold}Do you want to delete the template file \ + {self.cf_template_file_name} in S3?{self.end_bold}", + default=False, ) click.echo("\n") @@ -94,23 +106,23 @@ def run(self): click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) # Delete the artifacts + # Intentionally commented # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) # template = Template(None, None, self.uploaders, None) # template.delete(template_dict) - # Delete the template file using template_str + # Delete the CF template file in S3 if self.delete_cf_template_file: - self.s3_uploader.delete_artifact(cf_template_file_name) + self.s3_uploader.delete_artifact(self.cf_template_file_name) # Delete the folder of artifacts if s3_bucket and s3_prefix provided elif self.delete_artifacts_folder: - prefix_files = s3_client.list_objects_v2(Bucket=self.s3_bucket, Prefix=self.s3_prefix) - self.s3_uploader.delete_artifact(None, prefix_files) + self.s3_uploader.delete_prefix_artifacts() # Delete the ECR companion stack if self.cf_template_file_name: - click.echo("- deleting template file {0}".format(cf_template_file)) + click.echo("- deleting template file {0}".format(self.cf_template_file)) click.echo("\n") click.echo("delete complete") else: diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index b8bccdc651e..945aa5a09a2 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -1,10 +1,10 @@ """ -Delete Cloudformation stacks and s3 files +Delete Cloudformation stacks and s3 files """ -import botocore import logging +from botocore.exceptions import ClientError, BotoCoreError from samcli.commands.delete.exceptions import DeleteFailedError LOG = logging.getLogger(__name__) @@ -29,7 +29,7 @@ def has_stack(self, stack_name): stack = resp["Stacks"][0] return stack["StackStatus"] != "REVIEW_IN_PROGRESS" - except botocore.exceptions.ClientError as e: + except ClientError as e: # If a stack does not exist, describe_stacks will throw an # exception. Unfortunately we don't have a better way than parsing # the exception msg to understand the nature of this exception. @@ -37,9 +37,10 @@ def has_stack(self, stack_name): if "Stack with id {0} does not exist".format(stack_name) in str(e): LOG.debug("Stack with id %s does not exist", stack_name) return False - except botocore.exceptions.BotoCoreError as e: + raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e + except BotoCoreError as e: # If there are credentials, environment errors, - # catch that and throw a deploy failed error. + # catch that and throw a delete failed error. LOG.debug("Botocore Exception : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e @@ -50,26 +51,25 @@ def has_stack(self, stack_name): raise e def get_stack_template(self, stack_name, stage): + """ + Return the Cloudformation template of the given stack_name + + :param stack_name: Name or ID of the stack + :param stage: The Stage of the template Original or Processed + :return: Template body of the stack + """ try: resp = self._client.get_template(StackName=stack_name, TemplateStage=stage) if not resp["TemplateBody"]: - return "" + return None return resp["TemplateBody"] - except botocore.exceptions.ClientError as e: - # If a stack does not exist, get_stack_template will throw an - # exception. Unfortunately we don't have a better way than parsing - # the exception msg to understand the nature of this exception. - - if "Stack with id {0} does not exist".format(stack_name) in str(e): - LOG.debug("Stack with id %s does not exist", stack_name) - return "" - except botocore.exceptions.BotoCoreError as e: + except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, - # catch that and throw a deploy failed error. + # catch that and throw a delete failed error. - LOG.debug("Botocore Exception : %s", str(e)) + LOG.debug("Failed to delete stack : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: @@ -78,28 +78,24 @@ def get_stack_template(self, stack_name, stage): raise e def delete_stack(self, stack_name): + """ + Delete the Cloudformation stack with the given stack_name + + :param stack_name: Name or ID of the stack + :return: Status of deletion + """ try: resp = self._client.delete_stack(StackName=stack_name) - return resp - except botocore.exceptions.ClientError as e: - # If a stack does not exist, describe_stacks will throw an - # exception. Unfortunately we don't have a better way than parsing - # the exception msg to understand the nature of this exception. - - if "Stack with id {0} does not exist".format(stack_name) in str(e): - LOG.debug("Stack with id %s does not exist", stack_name) - return False - except botocore.exceptions.BotoCoreError as e: + except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, - # catch that and throw a deploy failed error. + # catch that and throw a delete failed error. - LOG.debug("Botocore Exception : %s", str(e)) + LOG.debug("Failed to delete stack : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: # We don't know anything about this exception. Don't handle LOG.debug("Unable to get stack details.", exc_info=e) raise e - diff --git a/samcli/lib/delete/utils.py b/samcli/lib/delete/utils.py index 280d24e462b..f6e6edeb4d9 100644 --- a/samcli/lib/delete/utils.py +++ b/samcli/lib/delete/utils.py @@ -5,7 +5,7 @@ from samcli.lib.utils.hash import file_checksum from samcli.lib.package.artifact_exporter import mktempfile -def get_cf_template_name(self, template_str, extension): +def get_cf_template_name(template_str, extension): with mktempfile() as temp_file: temp_file.write(template_str) temp_file.flush() @@ -13,4 +13,4 @@ def get_cf_template_name(self, template_str, extension): filemd5 = file_checksum(temp_file.name) remote_path = filemd5 + "." + extension - return remote_path \ No newline at end of file + return remote_path diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 08bbf4db233..3e445aec257 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -144,22 +144,24 @@ def upload_with_dedup( return self.upload(file_name, remote_path) - def delete_artifact(self, file_name: str, prefix_files=None): - + def delete_artifact(self, remote_path: str, is_key=False): + """ + Deletes a given file from S3 + :param remote_path: Path to the file that will be deleted + :param is_key: If the given remote_path is the key or a file_name + """ try: if not self.bucket_name: raise BucketNotSpecifiedError() - remote_path = file_name - if self.prefix: - if remote_path: - remote_path = "{0}/{1}".format(self.prefix, file_name) - print("- deleting", remote_path) - self.s3.delete_object(Bucket=self.bucket_name, Key=remote_path) - elif prefix_files: - for obj in prefix_files["Contents"]: - print("- deleting", obj["Key"]) - self.s3.delete_object(Bucket=self.bucket_name, Key=obj["Key"]) + key = remote_path + if self.prefix and not is_key: + key = "{0}/{1}".format(self.prefix, remote_path) + + # Deleting Specific file with key + print("- deleting", key) + resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) + return resp["ResponseMetadata"] except botocore.exceptions.ClientError as ex: error_code = ex.response["Error"]["Code"] @@ -167,6 +169,18 @@ def delete_artifact(self, file_name: str, prefix_files=None): raise NoSuchBucketError(bucket_name=self.bucket_name) from ex raise ex + def delete_prefix_artifacts(self): + """ + Deletes all the files from the prefix in S3 + """ + if not self.bucket_name: + raise BucketNotSpecifiedError() + if self.prefix: + prefix_files = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix) + + for obj in prefix_files["Contents"]: + self.delete_artifact(obj["Key"], True) + def file_exists(self, remote_path: str) -> bool: """ Check if the file we are trying to upload already exists in S3 diff --git a/tests/unit/lib/delete/__init__.py b/tests/unit/lib/delete/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py new file mode 100644 index 00000000000..37403a156f9 --- /dev/null +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -0,0 +1,71 @@ +from unittest.mock import patch, MagicMock, ANY, call +from unittest import TestCase + +from samcli.commands.delete.exceptions import DeleteFailedError +from botocore.exceptions import ClientError, BotoCoreError +from samcli.lib.delete.cf_utils import CfUtils + + +class TestCfUtils(TestCase): + def setUp(self): + self.session = MagicMock() + self.cloudformation_client = self.session.client("cloudformation") + self.s3_client = self.session.client("s3") + self.cf_utils = CfUtils(self.cloudformation_client) + + def test_cf_utils_init(self): + self.assertEqual(self.cf_utils._client, self.cloudformation_client) + + def test_cf_utils_has_no_stack(self): + self.cf_utils._client.describe_stacks = MagicMock(return_value={"Stacks": []}) + self.assertEqual(self.cf_utils.has_stack("test"), False) + + def test_cf_utils_has_stack_exception_non_exsistent(self): + self.cf_utils._client.describe_stacks = MagicMock( + side_effect=ClientError( + error_response={"Error": {"Message": "Stack with id test does not exist"}}, + operation_name="stack_status", + ) + ) + self.assertEqual(self.cf_utils.has_stack("test"), False) + + def test_cf_utils_has_stack_exception(self): + self.cf_utils._client.describe_stacks = MagicMock(side_effect=Exception()) + with self.assertRaises(Exception): + self.cf_utils.has_stack("test") + + def test_cf_utils_has_stack_in_review(self): + self.cf_utils._client.describe_stacks = MagicMock( + return_value={"Stacks": [{"StackStatus": "REVIEW_IN_PROGRESS"}]} + ) + self.assertEqual(self.cf_utils.has_stack("test"), False) + + def test_cf_utils_has_stack_exception_botocore(self): + self.cf_utils._client.describe_stacks = MagicMock(side_effect=BotoCoreError()) + with self.assertRaises(DeleteFailedError): + self.cf_utils.has_stack("test") + + def test_cf_utils_get_stack_template_exception_botocore(self): + self.cf_utils._client.get_template = MagicMock(side_effect=BotoCoreError()) + with self.assertRaises(DeleteFailedError): + self.cf_utils.get_stack_template("test", "Original") + + def test_cf_utils_get_stack_template_exception_botocore(self): + self.cf_utils._client.get_template = MagicMock(side_effect=BotoCoreError()) + with self.assertRaises(DeleteFailedError): + self.cf_utils.get_stack_template("test", "Original") + + def test_cf_utils_get_stack_template_exception(self): + self.cf_utils._client.get_template = MagicMock(side_effect=Exception()) + with self.assertRaises(Exception): + self.cf_utils.get_stack_template("test", "Original") + + def test_cf_utils_delete_stack_exception_botocore(self): + self.cf_utils._client.delete_stack = MagicMock(side_effect=BotoCoreError()) + with self.assertRaises(DeleteFailedError): + self.cf_utils.delete_stack("test") + + def test_cf_utils_delete_stack_exception(self): + self.cf_utils._client.delete_stack = MagicMock(side_effect=Exception()) + with self.assertRaises(Exception): + self.cf_utils.delete_stack("test") diff --git a/tests/unit/lib/package/test_s3_uploader.py b/tests/unit/lib/package/test_s3_uploader.py index c40c4c6cf43..07fed24211a 100644 --- a/tests/unit/lib/package/test_s3_uploader.py +++ b/tests/unit/lib/package/test_s3_uploader.py @@ -172,6 +172,51 @@ def test_s3_upload_no_bucket(self): s3_uploader.upload(f.name, remote_path) self.assertEqual(BucketNotSpecifiedError().message, str(ex)) + def test_s3_delete_artifact(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=None, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=self.force_upload, + no_progressbar=self.no_progressbar, + ) + s3_uploader.artifact_metadata = {"a": "b"} + with self.assertRaises(BucketNotSpecifiedError) as ex: + with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: + self.assertEqual(s3_uploader.delete_artifact(f.name), {"a": "b"}) + + def test_s3_delete_artifact_no_bucket(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=None, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=self.force_upload, + no_progressbar=self.no_progressbar, + ) + with self.assertRaises(BucketNotSpecifiedError) as ex: + with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: + s3_uploader.delete_artifact(f.name) + self.assertEqual(BucketNotSpecifiedError().message, str(ex)) + + def test_s3_upload_bucket_not_found(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=self.bucket_name, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=True, + no_progressbar=self.no_progressbar, + ) + + s3_uploader.s3.delete_object = MagicMock( + side_effect=ClientError(error_response={"Error": {"Code": "NoSuchBucket"}}, operation_name="create_object") + ) + with tempfile.NamedTemporaryFile() as f: + with self.assertRaises(NoSuchBucketError): + s3_uploader.delete_artifact(f.name) + def test_s3_upload_with_dedup(self): s3_uploader = S3Uploader( s3_client=self.s3, From d77f7c4831eefca0e4a975a159157205a903e4f7 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 24 Jun 2021 13:16:23 -0400 Subject: [PATCH 03/37] Removed s3_bucket and s3_prefix click options --- samcli/commands/delete/command.py | 23 +++++------------------ samcli/commands/delete/delete_context.py | 12 +++++------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 13483b40e18..7227be86e30 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -37,18 +37,6 @@ required=False, help="The name of the AWS CloudFormation stack you want to delete. ", ) -@click.option( - "--s3-bucket", - required=False, - help="The name of the S3 bucket where this command delets your " "CloudFormation artifacts.", -) -@click.option( - "--s3-prefix", - required=False, - help="A prefix name that the command uses to delete the " - "artifacts' that were deployed to the S3 bucket. " - "The prefix name is a path name (folder name) for the S3 bucket.", -) @aws_creds_options @common_options @pass_context @@ -57,8 +45,6 @@ def cli( ctx, stack_name, - s3_bucket, - s3_prefix, config_file, config_env, ): @@ -67,17 +53,18 @@ def cli( """ # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing - do_cli(stack_name, ctx.region, ctx.profile, s3_bucket, s3_prefix) # pragma: no cover + do_cli(stack_name, ctx.region, ctx.profile) # pragma: no cover -def do_cli(stack_name, region, profile, s3_bucket, s3_prefix): +def do_cli(stack_name, region, profile): """ Implementation of the ``cli`` method """ from samcli.commands.delete.delete_context import DeleteContext - # ctx = click.get_current_context() #This is here if s3_bucket and s3_prefix options are not used + ctx = click.get_current_context() + with DeleteContext( - stack_name=stack_name, region=region, s3_bucket=s3_bucket, s3_prefix=s3_prefix, profile=profile + stack_name=stack_name, region=region, s3_bucket=ctx.default_map.get("s3_bucket", None), s3_prefix=ctx.default_map.get("s3_prefix", None), profile=profile ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 7e4999442fb..919f1e4f994 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -74,7 +74,7 @@ def run(self): self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) docker_client = docker.from_env() - ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) self.cf_utils = CfUtils(cloudformation_client) @@ -87,15 +87,13 @@ def run(self): if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} \ - in S3 which contains the artifacts?{self.end_bold}", + f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", default=False, ) if not self.delete_artifacts_folder: self.cf_template_file_name = get_cf_template_name(template_str, "template") - delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file \ - {self.cf_template_file_name} in S3?{self.end_bold}", + self.delete_cf_template_file = confirm( + f"\t{self.start_bold}Do you want to delete the template file {self.cf_template_file_name} in S3?{self.end_bold}", default=False, ) @@ -122,7 +120,7 @@ def run(self): # Delete the ECR companion stack if self.cf_template_file_name: - click.echo("- deleting template file {0}".format(self.cf_template_file)) + click.echo(f"- deleting template file {self.cf_template_file_name}") click.echo("\n") click.echo("delete complete") else: From d50664880eb387343e40e25acf0f4eda3487576d Mon Sep 17 00:00:00 2001 From: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Date: Thu, 24 Jun 2021 13:34:28 -0700 Subject: [PATCH 04/37] chore: Increase awareness of same file warning during package (#2946) * chore: increase awareness of same file warning during package * fix formatting & grammar Co-authored-by: Mathieu Grandis <73313235+mgrandis@users.noreply.github.com> --- samcli/lib/package/s3_uploader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 4a64a983d07..34ac666b86c 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -85,7 +85,7 @@ def upload(self, file_name: str, remote_path: str) -> str: # Check if a file with same data exists if not self.force_upload and self.file_exists(remote_path): - LOG.debug("File with same data is already exists at %s. " "Skipping upload", remote_path) + LOG.info("File with same data already exists at %s, skipping upload", remote_path) return self.make_url(remote_path) try: From 698de67035967eff345a72fb3859bf7a06378c6b Mon Sep 17 00:00:00 2001 From: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com> Date: Thu, 24 Jun 2021 16:07:13 -0700 Subject: [PATCH 05/37] fix: Allow the base64Encoded field in REST Api, skip validation of unknown fields and validate missing statusCode for Http Api (#2941) * fix API Gateway emulator: - skip validating the non allowed fields for Http Api Gateway, as it always skip the unknown fields - add base64Encoded as an allowed field for Rest Api gateway - base64 decoding will be always done for Http API gateway if the lambda response isBase64Encoded is true regardless the content-type - validate if statusCode is missing in case of Http API, and payload version 1.0 * - accept "true", "True", "false", "False" as valid isBase64Encoded values. - Validate on other isBase64Encoded Values - add more integration && unit test cases * fix lint && black issues * use smaller image to test Base64 response --- samcli/local/apigw/local_apigw_service.py | 71 ++- .../local/start_api/test_start_api.py | 49 +- .../testdata/start_api/binarydata.gif | Bin 1951 -> 49 bytes .../start_api/image_package_type/main.py | 2 +- tests/integration/testdata/start_api/main.py | 36 +- .../testdata/start_api/swagger-template.yaml | 48 ++ .../local/apigw/test_local_apigw_service.py | 441 ++++++++++++++++-- 7 files changed, 577 insertions(+), 70 deletions(-) diff --git a/samcli/local/apigw/local_apigw_service.py b/samcli/local/apigw/local_apigw_service.py index cc2684c2005..5a6d397d543 100644 --- a/samcli/local/apigw/local_apigw_service.py +++ b/samcli/local/apigw/local_apigw_service.py @@ -333,7 +333,7 @@ def _request_handler(self, **kwargs): ) else: (status_code, headers, body) = self._parse_v1_payload_format_lambda_output( - lambda_response, self.api.binary_media_types, request + lambda_response, self.api.binary_media_types, request, route.event_type ) except LambdaResponseParseException as ex: LOG.error("Invalid lambda response received: %s", ex) @@ -379,13 +379,14 @@ def get_request_methods_endpoints(flask_request): # Consider moving this out to its own class. Logic is started to get dense and looks messy @jfuss @staticmethod - def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, flask_request): + def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, flask_request, event_type): """ Parses the output from the Lambda Container :param str lambda_output: Output from Lambda Invoke :param binary_types: list of binary types :param flask_request: flash request object + :param event_type: determines the route event type :return: Tuple(int, dict, str, bool) """ # pylint: disable-msg=too-many-statements @@ -397,6 +398,9 @@ def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, fla if not isinstance(json_output, dict): raise LambdaResponseParseException(f"Lambda returned {type(json_output)} instead of dict") + if event_type == Route.HTTP and json_output.get("statusCode") is None: + raise LambdaResponseParseException(f"Invalid API Gateway Response Key: statusCode is not in {json_output}") + status_code = json_output.get("statusCode") or 200 headers = LocalApigwService._merge_response_headers( json_output.get("headers") or {}, json_output.get("multiValueHeaders") or {} @@ -405,7 +409,8 @@ def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, fla body = json_output.get("body") if body is None: LOG.warning("Lambda returned empty body!") - is_base_64_encoded = json_output.get("isBase64Encoded") or False + + is_base_64_encoded = LocalApigwService.get_base_64_encoded(event_type, json_output) try: status_code = int(status_code) @@ -422,8 +427,10 @@ def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, fla f"Non null response bodies should be able to convert to string: {body}" ) from ex - invalid_keys = LocalApigwService._invalid_apig_response_keys(json_output) - if invalid_keys: + invalid_keys = LocalApigwService._invalid_apig_response_keys(json_output, event_type) + # HTTP API Gateway just skip the non allowed lambda response fields, but Rest API gateway fail on + # the non allowed fields + if event_type == Route.API and invalid_keys: raise LambdaResponseParseException(f"Invalid API Gateway Response Keys: {invalid_keys} in {json_output}") # If the customer doesn't define Content-Type default to application/json @@ -432,17 +439,51 @@ def _parse_v1_payload_format_lambda_output(lambda_output: str, binary_types, fla headers["Content-Type"] = "application/json" try: - if LocalApigwService._should_base64_decode_body(binary_types, flask_request, headers, is_base_64_encoded): + # HTTP API Gateway always decode the lambda response only if isBase64Encoded field in response is True + # regardless the response content-type + # Rest API Gateway depends on the response content-type and the API configured BinaryMediaTypes to decide + # if it will decode the response or not + if (event_type == Route.HTTP and is_base_64_encoded) or ( + event_type == Route.API + and LocalApigwService._should_base64_decode_body( + binary_types, flask_request, headers, is_base_64_encoded + ) + ): body = base64.b64decode(body) except ValueError as ex: LambdaResponseParseException(str(ex)) return status_code, headers, body + @staticmethod + def get_base_64_encoded(event_type, json_output): + # The following behaviour is undocumented behaviour, and based on some trials + # Http API gateway checks lambda response for isBase64Encoded field, and ignore base64Encoded + # Rest API gateway checks first the field base64Encoded field, if not exist, it checks isBase64Encoded field + + if event_type == Route.API and json_output.get("base64Encoded") is not None: + is_base_64_encoded = json_output.get("base64Encoded") + field_name = "base64Encoded" + elif json_output.get("isBase64Encoded") is not None: + is_base_64_encoded = json_output.get("isBase64Encoded") + field_name = "isBase64Encoded" + else: + is_base_64_encoded = False + field_name = "isBase64Encoded" + + if isinstance(is_base_64_encoded, str) and is_base_64_encoded in ["true", "True", "false", "False"]: + is_base_64_encoded = is_base_64_encoded in ["true", "True"] + elif not isinstance(is_base_64_encoded, bool): + raise LambdaResponseParseException( + f"Invalid API Gateway Response Key: {is_base_64_encoded} is not a valid" f"{field_name}" + ) + + return is_base_64_encoded + @staticmethod def _parse_v2_payload_format_lambda_output(lambda_output: str, binary_types, flask_request): """ - Parses the output from the Lambda Container + Parses the output from the Lambda Container. V2 Payload Format means that the event_type is only HTTP :param str lambda_output: Output from Lambda Invoke :param binary_types: list of binary types @@ -487,21 +528,15 @@ def _parse_v2_payload_format_lambda_output(lambda_output: str, binary_types, fla f"Non null response bodies should be able to convert to string: {body}" ) from ex - # API Gateway only accepts statusCode, body, headers, and isBase64Encoded in - # a response shape. - # Don't check the response keys when inferring a response, see - # https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html#http-api-develop-integrations-lambda.v2. - invalid_keys = LocalApigwService._invalid_apig_response_keys(json_output) - if "statusCode" in json_output and invalid_keys: - raise LambdaResponseParseException(f"Invalid API Gateway Response Keys: {invalid_keys} in {json_output}") - # If the customer doesn't define Content-Type default to application/json if "Content-Type" not in headers: LOG.info("No Content-Type given. Defaulting to 'application/json'.") headers["Content-Type"] = "application/json" try: - if LocalApigwService._should_base64_decode_body(binary_types, flask_request, headers, is_base_64_encoded): + # HTTP API Gateway always decode the lambda response only if isBase64Encoded field in response is True + # regardless the response content-type + if is_base_64_encoded: # Note(xinhol): here in this method we change the type of the variable body multiple times # and confused mypy, we might want to avoid this and use multiple variables here. body = base64.b64decode(body) # type: ignore @@ -511,8 +546,10 @@ def _parse_v2_payload_format_lambda_output(lambda_output: str, binary_types, fla return status_code, headers, body @staticmethod - def _invalid_apig_response_keys(output): + def _invalid_apig_response_keys(output, event_type): allowable = {"statusCode", "body", "headers", "multiValueHeaders", "isBase64Encoded", "cookies"} + if event_type == Route.API: + allowable.add("base64Encoded") invalid_keys = output.keys() - allowable return invalid_keys diff --git a/tests/integration/local/start_api/test_start_api.py b/tests/integration/local/start_api/test_start_api.py index e7e5ad59a16..0ddb8d5a317 100644 --- a/tests/integration/local/start_api/test_start_api.py +++ b/tests/integration/local/start_api/test_start_api.py @@ -1,3 +1,4 @@ +import base64 import uuid import random @@ -382,14 +383,14 @@ def test_valid_v2_lambda_integer_response(self): @pytest.mark.flaky(reruns=3) @pytest.mark.timeout(timeout=600, method="thread") - def test_invalid_v2_lambda_response(self): + def test_v2_lambda_response_skip_unexpected_fields(self): """ Patch Request to a path that was defined as ANY in SAM through AWS::Serverless::Function Events """ response = requests.get(self.url + "/invalidv2response", timeout=300) - self.assertEqual(response.status_code, 502) - self.assertEqual(response.json(), {"message": "Internal server error"}) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {"hello": "world"}) @pytest.mark.flaky(reruns=3) @pytest.mark.timeout(timeout=600, method="thread") @@ -538,6 +539,48 @@ def test_binary_response(self): self.assertEqual(response.headers.get("Content-Type"), "image/gif") self.assertEqual(response.content, expected) + @pytest.mark.flaky(reruns=3) + @pytest.mark.timeout(timeout=600, method="thread") + def test_non_decoded_binary_response(self): + """ + Binary data is returned correctly + """ + expected = base64.b64encode(self.get_binary_data(self.binary_data_file)) + + response = requests.get(self.url + "/nondecodedbase64response", timeout=300) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers.get("Content-Type"), "image/gif") + self.assertEqual(response.content, expected) + + @pytest.mark.flaky(reruns=3) + @pytest.mark.timeout(timeout=600, method="thread") + def test_decoded_binary_response_base64encoded_field(self): + """ + Binary data is returned correctly + """ + expected = self.get_binary_data(self.binary_data_file) + + response = requests.get(self.url + "/decodedbase64responsebas64encoded", timeout=300) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers.get("Content-Type"), "image/gif") + self.assertEqual(response.content, expected) + + @pytest.mark.flaky(reruns=3) + @pytest.mark.timeout(timeout=600, method="thread") + def test_decoded_binary_response_base64encoded_field_is_priority(self): + """ + Binary data is returned correctly + """ + expected = base64.b64encode(self.get_binary_data(self.binary_data_file)) + + response = requests.get(self.url + "/decodedbase64responsebas64encodedpriority", timeout=300) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers.get("Content-Type"), "image/gif") + self.assertEqual(response.content, expected) + class TestStartApiWithSwaggerHttpApis(StartApiIntegBaseClass): template_path = "/testdata/start_api/swagger-template-http-api.yaml" diff --git a/tests/integration/testdata/start_api/binarydata.gif b/tests/integration/testdata/start_api/binarydata.gif index 855b4041793a49335cf6d1b66d8c1e5059daf60f..3f40c2073daf9743db59e7bec58cf90e8f6d3fbc 100644 GIT binary patch literal 49 ucmZ?wbh9u|WMp7un8*ME|Ns97(+r9~SvVOOm>6_GT#!5i6O#)ggEau*tOpVR literal 1951 zcmd^8{a2EC9{qyKi?V`(uBZf}LzuRr)~uT8Lm4uyCYqdPgu-KB=kujfc^aqiu{cg<9!rLi^agVZ!h$> zz$jFYCx9m=a9GUthWTNxaL66B_yG8`|NMEqD=_-|!(y?4Ox6(zT1Eb5Tpax2!)j>g z9D`wwi17OyOxvr~evTZfkA%)-l?nh52n16s);tl!9S)F6rSD>5a!+u_16c$D;k!RV z8z^9aphxB6?Ck7zH^wN%$>8HdrBaOm%sPT8MZT5_$V9@snVFY?feeaMT*1Yfni{yP zYl=wJvRKDC&;WxLLUH=Ev;;wrvQ`0)jFj+ry}i8?C%A>V;#vl6P%OS(F3#ieBw}%I zRMgDOjM;3C2xN_Psj^wjHyRCWGQl%56)fhTSj>l#)E0pt^Gx0w4#!|Hm`tW51`Vds zioBT+i`i9Md0mo9H5#AxTv^r+J`nPvqM{lLQpI91ES19b^>aeuV`YiA#-Ihi-RIc= zfRmGdO0^)oZ= zZWZ=Ns_vx4T>8W5pr(&^R&b56w||iby+!f97hc}8&w3#9sT>ueJmu1_ZYwy=d`7lwzjCN37$e6$bVS~u7^ zlh8$yv&lx#oXPtkVMuiR^wG<3A+4zJoA1*ia<6L`0aCBq!@iw(uT+} zEE$l+3r))KFXy@J(i0HEqvzS{?!MWe z3~{=xc(t$lS_~WEMw-)ET;RAo2T!X1ymw&I9vzQ$SPxnn%&N|~$)t!1dr0NdOJ2oW zqr>eZKiztPxEHg-4&R;7(e11?mu0k$ShT;HqjbbRt{Btj@R0Yrx4oK&qPS~wE^)CD z)zsqF@7B`k^E%(G|6E*TqgLAvU_GNX?4u3|>Jh(GHDb;?$gqohqKwA?dG`Vb> zhPF}+J$2m=?_jFmm*S`XA?!)VE5!ReSCVjIYU-HX>a&Vd(4)B;#RdZ7PRNsb=#`^; zAB2)%Xksz5X4%p+iPeYoERx$FWmsAv^-ePhTUu zK00t9)3*ublF_u!DTukt+JTW9Gsq6qMZcvv@j<*XSgs06_24GDrW{UCJtth7ycJ-V zB8o;M&~5~4yOR>yIrDXe<}899OFM)JR{#5+fG-gr*>&TgDU3Wl^>@qn7&|G1eCst` zgmqivjoONao^O|{DyyT}D8(ppXCqwZx^KLAYl^y=`0drOt2p!{7roUzg_b8glt(HR zqIXaFXeS|T_F;r)BN(=w{f7s=PGw$yc_qM3$`8=Qr>F;U8^7rQ944|pm9TE6vyM$Omm*tOa#_@eN#A Date: Thu, 24 Jun 2021 21:42:20 -0400 Subject: [PATCH 06/37] Fixed lint errors and added few unit tests --- samcli/commands/delete/command.py | 9 +++- samcli/commands/delete/delete_context.py | 16 +++--- samcli/lib/package/s3_uploader.py | 3 +- tests/unit/commands/delete/__init__.py | 0 tests/unit/commands/delete/test_command.py | 49 +++++++++++++++++++ .../commands/delete/test_delete_context.py | 0 tests/unit/lib/delete/test_cf_utils.py | 17 ++++++- tests/unit/lib/package/test_s3_uploader.py | 2 +- 8 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 tests/unit/commands/delete/__init__.py create mode 100644 tests/unit/commands/delete/test_command.py create mode 100644 tests/unit/commands/delete/test_delete_context.py diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 7227be86e30..8fc04716b97 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -63,8 +63,13 @@ def do_cli(stack_name, region, profile): from samcli.commands.delete.delete_context import DeleteContext ctx = click.get_current_context() - + s3_bucket = ctx.default_map.get("s3_bucket", None) + s3_prefix = ctx.default_map.get("s3_prefix", None) with DeleteContext( - stack_name=stack_name, region=region, s3_bucket=ctx.default_map.get("s3_bucket", None), s3_prefix=ctx.default_map.get("s3_prefix", None), profile=profile + stack_name=stack_name, + region=region, + s3_bucket=s3_bucket, + s3_prefix=s3_prefix, + profile=profile ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 919f1e4f994..b2a861fa16a 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -3,7 +3,7 @@ """ import boto3 -import docker +# import docker import click from click import confirm from click import prompt @@ -12,7 +12,7 @@ from samcli.lib.delete.cf_utils import CfUtils from samcli.lib.delete.utils import get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader -from samcli.yamlhelper import yaml_parse +# from samcli.yamlhelper import yaml_parse # Intentionally commented # from samcli.lib.package.artifact_exporter import Template @@ -69,11 +69,11 @@ def run(self): ) s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) - ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + # ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - docker_client = docker.from_env() + # docker_client = docker.from_env() # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) self.cf_utils = CfUtils(cloudformation_client) @@ -83,17 +83,19 @@ def run(self): if is_deployed: template_str = self.cf_utils.get_stack_template(self.stack_name, "Original") - template_dict = yaml_parse(template_str) + # template_dict = yaml_parse(template_str) if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + f"\t{self.start_bold}Are you sure you want to delete the folder" + \ + f"{self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", default=False, ) if not self.delete_artifacts_folder: self.cf_template_file_name = get_cf_template_name(template_str, "template") self.delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file {self.cf_template_file_name} in S3?{self.end_bold}", + f"\t{self.start_bold}Do you want to delete the template file" + \ + f" {self.cf_template_file_name} in S3?{self.end_bold}", default=False, ) diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 3e445aec257..c0a4d88cf6d 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -22,6 +22,7 @@ from collections import abc from typing import Optional, Dict, Any, cast from urllib.parse import urlparse, parse_qs +import click import botocore import botocore.exceptions @@ -159,7 +160,7 @@ def delete_artifact(self, remote_path: str, is_key=False): key = "{0}/{1}".format(self.prefix, remote_path) # Deleting Specific file with key - print("- deleting", key) + click.echo("- deleting S3 file " + key) resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) return resp["ResponseMetadata"] diff --git a/tests/unit/commands/delete/__init__.py b/tests/unit/commands/delete/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py new file mode 100644 index 00000000000..0a0e58afec0 --- /dev/null +++ b/tests/unit/commands/delete/test_command.py @@ -0,0 +1,49 @@ +from unittest import TestCase +from unittest.mock import ANY, MagicMock, Mock, call, patch + +from samcli.commands.delete.command import do_cli +from tests.unit.cli.test_cli_config_file import MockContext + +def get_mock_sam_config(): + mock_sam_config = MagicMock() + mock_sam_config.exists = MagicMock(return_value=True) + return mock_sam_config + +MOCK_SAM_CONFIG = get_mock_sam_config() + +class TestDeleteCliCommand(TestCase): + def setUp(self): + + self.stack_name = "stack-name" + self.s3_bucket = "s3-bucket" + self.s3_prefix = "s3-prefix" + self.region = None + self.profile = None + self.config_env = "mock-default-env" + self.config_file = "mock-default-filename" + MOCK_SAM_CONFIG.reset_mock() + + + @patch("samcli.commands.delete.command.click") + @patch("samcli.commands.delete.delete_context.DeleteContext") + def test_all_args(self, mock_delete_context, mock_delete_click): + + context_mock = Mock() + mock_delete_context.return_value.__enter__.return_value = context_mock + + do_cli( + stack_name=self.stack_name, + region=self.region, + profile=self.profile, + ) + + mock_delete_context.assert_called_with( + stack_name=self.stack_name, + s3_bucket=mock_delete_click.get_current_context().default_map.get("s3_bucket", None), + s3_prefix=mock_delete_click.get_current_context().default_map.get("s3_prefix", None), + region=self.region, + profile=self.profile, + ) + + context_mock.run.assert_called_with() + self.assertEqual(context_mock.run.call_count, 1) diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py index 37403a156f9..20cb5288dc8 100644 --- a/tests/unit/lib/delete/test_cf_utils.py +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -28,6 +28,16 @@ def test_cf_utils_has_stack_exception_non_exsistent(self): ) ) self.assertEqual(self.cf_utils.has_stack("test"), False) + + def test_cf_utils_has_stack_exception_client_error(self): + self.cf_utils._client.describe_stacks = MagicMock( + side_effect=ClientError( + error_response={"Error": {"Message": "Error: The security token included in the request is expired"}}, + operation_name="stack_status", + ) + ) + with self.assertRaises(DeleteFailedError): + self.cf_utils.has_stack("test") def test_cf_utils_has_stack_exception(self): self.cf_utils._client.describe_stacks = MagicMock(side_effect=Exception()) @@ -45,8 +55,11 @@ def test_cf_utils_has_stack_exception_botocore(self): with self.assertRaises(DeleteFailedError): self.cf_utils.has_stack("test") - def test_cf_utils_get_stack_template_exception_botocore(self): - self.cf_utils._client.get_template = MagicMock(side_effect=BotoCoreError()) + def test_cf_utils_get_stack_template_exception_client_error(self): + self.cf_utils._client.get_template = MagicMock(side_effect=ClientError( + error_response={"Error": {"Message": "Stack with id test does not exist"}}, + operation_name="stack_status", + )) with self.assertRaises(DeleteFailedError): self.cf_utils.get_stack_template("test", "Original") diff --git a/tests/unit/lib/package/test_s3_uploader.py b/tests/unit/lib/package/test_s3_uploader.py index 07fed24211a..f1765c3f8ce 100644 --- a/tests/unit/lib/package/test_s3_uploader.py +++ b/tests/unit/lib/package/test_s3_uploader.py @@ -200,7 +200,7 @@ def test_s3_delete_artifact_no_bucket(self): s3_uploader.delete_artifact(f.name) self.assertEqual(BucketNotSpecifiedError().message, str(ex)) - def test_s3_upload_bucket_not_found(self): + def test_s3_delete_artifact_bucket_not_found(self): s3_uploader = S3Uploader( s3_client=self.s3, bucket_name=self.bucket_name, From af2f9296f30568f9e851f520f1d3853edce2350f Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 24 Jun 2021 22:14:25 -0400 Subject: [PATCH 07/37] Make black happy --- samcli/commands/delete/command.py | 6 +----- samcli/commands/delete/delete_context.py | 14 ++++++++------ samcli/lib/delete/cf_utils.py | 4 ++-- samcli/lib/delete/utils.py | 1 + tests/unit/commands/delete/test_command.py | 4 +++- tests/unit/lib/delete/test_cf_utils.py | 12 +++++++----- 6 files changed, 22 insertions(+), 19 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 8fc04716b97..412e4fc76f5 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -66,10 +66,6 @@ def do_cli(stack_name, region, profile): s3_bucket = ctx.default_map.get("s3_bucket", None) s3_prefix = ctx.default_map.get("s3_prefix", None) with DeleteContext( - stack_name=stack_name, - region=region, - s3_bucket=s3_bucket, - s3_prefix=s3_prefix, - profile=profile + stack_name=stack_name, region=region, s3_bucket=s3_bucket, s3_prefix=s3_prefix, profile=profile ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index b2a861fa16a..fb4a09e4e1a 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -3,6 +3,7 @@ """ import boto3 + # import docker import click from click import confirm @@ -12,6 +13,7 @@ from samcli.lib.delete.cf_utils import CfUtils from samcli.lib.delete.utils import get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader + # from samcli.yamlhelper import yaml_parse # Intentionally commented @@ -87,16 +89,16 @@ def run(self): if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder" + \ - f"{self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", - default=False, + f"\t{self.start_bold}Are you sure you want to delete the folder" + + f"{self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + default=False, ) if not self.delete_artifacts_folder: self.cf_template_file_name = get_cf_template_name(template_str, "template") self.delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file" + \ - f" {self.cf_template_file_name} in S3?{self.end_bold}", - default=False, + f"\t{self.start_bold}Do you want to delete the template file" + + f" {self.cf_template_file_name} in S3?{self.end_bold}", + default=False, ) click.echo("\n") diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index 945aa5a09a2..f0c7aa4731d 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -65,7 +65,7 @@ def get_stack_template(self, stack_name, stage): return resp["TemplateBody"] - except (ClientError, BotoCoreError) as e: + except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, # catch that and throw a delete failed error. @@ -88,7 +88,7 @@ def delete_stack(self, stack_name): resp = self._client.delete_stack(StackName=stack_name) return resp - except (ClientError, BotoCoreError) as e: + except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, # catch that and throw a delete failed error. diff --git a/samcli/lib/delete/utils.py b/samcli/lib/delete/utils.py index f6e6edeb4d9..497610f7760 100644 --- a/samcli/lib/delete/utils.py +++ b/samcli/lib/delete/utils.py @@ -5,6 +5,7 @@ from samcli.lib.utils.hash import file_checksum from samcli.lib.package.artifact_exporter import mktempfile + def get_cf_template_name(template_str, extension): with mktempfile() as temp_file: temp_file.write(template_str) diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py index 0a0e58afec0..a199c4e9600 100644 --- a/tests/unit/commands/delete/test_command.py +++ b/tests/unit/commands/delete/test_command.py @@ -4,13 +4,16 @@ from samcli.commands.delete.command import do_cli from tests.unit.cli.test_cli_config_file import MockContext + def get_mock_sam_config(): mock_sam_config = MagicMock() mock_sam_config.exists = MagicMock(return_value=True) return mock_sam_config + MOCK_SAM_CONFIG = get_mock_sam_config() + class TestDeleteCliCommand(TestCase): def setUp(self): @@ -23,7 +26,6 @@ def setUp(self): self.config_file = "mock-default-filename" MOCK_SAM_CONFIG.reset_mock() - @patch("samcli.commands.delete.command.click") @patch("samcli.commands.delete.delete_context.DeleteContext") def test_all_args(self, mock_delete_context, mock_delete_click): diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py index 20cb5288dc8..36f32ae7350 100644 --- a/tests/unit/lib/delete/test_cf_utils.py +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -28,7 +28,7 @@ def test_cf_utils_has_stack_exception_non_exsistent(self): ) ) self.assertEqual(self.cf_utils.has_stack("test"), False) - + def test_cf_utils_has_stack_exception_client_error(self): self.cf_utils._client.describe_stacks = MagicMock( side_effect=ClientError( @@ -56,10 +56,12 @@ def test_cf_utils_has_stack_exception_botocore(self): self.cf_utils.has_stack("test") def test_cf_utils_get_stack_template_exception_client_error(self): - self.cf_utils._client.get_template = MagicMock(side_effect=ClientError( - error_response={"Error": {"Message": "Stack with id test does not exist"}}, - operation_name="stack_status", - )) + self.cf_utils._client.get_template = MagicMock( + side_effect=ClientError( + error_response={"Error": {"Message": "Stack with id test does not exist"}}, + operation_name="stack_status", + ) + ) with self.assertRaises(DeleteFailedError): self.cf_utils.get_stack_template("test", "Original") From 1d70155554159e3add65cb17cc1afad77cb314c0 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 28 Jun 2021 12:41:21 -0400 Subject: [PATCH 08/37] Added methods for deleting template artifacts --- samcli/commands/delete/delete_context.py | 34 ++++++++--------- samcli/commands/package/exceptions.py | 22 +++++++++++ samcli/lib/package/artifact_exporter.py | 42 ++++++++++++++++----- samcli/lib/package/ecr_uploader.py | 32 +++++++++++++++- samcli/lib/package/packageable_resources.py | 27 +++++++++++++ tests/unit/lib/delete/test_utils.py | 8 ++++ 6 files changed, 136 insertions(+), 29 deletions(-) create mode 100644 tests/unit/lib/delete/test_utils.py diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index fb4a09e4e1a..1ae7122dbd3 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -4,7 +4,7 @@ import boto3 -# import docker +import docker import click from click import confirm from click import prompt @@ -14,12 +14,11 @@ from samcli.lib.delete.utils import get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader -# from samcli.yamlhelper import yaml_parse +from samcli.yamlhelper import yaml_parse -# Intentionally commented -# from samcli.lib.package.artifact_exporter import Template -# from samcli.lib.package.ecr_uploader import ECRUploader -# from samcli.lib.package.uploaders import Uploaders +from samcli.lib.package.artifact_exporter import Template +from samcli.lib.package.ecr_uploader import ECRUploader +from samcli.lib.package.uploaders import Uploaders class DeleteContext: @@ -33,7 +32,7 @@ def __init__(self, stack_name, region, s3_bucket, s3_prefix, profile): self.start_bold = "\033[1m" self.end_bold = "\033[0m" self.s3_uploader = None - # self.uploaders = None + self.uploaders = None self.cf_template_file_name = None self.delete_artifacts_folder = None self.delete_cf_template_file = None @@ -71,12 +70,12 @@ def run(self): ) s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) - # ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - # docker_client = docker.from_env() - # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + docker_client = docker.from_env() + ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) self.cf_utils = CfUtils(cloudformation_client) @@ -85,12 +84,12 @@ def run(self): if is_deployed: template_str = self.cf_utils.get_stack_template(self.stack_name, "Original") - # template_dict = yaml_parse(template_str) + template_dict = yaml_parse(template_str) if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( f"\t{self.start_bold}Are you sure you want to delete the folder" - + f"{self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + + f" {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", default=False, ) if not self.delete_artifacts_folder: @@ -103,15 +102,14 @@ def run(self): click.echo("\n") # Delete the primary stack + click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) self.cf_utils.delete_stack(self.stack_name) - click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) # Delete the artifacts - # Intentionally commented - # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) - # template = Template(None, None, self.uploaders, None) - # template.delete(template_dict) + self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) + template = Template(None, None, self.uploaders, None) + template.delete(template_dict) # Delete the CF template file in S3 if self.delete_cf_template_file: @@ -123,8 +121,6 @@ def run(self): # Delete the ECR companion stack - if self.cf_template_file_name: - click.echo(f"- deleting template file {self.cf_template_file_name}") click.echo("\n") click.echo("delete complete") else: diff --git a/samcli/commands/package/exceptions.py b/samcli/commands/package/exceptions.py index a650f628434..2e23cf74588 100644 --- a/samcli/commands/package/exceptions.py +++ b/samcli/commands/package/exceptions.py @@ -62,6 +62,28 @@ def __init__(self, resource_id, property_name, property_value, ex): ) +class DeleteArtifactFailedError(UserException): + def __init__(self, resource_id, property_name, ex): + self.resource_id = resource_id + self.property_name = property_name + self.ex = ex + + message_fmt = ( + "Unable to delete artifact referenced " + "by {property_name} parameter of {resource_id} resource." + "\n" + "{ex}" + ) + + super().__init__( + message=message_fmt.format( + property_name=self.property_name, + resource_id=self.resource_id, + ex=self.ex, + ) + ) + + class ImageNotFoundError(UserException): def __init__(self, resource_id, property_name): self.resource_id = resource_id diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index c0f2b945769..d0372730b6f 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -130,21 +130,22 @@ def __init__( """ Reads the template and makes it ready for export """ - if not (is_local_folder(parent_dir) and os.path.isabs(parent_dir)): - raise ValueError("parent_dir parameter must be " "an absolute path to a folder {0}".format(parent_dir)) + if template_path and parent_dir: + if not (is_local_folder(parent_dir) and os.path.isabs(parent_dir)): + raise ValueError("parent_dir parameter must be " "an absolute path to a folder {0}".format(parent_dir)) - abs_template_path = make_abs_path(parent_dir, template_path) - template_dir = os.path.dirname(abs_template_path) + abs_template_path = make_abs_path(parent_dir, template_path) + template_dir = os.path.dirname(abs_template_path) - with open(abs_template_path, "r") as handle: - template_str = handle.read() + with open(abs_template_path, "r") as handle: + template_str = handle.read() - self.template_dict = yaml_parse(template_str) - self.template_dir = template_dir + self.template_dict = yaml_parse(template_str) + self.template_dir = template_dir + self.code_signer = code_signer self.resources_to_export = resources_to_export self.metadata_to_export = metadata_to_export self.uploaders = uploaders - self.code_signer = code_signer def _export_global_artifacts(self, template_dict: Dict) -> Dict: """ @@ -235,3 +236,26 @@ def export(self) -> Dict: exporter.export(resource_id, resource_dict, self.template_dir) return self.template_dict + + def delete(self, template_dict): + self.template_dict = template_dict + + if "Resources" not in self.template_dict: + return self.template_dict + + self._apply_global_values() + + for resource_id, resource in self.template_dict["Resources"].items(): + + resource_type = resource.get("Type", None) + resource_dict = resource.get("Properties", {}) + + for exporter_class in self.resources_to_export: + if exporter_class.RESOURCE_TYPE != resource_type: + continue + if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: + continue + # Delete code resources + exporter = exporter_class(self.uploaders, None) + exporter.delete(resource_id, resource_dict) + return self.template_dict diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index fcf4e836e9e..4f8b0246d0b 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -5,12 +5,19 @@ import base64 import os +import click import botocore import docker from docker.errors import BuildError, APIError -from samcli.commands.package.exceptions import DockerPushFailedError, DockerLoginFailedError, ECRAuthorizationError +from samcli.commands.package.exceptions import ( + DockerPushFailedError, + DockerLoginFailedError, + ECRAuthorizationError, + ImageNotFoundError, + DeleteArtifactFailedError +) from samcli.lib.package.image_utils import tag_translation from samcli.lib.package.stream_cursor_utils import cursor_up, cursor_left, cursor_down, clear_line from samcli.lib.utils.osutils import stderr @@ -83,6 +90,29 @@ def upload(self, image, resource_name): return f"{repository}:{_tag}" + def delete_artifact(self, image_uri, resource_id, property_name): + try: + repo_image_tag = image_uri.split("/")[1].split(":") + repository = repo_image_tag[0] + image_tag = repo_image_tag[1] + resp = self.ecr_client.batch_delete_image(repositoryName=repository, + imageIds=[ + { + 'imageTag': image_tag + }, + ] + ) + if resp["failures"]: + image_details = resp["failures"][0] + if image_details["failureCode"] == "ImageNotFound": + LOG.debug("ImageNotFound Exception : ") + raise ImageNotFoundError(resource_id, property_name) + + click.echo("- deleting ECR image {0} in repository {1}".format(image_tag, repository)) + + except botocore.exceptions.ClientError as ex: + raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex + # TODO: move this to a generic class to allow for streaming logs back from docker. def _stream_progress(self, logs): """ diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 937b451a280..1169e23ebbb 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -79,6 +79,9 @@ def export(self, resource_id, resource_dict, parent_dir): def do_export(self, resource_id, resource_dict, parent_dir): pass + def delete(self, resource_id, resource_dict): + pass + class ResourceZip(Resource): """ @@ -154,6 +157,16 @@ def do_export(self, resource_id, resource_dict, parent_dir): ) set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) + def delete(self, resource_id, resource_dict): + + if resource_dict is None: + return + resource_path = resource_dict[self.PROPERTY_NAME] + parsed_s3_url = self.uploader.parse_s3_url(resource_path) + print(parsed_s3_url["Key"]) + if not self.uploader.bucket_name: + self.uploader.bucket_name = parsed_s3_url["Bucket"] + self.uploader.delete_artifact(parsed_s3_url["Key"], True) class ResourceImageDict(Resource): """ @@ -238,6 +251,8 @@ def do_export(self, resource_id, resource_dict, parent_dir): ) set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) + def delete(self, resource_id, resource_dict): + self.uploader.delete_artifact(resource_dict["ImageUri"], resource_id, self.PROPERTY_NAME) class ResourceWithS3UrlDict(ResourceZip): """ @@ -269,6 +284,18 @@ def do_export(self, resource_id, resource_dict, parent_dir): ) set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, parsed_url) + def delete(self, resource_id, resource_dict): + + if resource_dict is None: + return + resource_path = resource_dict[self.PROPERTY_NAME] + s3_bucket = resource_path[self.BUCKET_NAME_PROPERTY] + key = resource_path["Key"] + + if not self.uploader.bucket_name: + self.uploader.bucket_name = s3_bucket + self.uploader.delete_artifact(remote_path=key, is_key=True) + class ServerlessFunctionResource(ResourceZip): RESOURCE_TYPE = AWS_SERVERLESS_FUNCTION diff --git a/tests/unit/lib/delete/test_utils.py b/tests/unit/lib/delete/test_utils.py new file mode 100644 index 00000000000..c39f176d5c6 --- /dev/null +++ b/tests/unit/lib/delete/test_utils.py @@ -0,0 +1,8 @@ +from unittest import TestCase + +from samcli.lib.delete.utils import get_cf_template_name + +class TestCfUtils(TestCase): + + def test_utils(self): + self.assertEqual(get_cf_template_name("hello world!", "template"), "fc3ff98e8c6a0d3087d515c0473f8677.template") \ No newline at end of file From e3f787232dd21fcc121da77bcd9368bb54b59b31 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 28 Jun 2021 13:06:23 -0400 Subject: [PATCH 09/37] Wait method added for delete cf api --- samcli/commands/delete/delete_context.py | 1 + samcli/lib/delete/cf_utils.py | 23 ++++++++++++++++++++++- tests/unit/lib/delete/test_cf_utils.py | 24 ++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 1ae7122dbd3..6d470438e10 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -104,6 +104,7 @@ def run(self): # Delete the primary stack click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) self.cf_utils.delete_stack(self.stack_name) + self.cf_utils.wait_for_delete(self.stack_name) # Delete the artifacts diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index f0c7aa4731d..7d8be75601f 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -4,7 +4,7 @@ import logging -from botocore.exceptions import ClientError, BotoCoreError +from botocore.exceptions import ClientError, BotoCoreError, WaiterError from samcli.commands.delete.exceptions import DeleteFailedError LOG = logging.getLogger(__name__) @@ -99,3 +99,24 @@ def delete_stack(self, stack_name): # We don't know anything about this exception. Don't handle LOG.debug("Unable to get stack details.", exc_info=e) raise e + + def wait_for_delete(self, stack_name): + """ + Waits until the delete stack completes + + :param stack_name: Stack name + """ + + # Wait for Delete to Finish + waiter = self._client.get_waiter("stack_delete_complete") + # Poll every 5 seconds. + waiter_config = {"Delay": 5} + try: + waiter.wait(StackName=stack_name, WaiterConfig=waiter_config) + except WaiterError as ex: + + resp = ex.last_response + status = resp["Status"] + reason = resp["StatusReason"] + + raise DeleteFailedError(stack_name=stack_name, msg="ex: {0} Status: {1}. Reason: {2}".format(ex, status, reason)) from ex diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py index 36f32ae7350..8e57407231e 100644 --- a/tests/unit/lib/delete/test_cf_utils.py +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -2,9 +2,17 @@ from unittest import TestCase from samcli.commands.delete.exceptions import DeleteFailedError -from botocore.exceptions import ClientError, BotoCoreError +from botocore.exceptions import ClientError, BotoCoreError, WaiterError from samcli.lib.delete.cf_utils import CfUtils +class MockDeleteWaiter: + def __init__(self, ex=None): + self.ex = ex + + def wait(self, StackName, WaiterConfig): + if self.ex: + raise self.ex + return class TestCfUtils(TestCase): def setUp(self): @@ -20,7 +28,7 @@ def test_cf_utils_has_no_stack(self): self.cf_utils._client.describe_stacks = MagicMock(return_value={"Stacks": []}) self.assertEqual(self.cf_utils.has_stack("test"), False) - def test_cf_utils_has_stack_exception_non_exsistent(self): + def test_cf_utils_has_stack_exception_non_existent(self): self.cf_utils._client.describe_stacks = MagicMock( side_effect=ClientError( error_response={"Error": {"Message": "Stack with id test does not exist"}}, @@ -84,3 +92,15 @@ def test_cf_utils_delete_stack_exception(self): self.cf_utils._client.delete_stack = MagicMock(side_effect=Exception()) with self.assertRaises(Exception): self.cf_utils.delete_stack("test") + + def test_cf_utils_wait_for_delete_exception(self): + self.cf_utils._client.get_waiter = MagicMock( + return_value=MockDeleteWaiter( + ex=WaiterError( + name="wait_for_delete", + reason="unit-test", + last_response={"Status": "Failed", "StatusReason": "It's a unit test"}, + ) + )) + with self.assertRaises(DeleteFailedError): + self.cf_utils.wait_for_delete("test") From 99f7db4768e44bffed317bcbcd2d1ebba48174cb Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Tue, 29 Jun 2021 10:16:00 -0400 Subject: [PATCH 10/37] Added LOG statements --- samcli/lib/delete/cf_utils.py | 13 +++++++------ samcli/lib/package/s3_uploader.py | 4 ++++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index f0c7aa4731d..db36f11ce34 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -37,17 +37,18 @@ def has_stack(self, stack_name): if "Stack with id {0} does not exist".format(stack_name) in str(e): LOG.debug("Stack with id %s does not exist", stack_name) return False + LOG.error("ClientError Exception : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except BotoCoreError as e: # If there are credentials, environment errors, # catch that and throw a delete failed error. - LOG.debug("Botocore Exception : %s", str(e)) + LOG.error("Botocore Exception : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: # We don't know anything about this exception. Don't handle - LOG.debug("Unable to get stack details.", exc_info=e) + LOG.error("Unable to get stack details.", exc_info=e) raise e def get_stack_template(self, stack_name, stage): @@ -69,12 +70,12 @@ def get_stack_template(self, stack_name, stage): # If there are credentials, environment errors, # catch that and throw a delete failed error. - LOG.debug("Failed to delete stack : %s", str(e)) + LOG.error("Failed to delete stack : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: # We don't know anything about this exception. Don't handle - LOG.debug("Unable to get stack details.", exc_info=e) + LOG.error("Unable to get stack details.", exc_info=e) raise e def delete_stack(self, stack_name): @@ -92,10 +93,10 @@ def delete_stack(self, stack_name): # If there are credentials, environment errors, # catch that and throw a delete failed error. - LOG.debug("Failed to delete stack : %s", str(e)) + LOG.error("Failed to delete stack : %s", str(e)) raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: # We don't know anything about this exception. Don't handle - LOG.debug("Unable to get stack details.", exc_info=e) + LOG.error("Failed to delete stack. ", exc_info=e) raise e diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index c0a4d88cf6d..3fb7070d2be 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -153,6 +153,7 @@ def delete_artifact(self, remote_path: str, is_key=False): """ try: if not self.bucket_name: + LOG.error("Bucket not specified") raise BucketNotSpecifiedError() key = remote_path @@ -162,11 +163,13 @@ def delete_artifact(self, remote_path: str, is_key=False): # Deleting Specific file with key click.echo("- deleting S3 file " + key) resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) + LOG.debug("S3 method delete_object is called and returned: %s", resp["ResponseMetadata"]) return resp["ResponseMetadata"] except botocore.exceptions.ClientError as ex: error_code = ex.response["Error"]["Code"] if error_code == "NoSuchBucket": + LOG.error("Provided bucket %s does not exist ", self.bucket_name) raise NoSuchBucketError(bucket_name=self.bucket_name) from ex raise ex @@ -175,6 +178,7 @@ def delete_prefix_artifacts(self): Deletes all the files from the prefix in S3 """ if not self.bucket_name: + LOG.error("Bucket not specified") raise BucketNotSpecifiedError() if self.prefix: prefix_files = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix) From e7304ec2807bd4943363ca7ae791407fdfdf673f Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Tue, 29 Jun 2021 12:48:29 -0400 Subject: [PATCH 11/37] Added and updated changes based on CR --- samcli/commands/delete/command.py | 43 ++++-- samcli/commands/delete/delete_context.py | 144 +++++++++++------- samcli/lib/delete/utils.py | 17 --- samcli/lib/deploy/deployer.py | 7 +- samcli/lib/package/artifact_exporter.py | 6 +- samcli/lib/package/utils.py | 12 +- tests/unit/commands/delete/test_command.py | 6 +- .../lib/package/test_artifact_exporter.py | 28 ++-- 8 files changed, 153 insertions(+), 110 deletions(-) delete mode 100644 samcli/lib/delete/utils.py diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 412e4fc76f5..d95382bb51b 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -5,23 +5,21 @@ import logging import click -from samcli.cli.cli_config_file import TomlProvider, configuration_option from samcli.cli.main import aws_creds_options, common_options, pass_context, print_cmdline_args from samcli.lib.utils.version_checker import check_newer_version -SHORT_HELP = "Delete an AWS SAM application." +SHORT_HELP = "Delete an AWS SAM application and the artifacts created by sam deploy." -HELP_TEXT = """The sam delete command deletes a Cloudformation Stack and deletes all your resources which were created. +HELP_TEXT = """The sam delete command deletes the Cloudformation +Stack and all the artifacts which were created using sam deploy. \b -e.g. sam delete --stack-name sam-app --region us-east-1 +e.g. sam delete \b """ -CONFIG_SECTION = "parameters" -CONFIG_COMMAND = "deploy" LOG = logging.getLogger(__name__) @@ -31,12 +29,34 @@ context_settings={"ignore_unknown_options": False, "allow_interspersed_args": True, "allow_extra_args": True}, help=HELP_TEXT, ) -@configuration_option(provider=TomlProvider(section=CONFIG_SECTION, cmd_names=[CONFIG_COMMAND])) @click.option( "--stack-name", required=False, help="The name of the AWS CloudFormation stack you want to delete. ", ) +@click.option( + "--config-file", + required=False, + help=( + "The path and file name of the configuration file containing default parameter values to use. " + "Its default value is 'samconfig.toml' in project directory. For more information about configuration files, " + "see: " + "https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-cli-config.html." + ), + type=click.STRING, + default="samconfig.toml", +) +@click.option( + "--config-env", + required=False, + help=( + "The environment name specifying the default parameter values in the configuration file to use. " + "Its default value is 'default'. For more information about configuration files, see: " + "https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-cli-config.html." + ), + type=click.STRING, + default="default", +) @aws_creds_options @common_options @pass_context @@ -53,19 +73,16 @@ def cli( """ # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing - do_cli(stack_name, ctx.region, ctx.profile) # pragma: no cover + do_cli(stack_name, ctx.region, config_file, config_env, ctx.profile) # pragma: no cover -def do_cli(stack_name, region, profile): +def do_cli(stack_name, region, config_file, config_env, profile): """ Implementation of the ``cli`` method """ from samcli.commands.delete.delete_context import DeleteContext - ctx = click.get_current_context() - s3_bucket = ctx.default_map.get("s3_bucket", None) - s3_prefix = ctx.default_map.get("s3_prefix", None) with DeleteContext( - stack_name=stack_name, region=region, s3_bucket=s3_bucket, s3_prefix=s3_prefix, profile=profile + stack_name=stack_name, region=region, profile=profile, config_file=config_file, config_env=config_env ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index fb4a09e4e1a..95c62d4dd1d 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -8,11 +8,11 @@ import click from click import confirm from click import prompt - +from samcli.cli.cli_config_file import TomlProvider from samcli.lib.utils.botoconfig import get_boto_config_with_user_agent from samcli.lib.delete.cf_utils import CfUtils -from samcli.lib.delete.utils import get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader +from samcli.lib.package.artifact_exporter import mktempfile, get_cf_template_name # from samcli.yamlhelper import yaml_parse @@ -21,14 +21,20 @@ # from samcli.lib.package.ecr_uploader import ECRUploader # from samcli.lib.package.uploaders import Uploaders +CONFIG_COMMAND = "deploy" +CONFIG_SECTION = "parameters" +TEMPLATE_STAGE = "Original" + class DeleteContext: - def __init__(self, stack_name, region, s3_bucket, s3_prefix, profile): + def __init__(self, stack_name, region, profile, config_file, config_env): self.stack_name = stack_name self.region = region self.profile = profile - self.s3_bucket = s3_bucket - self.s3_prefix = s3_prefix + self.config_file = config_file + self.config_env = config_env + self.s3_bucket = None # s3_bucket + self.s3_prefix = None # s3_prefix self.cf_utils = None self.start_bold = "\033[1m" self.end_bold = "\033[0m" @@ -39,15 +45,7 @@ def __init__(self, stack_name, region, s3_bucket, s3_prefix, profile): self.delete_cf_template_file = None def __enter__(self): - return self - - def __exit__(self, *args): - pass - - def run(self): - """ - Delete the stack based on the argument provided by customers and samconfig.toml. - """ + self.parse_config_file() if not self.stack_name: self.stack_name = prompt( f"\t{self.start_bold}Enter stack name you want to delete{self.end_bold}", type=click.STRING @@ -57,8 +55,82 @@ def run(self): self.region = prompt( f"\t{self.start_bold}Enter region you want to delete from{self.end_bold}", type=click.STRING ) + return self + + def __exit__(self, *args): + pass + + def parse_config_file(self): + """ + Read the provided config file if it exists and assign the options values. + """ + toml_provider = TomlProvider(CONFIG_SECTION, [CONFIG_COMMAND]) + config_options = toml_provider( + config_path=self.config_file, config_env=self.config_env, cmd_names=[CONFIG_COMMAND] + ) + if config_options: + if not self.stack_name: + self.stack_name = config_options.get("stack_name", None) + if self.stack_name == config_options["stack_name"]: + if not self.region: + self.region = config_options.get("region", None) + if not self.profile: + self.profile = config_options.get("profile", None) + self.s3_bucket = config_options.get("s3_bucket", None) + self.s3_prefix = config_options.get("s3_prefix", None) + + def delete(self): + """ + Delete method calls for Cloudformation stacks and S3 and ECR artifacts + """ + template_str = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) + + # template_dict = yaml_parse(template_str) + + if self.s3_bucket and self.s3_prefix: + self.delete_artifacts_folder = confirm( + f"\t{self.start_bold}Are you sure you want to delete the folder" + + f" {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + default=False, + ) + if not self.delete_artifacts_folder: + with mktempfile() as temp_file: + self.cf_template_file_name = get_cf_template_name(temp_file, template_str, "template") + self.delete_cf_template_file = confirm( + f"\t{self.start_bold}Do you want to delete the template file" + + f" {self.cf_template_file_name} in S3?{self.end_bold}", + default=False, + ) + + click.echo("\n") + # Delete the primary stack + self.cf_utils.delete_stack(self.stack_name) + + click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) + + # Delete the artifacts + # Intentionally commented + # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) + # template = Template(None, None, self.uploaders, None) + # template.delete(template_dict) + + # Delete the CF template file in S3 + if self.delete_cf_template_file: + self.s3_uploader.delete_artifact(self.cf_template_file_name) + + # Delete the folder of artifacts if s3_bucket and s3_prefix provided + elif self.delete_artifacts_folder: + self.s3_uploader.delete_prefix_artifacts() + + # Delete the ECR companion stack + + def run(self): + """ + Delete the stack based on the argument provided by customers and samconfig.toml. + """ delete_stack = confirm( - f"\t{self.start_bold}Are you sure you want to delete the stack {self.stack_name}?{self.end_bold}", + f"\t{self.start_bold}Are you sure you want to delete the stack {self.stack_name}" + + f" in the region {self.region} ?{self.end_bold}", default=False, ) # Fetch the template using the stack-name @@ -83,48 +155,8 @@ def run(self): is_deployed = self.cf_utils.has_stack(self.stack_name) if is_deployed: - template_str = self.cf_utils.get_stack_template(self.stack_name, "Original") - - # template_dict = yaml_parse(template_str) - - if self.s3_bucket and self.s3_prefix: - self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder" - + f"{self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", - default=False, - ) - if not self.delete_artifacts_folder: - self.cf_template_file_name = get_cf_template_name(template_str, "template") - self.delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file" - + f" {self.cf_template_file_name} in S3?{self.end_bold}", - default=False, - ) - - click.echo("\n") - # Delete the primary stack - self.cf_utils.delete_stack(self.stack_name) - - click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) - - # Delete the artifacts - # Intentionally commented - # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) - # template = Template(None, None, self.uploaders, None) - # template.delete(template_dict) - - # Delete the CF template file in S3 - if self.delete_cf_template_file: - self.s3_uploader.delete_artifact(self.cf_template_file_name) - - # Delete the folder of artifacts if s3_bucket and s3_prefix provided - elif self.delete_artifacts_folder: - self.s3_uploader.delete_prefix_artifacts() - - # Delete the ECR companion stack + self.delete() - if self.cf_template_file_name: - click.echo(f"- deleting template file {self.cf_template_file_name}") click.echo("\n") click.echo("delete complete") else: diff --git a/samcli/lib/delete/utils.py b/samcli/lib/delete/utils.py deleted file mode 100644 index 497610f7760..00000000000 --- a/samcli/lib/delete/utils.py +++ /dev/null @@ -1,17 +0,0 @@ -""" -Utilities for Delete -""" - -from samcli.lib.utils.hash import file_checksum -from samcli.lib.package.artifact_exporter import mktempfile - - -def get_cf_template_name(template_str, extension): - with mktempfile() as temp_file: - temp_file.write(template_str) - temp_file.flush() - - filemd5 = file_checksum(temp_file.name) - remote_path = filemd5 + "." + extension - - return remote_path diff --git a/samcli/lib/deploy/deployer.py b/samcli/lib/deploy/deployer.py index 8aae03425e4..eeed0fd3219 100644 --- a/samcli/lib/deploy/deployer.py +++ b/samcli/lib/deploy/deployer.py @@ -34,7 +34,7 @@ ) from samcli.commands._utils.table_print import pprint_column_names, pprint_columns, newline_per_item, MIN_OFFSET from samcli.commands.deploy import exceptions as deploy_exceptions -from samcli.lib.package.artifact_exporter import mktempfile +from samcli.lib.package.artifact_exporter import mktempfile, get_cf_template_name from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.utils.time import utc_to_timestamp @@ -174,12 +174,11 @@ def create_changeset( # TemplateBody. This is required for large templates. if s3_uploader: with mktempfile() as temporary_file: - temporary_file.write(kwargs.pop("TemplateBody")) - temporary_file.flush() + remote_path = get_cf_template_name(temporary_file, kwargs.pop("TemplateBody"), "template") # TemplateUrl property requires S3 URL to be in path-style format parts = S3Uploader.parse_s3_url( - s3_uploader.upload_with_dedup(temporary_file.name, "template"), version_property="Version" + s3_uploader.upload(temporary_file.name, remote_path), version_property="Version" ) kwargs["TemplateURL"] = s3_uploader.to_path_style_s3_url(parts["Key"], parts.get("Version", None)) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index c0f2b945769..85b5792ef97 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -42,6 +42,7 @@ is_local_file, mktempfile, is_s3_url, + get_cf_template_name, ) from samcli.lib.utils.packagetype import ZIP from samcli.yamlhelper import yaml_parse, yaml_dump @@ -83,10 +84,9 @@ def do_export(self, resource_id, resource_dict, parent_dir): exported_template_str = yaml_dump(exported_template_dict) with mktempfile() as temporary_file: - temporary_file.write(exported_template_str) - temporary_file.flush() - url = self.uploader.upload_with_dedup(temporary_file.name, "template") + remote_path = get_cf_template_name(temporary_file, exported_template_str, "template") + url = self.uploader.upload(temporary_file.name, remote_path) # TemplateUrl property requires S3 URL to be in path-style format parts = S3Uploader.parse_s3_url(url, version_property="Version") diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index 6317c35a486..a8315188053 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -19,7 +19,7 @@ from samcli.commands.package.exceptions import ImageNotFoundError from samcli.lib.package.ecr_utils import is_ecr_url from samcli.lib.package.s3_uploader import S3Uploader -from samcli.lib.utils.hash import dir_checksum +from samcli.lib.utils.hash import dir_checksum, file_checksum LOG = logging.getLogger(__name__) @@ -284,3 +284,13 @@ def copy_to_temp_dir(filepath): dst = os.path.join(tmp_dir, os.path.basename(filepath)) shutil.copyfile(filepath, dst) return tmp_dir + + +def get_cf_template_name(temp_file, template_str, extension): + temp_file.write(template_str) + temp_file.flush() + + filemd5 = file_checksum(temp_file.name) + remote_path = filemd5 + "." + extension + + return remote_path diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py index a199c4e9600..4e268688ee8 100644 --- a/tests/unit/commands/delete/test_command.py +++ b/tests/unit/commands/delete/test_command.py @@ -36,15 +36,17 @@ def test_all_args(self, mock_delete_context, mock_delete_click): do_cli( stack_name=self.stack_name, region=self.region, + config_file=self.config_file, + config_env=self.config_env, profile=self.profile, ) mock_delete_context.assert_called_with( stack_name=self.stack_name, - s3_bucket=mock_delete_click.get_current_context().default_map.get("s3_bucket", None), - s3_prefix=mock_delete_click.get_current_context().default_map.get("s3_prefix", None), region=self.region, profile=self.profile, + config_file=self.config_file, + config_env=self.config_env, ) context_mock.run.assert_called_with() diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 7cc20f6be7e..6c85108c8eb 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -778,7 +778,7 @@ def test_export_cloudformation_stack(self, TemplateMock): TemplateMock.return_value = template_instance_mock template_instance_mock.export.return_value = exported_template_dict - self.s3_uploader_mock.upload_with_dedup.return_value = result_s3_url + self.s3_uploader_mock.upload.return_value = result_s3_url self.s3_uploader_mock.to_path_style_s3_url.return_value = result_path_style_s3_url with tempfile.NamedTemporaryFile() as handle: @@ -792,7 +792,7 @@ def test_export_cloudformation_stack(self, TemplateMock): TemplateMock.assert_called_once_with(template_path, parent_dir, self.uploaders_mock, self.code_signer_mock) template_instance_mock.export.assert_called_once_with() - self.s3_uploader_mock.upload_with_dedup.assert_called_once_with(mock.ANY, "template") + self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, "721aad13918f292d25bc9dc7d61b0e9c.template") self.s3_uploader_mock.to_path_style_s3_url.assert_called_once_with("world", None) def test_export_cloudformation_stack_no_upload_path_is_s3url(self): @@ -805,7 +805,7 @@ def test_export_cloudformation_stack_no_upload_path_is_s3url(self): # Case 1: Path is already S3 url stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], s3_url) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_cloudformation_stack_no_upload_path_is_httpsurl(self): stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) @@ -817,7 +817,7 @@ def test_export_cloudformation_stack_no_upload_path_is_httpsurl(self): # Case 1: Path is already S3 url stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], s3_url) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_cloudformation_stack_no_upload_path_is_s3_region_httpsurl(self): stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) @@ -829,7 +829,7 @@ def test_export_cloudformation_stack_no_upload_path_is_s3_region_httpsurl(self): stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], s3_url) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_cloudformation_stack_no_upload_path_is_empty(self): stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) @@ -842,7 +842,7 @@ def test_export_cloudformation_stack_no_upload_path_is_empty(self): resource_dict = {} stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict, {}) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_cloudformation_stack_no_upload_path_not_file(self): stack_resource = CloudFormationStackResource(self.uploaders_mock, self.code_signer_mock) @@ -855,7 +855,7 @@ def test_export_cloudformation_stack_no_upload_path_not_file(self): resource_dict = {property_name: dirname} with self.assertRaises(exceptions.ExportFailedError): stack_resource.export(resource_id, resource_dict, "dir") - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() @patch("samcli.lib.package.artifact_exporter.Template") def test_export_serverless_application(self, TemplateMock): @@ -871,7 +871,7 @@ def test_export_serverless_application(self, TemplateMock): TemplateMock.return_value = template_instance_mock template_instance_mock.export.return_value = exported_template_dict - self.s3_uploader_mock.upload_with_dedup.return_value = result_s3_url + self.s3_uploader_mock.upload.return_value = result_s3_url self.s3_uploader_mock.to_path_style_s3_url.return_value = result_path_style_s3_url with tempfile.NamedTemporaryFile() as handle: @@ -885,7 +885,7 @@ def test_export_serverless_application(self, TemplateMock): TemplateMock.assert_called_once_with(template_path, parent_dir, self.uploaders_mock, self.code_signer_mock) template_instance_mock.export.assert_called_once_with() - self.s3_uploader_mock.upload_with_dedup.assert_called_once_with(mock.ANY, "template") + self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, "721aad13918f292d25bc9dc7d61b0e9c.template") self.s3_uploader_mock.to_path_style_s3_url.assert_called_once_with("world", None) def test_export_serverless_application_no_upload_path_is_s3url(self): @@ -898,7 +898,7 @@ def test_export_serverless_application_no_upload_path_is_s3url(self): # Case 1: Path is already S3 url stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], s3_url) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_serverless_application_no_upload_path_is_httpsurl(self): stack_resource = ServerlessApplicationResource(self.uploaders_mock, self.code_signer_mock) @@ -910,7 +910,7 @@ def test_export_serverless_application_no_upload_path_is_httpsurl(self): # Case 1: Path is already S3 url stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], s3_url) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_serverless_application_no_upload_path_is_empty(self): stack_resource = ServerlessApplicationResource(self.uploaders_mock, self.code_signer_mock) @@ -921,7 +921,7 @@ def test_export_serverless_application_no_upload_path_is_empty(self): resource_dict = {} stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict, {}) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_serverless_application_no_upload_path_not_file(self): stack_resource = ServerlessApplicationResource(self.uploaders_mock, self.code_signer_mock) @@ -933,7 +933,7 @@ def test_export_serverless_application_no_upload_path_not_file(self): resource_dict = {property_name: dirname} with self.assertRaises(exceptions.ExportFailedError): stack_resource.export(resource_id, resource_dict, "dir") - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() def test_export_serverless_application_no_upload_path_is_dictionary(self): stack_resource = ServerlessApplicationResource(self.uploaders_mock, self.code_signer_mock) @@ -945,7 +945,7 @@ def test_export_serverless_application_no_upload_path_is_dictionary(self): resource_dict = {property_name: location} stack_resource.export(resource_id, resource_dict, "dir") self.assertEqual(resource_dict[property_name], location) - self.s3_uploader_mock.upload_with_dedup.assert_not_called() + self.s3_uploader_mock.upload.assert_not_called() @patch("samcli.lib.package.artifact_exporter.yaml_parse") def test_template_export_metadata(self, yaml_parse_mock): From c2e43db315aa292db027ba54f7a3f2fd006eb50e Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Tue, 29 Jun 2021 13:37:40 -0400 Subject: [PATCH 12/37] Fixed the unit tests in artifact_exporter.py --- tests/unit/lib/package/test_artifact_exporter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 6c85108c8eb..f7aceafef14 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -792,7 +792,7 @@ def test_export_cloudformation_stack(self, TemplateMock): TemplateMock.assert_called_once_with(template_path, parent_dir, self.uploaders_mock, self.code_signer_mock) template_instance_mock.export.assert_called_once_with() - self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, "721aad13918f292d25bc9dc7d61b0e9c.template") + self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) self.s3_uploader_mock.to_path_style_s3_url.assert_called_once_with("world", None) def test_export_cloudformation_stack_no_upload_path_is_s3url(self): @@ -885,7 +885,7 @@ def test_export_serverless_application(self, TemplateMock): TemplateMock.assert_called_once_with(template_path, parent_dir, self.uploaders_mock, self.code_signer_mock) template_instance_mock.export.assert_called_once_with() - self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, "721aad13918f292d25bc9dc7d61b0e9c.template") + self.s3_uploader_mock.upload.assert_called_once_with(mock.ANY, mock.ANY) self.s3_uploader_mock.to_path_style_s3_url.assert_called_once_with("world", None) def test_export_serverless_application_no_upload_path_is_s3url(self): From dd35d532d6a2f48fa007bd2907a337924262de3d Mon Sep 17 00:00:00 2001 From: hnnasit <84355507+hnnasit@users.noreply.github.com> Date: Tue, 29 Jun 2021 14:09:54 -0400 Subject: [PATCH 13/37] Update HELP_TEXT in delete/command.py Co-authored-by: Chris Rehn --- samcli/commands/delete/command.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index d95382bb51b..c0310bfa6ed 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -11,8 +11,8 @@ SHORT_HELP = "Delete an AWS SAM application and the artifacts created by sam deploy." -HELP_TEXT = """The sam delete command deletes the Cloudformation -Stack and all the artifacts which were created using sam deploy. +HELP_TEXT = """The sam delete command deletes the CloudFormation +stack and all the artifacts which were created using sam deploy. \b e.g. sam delete From f43e763e1a382ee946bf8fba45075c00326edc8c Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 30 Jun 2021 12:17:26 -0400 Subject: [PATCH 14/37] Updated code based on Chris' comments --- samcli/commands/delete/command.py | 12 +++-- samcli/commands/delete/delete_context.py | 66 ++++++++---------------- samcli/commands/delete/exceptions.py | 10 ++++ samcli/lib/delete/cf_utils.py | 19 ++++--- samcli/lib/package/s3_uploader.py | 4 +- tests/unit/lib/delete/test_cf_utils.py | 6 +-- 6 files changed, 55 insertions(+), 62 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index c0310bfa6ed..823e262bc6d 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -64,19 +64,21 @@ @print_cmdline_args def cli( ctx, - stack_name, - config_file, - config_env, + stack_name: str, + config_file: str, + config_env: str, ): """ `sam delete` command entry point """ # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing - do_cli(stack_name, ctx.region, config_file, config_env, ctx.profile) # pragma: no cover + do_cli( + stack_name=stack_name, region=ctx.region, config_file=config_file, config_env=config_env, profile=ctx.profile + ) # pragma: no cover -def do_cli(stack_name, region, config_file, config_env, profile): +def do_cli(stack_name: str, region: str, config_file: str, config_env: str, profile: str): """ Implementation of the ``cli`` method """ diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 95c62d4dd1d..a6d146d584d 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -4,7 +4,6 @@ import boto3 -# import docker import click from click import confirm from click import prompt @@ -14,32 +13,22 @@ from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.artifact_exporter import mktempfile, get_cf_template_name -# from samcli.yamlhelper import yaml_parse - -# Intentionally commented -# from samcli.lib.package.artifact_exporter import Template -# from samcli.lib.package.ecr_uploader import ECRUploader -# from samcli.lib.package.uploaders import Uploaders - CONFIG_COMMAND = "deploy" CONFIG_SECTION = "parameters" TEMPLATE_STAGE = "Original" class DeleteContext: - def __init__(self, stack_name, region, profile, config_file, config_env): + def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str): self.stack_name = stack_name self.region = region self.profile = profile self.config_file = config_file self.config_env = config_env - self.s3_bucket = None # s3_bucket - self.s3_prefix = None # s3_prefix + self.s3_bucket = None + self.s3_prefix = None self.cf_utils = None - self.start_bold = "\033[1m" - self.end_bold = "\033[0m" self.s3_uploader = None - # self.uploaders = None self.cf_template_file_name = None self.delete_artifacts_folder = None self.delete_cf_template_file = None @@ -48,13 +37,11 @@ def __enter__(self): self.parse_config_file() if not self.stack_name: self.stack_name = prompt( - f"\t{self.start_bold}Enter stack name you want to delete{self.end_bold}", type=click.STRING + click.style("\tEnter stack name you want to delete:", bold=True), type=click.STRING ) if not self.region: - self.region = prompt( - f"\t{self.start_bold}Enter region you want to delete from{self.end_bold}", type=click.STRING - ) + self.region = prompt(click.style("\tEnter region you want to delete from:", bold=True), type=click.STRING) return self def __exit__(self, *args): @@ -85,52 +72,48 @@ def delete(self): """ template_str = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) - # template_dict = yaml_parse(template_str) - if self.s3_bucket and self.s3_prefix: self.delete_artifacts_folder = confirm( - f"\t{self.start_bold}Are you sure you want to delete the folder" - + f" {self.s3_prefix} in S3 which contains the artifacts?{self.end_bold}", + click.style( + "\tAre you sure you want to delete the folder" + + f" {self.s3_prefix} in S3 which contains the artifacts?", + bold=True, + ), default=False, ) if not self.delete_artifacts_folder: with mktempfile() as temp_file: self.cf_template_file_name = get_cf_template_name(temp_file, template_str, "template") self.delete_cf_template_file = confirm( - f"\t{self.start_bold}Do you want to delete the template file" - + f" {self.cf_template_file_name} in S3?{self.end_bold}", + click.style( + "\tDo you want to delete the template file" + f" {self.cf_template_file_name} in S3?", bold=True + ), default=False, ) click.echo("\n") # Delete the primary stack - self.cf_utils.delete_stack(self.stack_name) + self.cf_utils.delete_stack(stack_name=self.stack_name) - click.echo("- deleting Cloudformation stack {0}".format(self.stack_name)) - - # Delete the artifacts - # Intentionally commented - # self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) - # template = Template(None, None, self.uploaders, None) - # template.delete(template_dict) + click.echo(f"- deleting Cloudformation stack {self.stack_name}") # Delete the CF template file in S3 if self.delete_cf_template_file: - self.s3_uploader.delete_artifact(self.cf_template_file_name) + self.s3_uploader.delete_artifact(remote_path=self.cf_template_file_name) # Delete the folder of artifacts if s3_bucket and s3_prefix provided elif self.delete_artifacts_folder: self.s3_uploader.delete_prefix_artifacts() - # Delete the ECR companion stack - def run(self): """ Delete the stack based on the argument provided by customers and samconfig.toml. """ delete_stack = confirm( - f"\t{self.start_bold}Are you sure you want to delete the stack {self.stack_name}" - + f" in the region {self.region} ?{self.end_bold}", + click.style( + f"\tAre you sure you want to delete the stack {self.stack_name}" + f" in the region {self.region} ?", + bold=True, + ), default=False, ) # Fetch the template using the stack-name @@ -143,16 +126,11 @@ def run(self): ) s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) - # ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - - # docker_client = docker.from_env() - # ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) - self.cf_utils = CfUtils(cloudformation_client) - is_deployed = self.cf_utils.has_stack(self.stack_name) + is_deployed = self.cf_utils.has_stack(stack_name=self.stack_name) if is_deployed: self.delete() @@ -160,4 +138,4 @@ def run(self): click.echo("\n") click.echo("delete complete") else: - click.echo("Error: The input stack {0} does not exist on Cloudformation".format(self.stack_name)) + click.echo(f"Error: The input stack {self.stack_name} does not exist on Cloudformation") diff --git a/samcli/commands/delete/exceptions.py b/samcli/commands/delete/exceptions.py index 82c56b6bb67..7e2ba5105c1 100644 --- a/samcli/commands/delete/exceptions.py +++ b/samcli/commands/delete/exceptions.py @@ -12,3 +12,13 @@ def __init__(self, stack_name, msg): message_fmt = "Failed to delete the stack: {stack_name}, {msg}" super().__init__(message=message_fmt.format(stack_name=self.stack_name, msg=msg)) + + +class FetchTemplateFailedError(UserException): + def __init__(self, stack_name, msg): + self.stack_name = stack_name + self.msg = msg + + message_fmt = "Failed to fetch the template for the stack: {stack_name}, {msg}" + + super().__init__(message=message_fmt.format(stack_name=self.stack_name, msg=msg)) diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index db36f11ce34..8644a514457 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -5,7 +5,7 @@ import logging from botocore.exceptions import ClientError, BotoCoreError -from samcli.commands.delete.exceptions import DeleteFailedError +from samcli.commands.delete.exceptions import DeleteFailedError, FetchTemplateFailedError LOG = logging.getLogger(__name__) @@ -14,7 +14,7 @@ class CfUtils: def __init__(self, cloudformation_client): self._client = cloudformation_client - def has_stack(self, stack_name): + def has_stack(self, stack_name: str): """ Checks if a CloudFormation stack with given name exists @@ -27,6 +27,10 @@ def has_stack(self, stack_name): return False stack = resp["Stacks"][0] + # Note: Stacks with REVIEW_IN_PROGRESS can be deleted + # using delete_stack but get_template does not return + # the template_str for this stack restricting deletion of + # artifacts. return stack["StackStatus"] != "REVIEW_IN_PROGRESS" except ClientError as e: @@ -51,7 +55,7 @@ def has_stack(self, stack_name): LOG.error("Unable to get stack details.", exc_info=e) raise e - def get_stack_template(self, stack_name, stage): + def get_stack_template(self, stack_name: str, stage: str): """ Return the Cloudformation template of the given stack_name @@ -62,23 +66,22 @@ def get_stack_template(self, stack_name, stage): try: resp = self._client.get_template(StackName=stack_name, TemplateStage=stage) if not resp["TemplateBody"]: - return None - + return "" return resp["TemplateBody"] except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, # catch that and throw a delete failed error. - LOG.error("Failed to delete stack : %s", str(e)) - raise DeleteFailedError(stack_name=stack_name, msg=str(e)) from e + LOG.error("Failed to fetch template for the stack : %s", str(e)) + raise FetchTemplateFailedError(stack_name=stack_name, msg=str(e)) from e except Exception as e: # We don't know anything about this exception. Don't handle LOG.error("Unable to get stack details.", exc_info=e) raise e - def delete_stack(self, stack_name): + def delete_stack(self, stack_name: str): """ Delete the Cloudformation stack with the given stack_name diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 3fb7070d2be..c9a5e3f6f00 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -145,7 +145,7 @@ def upload_with_dedup( return self.upload(file_name, remote_path) - def delete_artifact(self, remote_path: str, is_key=False): + def delete_artifact(self, remote_path: str, is_key: Optional[bool] = False): """ Deletes a given file from S3 :param remote_path: Path to the file that will be deleted @@ -161,7 +161,7 @@ def delete_artifact(self, remote_path: str, is_key=False): key = "{0}/{1}".format(self.prefix, remote_path) # Deleting Specific file with key - click.echo("- deleting S3 file " + key) + click.echo(f"- deleting S3 file {key}") resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) LOG.debug("S3 method delete_object is called and returned: %s", resp["ResponseMetadata"]) return resp["ResponseMetadata"] diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py index 36f32ae7350..9e80a00d4a1 100644 --- a/tests/unit/lib/delete/test_cf_utils.py +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -1,7 +1,7 @@ from unittest.mock import patch, MagicMock, ANY, call from unittest import TestCase -from samcli.commands.delete.exceptions import DeleteFailedError +from samcli.commands.delete.exceptions import DeleteFailedError, FetchTemplateFailedError from botocore.exceptions import ClientError, BotoCoreError from samcli.lib.delete.cf_utils import CfUtils @@ -62,12 +62,12 @@ def test_cf_utils_get_stack_template_exception_client_error(self): operation_name="stack_status", ) ) - with self.assertRaises(DeleteFailedError): + with self.assertRaises(FetchTemplateFailedError): self.cf_utils.get_stack_template("test", "Original") def test_cf_utils_get_stack_template_exception_botocore(self): self.cf_utils._client.get_template = MagicMock(side_effect=BotoCoreError()) - with self.assertRaises(DeleteFailedError): + with self.assertRaises(FetchTemplateFailedError): self.cf_utils.get_stack_template("test", "Original") def test_cf_utils_get_stack_template_exception(self): From 5779cd34c91b9f1aa1540362fab38da144e5f4e7 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 30 Jun 2021 14:57:29 -0400 Subject: [PATCH 15/37] Added condition for resources that have deletionpolicy specified --- samcli/lib/package/artifact_exporter.py | 19 ++++++++++--------- samcli/lib/package/packageable_resources.py | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index d0372730b6f..a1b2a98fe43 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -249,13 +249,14 @@ def delete(self, template_dict): resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", {}) - - for exporter_class in self.resources_to_export: - if exporter_class.RESOURCE_TYPE != resource_type: - continue - if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: - continue - # Delete code resources - exporter = exporter_class(self.uploaders, None) - exporter.delete(resource_id, resource_dict) + resource_deletion_policy = resource.get("DeletionPolicy", None) + if resource_deletion_policy != "Retain": + for exporter_class in self.resources_to_export: + if exporter_class.RESOURCE_TYPE != resource_type: + continue + if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: + continue + # Delete code resources + exporter = exporter_class(self.uploaders, None) + exporter.delete(resource_id, resource_dict) return self.template_dict diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 1169e23ebbb..aabe675e265 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -22,6 +22,7 @@ upload_local_image_artifacts, is_s3_protocol_url, is_path_value_valid, + is_ecr_url ) from samcli.commands._utils.resources import ( @@ -210,6 +211,14 @@ def do_export(self, resource_id, resource_dict, parent_dir): ) set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, {self.EXPORT_PROPERTY_CODE_KEY: uploaded_url}) + def delete(self, resource_id, resource_dict): + if resource_dict is None: + return + + remote_path = resource_dict[self.PROPERTY_NAME][self.EXPORT_PROPERTY_CODE_KEY] + if is_ecr_url(remote_path): + self.uploader.delete_artifact(remote_path, resource_id, self.PROPERTY_NAME) + class ResourceImage(Resource): """ @@ -252,7 +261,12 @@ def do_export(self, resource_id, resource_dict, parent_dir): set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) def delete(self, resource_id, resource_dict): - self.uploader.delete_artifact(resource_dict["ImageUri"], resource_id, self.PROPERTY_NAME) + if resource_dict is None: + return + + remote_path = resource_dict[self.PROPERTY_NAME] + if is_ecr_url(remote_path): + self.uploader.delete_artifact(remote_path, resource_id, self.PROPERTY_NAME) class ResourceWithS3UrlDict(ResourceZip): """ @@ -290,7 +304,7 @@ def delete(self, resource_id, resource_dict): return resource_path = resource_dict[self.PROPERTY_NAME] s3_bucket = resource_path[self.BUCKET_NAME_PROPERTY] - key = resource_path["Key"] + key = resource_path[self.OBJECT_KEY_PROPERTY] if not self.uploader.bucket_name: self.uploader.bucket_name = s3_bucket From bc9db140b2a84b7c4457a8eafc63eb9c23526fca Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 30 Jun 2021 16:18:58 -0400 Subject: [PATCH 16/37] Small changes and fixes based on the comments --- samcli/commands/delete/command.py | 4 ++-- samcli/commands/delete/delete_context.py | 12 +++++------- samcli/lib/delete/cf_utils.py | 15 +++++++-------- samcli/lib/package/s3_uploader.py | 8 +++++--- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 823e262bc6d..266d093a36b 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -36,7 +36,6 @@ ) @click.option( "--config-file", - required=False, help=( "The path and file name of the configuration file containing default parameter values to use. " "Its default value is 'samconfig.toml' in project directory. For more information about configuration files, " @@ -45,10 +44,10 @@ ), type=click.STRING, default="samconfig.toml", + show_default=True, ) @click.option( "--config-env", - required=False, help=( "The environment name specifying the default parameter values in the configuration file to use. " "Its default value is 'default'. For more information about configuration files, see: " @@ -56,6 +55,7 @@ ), type=click.STRING, default="default", + show_default=True, ) @aws_creds_options @common_options diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index a6d146d584d..8f12402cde6 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -70,9 +70,10 @@ def delete(self): """ Delete method calls for Cloudformation stacks and S3 and ECR artifacts """ - template_str = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) + template = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) + template_str = template.get("TemplateBody", None) - if self.s3_bucket and self.s3_prefix: + if self.s3_bucket and self.s3_prefix and template_str: self.delete_artifacts_folder = confirm( click.style( "\tAre you sure you want to delete the folder" @@ -91,11 +92,10 @@ def delete(self): default=False, ) - click.echo("\n") # Delete the primary stack self.cf_utils.delete_stack(stack_name=self.stack_name) - click.echo(f"- deleting Cloudformation stack {self.stack_name}") + click.echo(f"\n\t- Deleting Cloudformation stack {self.stack_name}") # Delete the CF template file in S3 if self.delete_cf_template_file: @@ -134,8 +134,6 @@ def run(self): if is_deployed: self.delete() - - click.echo("\n") - click.echo("delete complete") + click.echo("\nDeleted successfully") else: click.echo(f"Error: The input stack {self.stack_name} does not exist on Cloudformation") diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index 8644a514457..a78ed6d38ba 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -4,6 +4,7 @@ import logging +from typing import Dict from botocore.exceptions import ClientError, BotoCoreError from samcli.commands.delete.exceptions import DeleteFailedError, FetchTemplateFailedError @@ -14,7 +15,7 @@ class CfUtils: def __init__(self, cloudformation_client): self._client = cloudformation_client - def has_stack(self, stack_name: str): + def has_stack(self, stack_name: str) -> bool: """ Checks if a CloudFormation stack with given name exists @@ -31,7 +32,7 @@ def has_stack(self, stack_name: str): # using delete_stack but get_template does not return # the template_str for this stack restricting deletion of # artifacts. - return stack["StackStatus"] != "REVIEW_IN_PROGRESS" + return bool(stack["StackStatus"] != "REVIEW_IN_PROGRESS") except ClientError as e: # If a stack does not exist, describe_stacks will throw an @@ -55,7 +56,7 @@ def has_stack(self, stack_name: str): LOG.error("Unable to get stack details.", exc_info=e) raise e - def get_stack_template(self, stack_name: str, stage: str): + def get_stack_template(self, stack_name: str, stage: str) -> Dict: """ Return the Cloudformation template of the given stack_name @@ -66,8 +67,8 @@ def get_stack_template(self, stack_name: str, stage: str): try: resp = self._client.get_template(StackName=stack_name, TemplateStage=stage) if not resp["TemplateBody"]: - return "" - return resp["TemplateBody"] + return {} + return dict(resp) except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, @@ -86,11 +87,9 @@ def delete_stack(self, stack_name: str): Delete the Cloudformation stack with the given stack_name :param stack_name: Name or ID of the stack - :return: Status of deletion """ try: - resp = self._client.delete_stack(StackName=stack_name) - return resp + self._client.delete_stack(StackName=stack_name) except (ClientError, BotoCoreError) as e: # If there are credentials, environment errors, diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index c9a5e3f6f00..61a69884161 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -145,11 +145,13 @@ def upload_with_dedup( return self.upload(file_name, remote_path) - def delete_artifact(self, remote_path: str, is_key: Optional[bool] = False): + def delete_artifact(self, remote_path: str, is_key: bool = False) -> Dict: """ Deletes a given file from S3 :param remote_path: Path to the file that will be deleted :param is_key: If the given remote_path is the key or a file_name + + :return: metadata dict of the deleted object """ try: if not self.bucket_name: @@ -161,10 +163,10 @@ def delete_artifact(self, remote_path: str, is_key: Optional[bool] = False): key = "{0}/{1}".format(self.prefix, remote_path) # Deleting Specific file with key - click.echo(f"- deleting S3 file {key}") + click.echo(f"\t- Deleting S3 file {key}") resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) LOG.debug("S3 method delete_object is called and returned: %s", resp["ResponseMetadata"]) - return resp["ResponseMetadata"] + return dict(resp["ResponseMetadata"]) except botocore.exceptions.ClientError as ex: error_code = ex.response["Error"]["Code"] From 401a950f1b384c07892ba2d2a9ad76266e94ef46 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 30 Jun 2021 19:31:40 -0400 Subject: [PATCH 17/37] Removed region prompt --- samcli/commands/delete/delete_context.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 8f12402cde6..5a70fa9f07b 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -40,8 +40,6 @@ def __enter__(self): click.style("\tEnter stack name you want to delete:", bold=True), type=click.STRING ) - if not self.region: - self.region = prompt(click.style("\tEnter region you want to delete from:", bold=True), type=click.STRING) return self def __exit__(self, *args): From 0a38340649c3cdf21bba5ba8d7f048ff558d5edf Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Sat, 3 Jul 2021 16:59:41 -0400 Subject: [PATCH 18/37] Added unit tests for ecr delete method and typing for methods --- samcli/lib/delete/cf_utils.py | 4 +- samcli/lib/package/ecr_uploader.py | 17 ++++---- samcli/lib/package/packageable_resources.py | 28 +++++++++--- tests/unit/lib/delete/test_cf_utils.py | 5 ++- tests/unit/lib/delete/test_utils.py | 8 ---- tests/unit/lib/package/test_ecr_uploader.py | 47 ++++++++++++++++++++- 6 files changed, 84 insertions(+), 25 deletions(-) delete mode 100644 tests/unit/lib/delete/test_utils.py diff --git a/samcli/lib/delete/cf_utils.py b/samcli/lib/delete/cf_utils.py index 7d8be75601f..8e257a0cd9c 100644 --- a/samcli/lib/delete/cf_utils.py +++ b/samcli/lib/delete/cf_utils.py @@ -119,4 +119,6 @@ def wait_for_delete(self, stack_name): status = resp["Status"] reason = resp["StatusReason"] - raise DeleteFailedError(stack_name=stack_name, msg="ex: {0} Status: {1}. Reason: {2}".format(ex, status, reason)) from ex + raise DeleteFailedError( + stack_name=stack_name, msg="ex: {0} Status: {1}. Reason: {2}".format(ex, status, reason) + ) from ex diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index 4f8b0246d0b..402ebbf4cf6 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -16,7 +16,7 @@ DockerLoginFailedError, ECRAuthorizationError, ImageNotFoundError, - DeleteArtifactFailedError + DeleteArtifactFailedError, ) from samcli.lib.package.image_utils import tag_translation from samcli.lib.package.stream_cursor_utils import cursor_up, cursor_left, cursor_down, clear_line @@ -90,27 +90,28 @@ def upload(self, image, resource_name): return f"{repository}:{_tag}" - def delete_artifact(self, image_uri, resource_id, property_name): + def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): try: repo_image_tag = image_uri.split("/")[1].split(":") repository = repo_image_tag[0] image_tag = repo_image_tag[1] - resp = self.ecr_client.batch_delete_image(repositoryName=repository, + resp = self.ecr_client.batch_delete_image( + repositoryName=repository, imageIds=[ - { - 'imageTag': image_tag - }, - ] + {"imageTag": image_tag}, + ], ) if resp["failures"]: + # Image not found image_details = resp["failures"][0] if image_details["failureCode"] == "ImageNotFound": LOG.debug("ImageNotFound Exception : ") raise ImageNotFoundError(resource_id, property_name) - click.echo("- deleting ECR image {0} in repository {1}".format(image_tag, repository)) + click.echo(f"- Deleting ECR image {image_tag} in repository {repository}") except botocore.exceptions.ClientError as ex: + # Handle Client errors such as RepositoryNotFoundException or InvalidParameterException raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex # TODO: move this to a generic class to allow for streaming logs back from docker. diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index aabe675e265..cc2fe999d9b 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -22,7 +22,7 @@ upload_local_image_artifacts, is_s3_protocol_url, is_path_value_valid, - is_ecr_url + is_ecr_url, ) from samcli.commands._utils.resources import ( @@ -159,16 +159,18 @@ def do_export(self, resource_id, resource_dict, parent_dir): set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) def delete(self, resource_id, resource_dict): - + """ + Delete the S3 artifact using S3 url referenced by PROPERTY_NAME + """ if resource_dict is None: return resource_path = resource_dict[self.PROPERTY_NAME] parsed_s3_url = self.uploader.parse_s3_url(resource_path) - print(parsed_s3_url["Key"]) if not self.uploader.bucket_name: self.uploader.bucket_name = parsed_s3_url["Bucket"] self.uploader.delete_artifact(parsed_s3_url["Key"], True) + class ResourceImageDict(Resource): """ Base class representing a CFN Image based resource that can be exported. @@ -212,12 +214,17 @@ def do_export(self, resource_id, resource_dict, parent_dir): set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, {self.EXPORT_PROPERTY_CODE_KEY: uploaded_url}) def delete(self, resource_id, resource_dict): + """ + Delete the ECR artifact using ECR url in PROPERTY_NAME referenced by EXPORT_PROPERTY_CODE_KEY + """ if resource_dict is None: return remote_path = resource_dict[self.PROPERTY_NAME][self.EXPORT_PROPERTY_CODE_KEY] if is_ecr_url(remote_path): - self.uploader.delete_artifact(remote_path, resource_id, self.PROPERTY_NAME) + self.uploader.delete_artifact( + image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME + ) class ResourceImage(Resource): @@ -261,12 +268,18 @@ def do_export(self, resource_id, resource_dict, parent_dir): set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, uploaded_url) def delete(self, resource_id, resource_dict): + """ + Delete the ECR artifact using ECR url referenced by property_name + """ if resource_dict is None: return remote_path = resource_dict[self.PROPERTY_NAME] if is_ecr_url(remote_path): - self.uploader.delete_artifact(remote_path, resource_id, self.PROPERTY_NAME) + self.uploader.delete_artifact( + image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME + ) + class ResourceWithS3UrlDict(ResourceZip): """ @@ -299,7 +312,10 @@ def do_export(self, resource_id, resource_dict, parent_dir): set_value_from_jmespath(resource_dict, self.PROPERTY_NAME, parsed_url) def delete(self, resource_id, resource_dict): - + """ + Delete the S3 artifact using S3 url in the dict PROPERTY_NAME + using the bucket at BUCKET_NAME_PROPERTY and key at OBJECT_KEY_PROPERTY + """ if resource_dict is None: return resource_path = resource_dict[self.PROPERTY_NAME] diff --git a/tests/unit/lib/delete/test_cf_utils.py b/tests/unit/lib/delete/test_cf_utils.py index 8e57407231e..b9bc00faba2 100644 --- a/tests/unit/lib/delete/test_cf_utils.py +++ b/tests/unit/lib/delete/test_cf_utils.py @@ -5,6 +5,7 @@ from botocore.exceptions import ClientError, BotoCoreError, WaiterError from samcli.lib.delete.cf_utils import CfUtils + class MockDeleteWaiter: def __init__(self, ex=None): self.ex = ex @@ -14,6 +15,7 @@ def wait(self, StackName, WaiterConfig): raise self.ex return + class TestCfUtils(TestCase): def setUp(self): self.session = MagicMock() @@ -101,6 +103,7 @@ def test_cf_utils_wait_for_delete_exception(self): reason="unit-test", last_response={"Status": "Failed", "StatusReason": "It's a unit test"}, ) - )) + ) + ) with self.assertRaises(DeleteFailedError): self.cf_utils.wait_for_delete("test") diff --git a/tests/unit/lib/delete/test_utils.py b/tests/unit/lib/delete/test_utils.py deleted file mode 100644 index c39f176d5c6..00000000000 --- a/tests/unit/lib/delete/test_utils.py +++ /dev/null @@ -1,8 +0,0 @@ -from unittest import TestCase - -from samcli.lib.delete.utils import get_cf_template_name - -class TestCfUtils(TestCase): - - def test_utils(self): - self.assertEqual(get_cf_template_name("hello world!", "template"), "fc3ff98e8c6a0d3087d515c0473f8677.template") \ No newline at end of file diff --git a/tests/unit/lib/package/test_ecr_uploader.py b/tests/unit/lib/package/test_ecr_uploader.py index 91798d43f9c..a66207efca2 100644 --- a/tests/unit/lib/package/test_ecr_uploader.py +++ b/tests/unit/lib/package/test_ecr_uploader.py @@ -5,7 +5,13 @@ from docker.errors import APIError, BuildError from parameterized import parameterized -from samcli.commands.package.exceptions import DockerLoginFailedError, DockerPushFailedError, ECRAuthorizationError +from samcli.commands.package.exceptions import ( + DockerLoginFailedError, + DockerPushFailedError, + ECRAuthorizationError, + ImageNotFoundError, + DeleteArtifactFailedError, +) from samcli.lib.package.ecr_uploader import ECRUploader from samcli.lib.utils.stream_writer import StreamWriter @@ -23,6 +29,9 @@ def setUp(self): BuildError.__name__: {"reason": "mock_reason", "build_log": "mock_build_log"}, APIError.__name__: {"message": "mock message"}, } + self.image_uri = "900643008914.dkr.ecr.us-east-1.amazonaws.com/" + self.ecr_repo + ":" + self.tag + self.property_name = "AWS::Serverless::Function" + self.resource_id = "HelloWorldFunction" def test_ecr_uploader_init(self): ecr_uploader = ECRUploader( @@ -166,3 +175,39 @@ def test_upload_failure_while_streaming(self): ecr_uploader.login = MagicMock() with self.assertRaises(DockerPushFailedError): ecr_uploader.upload(image, resource_name="HelloWorldFunction") + + def test_delete_artifact_no_image_error(self): + ecr_uploader = ECRUploader( + docker_client=self.docker_client, + ecr_client=self.ecr_client, + ecr_repo=self.ecr_repo, + ecr_repo_multi=self.ecr_repo_multi, + tag=self.tag, + ) + ecr_uploader.ecr_client.batch_delete_image.return_value = { + "failures": [{"imageId": {"imageTag": self.tag}, "failureCode": "ImageNotFound"}] + } + + with self.assertRaises(ImageNotFoundError): + ecr_uploader.delete_artifact( + image_uri=self.image_uri, resource_id=self.resource_id, property_name=self.property_name + ) + + def test_delete_artifact_client_error(self): + ecr_uploader = ECRUploader( + docker_client=self.docker_client, + ecr_client=self.ecr_client, + ecr_repo=self.ecr_repo, + ecr_repo_multi=self.ecr_repo_multi, + tag=self.tag, + ) + ecr_uploader.ecr_client.batch_delete_image = MagicMock( + side_effect=ClientError( + error_response={"Error": {"Message": "mock client error"}}, operation_name="batch_delete_image" + ) + ) + + with self.assertRaises(DeleteArtifactFailedError): + ecr_uploader.delete_artifact( + image_uri=self.image_uri, resource_id=self.resource_id, property_name=self.property_name + ) From aaa1b05003eebb12a28c78200bad45b8aa4469c7 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 5 Jul 2021 09:34:59 -0400 Subject: [PATCH 19/37] Reformatted delete_context and added option to skip user prompts --- samcli/commands/delete/command.py | 30 +++-- samcli/commands/delete/delete_context.py | 135 +++++++++++++-------- samcli/lib/package/ecr_uploader.py | 2 +- tests/unit/commands/delete/test_command.py | 3 + 4 files changed, 111 insertions(+), 59 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 266d093a36b..4e0b9ec6bc0 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -57,34 +57,46 @@ default="default", show_default=True, ) +@click.option( + "--force", + help=("Specify this flag to allow SAM CLI to skip through the guided prompts" ""), + is_flag=True, + type=click.BOOL, + required=False, +) @aws_creds_options @common_options @pass_context @check_newer_version @print_cmdline_args -def cli( - ctx, - stack_name: str, - config_file: str, - config_env: str, -): +def cli(ctx, stack_name: str, config_file: str, config_env: str, force: bool): """ `sam delete` command entry point """ # All logic must be implemented in the ``do_cli`` method. This helps with easy unit testing do_cli( - stack_name=stack_name, region=ctx.region, config_file=config_file, config_env=config_env, profile=ctx.profile + stack_name=stack_name, + region=ctx.region, + config_file=config_file, + config_env=config_env, + profile=ctx.profile, + force=force, ) # pragma: no cover -def do_cli(stack_name: str, region: str, config_file: str, config_env: str, profile: str): +def do_cli(stack_name: str, region: str, config_file: str, config_env: str, profile: str, force: bool): """ Implementation of the ``cli`` method """ from samcli.commands.delete.delete_context import DeleteContext with DeleteContext( - stack_name=stack_name, region=region, profile=profile, config_file=config_file, config_env=config_env + stack_name=stack_name, + region=region, + profile=profile, + config_file=config_file, + config_env=config_env, + force=force, ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 9b1d5210b5d..597f8c64e07 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -25,18 +25,21 @@ CONFIG_SECTION = "parameters" TEMPLATE_STAGE = "Original" + class DeleteContext: - def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str): + def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str, force: bool): self.stack_name = stack_name self.region = region self.profile = profile self.config_file = config_file self.config_env = config_env + self.force = force self.s3_bucket = None self.s3_prefix = None self.cf_utils = None self.s3_uploader = None self.uploaders = None + self.template = None self.cf_template_file_name = None self.delete_artifacts_folder = None self.delete_cf_template_file = None @@ -48,6 +51,7 @@ def __enter__(self): click.style("\tEnter stack name you want to delete:", bold=True), type=click.STRING ) + self.init_clients() return self def __exit__(self, *args): @@ -72,43 +76,85 @@ def parse_config_file(self): self.s3_bucket = config_options.get("s3_bucket", None) self.s3_prefix = config_options.get("s3_prefix", None) - def delete(self): + def init_clients(self): """ - Delete method calls for Cloudformation stacks and S3 and ECR artifacts + Initialize all the clients being used by sam delete. """ - template = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) - template_str = template.get("TemplateBody", None) - template_dict = yaml_parse(template_str) + boto_config = get_boto_config_with_user_agent() - if self.s3_bucket and self.s3_prefix and template_str: - self.delete_artifacts_folder = confirm( - click.style( - "\tAre you sure you want to delete the folder" - + f" {self.s3_prefix} in S3 which contains the artifacts?", - bold=True, - ), - default=False, - ) + # Define cf_client based on the region as different regions can have same stack-names + cloudformation_client = boto3.client( + "cloudformation", region_name=self.region if self.region else None, config=boto_config + ) + + s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) + ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + + self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) + + docker_client = docker.from_env() + ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + + self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) + self.cf_utils = CfUtils(cloudformation_client) + self.template = Template(None, None, self.uploaders, None) + + def guided_prompts(self): + """ + Guided prompts asking customer to delete artifacts + """ + # Note: s3_bucket and s3_prefix information is only + # available if a local toml file is present or if + # this information is obtained from the template resources and so if this + # information is not found, warn the customer that S3 artifacts + # will need to be manually deleted. + + if not self.force and self.s3_bucket: + if self.s3_prefix: + self.delete_artifacts_folder = confirm( + click.style( + "\tAre you sure you want to delete the folder" + + f" {self.s3_prefix} in S3 which contains the artifacts?", + bold=True, + ), + default=False, + ) if not self.delete_artifacts_folder: - with mktempfile() as temp_file: - self.cf_template_file_name = get_cf_template_name(temp_file, template_str, "template") self.delete_cf_template_file = confirm( click.style( "\tDo you want to delete the template file" + f" {self.cf_template_file_name} in S3?", bold=True ), default=False, ) + elif self.s3_bucket: + if self.s3_prefix: + self.delete_artifacts_folder = True + else: + self.delete_cf_template_file = True + + def delete(self): + """ + Delete method calls for Cloudformation stacks and S3 and ECR artifacts + """ + # Fetch the template using the stack-name + template = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) + template_str = template.get("TemplateBody", None) + template_dict = yaml_parse(template_str) + + # Get the cloudformation template name using template_str + with mktempfile() as temp_file: + self.cf_template_file_name = get_cf_template_name(temp_file, template_str, "template") + + self.guided_prompts() # Delete the primary stack + click.echo(f"\n\t- Deleting Cloudformation stack {self.stack_name}") self.cf_utils.delete_stack(stack_name=self.stack_name) self.cf_utils.wait_for_delete(self.stack_name) - - click.echo(f"\n\t- Deleting Cloudformation stack {self.stack_name}") - + # Delete the artifacts - template = Template(None, None, self.uploaders, None) - template.delete(template_dict) - + self.template.delete(template_dict) + # Delete the CF template file in S3 if self.delete_cf_template_file: self.s3_uploader.delete_artifact(remote_path=self.cf_template_file_name) @@ -117,39 +163,30 @@ def delete(self): elif self.delete_artifacts_folder: self.s3_uploader.delete_prefix_artifacts() + else: + click.secho( + "\nWarning: s3_bucket and s3_prefix information cannot be obtained," + " delete the files manually if required", + fg="yellow", + ) + def run(self): """ Delete the stack based on the argument provided by customers and samconfig.toml. """ - delete_stack = confirm( - click.style( - f"\tAre you sure you want to delete the stack {self.stack_name}" + f" in the region {self.region} ?", - bold=True, - ), - default=False, - ) - # Fetch the template using the stack-name - if delete_stack and self.region: - boto_config = get_boto_config_with_user_agent() - - # Define cf_client based on the region as different regions can have same stack-names - cloudformation_client = boto3.client( - "cloudformation", region_name=self.region if self.region else None, config=boto_config + if not self.force: + delete_stack = confirm( + click.style( + f"\tAre you sure you want to delete the stack {self.stack_name}" + + f" in the region {self.region} ?", + bold=True, + ), + default=False, ) - s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) - ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) - - self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - - docker_client = docker.from_env() - ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) - - self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) - self.cf_utils = CfUtils(cloudformation_client) - + if self.force or delete_stack: is_deployed = self.cf_utils.has_stack(stack_name=self.stack_name) - + # Check if the provided stack-name exists if is_deployed: self.delete() click.echo("\nDeleted successfully") diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index 402ebbf4cf6..d046c83e680 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -108,7 +108,7 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): LOG.debug("ImageNotFound Exception : ") raise ImageNotFoundError(resource_id, property_name) - click.echo(f"- Deleting ECR image {image_tag} in repository {repository}") + click.echo(f"\t- Deleting ECR image {image_tag} in repository {repository}") except botocore.exceptions.ClientError as ex: # Handle Client errors such as RepositoryNotFoundException or InvalidParameterException diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py index 4e268688ee8..9a17ec61141 100644 --- a/tests/unit/commands/delete/test_command.py +++ b/tests/unit/commands/delete/test_command.py @@ -22,6 +22,7 @@ def setUp(self): self.s3_prefix = "s3-prefix" self.region = None self.profile = None + self.force = None self.config_env = "mock-default-env" self.config_file = "mock-default-filename" MOCK_SAM_CONFIG.reset_mock() @@ -39,6 +40,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): config_file=self.config_file, config_env=self.config_env, profile=self.profile, + force=self.force ) mock_delete_context.assert_called_with( @@ -47,6 +49,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): profile=self.profile, config_file=self.config_file, config_env=self.config_env, + force=self.force ) context_mock.run.assert_called_with() From e2e85a906302562ce2537cdacd18f3bcfdb2559a Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 5 Jul 2021 09:36:09 -0400 Subject: [PATCH 20/37] Removed return type from artifact_exporter for delete method --- samcli/lib/package/artifact_exporter.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 2a8f484d87d..a42181f22ab 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -238,10 +238,13 @@ def export(self) -> Dict: return self.template_dict def delete(self, template_dict): + """ + Deletes all the artifacts referenced by the given Cloudformation template + """ self.template_dict = template_dict if "Resources" not in self.template_dict: - return self.template_dict + return self._apply_global_values() @@ -259,4 +262,4 @@ def delete(self, template_dict): # Delete code resources exporter = exporter_class(self.uploaders, None) exporter.delete(resource_id, resource_dict) - return self.template_dict + From c98e6ee6e920a50c5b5fb1dfea90e3b300fa15a0 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 5 Jul 2021 13:41:03 -0400 Subject: [PATCH 21/37] Added unit tests for artifact_exporter and delete_context --- samcli/commands/delete/delete_context.py | 1 + samcli/lib/package/artifact_exporter.py | 1 - tests/unit/commands/delete/test_command.py | 14 +---- .../commands/delete/test_delete_context.py | 53 +++++++++++++++++++ .../lib/package/test_artifact_exporter.py | 35 ++++++++++++ 5 files changed, 91 insertions(+), 13 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 597f8c64e07..ae4e7444ddc 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -90,6 +90,7 @@ def init_clients(self): s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + self.region = s3_client._client_config.region_name if s3_client else self.region # pylint: disable=W0212 self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) docker_client = docker.from_env() diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index a42181f22ab..dea0e6d9606 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -262,4 +262,3 @@ def delete(self, template_dict): # Delete code resources exporter = exporter_class(self.uploaders, None) exporter.delete(resource_id, resource_dict) - diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py index 9a17ec61141..73ea9e3f199 100644 --- a/tests/unit/commands/delete/test_command.py +++ b/tests/unit/commands/delete/test_command.py @@ -5,15 +5,6 @@ from tests.unit.cli.test_cli_config_file import MockContext -def get_mock_sam_config(): - mock_sam_config = MagicMock() - mock_sam_config.exists = MagicMock(return_value=True) - return mock_sam_config - - -MOCK_SAM_CONFIG = get_mock_sam_config() - - class TestDeleteCliCommand(TestCase): def setUp(self): @@ -25,7 +16,6 @@ def setUp(self): self.force = None self.config_env = "mock-default-env" self.config_file = "mock-default-filename" - MOCK_SAM_CONFIG.reset_mock() @patch("samcli.commands.delete.command.click") @patch("samcli.commands.delete.delete_context.DeleteContext") @@ -40,7 +30,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): config_file=self.config_file, config_env=self.config_env, profile=self.profile, - force=self.force + force=self.force, ) mock_delete_context.assert_called_with( @@ -49,7 +39,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): profile=self.profile, config_file=self.config_file, config_env=self.config_env, - force=self.force + force=self.force, ) context_mock.run.assert_called_with() diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index e69de29bb2d..4d84ed05615 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -0,0 +1,53 @@ +from unittest import TestCase +from unittest.mock import patch, call, MagicMock + +import click + +from samcli.commands.delete.delete_context import DeleteContext +from samcli.cli.cli_config_file import TomlProvider + + +class TestDeleteContext(TestCase): + @patch.object(DeleteContext, "parse_config_file", MagicMock()) + @patch.object(DeleteContext, "init_clients", MagicMock()) + def test_delete_context_enter(self): + with DeleteContext( + stack_name="test", + region="us-east-1", + config_file="samconfig.toml", + config_env="default", + profile="test", + force=True, + ) as delete_context: + self.assertEqual(delete_context.parse_config_file.call_count, 1) + self.assertEqual(delete_context.init_clients.call_count, 1) + + @patch.object( + TomlProvider, + "__call__", + MagicMock( + return_value=( + { + "stack_name": "test", + "region": "us-east-1", + "profile": "developer", + "s3_bucket": "s3-bucket", + "s3_prefix": "s3-prefix", + } + ) + ), + ) + def test_delete_context_parse_config_file(self): + with DeleteContext( + stack_name=None, + region=None, + config_file="samconfig.toml", + config_env="default", + profile=None, + force=True, + ) as delete_context: + self.assertEqual(delete_context.stack_name, "test") + self.assertEqual(delete_context.region, "us-east-1") + self.assertEqual(delete_context.profile, "developer") + self.assertEqual(delete_context.s3_bucket, "s3-bucket") + self.assertEqual(delete_context.s3_prefix, "s3-prefix") diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index f7aceafef14..52a450f5861 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1377,3 +1377,38 @@ def example_yaml_template(self): Timeout: 20 Runtime: nodejs4.3 """ + + def test_template_delete(self): + template_str = self.example_yaml_template() + + resource_type1_class = Mock() + resource_type1_class.RESOURCE_TYPE = "resource_type1" + resource_type1_class.ARTIFACT_TYPE = ZIP + resource_type1_class.EXPORT_DESTINATION = Destination.S3 + resource_type1_instance = Mock() + resource_type1_class.return_value = resource_type1_instance + resource_type2_class = Mock() + resource_type2_class.RESOURCE_TYPE = "resource_type2" + resource_type2_class.ARTIFACT_TYPE = ZIP + resource_type2_class.EXPORT_DESTINATION = Destination.S3 + resource_type2_instance = Mock() + resource_type2_class.return_value = resource_type2_instance + + resources_to_export = [resource_type1_class, resource_type2_class] + + properties = {"foo": "bar"} + template_dict = { + "Resources": { + "Resource1": {"Type": "resource_type1", "Properties": properties}, + "Resource2": {"Type": "resource_type2", "Properties": properties}, + "Resource3": {"Type": "some-other-type", "Properties": properties}, + } + } + + template_exporter = Template(None, None, self.uploaders_mock, None, resources_to_export) + template_exporter.delete(template_dict) + + resource_type1_class.assert_called_once_with(self.uploaders_mock, None) + resource_type1_instance.delete.assert_called_once_with("Resource1", mock.ANY) + resource_type2_class.assert_called_once_with(self.uploaders_mock, None) + resource_type2_instance.delete.assert_called_once_with("Resource2", mock.ANY) From 17a427a5e15a28d979ee27d2a128cebe52f8ecfc Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 5 Jul 2021 19:06:21 -0400 Subject: [PATCH 22/37] Added more unit tests for delete_context and artifact_exporter --- .../commands/delete/test_delete_context.py | 40 ++++++++++++++++++- .../lib/package/test_artifact_exporter.py | 18 +++++++-- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index 4d84ed05615..b7f230e7aeb 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -5,7 +5,8 @@ from samcli.commands.delete.delete_context import DeleteContext from samcli.cli.cli_config_file import TomlProvider - +from samcli.lib.delete.cf_utils import CfUtils +from samcli.lib.package.s3_uploader import S3Uploader class TestDeleteContext(TestCase): @patch.object(DeleteContext, "parse_config_file", MagicMock()) @@ -51,3 +52,40 @@ def test_delete_context_parse_config_file(self): self.assertEqual(delete_context.profile, "developer") self.assertEqual(delete_context.s3_bucket, "s3-bucket") self.assertEqual(delete_context.s3_prefix, "s3-prefix") + + @patch.object( + TomlProvider, + "__call__", + MagicMock( + return_value=( + { + "stack_name": "test", + "region": "us-east-1", + "profile": "developer", + "s3_bucket": "s3-bucket", + "s3_prefix": "s3-prefix", + } + ) + ), + ) + @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) + @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) + @patch.object(CfUtils, "delete_stack", MagicMock()) + @patch.object(CfUtils, "wait_for_delete", MagicMock()) + @patch.object(S3Uploader, "delete_prefix_artifacts", MagicMock()) + def test_delete_context_valid_execute_run(self): + with DeleteContext( + stack_name=None, + region=None, + config_file="samconfig.toml", + config_env="default", + profile=None, + force=True, + ) as delete_context: + delete_context.run() + + self.assertEqual(CfUtils.has_stack.call_count, 1) + self.assertEqual(CfUtils.get_stack_template.call_count, 1) + self.assertEqual(CfUtils.delete_stack.call_count, 1) + self.assertEqual(CfUtils.wait_for_delete.call_count, 1) + self.assertEqual(S3Uploader.delete_prefix_artifacts.call_count, 1) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 52a450f5861..36430ef44a6 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -7,7 +7,7 @@ from contextlib import contextmanager, closing from unittest import mock -from unittest.mock import patch, Mock +from unittest.mock import patch, Mock, MagicMock from samcli.commands.package.exceptions import ExportFailedError from samcli.lib.package.s3_uploader import S3Uploader @@ -56,7 +56,7 @@ class TestArtifactExporter(unittest.TestCase): def setUp(self): - self.s3_uploader_mock = Mock() + self.s3_uploader_mock = MagicMock() self.s3_uploader_mock.s3.meta.endpoint_url = "https://s3.some-valid-region.amazonaws.com" self.ecr_uploader_mock = Mock() @@ -411,6 +411,10 @@ class MockResource(ResourceZip): self.assertEqual(resource_dict[resource.PROPERTY_NAME], s3_url) + self.s3_uploader_mock.delete_artifact = MagicMock() + resource.delete(resource_id, resource_dict) + self.assertEqual(self.s3_uploader_mock.delete_artifact.call_count, 1) + @patch("samcli.lib.package.packageable_resources.upload_local_image_artifacts") def test_resource_lambda_image(self, upload_local_image_artifacts_mock): # Property value is a path to an image @@ -1393,6 +1397,12 @@ def test_template_delete(self): resource_type2_class.EXPORT_DESTINATION = Destination.S3 resource_type2_instance = Mock() resource_type2_class.return_value = resource_type2_instance + resource_type3_class = Mock() + resource_type3_class.RESOURCE_TYPE = "resource_type3" + resource_type3_class.ARTIFACT_TYPE = ZIP + resource_type3_class.EXPORT_DESTINATION = Destination.S3 + resource_type3_instance = Mock() + resource_type3_class.return_value = resource_type3_instance resources_to_export = [resource_type1_class, resource_type2_class] @@ -1401,7 +1411,7 @@ def test_template_delete(self): "Resources": { "Resource1": {"Type": "resource_type1", "Properties": properties}, "Resource2": {"Type": "resource_type2", "Properties": properties}, - "Resource3": {"Type": "some-other-type", "Properties": properties}, + "Resource3": {"Type": "some-other-type", "Properties": properties, "DeletionPolicy": "Retain"}, } } @@ -1412,3 +1422,5 @@ def test_template_delete(self): resource_type1_instance.delete.assert_called_once_with("Resource1", mock.ANY) resource_type2_class.assert_called_once_with(self.uploaders_mock, None) resource_type2_instance.delete.assert_called_once_with("Resource2", mock.ANY) + resource_type3_class.assert_not_called() + resource_type3_instance.delete.assert_not_called() From e577d7f65ac53de7ba263d03f2e73f786bd7f0f7 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Tue, 6 Jul 2021 15:42:51 -0400 Subject: [PATCH 23/37] Added more unit tests for delete_context and artifact_exporter --- samcli/commands/delete/delete_context.py | 3 +- samcli/lib/package/ecr_uploader.py | 4 +- .../commands/delete/test_delete_context.py | 138 ++++++++++++++++++ .../lib/package/test_artifact_exporter.py | 10 +- 4 files changed, 152 insertions(+), 3 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index ae4e7444ddc..13577c9774c 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -164,7 +164,8 @@ def delete(self): elif self.delete_artifacts_folder: self.s3_uploader.delete_prefix_artifacts() - else: + # If s3_bucket information is not available + elif not self.s3_bucket: click.secho( "\nWarning: s3_bucket and s3_prefix information cannot be obtained," " delete the files manually if required", diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index d046c83e680..8306e000d04 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -105,13 +105,15 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): # Image not found image_details = resp["failures"][0] if image_details["failureCode"] == "ImageNotFound": - LOG.debug("ImageNotFound Exception : ") + LOG.error("ImageNotFound Exception : ") raise ImageNotFoundError(resource_id, property_name) + LOG.debug("Deleting ECR image with tag %s", image_tag) click.echo(f"\t- Deleting ECR image {image_tag} in repository {repository}") except botocore.exceptions.ClientError as ex: # Handle Client errors such as RepositoryNotFoundException or InvalidParameterException + LOG.error("DeleteArtifactFailedError Exception : %s", str(ex)) raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex # TODO: move this to a generic class to allow for streaming logs back from docker. diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index b7f230e7aeb..e353dc17d40 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -8,7 +8,26 @@ from samcli.lib.delete.cf_utils import CfUtils from samcli.lib.package.s3_uploader import S3Uploader + class TestDeleteContext(TestCase): + @patch("samcli.commands.deploy.guided_context.click.echo") + @patch.object(CfUtils, "has_stack", MagicMock(return_value=(False))) + def test_delete_context_stack_does_not_exist(self, patched_click_echo): + with DeleteContext( + stack_name="test", + region="us-east-1", + config_file="samconfig.toml", + config_env="default", + profile="test", + force=True, + ) as delete_context: + + delete_context.run() + expected_click_echo_calls = [ + call(f"Error: The input stack test does not exist on Cloudformation"), + ] + self.assertEqual(expected_click_echo_calls, patched_click_echo.call_args_list) + @patch.object(DeleteContext, "parse_config_file", MagicMock()) @patch.object(DeleteContext, "init_clients", MagicMock()) def test_delete_context_enter(self): @@ -89,3 +108,122 @@ def test_delete_context_valid_execute_run(self): self.assertEqual(CfUtils.delete_stack.call_count, 1) self.assertEqual(CfUtils.wait_for_delete.call_count, 1) self.assertEqual(S3Uploader.delete_prefix_artifacts.call_count, 1) + + @patch("samcli.commands.deploy.guided_context.click.secho") + @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) + @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) + @patch.object(CfUtils, "delete_stack", MagicMock()) + @patch.object(CfUtils, "wait_for_delete", MagicMock()) + def test_delete_context_no_s3_bucket(self, patched_click_secho): + with DeleteContext( + stack_name="test", + region="us-east-1", + config_file="samconfig.toml", + config_env="default", + profile="test", + force=True, + ) as delete_context: + + delete_context.run() + expected_click_secho_calls = [ + call( + "\nWarning: s3_bucket and s3_prefix information cannot be obtained," + " delete the files manually if required", + fg="yellow", + ), + ] + self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) + + @patch("samcli.commands.delete.delete_context.confirm") + @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) + @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) + @patch.object(CfUtils, "delete_stack", MagicMock()) + @patch.object(CfUtils, "wait_for_delete", MagicMock()) + @patch.object(S3Uploader, "delete_artifact", MagicMock()) + def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confirm): + + with DeleteContext( + stack_name="test", + region="us-east-1", + config_file="samconfig.toml", + config_env="default", + profile="test", + force=None, + ) as delete_context: + patched_confirm.side_effect = [True, False, True] + delete_context.cf_template_file_name = "hello.template" + delete_context.s3_bucket = "s3_bucket" + delete_context.s3_prefix = "s3_prefix" + + delete_context.run() + # Now to check for all the defaults on confirmations. + expected_confirmation_calls = [ + call( + click.style( + f"\tAre you sure you want to delete the stack test" + f" in the region us-east-1 ?", + bold=True, + ), + default=False, + ), + call( + click.style( + "\tAre you sure you want to delete the folder" + + f" s3_prefix in S3 which contains the artifacts?", + bold=True, + ), + default=False, + ), + call( + click.style( + "\tDo you want to delete the template file b10a8db164e0754105b7a99be72e3fe5.template in S3?", + bold=True, + ), + default=False, + ), + ] + + self.assertEqual(expected_confirmation_calls, patched_confirm.call_args_list) + self.assertFalse(delete_context.delete_artifacts_folder) + self.assertTrue(delete_context.delete_cf_template_file) + + @patch("samcli.commands.delete.delete_context.confirm") + @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) + @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) + @patch.object(CfUtils, "delete_stack", MagicMock()) + @patch.object(CfUtils, "wait_for_delete", MagicMock()) + @patch.object(S3Uploader, "delete_artifact", MagicMock()) + def test_guided_prompts_s3_bucket_present_no_prefix_execute_run(self, patched_confirm): + + with DeleteContext( + stack_name="test", + region="us-east-1", + config_file="samconfig.toml", + config_env="default", + profile="test", + force=None, + ) as delete_context: + patched_confirm.side_effect = [True, True] + delete_context.cf_template_file_name = "hello.template" + delete_context.s3_bucket = "s3_bucket" + + delete_context.run() + # Now to check for all the defaults on confirmations. + expected_confirmation_calls = [ + call( + click.style( + f"\tAre you sure you want to delete the stack test" + f" in the region us-east-1 ?", + bold=True, + ), + default=False, + ), + call( + click.style( + "\tDo you want to delete the template file b10a8db164e0754105b7a99be72e3fe5.template in S3?", + bold=True, + ), + default=False, + ), + ] + + self.assertEqual(expected_confirmation_calls, patched_confirm.call_args_list) + self.assertTrue(delete_context.delete_cf_template_file) diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 36430ef44a6..1167876ece7 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -414,7 +414,7 @@ class MockResource(ResourceZip): self.s3_uploader_mock.delete_artifact = MagicMock() resource.delete(resource_id, resource_dict) self.assertEqual(self.s3_uploader_mock.delete_artifact.call_count, 1) - + @patch("samcli.lib.package.packageable_resources.upload_local_image_artifacts") def test_resource_lambda_image(self, upload_local_image_artifacts_mock): # Property value is a path to an image @@ -440,6 +440,10 @@ class MockResource(ResourceImage): self.assertEqual(resource_dict[resource.PROPERTY_NAME], ecr_url) + self.ecr_uploader_mock.delete_artifact = MagicMock() + resource.delete(resource_id, resource_dict) + self.assertEqual(self.ecr_uploader_mock.delete_artifact.call_count, 1) + def test_lambda_image_resource_package_success(self): # Property value is set to an image @@ -750,6 +754,10 @@ class MockResource(ResourceWithS3UrlDict): resource_dict[resource.PROPERTY_NAME], {"b": "bucket", "o": "key1/key2", "v": "SomeVersionNumber"} ) + self.s3_uploader_mock.delete_artifact = MagicMock() + resource.delete(resource_id, resource_dict) + self.s3_uploader_mock.delete_artifact.assert_called_once_with(remote_path="key1/key2", is_key=True) + @patch("samcli.lib.package.packageable_resources.upload_local_artifacts") def test_resource_with_signing_configuration(self, upload_local_artifacts_mock): class MockResource(ResourceZip): From 58ead7198982b4211eaef6d4b85269013e580ad9 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Tue, 6 Jul 2021 17:30:33 -0400 Subject: [PATCH 24/37] Added docs and comments for artifact_exporter and ecr_uploader --- samcli/lib/package/artifact_exporter.py | 2 ++ samcli/lib/package/ecr_uploader.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index f005a642cc3..2ec18eec68f 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -255,6 +255,8 @@ def delete(self, template_dict): resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", {}) resource_deletion_policy = resource.get("DeletionPolicy", None) + # If the deletion policy is set to Retain, + # do not delete the artifact for the resource. if resource_deletion_policy != "Retain": for exporter_class in self.resources_to_export: if exporter_class.RESOURCE_TYPE != resource_type: diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index 8306e000d04..7e70b885932 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -91,6 +91,14 @@ def upload(self, image, resource_name): return f"{repository}:{_tag}" def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): + """ + Delete the given ECR image by extracting the repository and image_tag from + image_uri + + :param image_uri: image_uri of the image to be deleted + :param resource_id: id of the resource for which the image is deleted + :param property_name: provided property_name for the resource + """ try: repo_image_tag = image_uri.split("/")[1].split(":") repository = repo_image_tag[0] From 45ee66fbb4816ef5876a6f5627da024086193416 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 7 Jul 2021 10:58:45 -0400 Subject: [PATCH 25/37] Added log statements in delete_context and some updates in unit tests --- samcli/commands/delete/delete_context.py | 11 +++++++- .../commands/delete/test_delete_context.py | 25 ++++++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 27e36d6799a..e9c4575813e 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -1,7 +1,7 @@ """ Delete a SAM stack """ - +import logging import boto3 @@ -25,6 +25,8 @@ CONFIG_SECTION = "parameters" TEMPLATE_STAGE = "Original" +LOG = logging.getLogger(__name__) + class DeleteContext: def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str, force: bool): @@ -47,6 +49,7 @@ def __init__(self, stack_name: str, region: str, profile: str, config_file: str, def __enter__(self): self.parse_config_file() if not self.stack_name: + LOG.debug("No stack-name input found") self.stack_name = prompt( click.style("\tEnter stack name you want to delete:", bold=True), type=click.STRING ) @@ -71,6 +74,7 @@ def parse_config_file(self): # If the stack_name is same as the one present in samconfig file, # get the information about parameters if not specified by customer. if self.stack_name and self.stack_name == config_options.get("stack_name", None): + LOG.debug("Local config present and using the defined options") if not self.region: self.region = config_options.get("region", None) click.get_current_context().region = self.region @@ -125,6 +129,7 @@ def guided_prompts(self): default=False, ) if not self.delete_artifacts_folder: + LOG.debug("S3 prefix not present or user does not want to delete the prefix folder") self.delete_cf_template_file = confirm( click.style( "\tDo you want to delete the template file" + f" {self.cf_template_file_name} in S3?", bold=True @@ -156,6 +161,7 @@ def delete(self): click.echo(f"\n\t- Deleting Cloudformation stack {self.stack_name}") self.cf_utils.delete_stack(stack_name=self.stack_name) self.cf_utils.wait_for_delete(self.stack_name) + LOG.debug("Deleted Cloudformation stack: %s", self.stack_name) # Delete the artifacts self.template.delete(template_dict) @@ -170,6 +176,7 @@ def delete(self): # If s3_bucket information is not available elif not self.s3_bucket: + LOG.debug("Cannot delete s3 files as no s3_bucket found") click.secho( "\nWarning: s3_bucket and s3_prefix information cannot be obtained," " delete the files manually if required", @@ -194,7 +201,9 @@ def run(self): is_deployed = self.cf_utils.has_stack(stack_name=self.stack_name) # Check if the provided stack-name exists if is_deployed: + LOG.debug("Input stack is deployed, continue deleting") self.delete() click.echo("\nDeleted successfully") else: + LOG.debug("Input stack does not exists on Cloudformation") click.echo(f"Error: The input stack {self.stack_name} does not exist on Cloudformation") diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index 39aa38b0101..0d4dfcbd52a 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -10,7 +10,7 @@ class TestDeleteContext(TestCase): - @patch("samcli.commands.deploy.guided_context.click.echo") + @patch("samcli.commands.delete.delete_context.click.echo") @patch.object(CfUtils, "has_stack", MagicMock(return_value=(False))) def test_delete_context_stack_does_not_exist(self, patched_click_echo): with DeleteContext( @@ -113,12 +113,13 @@ def test_delete_context_valid_execute_run(self, patched_click_get_current_contex self.assertEqual(CfUtils.wait_for_delete.call_count, 1) self.assertEqual(S3Uploader.delete_prefix_artifacts.call_count, 1) + @patch("samcli.commands.delete.delete_context.click.echo") @patch("samcli.commands.deploy.guided_context.click.secho") @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) @patch.object(CfUtils, "delete_stack", MagicMock()) @patch.object(CfUtils, "wait_for_delete", MagicMock()) - def test_delete_context_no_s3_bucket(self, patched_click_secho): + def test_delete_context_no_s3_bucket(self, patched_click_secho, patched_click_echo): with DeleteContext( stack_name="test", region="us-east-1", @@ -138,14 +139,22 @@ def test_delete_context_no_s3_bucket(self, patched_click_secho): ] self.assertEqual(expected_click_secho_calls, patched_click_secho.call_args_list) + expected_click_echo_calls = [ + call("\n\t- Deleting Cloudformation stack test"), + call("\nDeleted successfully"), + ] + self.assertEqual(expected_click_echo_calls, patched_click_echo.call_args_list) + + @patch("samcli.commands.delete.delete_context.get_cf_template_name") @patch("samcli.commands.delete.delete_context.confirm") @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) @patch.object(CfUtils, "delete_stack", MagicMock()) @patch.object(CfUtils, "wait_for_delete", MagicMock()) @patch.object(S3Uploader, "delete_artifact", MagicMock()) - def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confirm): + def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confirm, patched_get_cf_template_name): + patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( stack_name="test", region="us-east-1", @@ -179,7 +188,7 @@ def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confi ), call( click.style( - "\tDo you want to delete the template file b10a8db164e0754105b7a99be72e3fe5.template in S3?", + "\tDo you want to delete the template file hello.template in S3?", bold=True, ), default=False, @@ -190,14 +199,18 @@ def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confi self.assertFalse(delete_context.delete_artifacts_folder) self.assertTrue(delete_context.delete_cf_template_file) + @patch("samcli.commands.delete.delete_context.get_cf_template_name") @patch("samcli.commands.delete.delete_context.confirm") @patch.object(CfUtils, "has_stack", MagicMock(return_value=(True))) @patch.object(CfUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) @patch.object(CfUtils, "delete_stack", MagicMock()) @patch.object(CfUtils, "wait_for_delete", MagicMock()) @patch.object(S3Uploader, "delete_artifact", MagicMock()) - def test_guided_prompts_s3_bucket_present_no_prefix_execute_run(self, patched_confirm): + def test_guided_prompts_s3_bucket_present_no_prefix_execute_run( + self, patched_confirm, patched_get_cf_template_name + ): + patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( stack_name="test", region="us-east-1", @@ -222,7 +235,7 @@ def test_guided_prompts_s3_bucket_present_no_prefix_execute_run(self, patched_co ), call( click.style( - "\tDo you want to delete the template file b10a8db164e0754105b7a99be72e3fe5.template in S3?", + "\tDo you want to delete the template file hello.template in S3?", bold=True, ), default=False, From d151b019d1c001f69b2dc4cec37d70bfcff5ef43 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 8 Jul 2021 17:27:06 -0400 Subject: [PATCH 26/37] Changed force to no-prompts and updated ecr delete method error handling --- samcli/commands/delete/command.py | 13 +++++------ samcli/commands/delete/delete_context.py | 14 +++++------ samcli/commands/package/exceptions.py | 4 +--- samcli/lib/package/ecr_uploader.py | 18 +++++++++++++-- samcli/lib/package/utils.py | 3 ++- tests/unit/commands/delete/test_command.py | 6 ++--- .../commands/delete/test_delete_context.py | 14 +++++------ tests/unit/lib/package/test_ecr_uploader.py | 23 +++++++++++++++++++ 8 files changed, 64 insertions(+), 31 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 4e0b9ec6bc0..06130eb68b1 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -58,10 +58,9 @@ show_default=True, ) @click.option( - "--force", - help=("Specify this flag to allow SAM CLI to skip through the guided prompts" ""), + "--no-prompts", + help=("Specify this flag to allow SAM CLI to skip through the guided prompts."), is_flag=True, - type=click.BOOL, required=False, ) @aws_creds_options @@ -69,7 +68,7 @@ @pass_context @check_newer_version @print_cmdline_args -def cli(ctx, stack_name: str, config_file: str, config_env: str, force: bool): +def cli(ctx, stack_name: str, config_file: str, config_env: str, no_prompts: bool): """ `sam delete` command entry point """ @@ -81,11 +80,11 @@ def cli(ctx, stack_name: str, config_file: str, config_env: str, force: bool): config_file=config_file, config_env=config_env, profile=ctx.profile, - force=force, + no_prompts=no_prompts, ) # pragma: no cover -def do_cli(stack_name: str, region: str, config_file: str, config_env: str, profile: str, force: bool): +def do_cli(stack_name: str, region: str, config_file: str, config_env: str, profile: str, no_prompts: bool): """ Implementation of the ``cli`` method """ @@ -97,6 +96,6 @@ def do_cli(stack_name: str, region: str, config_file: str, config_env: str, prof profile=profile, config_file=config_file, config_env=config_env, - force=force, + no_prompts=no_prompts, ) as delete_context: delete_context.run() diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index e9c4575813e..29147623189 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -5,7 +5,6 @@ import boto3 -import docker import click from click import confirm from click import prompt @@ -29,13 +28,13 @@ class DeleteContext: - def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str, force: bool): + def __init__(self, stack_name: str, region: str, profile: str, config_file: str, config_env: str, no_prompts: bool): self.stack_name = stack_name self.region = region self.profile = profile self.config_file = config_file self.config_env = config_env - self.force = force + self.no_prompts = no_prompts self.s3_bucket = None self.s3_prefix = None self.cf_utils = None @@ -101,8 +100,7 @@ def init_clients(self): self.region = s3_client._client_config.region_name if s3_client else self.region # pylint: disable=W0212 self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - docker_client = docker.from_env() - ecr_uploader = ECRUploader(docker_client, ecr_client, None, None) + ecr_uploader = ECRUploader(docker_client=None, ecr_client=ecr_client, ecr_repo=None, ecr_repo_multi=None) self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) self.cf_utils = CfUtils(cloudformation_client) @@ -118,7 +116,7 @@ def guided_prompts(self): # information is not found, warn the customer that S3 artifacts # will need to be manually deleted. - if not self.force and self.s3_bucket: + if not self.no_prompts and self.s3_bucket: if self.s3_prefix: self.delete_artifacts_folder = confirm( click.style( @@ -187,7 +185,7 @@ def run(self): """ Delete the stack based on the argument provided by customers and samconfig.toml. """ - if not self.force: + if not self.no_prompts: delete_stack = confirm( click.style( f"\tAre you sure you want to delete the stack {self.stack_name}" @@ -197,7 +195,7 @@ def run(self): default=False, ) - if self.force or delete_stack: + if self.no_prompts or delete_stack: is_deployed = self.cf_utils.has_stack(stack_name=self.stack_name) # Check if the provided stack-name exists if is_deployed: diff --git a/samcli/commands/package/exceptions.py b/samcli/commands/package/exceptions.py index 2e23cf74588..70ed0ba958d 100644 --- a/samcli/commands/package/exceptions.py +++ b/samcli/commands/package/exceptions.py @@ -85,12 +85,10 @@ def __init__(self, resource_id, property_name, ex): class ImageNotFoundError(UserException): - def __init__(self, resource_id, property_name): + def __init__(self, resource_id, property_name, message_fmt): self.resource_id = resource_id self.property_name = property_name - message_fmt = "Image not found for {property_name} parameter of {resource_id} resource. \n" - super().__init__( message=message_fmt.format( property_name=self.property_name, diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index 7e70b885932..7edc41ad410 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -113,8 +113,22 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): # Image not found image_details = resp["failures"][0] if image_details["failureCode"] == "ImageNotFound": - LOG.error("ImageNotFound Exception : ") - raise ImageNotFoundError(resource_id, property_name) + LOG.error("ImageNotFound Exception") + message_fmt = ( + "Could not delete image for {property_name}" + " parameter of {resource_id} resource as it does not exist. \n" + ) + raise ImageNotFoundError(resource_id, property_name, message_fmt=message_fmt) + + LOG.error( + "Could not delete the image for the resource %s. FailureCode: %s, FailureReason: %s", + property_name, + image_details["failureCode"], + image_details["failureReason"], + ) + raise DeleteArtifactFailedError( + resource_id=resource_id, property_name=property_name, ex=image_details["failureReason"] + ) LOG.debug("Deleting ECR image with tag %s", image_tag) click.echo(f"\t- Deleting ECR image {image_tag} in repository {repository}") diff --git a/samcli/lib/package/utils.py b/samcli/lib/package/utils.py index c33b2b3de7b..c152b37aa07 100644 --- a/samcli/lib/package/utils.py +++ b/samcli/lib/package/utils.py @@ -110,7 +110,8 @@ def upload_local_image_artifacts(resource_id, resource_dict, property_name, pare image_path = jmespath.search(property_name, resource_dict) if not image_path: - raise ImageNotFoundError(property_name=property_name, resource_id=resource_id) + message_fmt = "Image not found for {property_name} parameter of {resource_id} resource. \n" + raise ImageNotFoundError(property_name=property_name, resource_id=resource_id, message_fmt=message_fmt) if is_ecr_url(image_path): LOG.debug("Property %s of %s is already an ECR URL", property_name, resource_id) diff --git a/tests/unit/commands/delete/test_command.py b/tests/unit/commands/delete/test_command.py index 73ea9e3f199..7160553793f 100644 --- a/tests/unit/commands/delete/test_command.py +++ b/tests/unit/commands/delete/test_command.py @@ -13,7 +13,7 @@ def setUp(self): self.s3_prefix = "s3-prefix" self.region = None self.profile = None - self.force = None + self.no_prompts = None self.config_env = "mock-default-env" self.config_file = "mock-default-filename" @@ -30,7 +30,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): config_file=self.config_file, config_env=self.config_env, profile=self.profile, - force=self.force, + no_prompts=self.no_prompts, ) mock_delete_context.assert_called_with( @@ -39,7 +39,7 @@ def test_all_args(self, mock_delete_context, mock_delete_click): profile=self.profile, config_file=self.config_file, config_env=self.config_env, - force=self.force, + no_prompts=self.no_prompts, ) context_mock.run.assert_called_with() diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index 0d4dfcbd52a..f0975f144e8 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -19,7 +19,7 @@ def test_delete_context_stack_does_not_exist(self, patched_click_echo): config_file="samconfig.toml", config_env="default", profile="test", - force=True, + no_prompts=True, ) as delete_context: delete_context.run() @@ -37,7 +37,7 @@ def test_delete_context_enter(self): config_file="samconfig.toml", config_env="default", profile="test", - force=True, + no_prompts=True, ) as delete_context: self.assertEqual(delete_context.parse_config_file.call_count, 1) self.assertEqual(delete_context.init_clients.call_count, 1) @@ -66,7 +66,7 @@ def test_delete_context_parse_config_file(self, patched_click_get_current_contex config_file="samconfig.toml", config_env="default", profile=None, - force=True, + no_prompts=True, ) as delete_context: self.assertEqual(delete_context.stack_name, "test") self.assertEqual(delete_context.region, "us-east-1") @@ -103,7 +103,7 @@ def test_delete_context_valid_execute_run(self, patched_click_get_current_contex config_file="samconfig.toml", config_env="default", profile=None, - force=True, + no_prompts=True, ) as delete_context: delete_context.run() @@ -126,7 +126,7 @@ def test_delete_context_no_s3_bucket(self, patched_click_secho, patched_click_ec config_file="samconfig.toml", config_env="default", profile="test", - force=True, + no_prompts=True, ) as delete_context: delete_context.run() @@ -161,7 +161,7 @@ def test_guided_prompts_s3_bucket_prefix_present_execute_run(self, patched_confi config_file="samconfig.toml", config_env="default", profile="test", - force=None, + no_prompts=None, ) as delete_context: patched_confirm.side_effect = [True, False, True] delete_context.cf_template_file_name = "hello.template" @@ -217,7 +217,7 @@ def test_guided_prompts_s3_bucket_present_no_prefix_execute_run( config_file="samconfig.toml", config_env="default", profile="test", - force=None, + no_prompts=None, ) as delete_context: patched_confirm.side_effect = [True, True] delete_context.cf_template_file_name = "hello.template" diff --git a/tests/unit/lib/package/test_ecr_uploader.py b/tests/unit/lib/package/test_ecr_uploader.py index a66207efca2..3d4f962b06d 100644 --- a/tests/unit/lib/package/test_ecr_uploader.py +++ b/tests/unit/lib/package/test_ecr_uploader.py @@ -193,6 +193,29 @@ def test_delete_artifact_no_image_error(self): image_uri=self.image_uri, resource_id=self.resource_id, property_name=self.property_name ) + def test_delete_artifact_resp_failure(self): + ecr_uploader = ECRUploader( + docker_client=self.docker_client, + ecr_client=self.ecr_client, + ecr_repo=self.ecr_repo, + ecr_repo_multi=self.ecr_repo_multi, + tag=self.tag, + ) + ecr_uploader.ecr_client.batch_delete_image.return_value = { + "failures": [ + { + "imageId": {"imageTag": self.tag}, + "failureCode": "Mock response Failure", + "failureReason": "Mock ECR testing", + } + ] + } + + with self.assertRaises(DeleteArtifactFailedError): + ecr_uploader.delete_artifact( + image_uri=self.image_uri, resource_id=self.resource_id, property_name=self.property_name + ) + def test_delete_artifact_client_error(self): ecr_uploader = ECRUploader( docker_client=self.docker_client, From 6f542400faeb7b267d7899b2a82ad9f59bbba951 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 8 Jul 2021 22:07:47 -0400 Subject: [PATCH 27/37] Created a separate function for parsing ecr url in ecr_uploader --- samcli/lib/package/ecr_uploader.py | 26 ++++++++++++++++++--- samcli/lib/package/packageable_resources.py | 4 ++++ tests/unit/lib/package/test_ecr_uploader.py | 18 ++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/samcli/lib/package/ecr_uploader.py b/samcli/lib/package/ecr_uploader.py index 7edc41ad410..9aa6aaa159e 100644 --- a/samcli/lib/package/ecr_uploader.py +++ b/samcli/lib/package/ecr_uploader.py @@ -5,6 +5,7 @@ import base64 import os +from typing import Dict import click import botocore import docker @@ -100,9 +101,9 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): :param property_name: provided property_name for the resource """ try: - repo_image_tag = image_uri.split("/")[1].split(":") - repository = repo_image_tag[0] - image_tag = repo_image_tag[1] + repo_image_tag = self.parse_ecr_url(image_uri=image_uri) + repository = repo_image_tag["repository"] + image_tag = repo_image_tag["image_tag"] resp = self.ecr_client.batch_delete_image( repositoryName=repository, imageIds=[ @@ -138,6 +139,25 @@ def delete_artifact(self, image_uri: str, resource_id: str, property_name: str): LOG.error("DeleteArtifactFailedError Exception : %s", str(ex)) raise DeleteArtifactFailedError(resource_id=resource_id, property_name=property_name, ex=ex) from ex + @staticmethod + def parse_ecr_url(image_uri: str) -> Dict: + result = {} + registry_repo_tag = image_uri.split("/") + repo_colon_image_tag = None + if len(registry_repo_tag) == 1: + # If there is no registry specified, e.g. repo:tag + repo_colon_image_tag = registry_repo_tag[0] + else: + # Registry present, e.g. registry/repo:tag + repo_colon_image_tag = registry_repo_tag[1] + repo_image_tag_split = repo_colon_image_tag.split(":") + + # If no tag is specified, use latest + result["repository"] = repo_image_tag_split[0] + result["image_tag"] = repo_image_tag_split[1] if len(repo_image_tag_split) > 1 else "latest" + + return result + # TODO: move this to a generic class to allow for streaming logs back from docker. def _stream_progress(self, logs): """ diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index cc2fe999d9b..02d76faeb65 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -225,6 +225,8 @@ def delete(self, resource_id, resource_dict): self.uploader.delete_artifact( image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME ) + else: + raise ValueError("URL given to the parse method is not a valid ECR url " "{0}".format(remote_path)) class ResourceImage(Resource): @@ -279,6 +281,8 @@ def delete(self, resource_id, resource_dict): self.uploader.delete_artifact( image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME ) + else: + raise ValueError("URL given to the parse method is not a valid ECR url " "{0}".format(remote_path)) class ResourceWithS3UrlDict(ResourceZip): diff --git a/tests/unit/lib/package/test_ecr_uploader.py b/tests/unit/lib/package/test_ecr_uploader.py index 3d4f962b06d..6264fe5d6bd 100644 --- a/tests/unit/lib/package/test_ecr_uploader.py +++ b/tests/unit/lib/package/test_ecr_uploader.py @@ -234,3 +234,21 @@ def test_delete_artifact_client_error(self): ecr_uploader.delete_artifact( image_uri=self.image_uri, resource_id=self.resource_id, property_name=self.property_name ) + + def test_parse_ecr_url(self): + + valid = [ + {"url": self.image_uri, "result": {"repository": "mock-image-repo", "image_tag": "mock-tag"}}, + {"url": "mock-image-rep:mock-tag", "result": {"repository": "mock-image-rep", "image_tag": "mock-tag"}}, + { + "url": "mock-image-repo", + "result": {"repository": "mock-image-repo", "image_tag": "latest"}, + } + ] + + for config in valid: + result = ECRUploader.parse_ecr_url( + image_uri=config["url"] + ) + + self.assertEqual(result, config["result"]) From 30e3c02abd975ac09e7ef8ef921845de838de127 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 8 Jul 2021 23:03:33 -0400 Subject: [PATCH 28/37] Reformatted Template class init to pass template_str and init template_dict --- samcli/commands/delete/delete_context.py | 13 ++++++------- samcli/lib/package/artifact_exporter.py | 11 +++++------ tests/unit/lib/package/test_artifact_exporter.py | 14 ++++++++++++-- tests/unit/lib/package/test_ecr_uploader.py | 8 +++----- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 29147623189..64913072470 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -14,7 +14,6 @@ from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.artifact_exporter import mktempfile, get_cf_template_name -from samcli.yamlhelper import yaml_parse from samcli.lib.package.artifact_exporter import Template from samcli.lib.package.ecr_uploader import ECRUploader @@ -40,7 +39,6 @@ def __init__(self, stack_name: str, region: str, profile: str, config_file: str, self.cf_utils = None self.s3_uploader = None self.uploaders = None - self.template = None self.cf_template_file_name = None self.delete_artifacts_folder = None self.delete_cf_template_file = None @@ -104,7 +102,6 @@ def init_clients(self): self.uploaders = Uploaders(self.s3_uploader, ecr_uploader) self.cf_utils = CfUtils(cloudformation_client) - self.template = Template(None, None, self.uploaders, None) def guided_prompts(self): """ @@ -145,9 +142,8 @@ def delete(self): Delete method calls for Cloudformation stacks and S3 and ECR artifacts """ # Fetch the template using the stack-name - template = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) - template_str = template.get("TemplateBody", None) - template_dict = yaml_parse(template_str) + cf_template = self.cf_utils.get_stack_template(self.stack_name, TEMPLATE_STAGE) + template_str = cf_template.get("TemplateBody", None) # Get the cloudformation template name using template_str with mktempfile() as temp_file: @@ -162,7 +158,10 @@ def delete(self): LOG.debug("Deleted Cloudformation stack: %s", self.stack_name) # Delete the artifacts - self.template.delete(template_dict) + template = Template( + template_path=None, parent_dir=None, uploaders=self.uploaders, code_signer=None, template_str=template_str + ) + template.delete() # Delete the CF template file in S3 if self.delete_cf_template_file: diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 2ec18eec68f..00fa5cb089d 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -16,7 +16,7 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import os -from typing import Dict +from typing import Dict, Optional from botocore.utils import set_value_from_jmespath @@ -128,11 +128,12 @@ def __init__( RESOURCES_EXPORT_LIST + [CloudFormationStackResource, ServerlessApplicationResource] ), metadata_to_export=frozenset(METADATA_EXPORT_LIST), + template_str: Optional[str] = None, ): """ Reads the template and makes it ready for export """ - if template_path and parent_dir: + if not template_str: if not (is_local_folder(parent_dir) and os.path.isabs(parent_dir)): raise ValueError("parent_dir parameter must be " "an absolute path to a folder {0}".format(parent_dir)) @@ -142,9 +143,9 @@ def __init__( with open(abs_template_path, "r") as handle: template_str = handle.read() - self.template_dict = yaml_parse(template_str) self.template_dir = template_dir self.code_signer = code_signer + self.template_dict = yaml_parse(template_str) self.resources_to_export = resources_to_export self.metadata_to_export = metadata_to_export self.uploaders = uploaders @@ -239,12 +240,10 @@ def export(self) -> Dict: return self.template_dict - def delete(self, template_dict): + def delete(self): """ Deletes all the artifacts referenced by the given Cloudformation template """ - self.template_dict = template_dict - if "Resources" not in self.template_dict: return diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 1167876ece7..750317ed209 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1,3 +1,4 @@ +import json import tempfile import os import string @@ -1422,9 +1423,18 @@ def test_template_delete(self): "Resource3": {"Type": "some-other-type", "Properties": properties, "DeletionPolicy": "Retain"}, } } + template_str = json.dumps(template_dict, indent=4, ensure_ascii=False) + + template_exporter = Template( + template_path=None, + parent_dir=None, + uploaders=self.uploaders_mock, + code_signer=None, + resources_to_export=resources_to_export, + template_str=template_str, + ) - template_exporter = Template(None, None, self.uploaders_mock, None, resources_to_export) - template_exporter.delete(template_dict) + template_exporter.delete() resource_type1_class.assert_called_once_with(self.uploaders_mock, None) resource_type1_instance.delete.assert_called_once_with("Resource1", mock.ANY) diff --git a/tests/unit/lib/package/test_ecr_uploader.py b/tests/unit/lib/package/test_ecr_uploader.py index 6264fe5d6bd..2fa0e0433d0 100644 --- a/tests/unit/lib/package/test_ecr_uploader.py +++ b/tests/unit/lib/package/test_ecr_uploader.py @@ -243,12 +243,10 @@ def test_parse_ecr_url(self): { "url": "mock-image-repo", "result": {"repository": "mock-image-repo", "image_tag": "latest"}, - } + }, ] - + for config in valid: - result = ECRUploader.parse_ecr_url( - image_uri=config["url"] - ) + result = ECRUploader.parse_ecr_url(image_uri=config["url"]) self.assertEqual(result, config["result"]) From b8a2591f3c2505d50728296eb597ce8c82a21910 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Fri, 9 Jul 2021 12:22:09 -0400 Subject: [PATCH 29/37] Changed how s3 url is obtained for resource_zip edge-case: aws:glue:job --- samcli/lib/package/packageable_resources.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 02d76faeb65..486e90ebb47 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -164,7 +164,7 @@ def delete(self, resource_id, resource_dict): """ if resource_dict is None: return - resource_path = resource_dict[self.PROPERTY_NAME] + resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict) parsed_s3_url = self.uploader.parse_s3_url(resource_path) if not self.uploader.bucket_name: self.uploader.bucket_name = parsed_s3_url["Bucket"] From 7292353ec8339d4bfad9e411e45fd6424e07a84b Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Fri, 9 Jul 2021 15:30:23 -0400 Subject: [PATCH 30/37] Fixed edge case where resource artifact points to a path style url --- samcli/lib/package/packageable_resources.py | 9 +++++++- samcli/lib/package/s3_uploader.py | 22 +++++++++++++++++++ .../lib/package/test_artifact_exporter.py | 22 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 486e90ebb47..15ce8fc3613 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -165,7 +165,14 @@ def delete(self, resource_id, resource_dict): if resource_dict is None: return resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict) - parsed_s3_url = self.uploader.parse_s3_url(resource_path) + parsed_s3_url = [] + if isinstance(resource_path, str) and resource_path.startswith("https://s3"): + # Path-style s3 url parsing for resources that return these urls + # For resources e.g. CloudFormation::Stack and Serverless::Application + parsed_s3_url = self.uploader.parse_path_style_s3_url(resource_path) + else: + # urls which start with s3:// + parsed_s3_url = self.uploader.parse_s3_url(resource_path) if not self.uploader.bucket_name: self.uploader.bucket_name = parsed_s3_url["Bucket"] self.uploader.delete_artifact(parsed_s3_url["Key"], True) diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 76b7ff1ec7e..cf67efa4ea2 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -263,6 +263,28 @@ def parse_s3_url( raise ValueError("URL given to the parse method is not a valid S3 url " "{0}".format(url)) + @staticmethod + def parse_path_style_s3_url( + url: Any, + bucket_name_property: str = "Bucket", + object_key_property: str = "Key", + ) -> Dict: + """ + Static method for parsing path style s3 urls. + e.g. https://s3.us-east-1.amazonaws.com/bucket/key + """ + if isinstance(url, str) and url.startswith("https://s3"): + parsed = urlparse(url) + result = dict() + # path would point to /bucket/key + s3_bucket_key = parsed.path.split('/', 2)[1:] + + result[bucket_name_property] = s3_bucket_key[0] + result[object_key_property] = s3_bucket_key[1] + + return result + raise ValueError("URL given to the parse method is not a valid path-style S3 url " "{0}".format(url)) + class ProgressPercentage: # This class was copied directly from S3Transfer docs diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 750317ed209..62e185c7f3b 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -241,6 +241,28 @@ def test_parse_s3_url(self): with self.assertRaises(ValueError): S3Uploader.parse_s3_url(url) + def test_parse_path_style_s3_url(self): + valid = [ + {"url": "https://s3-eu-west-1.amazonaws.com/bucket/long/key", "result": {"Bucket": "bucket", "Key": "long/key"}}, + {"url": "https://s3.us-east-1.amazonaws.com/bucket/key", "result": {"Bucket": "bucket", "Key": "key"}}, + ] + + invalid = [ + "https://www.amazon.com", + "https://bucket-name.s3.Region.amazonaws.com/key" + ] + + for config in valid: + result = S3Uploader.parse_path_style_s3_url( + config["url"], bucket_name_property="Bucket", object_key_property="Key" + ) + + self.assertEqual(result, config["result"]) + + for url in invalid: + with self.assertRaises(ValueError): + S3Uploader.parse_path_style_s3_url(url) + def test_is_local_file(self): with tempfile.NamedTemporaryFile() as handle: self.assertTrue(is_local_file(handle.name)) From 6ff2e22c6832b22beef985dfd754986a1804c666 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Fri, 9 Jul 2021 15:31:24 -0400 Subject: [PATCH 31/37] run Make black --- samcli/lib/package/s3_uploader.py | 2 +- tests/unit/lib/package/test_artifact_exporter.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index cf67efa4ea2..dfd5db90fe5 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -277,7 +277,7 @@ def parse_path_style_s3_url( parsed = urlparse(url) result = dict() # path would point to /bucket/key - s3_bucket_key = parsed.path.split('/', 2)[1:] + s3_bucket_key = parsed.path.split("/", 2)[1:] result[bucket_name_property] = s3_bucket_key[0] result[object_key_property] = s3_bucket_key[1] diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 62e185c7f3b..d452403136d 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -243,14 +243,14 @@ def test_parse_s3_url(self): def test_parse_path_style_s3_url(self): valid = [ - {"url": "https://s3-eu-west-1.amazonaws.com/bucket/long/key", "result": {"Bucket": "bucket", "Key": "long/key"}}, + { + "url": "https://s3-eu-west-1.amazonaws.com/bucket/long/key", + "result": {"Bucket": "bucket", "Key": "long/key"}, + }, {"url": "https://s3.us-east-1.amazonaws.com/bucket/key", "result": {"Bucket": "bucket", "Key": "key"}}, ] - invalid = [ - "https://www.amazon.com", - "https://bucket-name.s3.Region.amazonaws.com/key" - ] + invalid = ["https://www.amazon.com", "https://bucket-name.s3.Region.amazonaws.com/key"] for config in valid: result = S3Uploader.parse_path_style_s3_url( From 6c9a060fa886832b9cd04198d524430d6a40d06a Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Sat, 10 Jul 2021 17:46:48 -0400 Subject: [PATCH 32/37] Made the parse s3 url funcs protected and defined a parent method and modified delete method for ResourceImageDict --- samcli/lib/package/packageable_resources.py | 15 ++--- samcli/lib/package/s3_uploader.py | 61 +++++++++++++------ .../lib/package/test_artifact_exporter.py | 39 ++++-------- 3 files changed, 59 insertions(+), 56 deletions(-) diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 15ce8fc3613..39d9b738671 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -165,14 +165,7 @@ def delete(self, resource_id, resource_dict): if resource_dict is None: return resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict) - parsed_s3_url = [] - if isinstance(resource_path, str) and resource_path.startswith("https://s3"): - # Path-style s3 url parsing for resources that return these urls - # For resources e.g. CloudFormation::Stack and Serverless::Application - parsed_s3_url = self.uploader.parse_path_style_s3_url(resource_path) - else: - # urls which start with s3:// - parsed_s3_url = self.uploader.parse_s3_url(resource_path) + parsed_s3_url = self.uploader.parse_s3_url(resource_path) if not self.uploader.bucket_name: self.uploader.bucket_name = parsed_s3_url["Bucket"] self.uploader.delete_artifact(parsed_s3_url["Key"], True) @@ -227,13 +220,13 @@ def delete(self, resource_id, resource_dict): if resource_dict is None: return - remote_path = resource_dict[self.PROPERTY_NAME][self.EXPORT_PROPERTY_CODE_KEY] + remote_path = resource_dict.get(self.PROPERTY_NAME, {}).get(self.EXPORT_PROPERTY_CODE_KEY) if is_ecr_url(remote_path): self.uploader.delete_artifact( image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME ) else: - raise ValueError("URL given to the parse method is not a valid ECR url " "{0}".format(remote_path)) + raise ValueError("URL given to the parse method is not a valid ECR url {0}".format(remote_path)) class ResourceImage(Resource): @@ -289,7 +282,7 @@ def delete(self, resource_id, resource_dict): image_uri=remote_path, resource_id=resource_id, property_name=self.PROPERTY_NAME ) else: - raise ValueError("URL given to the parse method is not a valid ECR url " "{0}".format(remote_path)) + raise ValueError("URL given to the parse method is not a valid ECR url {0}".format(remote_path)) class ResourceWithS3UrlDict(ResourceZip): diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index dfd5db90fe5..b3fbe53c7db 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -243,28 +243,51 @@ def parse_s3_url( object_key_property: str = "Key", version_property: Optional[str] = None, ) -> Dict: - if isinstance(url, str) and url.startswith("s3://"): - parsed = urlparse(url) - query = parse_qs(parsed.query) + return S3Uploader._parse_s3_format_url( + url=url, + bucket_name_property=bucket_name_property, + object_key_property=object_key_property, + version_property=version_property, + ) + + if isinstance(url, str) and url.startswith("https://s3"): + return S3Uploader._parse_path_style_s3_url( + url=url, bucket_name_property=bucket_name_property, object_key_property=object_key_property + ) - if parsed.netloc and parsed.path: - result = dict() - result[bucket_name_property] = parsed.netloc - result[object_key_property] = parsed.path.lstrip("/") + raise ValueError("URL given to the parse method is not a valid S3 url {0}".format(url)) + + @staticmethod + def _parse_s3_format_url( + url: Any, + bucket_name_property: str = "Bucket", + object_key_property: str = "Key", + version_property: Optional[str] = None, + ) -> Dict: + """ + Method for parsing s3 urls that begin with s3:// + e.g. s3://bucket/key + """ + parsed = urlparse(url) + query = parse_qs(parsed.query) + if parsed.netloc and parsed.path: + result = dict() + result[bucket_name_property] = parsed.netloc + result[object_key_property] = parsed.path.lstrip("/") - # If there is a query string that has a single versionId field, - # set the object version and return - if version_property is not None and "versionId" in query and len(query["versionId"]) == 1: - result[version_property] = query["versionId"][0] + # If there is a query string that has a single versionId field, + # set the object version and return + if version_property is not None and "versionId" in query and len(query["versionId"]) == 1: + result[version_property] = query["versionId"][0] - return result + return result - raise ValueError("URL given to the parse method is not a valid S3 url " "{0}".format(url)) + raise ValueError("URL given to the parse method is not a valid S3 url {0}".format(url)) @staticmethod - def parse_path_style_s3_url( + def _parse_path_style_s3_url( url: Any, bucket_name_property: str = "Bucket", object_key_property: str = "Key", @@ -273,17 +296,17 @@ def parse_path_style_s3_url( Static method for parsing path style s3 urls. e.g. https://s3.us-east-1.amazonaws.com/bucket/key """ - if isinstance(url, str) and url.startswith("https://s3"): - parsed = urlparse(url) - result = dict() - # path would point to /bucket/key + parsed = urlparse(url) + result = dict() + # parsed.path would point to /bucket/key + if parsed.path: s3_bucket_key = parsed.path.split("/", 2)[1:] result[bucket_name_property] = s3_bucket_key[0] result[object_key_property] = s3_bucket_key[1] return result - raise ValueError("URL given to the parse method is not a valid path-style S3 url " "{0}".format(url)) + raise ValueError("URL given to the parse method is not a valid S3 url {0}".format(url)) class ProgressPercentage: diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index d452403136d..e6b9d14320d 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -181,14 +181,14 @@ def test_is_s3_url(self): "s3://foo/bar/baz?versionId=abc", "s3://www.amazon.com/foo/bar", "s3://my-new-bucket/foo/bar?a=1&a=2&a=3&b=1", + "https://s3-eu-west-1.amazonaws.com/bucket/key", + "https://s3.us-east-1.amazonaws.com/bucket/key", ] invalid = [ # For purposes of exporter, we need S3 URLs to point to an object # and not a bucket "s3://foo", - # two versionIds is invalid - "https://s3-eu-west-1.amazonaws.com/bucket/key", "https://www.amazon.com", ] @@ -219,15 +219,24 @@ def test_parse_s3_url(self): "url": "s3://foo/bar/baz?versionId=abc&versionId=123", "result": {"Bucket": "foo", "Key": "bar/baz"}, }, + { + # Path style url + "url": "https://s3-eu-west-1.amazonaws.com/bucket/key", + "result": {"Bucket": "bucket", "Key": "key"}, + }, + { + # Path style url + "url": "https://s3.us-east-1.amazonaws.com/bucket/key", + "result": {"Bucket": "bucket", "Key": "key"}, + }, ] invalid = [ # For purposes of exporter, we need S3 URLs to point to an object # and not a bucket "s3://foo", - # two versionIds is invalid - "https://s3-eu-west-1.amazonaws.com/bucket/key", "https://www.amazon.com", + "https://s3.us-east-1.amazonaws.com", ] for config in valid: @@ -241,28 +250,6 @@ def test_parse_s3_url(self): with self.assertRaises(ValueError): S3Uploader.parse_s3_url(url) - def test_parse_path_style_s3_url(self): - valid = [ - { - "url": "https://s3-eu-west-1.amazonaws.com/bucket/long/key", - "result": {"Bucket": "bucket", "Key": "long/key"}, - }, - {"url": "https://s3.us-east-1.amazonaws.com/bucket/key", "result": {"Bucket": "bucket", "Key": "key"}}, - ] - - invalid = ["https://www.amazon.com", "https://bucket-name.s3.Region.amazonaws.com/key"] - - for config in valid: - result = S3Uploader.parse_path_style_s3_url( - config["url"], bucket_name_property="Bucket", object_key_property="Key" - ) - - self.assertEqual(result, config["result"]) - - for url in invalid: - with self.assertRaises(ValueError): - S3Uploader.parse_path_style_s3_url(url) - def test_is_local_file(self): with tempfile.NamedTemporaryFile() as handle: self.assertTrue(is_local_file(handle.name)) From 49e0967683a4729c73c7b9118c860affaabcd4fc Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Sun, 11 Jul 2021 15:33:38 -0400 Subject: [PATCH 33/37] Added methods to extract s3 info from cf template --- samcli/commands/delete/delete_context.py | 17 +++++++-- samcli/lib/package/artifact_exporter.py | 42 ++++++++++++++++++++- samcli/lib/package/packageable_resources.py | 33 ++++++++++++---- 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 64913072470..dcfd9071b07 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -149,6 +149,20 @@ def delete(self): with mktempfile() as temp_file: self.cf_template_file_name = get_cf_template_name(temp_file, template_str, "template") + template = Template( + template_path=None, parent_dir=None, uploaders=self.uploaders, code_signer=None, template_str=template_str + ) + + # If s3 info is not available, try to obtain it from CF + # template resources. + if not self.s3_bucket: + s3_info = template.get_s3_info() + self.s3_bucket = s3_info["s3_bucket"] + self.s3_uploader.bucket_name = self.s3_bucket + + self.s3_prefix = s3_info["s3_prefix"] + self.s3_uploader.prefix = self.s3_prefix + self.guided_prompts() # Delete the primary stack @@ -158,9 +172,6 @@ def delete(self): LOG.debug("Deleted Cloudformation stack: %s", self.stack_name) # Delete the artifacts - template = Template( - template_path=None, parent_dir=None, uploaders=self.uploaders, code_signer=None, template_str=template_str - ) template.delete() # Delete the CF template file in S3 diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 00fa5cb089d..8c58ff4f896 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -35,7 +35,7 @@ ResourceZip, ) from samcli.lib.package.s3_uploader import S3Uploader -from samcli.lib.package.uploaders import Uploaders +from samcli.lib.package.uploaders import Uploaders, Destination from samcli.lib.package.utils import ( is_local_folder, make_abs_path, @@ -265,3 +265,43 @@ def delete(self): # Delete code resources exporter = exporter_class(self.uploaders, None) exporter.delete(resource_id, resource_dict) + + def get_s3_info(self): + """ + Iterates the template_dict resources with S3 EXPORT_DESTINATION to get the + s3_bucket and s3_prefix information for the purpose of deletion. + """ + result = {"s3_bucket": None, "s3_prefix": None} + if "Resources" not in self.template_dict: + return result + + self._apply_global_values() + + for _, resource in self.template_dict["Resources"].items(): + + resource_type = resource.get("Type", None) + resource_dict = resource.get("Properties", {}) + + for exporter_class in self.resources_to_export: + # Skip the resources which don't give s3 information + if exporter_class.EXPORT_DESTINATION != Destination.S3: + continue + if exporter_class.RESOURCE_TYPE != resource_type: + continue + if resource_dict.get("PackageType", ZIP) != exporter_class.ARTIFACT_TYPE: + continue + + exporter = exporter_class(self.uploaders, None) + s3_info = exporter.get_s3_info(resource_dict) + + result["s3_bucket"] = s3_info["Bucket"] + s3_key = s3_info["Key"] + + # Extract the prefix from the key + if s3_key: + key_split = s3_key.rsplit("/", 1) + if len(key_split) > 1: + result["s3_prefix"] = key_split[0] + break + + return result diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 39d9b738671..ba74b9bf81e 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -164,11 +164,19 @@ def delete(self, resource_id, resource_dict): """ if resource_dict is None: return + + s3_info = self.get_s3_info(resource_dict) + self.uploader.delete_artifact(s3_info["Key"], True) + + def get_s3_info(self, resource_dict): + """ + Get the s3 information from this resource + """ + if resource_dict is None: + return {"Bucket": None, "Key": None} + resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict) - parsed_s3_url = self.uploader.parse_s3_url(resource_path) - if not self.uploader.bucket_name: - self.uploader.bucket_name = parsed_s3_url["Bucket"] - self.uploader.delete_artifact(parsed_s3_url["Key"], True) + return self.uploader.parse_s3_url(resource_path) class ResourceImageDict(Resource): @@ -322,13 +330,22 @@ def delete(self, resource_id, resource_dict): """ if resource_dict is None: return + + s3_info = self.get_s3_info(resource_dict) + self.uploader.delete_artifact(remote_path=s3_info["Key"], is_key=True) + + def get_s3_info(self, resource_dict): + """ + Get the s3 information from this resource + """ + if resource_dict is None: + return {"Bucket": None, "Key": None} + resource_path = resource_dict[self.PROPERTY_NAME] s3_bucket = resource_path[self.BUCKET_NAME_PROPERTY] - key = resource_path[self.OBJECT_KEY_PROPERTY] - if not self.uploader.bucket_name: - self.uploader.bucket_name = s3_bucket - self.uploader.delete_artifact(remote_path=key, is_key=True) + key = resource_path[self.OBJECT_KEY_PROPERTY] + return {"Bucket": s3_bucket, "Key": key} class ServerlessFunctionResource(ResourceZip): From 44508863e513da1b6dd481039ff352ddee38304a Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Sun, 11 Jul 2021 21:51:54 -0400 Subject: [PATCH 34/37] Added testing for get_s3_info method for artifact_exporter and s3_uploader methods --- samcli/lib/package/artifact_exporter.py | 7 ++- .../lib/package/test_artifact_exporter.py | 52 ++++++++++++++++++- tests/unit/lib/package/test_s3_uploader.py | 29 +++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 8c58ff4f896..8d23660b90f 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -18,6 +18,7 @@ import os from typing import Dict, Optional +# import logging from botocore.utils import set_value_from_jmespath from samcli.commands._utils.resources import ( @@ -47,7 +48,7 @@ from samcli.lib.utils.packagetype import ZIP from samcli.yamlhelper import yaml_parse, yaml_dump - +# LOG = logging.getLogger(__name__) # NOTE: sriram-mv, A cyclic dependency on `Template` needs to be broken. @@ -270,6 +271,8 @@ def get_s3_info(self): """ Iterates the template_dict resources with S3 EXPORT_DESTINATION to get the s3_bucket and s3_prefix information for the purpose of deletion. + Method finds the first resource with s3 information, extracts the information + and then terminates. """ result = {"s3_bucket": None, "s3_prefix": None} if "Resources" not in self.template_dict: @@ -303,5 +306,7 @@ def get_s3_info(self): if len(key_split) > 1: result["s3_prefix"] = key_split[0] break + if result["s3_bucket"]: + break return result diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index e6b9d14320d..0eb9042263a 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -14,7 +14,7 @@ from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.uploaders import Destination from samcli.lib.package.utils import zip_folder, make_zip -from samcli.lib.utils.packagetype import ZIP +from samcli.lib.utils.packagetype import ZIP, IMAGE from tests.testing_utils import FileCreator from samcli.commands.package import exceptions from samcli.lib.package.artifact_exporter import ( @@ -1401,7 +1401,6 @@ def example_yaml_template(self): """ def test_template_delete(self): - template_str = self.example_yaml_template() resource_type1_class = Mock() resource_type1_class.RESOURCE_TYPE = "resource_type1" @@ -1451,3 +1450,52 @@ def test_template_delete(self): resource_type2_instance.delete.assert_called_once_with("Resource2", mock.ANY) resource_type3_class.assert_not_called() resource_type3_instance.delete.assert_not_called() + + def test_template_get_s3_info(self): + + resource_type1_class = Mock() + resource_type1_class.RESOURCE_TYPE = "resource_type1" + resource_type1_class.ARTIFACT_TYPE = ZIP + resource_type1_class.PROPERTY_NAME = "CodeUri" + resource_type1_class.EXPORT_DESTINATION = Destination.S3 + resource_type1_instance = Mock() + resource_type1_class.return_value = resource_type1_instance + resource_type1_instance.get_s3_info = Mock() + resource_type1_instance.get_s3_info.return_value = {"Bucket": "bucket", "Key": "prefix/file"} + + resource_type2_class = Mock() + resource_type2_class.RESOURCE_TYPE = "resource_type2" + resource_type2_class.ARTIFACT_TYPE = ZIP + resource_type2_class.EXPORT_DESTINATION = Destination.S3 + resource_type2_instance = Mock() + resource_type2_class.return_value = resource_type2_instance + + resource_type3_class = Mock() + resource_type3_class.RESOURCE_TYPE = "resource_type3" + resource_type3_class.ARTIFACT_TYPE = IMAGE + resource_type3_class.EXPORT_DESTINATION = Destination.ECR + resource_type3_instance = Mock() + resource_type3_class.return_value = resource_type3_instance + + resources_to_export = [resource_type3_class, resource_type2_class, resource_type1_class] + + properties = {"foo": "bar", "CodeUri": "s3://bucket/prefix/file"} + template_dict = { + "Resources": { + "Resource1": {"Type": "resource_type1", "Properties": properties}, + } + } + template_str = json.dumps(template_dict, indent=4, ensure_ascii=False) + + template_exporter = Template( + template_path=None, + parent_dir=None, + uploaders=self.uploaders_mock, + code_signer=None, + resources_to_export=resources_to_export, + template_str=template_str, + ) + + s3_info = template_exporter.get_s3_info() + self.assertEqual(s3_info, {"s3_bucket": "bucket", "s3_prefix": "prefix"}) + resource_type1_instance.get_s3_info.assert_called_once_with(properties) diff --git a/tests/unit/lib/package/test_s3_uploader.py b/tests/unit/lib/package/test_s3_uploader.py index f1765c3f8ce..a28cd33bf53 100644 --- a/tests/unit/lib/package/test_s3_uploader.py +++ b/tests/unit/lib/package/test_s3_uploader.py @@ -217,6 +217,35 @@ def test_s3_delete_artifact_bucket_not_found(self): with self.assertRaises(NoSuchBucketError): s3_uploader.delete_artifact(f.name) + def test_delete_prefix_artifacts_no_bucket(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=None, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=self.force_upload, + no_progressbar=self.no_progressbar, + ) + with self.assertRaises(BucketNotSpecifiedError): + s3_uploader.delete_prefix_artifacts() + + def test_delete_prefix_artifacts_execute(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=self.bucket_name, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=self.force_upload, + no_progressbar=self.no_progressbar, + ) + + s3_uploader.s3.delete_object = MagicMock() + + s3_uploader.s3.list_objects_v2 = MagicMock(return_value={"Contents": [{"Key": "key"}]}) + + s3_uploader.delete_prefix_artifacts() + s3_uploader.s3.delete_object.assert_called_once_with(Bucket="mock-bucket", Key="key") + def test_s3_upload_with_dedup(self): s3_uploader = S3Uploader( s3_client=self.s3, From 29add30c1b5dc315d866bb390fbef98531399e0a Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Thu, 15 Jul 2021 20:04:17 -0400 Subject: [PATCH 35/37] Removed commented code and updated method docstring --- samcli/lib/package/artifact_exporter.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 8d23660b90f..791873fe655 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -18,7 +18,6 @@ import os from typing import Dict, Optional -# import logging from botocore.utils import set_value_from_jmespath from samcli.commands._utils.resources import ( @@ -48,7 +47,6 @@ from samcli.lib.utils.packagetype import ZIP from samcli.yamlhelper import yaml_parse, yaml_dump -# LOG = logging.getLogger(__name__) # NOTE: sriram-mv, A cyclic dependency on `Template` needs to be broken. @@ -272,7 +270,8 @@ def get_s3_info(self): Iterates the template_dict resources with S3 EXPORT_DESTINATION to get the s3_bucket and s3_prefix information for the purpose of deletion. Method finds the first resource with s3 information, extracts the information - and then terminates. + and then terminates. It is safe to assume that all the packaged files using the + commands package and deploy are in the same s3 bucket with the same s3 prefix. """ result = {"s3_bucket": None, "s3_prefix": None} if "Resources" not in self.template_dict: From 308f859eb3fc7b0964d4105d7093071462199c3f Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Sat, 17 Jul 2021 18:11:04 -0400 Subject: [PATCH 36/37] Better error handling for s3 delete artifacts and fixed bug for getting s3 resources information --- samcli/lib/package/packageable_resources.py | 16 ++++++++++------ samcli/lib/package/s3_uploader.py | 16 +++++++++++----- tests/unit/lib/package/test_s3_uploader.py | 20 ++++++++++++++++++-- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index ba74b9bf81e..4b621fa036c 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -166,7 +166,8 @@ def delete(self, resource_id, resource_dict): return s3_info = self.get_s3_info(resource_dict) - self.uploader.delete_artifact(s3_info["Key"], True) + if s3_info["Key"]: + self.uploader.delete_artifact(s3_info["Key"], True) def get_s3_info(self, resource_dict): """ @@ -176,7 +177,9 @@ def get_s3_info(self, resource_dict): return {"Bucket": None, "Key": None} resource_path = jmespath.search(self.PROPERTY_NAME, resource_dict) - return self.uploader.parse_s3_url(resource_path) + if resource_path: + return self.uploader.parse_s3_url(resource_path) + return {"Bucket": None, "Key": None} class ResourceImageDict(Resource): @@ -332,7 +335,8 @@ def delete(self, resource_id, resource_dict): return s3_info = self.get_s3_info(resource_dict) - self.uploader.delete_artifact(remote_path=s3_info["Key"], is_key=True) + if s3_info["Key"]: + self.uploader.delete_artifact(remote_path=s3_info["Key"], is_key=True) def get_s3_info(self, resource_dict): """ @@ -341,10 +345,10 @@ def get_s3_info(self, resource_dict): if resource_dict is None: return {"Bucket": None, "Key": None} - resource_path = resource_dict[self.PROPERTY_NAME] - s3_bucket = resource_path[self.BUCKET_NAME_PROPERTY] + resource_path = resource_dict.get(self.PROPERTY_NAME, {}) + s3_bucket = resource_path.get(self.BUCKET_NAME_PROPERTY, None) - key = resource_path[self.OBJECT_KEY_PROPERTY] + key = resource_path.get(self.OBJECT_KEY_PROPERTY, None) return {"Bucket": s3_bucket, "Key": key} diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index b3fbe53c7db..5b8c5553660 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -145,7 +145,7 @@ def upload_with_dedup( return self.upload(file_name, remote_path) - def delete_artifact(self, remote_path: str, is_key: bool = False) -> Dict: + def delete_artifact(self, remote_path: str, is_key: bool = False) -> bool: """ Deletes a given file from S3 :param remote_path: Path to the file that will be deleted @@ -163,10 +163,16 @@ def delete_artifact(self, remote_path: str, is_key: bool = False) -> Dict: key = "{0}/{1}".format(self.prefix, remote_path) # Deleting Specific file with key - click.echo(f"\t- Deleting S3 file {key}") - resp = self.s3.delete_object(Bucket=self.bucket_name, Key=key) - LOG.debug("S3 method delete_object is called and returned: %s", resp["ResponseMetadata"]) - return dict(resp["ResponseMetadata"]) + if self.file_exists(remote_path=key): + click.echo(f"\t- Deleting S3 object with key {key} in the bucket {self.bucket_name}") + self.s3.delete_object(Bucket=self.bucket_name, Key=key) + LOG.debug("Deleted s3 object with key %s successfully", key) + return True + + # Given s3 object key does not exist + LOG.debug("Could not find the S3 file with the key %s", key) + click.echo(f"\t- Could not find and delete the S3 object with the key {key}") + return False except botocore.exceptions.ClientError as ex: error_code = ex.response["Error"]["Code"] diff --git a/tests/unit/lib/package/test_s3_uploader.py b/tests/unit/lib/package/test_s3_uploader.py index a28cd33bf53..55b1bfbb2d4 100644 --- a/tests/unit/lib/package/test_s3_uploader.py +++ b/tests/unit/lib/package/test_s3_uploader.py @@ -181,10 +181,26 @@ def test_s3_delete_artifact(self): force_upload=self.force_upload, no_progressbar=self.no_progressbar, ) - s3_uploader.artifact_metadata = {"a": "b"} + self.s3.delete_object = MagicMock() + self.s3.head_object = MagicMock() + with self.assertRaises(BucketNotSpecifiedError) as ex: + with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: + self.assertTrue(s3_uploader.delete_artifact(f.name)) + + def test_s3_delete_non_existant_artifact(self): + s3_uploader = S3Uploader( + s3_client=self.s3, + bucket_name=None, + prefix=self.prefix, + kms_key_id=self.kms_key_id, + force_upload=self.force_upload, + no_progressbar=self.no_progressbar, + ) + self.s3.delete_object = MagicMock() + self.s3.head_object = MagicMock(side_effect=ClientError(error_response={}, operation_name="head_object")) with self.assertRaises(BucketNotSpecifiedError) as ex: with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: - self.assertEqual(s3_uploader.delete_artifact(f.name), {"a": "b"}) + self.assertFalse(s3_uploader.delete_artifact(f.name)) def test_s3_delete_artifact_no_bucket(self): s3_uploader = S3Uploader( From 4275e349722c13dbb9fb91352237de0a28532072 Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Mon, 19 Jul 2021 11:09:16 -0400 Subject: [PATCH 37/37] Changed get_s3_info to get_property_value and changed output text for s3 delete method --- samcli/lib/package/artifact_exporter.py | 2 +- samcli/lib/package/packageable_resources.py | 12 ++++++------ samcli/lib/package/s3_uploader.py | 8 ++++---- tests/unit/lib/package/test_artifact_exporter.py | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/samcli/lib/package/artifact_exporter.py b/samcli/lib/package/artifact_exporter.py index 791873fe655..99567558e8f 100644 --- a/samcli/lib/package/artifact_exporter.py +++ b/samcli/lib/package/artifact_exporter.py @@ -294,7 +294,7 @@ def get_s3_info(self): continue exporter = exporter_class(self.uploaders, None) - s3_info = exporter.get_s3_info(resource_dict) + s3_info = exporter.get_property_value(resource_dict) result["s3_bucket"] = s3_info["Bucket"] s3_key = s3_info["Key"] diff --git a/samcli/lib/package/packageable_resources.py b/samcli/lib/package/packageable_resources.py index 4b621fa036c..49f0e476538 100644 --- a/samcli/lib/package/packageable_resources.py +++ b/samcli/lib/package/packageable_resources.py @@ -165,13 +165,13 @@ def delete(self, resource_id, resource_dict): if resource_dict is None: return - s3_info = self.get_s3_info(resource_dict) + s3_info = self.get_property_value(resource_dict) if s3_info["Key"]: self.uploader.delete_artifact(s3_info["Key"], True) - def get_s3_info(self, resource_dict): + def get_property_value(self, resource_dict): """ - Get the s3 information from this resource + Get the s3 property value for this resource """ if resource_dict is None: return {"Bucket": None, "Key": None} @@ -334,13 +334,13 @@ def delete(self, resource_id, resource_dict): if resource_dict is None: return - s3_info = self.get_s3_info(resource_dict) + s3_info = self.get_property_value(resource_dict) if s3_info["Key"]: self.uploader.delete_artifact(remote_path=s3_info["Key"], is_key=True) - def get_s3_info(self, resource_dict): + def get_property_value(self, resource_dict): """ - Get the s3 information from this resource + Get the s3 property value for this resource """ if resource_dict is None: return {"Bucket": None, "Key": None} diff --git a/samcli/lib/package/s3_uploader.py b/samcli/lib/package/s3_uploader.py index 5b8c5553660..a7f1a9a8b97 100644 --- a/samcli/lib/package/s3_uploader.py +++ b/samcli/lib/package/s3_uploader.py @@ -164,7 +164,7 @@ def delete_artifact(self, remote_path: str, is_key: bool = False) -> bool: # Deleting Specific file with key if self.file_exists(remote_path=key): - click.echo(f"\t- Deleting S3 object with key {key} in the bucket {self.bucket_name}") + click.echo(f"\t- Deleting S3 object with key {key}") self.s3.delete_object(Bucket=self.bucket_name, Key=key) LOG.debug("Deleted s3 object with key %s successfully", key) return True @@ -189,9 +189,9 @@ def delete_prefix_artifacts(self): LOG.error("Bucket not specified") raise BucketNotSpecifiedError() if self.prefix: - prefix_files = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix) - - for obj in prefix_files["Contents"]: + response = self.s3.list_objects_v2(Bucket=self.bucket_name, Prefix=self.prefix) + prefix_files = response.get("Contents", []) + for obj in prefix_files: self.delete_artifact(obj["Key"], True) def file_exists(self, remote_path: str) -> bool: diff --git a/tests/unit/lib/package/test_artifact_exporter.py b/tests/unit/lib/package/test_artifact_exporter.py index 0eb9042263a..0fea4bf3cbd 100644 --- a/tests/unit/lib/package/test_artifact_exporter.py +++ b/tests/unit/lib/package/test_artifact_exporter.py @@ -1460,8 +1460,8 @@ def test_template_get_s3_info(self): resource_type1_class.EXPORT_DESTINATION = Destination.S3 resource_type1_instance = Mock() resource_type1_class.return_value = resource_type1_instance - resource_type1_instance.get_s3_info = Mock() - resource_type1_instance.get_s3_info.return_value = {"Bucket": "bucket", "Key": "prefix/file"} + resource_type1_instance.get_property_value = Mock() + resource_type1_instance.get_property_value.return_value = {"Bucket": "bucket", "Key": "prefix/file"} resource_type2_class = Mock() resource_type2_class.RESOURCE_TYPE = "resource_type2" @@ -1498,4 +1498,4 @@ def test_template_get_s3_info(self): s3_info = template_exporter.get_s3_info() self.assertEqual(s3_info, {"s3_bucket": "bucket", "s3_prefix": "prefix"}) - resource_type1_instance.get_s3_info.assert_called_once_with(properties) + resource_type1_instance.get_property_value.assert_called_once_with(properties)