feat: Add flag for import existing resources#8114
feat: Add flag for import existing resources#8114electrofelix wants to merge 11 commits intoaws:developfrom
Conversation
Pass through option for importing existing resources which allows using Retain for DeletionPolicy on resources to avoid removal on data resources if stack needs to be deleted and recreated. Fixes: aws#1699
161e41b to
dcc51f7
Compare
|
Main use case I have for this is that sometimes the stack gets into a funk and needs to be deleted and recreated, however if there are data resources in the same application stack specifying The workarounds are to move data resources out to separate stacks, which can work for event buses and DBs, but is a pain when working with queues. End up needing to pass a multitude of resource identifiers in via parameters. Using this approach, I can mark queues, event buses (+ archives), and any DBs to be retained and be able to teardown and redeploy without dropping messages/events/data, and at the same time avoid needing a complex orchastrated deployment spanning multiple repositories or needing complex CI jobs to handle what stacks to delete vs deploy. Note: when testing I noticed CloudFormation doesn't handle automatically re-importing I have a fairly rough script that does the same as this code the AWS CLI, which encountered the same issue suggesting there may be a bug in Cloudformation. Appears it fails to remove some information from resources associated with the original stack meaning they still appear to be part of a stack and it refuses to import. Solving this in Cloudformation would automatically fix it for this feature as well. It would be nicer for it to be available built in rather than needing to run the script sometimes: |
|
Thank you for your contribution, @electrofelix. The current code implementation is clear and concise. We just need to verify both forward and backward deployment scenarios that this update would not leave a stack in an unrecoverable state if deployment fail. Would you be able to enhance this further by adding integration tests to cover your specific deployment use case as well? |
|
@vicheey sure, I presume it's just follow the existing integration tests such as https://github.com/aws/aws-sam-cli/blob/develop/tests/integration/deploy/test_deploy_command.py#L1542-L1593 going with the following steps:
Presumably some work around tear will be needed for this scenario? Extend add_left_over_resources_from_stack to cover the additional resource types added? Is this better as a separate test file to cover the import functionality under the deploy tests? Might be a few weeks before I will get back to updating |
|
Do you mind help with the |
Not at all, just delayed getting back to it |
|
Thanks so much for this @electrofelix ! @vicheey is this up for consideration? This would help me a lot move from terraform to SAM for some resources. Thanks! |
|
Hi @electrofelix, I've pushed a small follow-up commit on top of your work:
@c-ameron, |
(cherry picked from commit ab8a15b)
…gration test - Switch from SNS (SCP-blocked) to CloudWatch LogGroup - Use static name in template (required for CFN auto-import) - Generate template at test time for unique names per run - Remove unused aws-sns-topic-import.yaml template Auto-import requires a settable primaryIdentifier (not read-only). AWS::Logs::LogGroup (LogGroupName) meets this requirement, unlike AWS::SQS::Queue (QueueUrl) or AWS::SNS::Topic \Ref: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/import-resources-automatically.html
|
The integration test: https://github.com/aws/aws-sam-cli/actions/runs/23670588901 |
| "default", | ||
| False, | ||
| True, | ||
| False, |
There was a problem hiding this comment.
can we add a test for reading this param from the samconfig?
There was a problem hiding this comment.
While the idea of the PR is solid, I think there's a hidden issue with how this will interact with CloudFormation. In the past, the reason we've had difficulty implementing this is because the CloudFormation import feature does not support AWS::Serverless resources. This might have changed, but I think it's because the import step is happening before the transform is server-side. The reason the integration tests are passing is because it's not using a AWS::Serverless resource.
We do this resource import in the AWS Toolkit, but when we do it there we actually deploy the stack as raw CloudFormation before translating it back to SAM. I don't think that would work well in this case considering we're already coming with a user-defined template.
That being said, I think there needs to be changes to warn that this won't work or to not allow it all for serverless resources.
| TEMPLATE = """\ | ||
| AWSTemplateFormatVersion: '2010-09-09' | ||
| Description: Template for testing --import-existing-resources flag. | ||
| Resources: | ||
| TestLogGroup: | ||
| DeletionPolicy: Retain | ||
| Type: AWS::Logs::LogGroup | ||
| Properties: | ||
| LogGroupName: {log_group_name} | ||
| """ |
There was a problem hiding this comment.
I think in other places in the code, we don't have the template inline, but rather in tests/integration/testdata
Pass through option for importing existing resources which allows using
Retain for DeletionPolicy on resources to avoid removal on data
resources if stack needs to be deleted and recreated.
Fixes: #1699