-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add possibility to ignore specific unsupported resource types in CloudFormation #13496
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| from localstack.services.cloudformation.engine.v2.change_set_model import ( | ||
| NodeResource, | ||
| ) | ||
| from localstack.aws.api.cloudformation import ChangeSetType | ||
| from localstack.services.cloudformation.engine.v2.change_set_model import NodeResource | ||
| from localstack.services.cloudformation.engine.v2.change_set_model_visitor import ( | ||
| ChangeSetModelVisitor, | ||
| ) | ||
| from localstack.services.cloudformation.engine.v2.unsupported_resource import ( | ||
| should_ignore_unsupported_resource_type, | ||
| ) | ||
| from localstack.services.cloudformation.resources import AWS_AVAILABLE_CFN_RESOURCES | ||
| from localstack.utils.catalog.catalog import ( | ||
| AwsServicesSupportStatus, | ||
|
|
@@ -81,17 +83,23 @@ def _build_resource_failure_message( | |
|
|
||
|
|
||
| class ChangeSetResourceSupportChecker(ChangeSetModelVisitor): | ||
| change_set_type: ChangeSetType | str | None | ||
| catalog: CatalogPlugin | ||
|
|
||
| TITLE_MESSAGE = "Unsupported resources detected:" | ||
|
|
||
| def __init__(self): | ||
| def __init__(self, change_set_type: ChangeSetType | str | None = None): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it would be nice to not need the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah you're right. I think in some place I defined it as a string once and then had curly lines below, which I then auto-fixed. Just setting |
||
| self._resource_failure_messages: dict[str, str] = {} | ||
| self.change_set_type = change_set_type | ||
| self.catalog = get_aws_catalog() | ||
|
|
||
| def visit_node_resource(self, node_resource: NodeResource): | ||
| resource_type = node_resource.type_.value | ||
| if resource_type not in self._resource_failure_messages: | ||
| ignore_unsupported = should_ignore_unsupported_resource_type( | ||
| resource_type=resource_type, change_set_type=self.change_set_type | ||
| ) | ||
|
|
||
| if resource_type not in self._resource_failure_messages and not ignore_unsupported: | ||
| if resource_type not in AWS_AVAILABLE_CFN_RESOURCES: | ||
| # Ignore non-AWS resources | ||
| pass | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from localstack import config | ||
| from localstack.aws.api.cloudformation import ChangeSetType | ||
|
|
||
|
|
||
| def should_ignore_unsupported_resource_type( | ||
| resource_type: str, change_set_type: ChangeSetType | str | None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: as stated above, under what circumstances is |
||
| ) -> bool: | ||
| if config.CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES: | ||
| return True | ||
|
|
||
| match change_set_type: | ||
| case ChangeSetType.CREATE: | ||
| return resource_type in config.CFN_IGNORE_UNSUPPORTED_TYPE_CREATE | ||
| case ChangeSetType.UPDATE | ChangeSetType.IMPORT: | ||
| return resource_type in config.CFN_IGNORE_UNSUPPORTED_TYPE_UPDATE | ||
| case _: | ||
| return False | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -86,6 +86,97 @@ def testing_catalog(monkeypatch): | |||||||||||||||||||||||||||
| return plugin | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @markers.aws.only_localstack | ||||||||||||||||||||||||||||
| def test_ignore_unsupported_resources_toggle(testing_catalog, aws_client, monkeypatch, cleanups): | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: in general I would prefer not defining inner functions (especially if they only have one use), and to use the waiters where possible. I don't find them easy to use, and the waiters code is more straightforward to read in the test itself. For the successful cases we swap a function definition and call with a single line, and for the failure cases using the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks I didn't like it too much either but also didn't know about the waiters. Will switch it over :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've fixed it and also went ahead and fixed the old test to use the waiters. It's much cleaner now :) |
||||||||||||||||||||||||||||
| unsupported_resource = "AWS::LatestService::NotSupported" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def _create_change_set(stack_name: str, template_body: str): | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit I personally don't like this helper and would prefer the actual cfn function calls in the test, however I can see it's value as we perform this operation twice. |
||||||||||||||||||||||||||||
| change_set_name = f"cs-{short_uid()}" | ||||||||||||||||||||||||||||
| response = aws_client.cloudformation.create_change_set( | ||||||||||||||||||||||||||||
| StackName=stack_name, | ||||||||||||||||||||||||||||
| ChangeSetName=change_set_name, | ||||||||||||||||||||||||||||
| TemplateBody=template_body, | ||||||||||||||||||||||||||||
| ChangeSetType="CREATE", | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| return response["Id"], response["StackId"] | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def _wait_for_failed_change_set(change_set_id: str): | ||||||||||||||||||||||||||||
| def _describe(): | ||||||||||||||||||||||||||||
| result = aws_client.cloudformation.describe_change_set(ChangeSetName=change_set_id) | ||||||||||||||||||||||||||||
| status = result["Status"] | ||||||||||||||||||||||||||||
| if status == "FAILED": | ||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||
| if status == "CREATE_COMPLETE": | ||||||||||||||||||||||||||||
| pytest.fail("expected change set creation to fail") | ||||||||||||||||||||||||||||
| raise Exception("still waiting for change set to fail") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return retry(_describe, retries=15, sleep=2) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def _wait_for_complete_change_set(change_set_id: str): | ||||||||||||||||||||||||||||
| def _describe(): | ||||||||||||||||||||||||||||
| result = aws_client.cloudformation.describe_change_set(ChangeSetName=change_set_id) | ||||||||||||||||||||||||||||
| status = result["Status"] | ||||||||||||||||||||||||||||
| if status == "CREATE_COMPLETE": | ||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||
| if status == "FAILED": | ||||||||||||||||||||||||||||
| pytest.fail(f"change set unexpectedly failed: {result.get('StatusReason')}") | ||||||||||||||||||||||||||||
| raise Exception("still waiting for change set") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return retry(_describe, retries=15, sleep=2) | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: this function can be replaced by |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def _wait_for_stack_status(stack_name: str, expected: str): | ||||||||||||||||||||||||||||
| def _describe(): | ||||||||||||||||||||||||||||
| stack = aws_client.cloudformation.describe_stacks(StackName=stack_name)["Stacks"][0] | ||||||||||||||||||||||||||||
| status = stack["StackStatus"] | ||||||||||||||||||||||||||||
| if status == expected: | ||||||||||||||||||||||||||||
| return stack | ||||||||||||||||||||||||||||
| if status.endswith("FAILED") or "ROLLBACK" in status: | ||||||||||||||||||||||||||||
| pytest.fail(f"stack ended in failure: {status} ({stack.get('StackStatusReason')})") | ||||||||||||||||||||||||||||
| raise Exception("still waiting for stack") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return retry(_describe, retries=30, sleep=2) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # template with one supported and one unsupported resource | ||||||||||||||||||||||||||||
| bucket_name = f"cfn-toggle-{short_uid()}" | ||||||||||||||||||||||||||||
| template_body = textwrap.dedent( | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: for inline templates like this, using a dictionary and rendering to a JSON string might be nicer |
||||||||||||||||||||||||||||
| f""" | ||||||||||||||||||||||||||||
| AWSTemplateFormatVersion: '2010-09-09' | ||||||||||||||||||||||||||||
| Resources: | ||||||||||||||||||||||||||||
| SupportedBucket: | ||||||||||||||||||||||||||||
| Type: AWS::S3::Bucket | ||||||||||||||||||||||||||||
| Properties: | ||||||||||||||||||||||||||||
| BucketName: {bucket_name} | ||||||||||||||||||||||||||||
| Unsupported: | ||||||||||||||||||||||||||||
| Type: {unsupported_resource} | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # 1) ignore lists empty -> change set should fail | ||||||||||||||||||||||||||||
| monkeypatch.setattr(config, "CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES", False) | ||||||||||||||||||||||||||||
| monkeypatch.setattr(config, "CFN_IGNORE_UNSUPPORTED_TYPE_CREATE", []) | ||||||||||||||||||||||||||||
| stack_name_fail = f"stack-fail-{short_uid()}" | ||||||||||||||||||||||||||||
| cs_id_fail, stack_id_fail = _create_change_set(stack_name_fail, template_body) | ||||||||||||||||||||||||||||
| failed_cs = _wait_for_failed_change_set(cs_id_fail) | ||||||||||||||||||||||||||||
| status_reason = failed_cs.get("StatusReason", "") | ||||||||||||||||||||||||||||
| assert ChangeSetResourceSupportChecker.TITLE_MESSAGE in status_reason | ||||||||||||||||||||||||||||
| assert unsupported_resource in status_reason | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: then we don't need the inner function
Suggested change
|
||||||||||||||||||||||||||||
| cleanups.append(lambda: aws_client.cloudformation.delete_change_set(ChangeSetName=cs_id_fail)) | ||||||||||||||||||||||||||||
| cleanups.append(lambda: aws_client.cloudformation.delete_stack(StackName=stack_id_fail)) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # 2) add unsupported resource to create ignore list -> deployment succeeds and bucket is present | ||||||||||||||||||||||||||||
| monkeypatch.setattr(config, "CFN_IGNORE_UNSUPPORTED_TYPE_CREATE", [unsupported_resource]) | ||||||||||||||||||||||||||||
| stack_name_ok = f"stack-ok-{short_uid()}" | ||||||||||||||||||||||||||||
| cs_id_ok, stack_id_ok = _create_change_set(stack_name_ok, template_body) | ||||||||||||||||||||||||||||
| _wait_for_complete_change_set(cs_id_ok) | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||
| aws_client.cloudformation.execute_change_set(ChangeSetName=cs_id_ok) | ||||||||||||||||||||||||||||
| _wait_for_stack_status(stack_name_ok, "CREATE_COMPLETE") | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit can we use the waiters here?
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| buckets = aws_client.s3.list_buckets()["Buckets"] | ||||||||||||||||||||||||||||
| assert any(b["Name"] == bucket_name for b in buckets) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| cleanups.append(lambda: aws_client.cloudformation.delete_stack(StackName=stack_id_ok)) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @markers.aws.only_localstack | ||||||||||||||||||||||||||||
| @pytest.mark.parametrize( | ||||||||||||||||||||||||||||
| "unsupported_resource, expected_service", | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.