Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add support for Fn::ForEach replicated resources #845

Merged
merged 9 commits into from
Jan 17, 2025

Conversation

LaikaN57
Copy link
Contributor

@LaikaN57 LaikaN57 commented Jan 16, 2025

Overview

Fixes: #846

 _            _             _   
| |_ __ _ ___| | _____ __ _| |_ 
| __/ _` / __| |/ / __/ _` | __|
| || (_| \__ \   < (_| (_| | |_ 
 \__\__,_|___/_|\_\___\__,_|\__|
                                


version 0.9.55
Not in terminal, reprint now using normal build-in print function.

[WARN   ] : No stacks were created... skipping cleanup.
[ERROR  ] : TypeError list indices must be integers or slices, not str
Traceback (most recent call last):
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_cli.py", line 46, in main
    cli.run()
    ~~~~~~~^^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_cli_core.py", line 316, in run
    return getattr(command(), subcommand)(**args)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_cli_modules/test.py", line 166, in run
    with test:
         ^^^^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/testing/base_test.py", line 58, in __enter__
    raise ex
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/testing/base_test.py", line 55, in __enter__
    self.run()
    ~~~~~~~~^^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/testing/_cfn_test.py", line 83, in run
    templates = self.config.get_templates()
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_config.py", line 416, in get_templates
    templates[test_name] = Template(
                           ~~~~~~~~^
        template_path=self.project_root / test.template,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ...<2 lines>...
        template_cache=tcat_template_cache,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_cfn/template.py", line 69, in __init__
    self._find_children()
    ~~~~~~~~~~~~~~~~~~~^^
  File "/Users/akennedy/src/[REDACTED]/.venv/lib/python3.13/site-packages/taskcat/_cfn/template.py", line 158, in _find_children
    if resource["Type"] == "AWS::CloudFormation::Stack":
       ~~~~~~~~^^^^^^^^
TypeError: list indices must be integers or slices, not str

Adds support for Fn::ForEach replicated resources.

This fixes a crash bug when you are using a Fn::ForEach in the Resources section of a template.

It also adds the ability for taskcat to inspect inside of a Fn::ForEach in the Resources section of a template to see if there are any replicates resources that are of Type: AWS::Cloudformation::Stack so we can add those as children as well.

Fn::ForEach replicated resources example

Testing/Steps taken to ensure quality

I tested the alternative version with a large RDS stack containing multiple RDS instances deployed via a Fn::ForEach block and it also deploys other resource that are of Type: AWS::Cloudformation::Stack (outside of the Fn::ForEach block).

I have yet to test the proposed fix.

Notes

Alternatively, we could just ignore Fn::ForEach blocks entirely like this.

        for resource in self.template["Resources"].keys():
            if "Fn::ForEach::" in resource:
                continue
            resource = self.template["Resources"][resource]

Testing Instructions

  • Create a template with a nested stack inside of a Fn::ForEach block in the Resources section
  • Ensure the nested stack is tracked in the report (?)

taskcat/_cfn/template.py Outdated Show resolved Hide resolved
Comment on lines +157 to +166
if replicated_resource["Type"] == "AWS::CloudFormation::Stack":
child_name = self._template_url_to_path(
template_url=replicated_resource["Properties"][
"TemplateURL"
],
)
# print(child_name)
if child_name:
# for child_url in child_name:
children.add(child_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stolen from below.

taskcat/_cfn/template.py Outdated Show resolved Hide resolved
@LaikaN57
Copy link
Contributor Author

@andrew-glenn I would be more than happy to add unit tests as well. I am just a bit confused about the appropriate file to modify to add my tests (or alternatively, what dir and file naming I should use). Any advice is appreciated!

@andrew-glenn
Copy link
Collaborator

@LaikaN57 that would be most appreciated. I'm setting a reminder to follow up on this tomorrow. I wanted to quickly acknowledge your offer even though I don't have the bandwidth to respond further today

@LaikaN57
Copy link
Contributor Author

(for tracking, AWS Case #173701840400885)

@andrew-glenn
Copy link
Collaborator

I found some surprise bandwidth!

I think putting your unit tests in https://github.com/aws-ia/taskcat/blob/main/tests/test_cfn_template.py is adequate. From my perspective it'd just be a second testclass and then the happy-path and sad-path templates would go under tests/data/template-foreach or something similar.

Copy link

Static analysis has failed. Please review and take action as appropriate.

@LaikaN57
Copy link
Contributor Author

LaikaN57 commented Jan 17, 2025

I added a test case by just copying and modifying the existing test case as it seemed to generally test for what we want to look for in the Fn::ForEach tests as well.

diff -bur tests/data/nested-fail/.taskcat.yml tests/data/fn-foreach-resource-fail/.taskcat.yml
--- tests/data/nested-fail/.taskcat.yml 2025-01-16 00:25:03
+++ tests/data/fn-foreach-resource-fail/.taskcat.yml    2025-01-16 17:43:32
@@ -1,7 +1,7 @@
 ---
 project:
   owner: owner@company.com
-  name: nested-fail
+  name: fn-foreach-resource-fail
   regions:
     - us-east-1
     - us-west-2
diff -bur tests/data/nested-fail/templates/test.template.yaml tests/data/fn-foreach-resource-fail/templates/test.template.yaml
--- tests/data/nested-fail/templates/test.template.yaml 2025-01-16 00:25:03
+++ tests/data/fn-foreach-resource-fail/templates/test.template.yaml    2025-01-16 17:54:24
@@ -1,4 +1,5 @@
 AWSTemplateFormatVersion: '2010-09-09'
+Transform: AWS::LanguageExtensions
 Metadata:
   taskcat:
     project:
@@ -12,13 +13,19 @@
     Default: aws-quickstart
     Type: String
   QSS3KeyPrefix:
-    Default: nested-fail/
+    Default: fn-foreach-resource-fail/
     Type: String
 Resources:
-  ChildStack:
+  Fn::ForEach::ChildStackX:
+    - Identifier
+    - - "1"
+    - ChildStack${Identifier}:
         Type: AWS::CloudFormation::Stack
         Properties:
           TemplateURL: !Sub 'https://${QSS3BucketName}.s3.amazonaws.com/${QSS3KeyPrefix}templates/test.template_middle.yaml'
           Parameters:
             QSS3KeyPrefix: !Ref QSS3KeyPrefix
             QSS3BucketName: !Ref QSS3BucketName
+          Tags:
+            - Key: child-stack-foreach-id
+              Value: !Sub "id-${Identifier}"
diff -bur tests/data/nested-fail/templates/test.template_middle.yaml tests/data/fn-foreach-resource-fail/templates/test.template_middle.yaml
--- tests/data/nested-fail/templates/test.template_middle.yaml  2025-01-16 00:25:03
+++ tests/data/fn-foreach-resource-fail/templates/test.template_middle.yaml     2025-01-16 17:54:20
@@ -1,16 +1,23 @@
 AWSTemplateFormatVersion: '2010-09-09'
+Transform: AWS::LanguageExtensions
 Parameters:
   QSS3BucketName:
     Default: aws-quickstart
     Type: String
   QSS3KeyPrefix:
-    Default: nested-fail/
+    Default: fn-foreach-resource-fail/
     Type: String
 Resources:
-  ChildStack:
+  Fn::ForEach::ChildStackX:
+    - Identifier
+    - - "1"
+    - ChildStack${Identifier}:
         Type: AWS::CloudFormation::Stack
         Properties:
           TemplateURL: !Sub 'https://${QSS3BucketName}.s3.amazonaws.com/${QSS3KeyPrefix}templates/test.template_inner.yaml'
+          Tags:
+            - Key: child-stack-foreach-id
+              Value: !Sub "id-${Identifier}"
   Middle2Stack:
     Type: AWS::CloudFormation::Stack
     Properties:
diff -bur tests/data/nested-fail/templates/test.template_middle2.yaml tests/data/fn-foreach-resource-fail/templates/test.template_middle2.yaml
--- tests/data/nested-fail/templates/test.template_middle2.yaml 2025-01-16 00:25:03
+++ tests/data/fn-foreach-resource-fail/templates/test.template_middle2.yaml    2025-01-16 17:43:32
@@ -4,7 +4,7 @@
     Default: aws-quickstart
     Type: String
   QSS3KeyPrefix:
-    Default: nested-fail/
+    Default: fn-foreach-resource-fail/
     Type: String
 Resources:
   ChildStack:
diff -bur tests/data/nested-fail/templates/test.template_middle3.yaml tests/data/fn-foreach-resource-fail/templates/test.template_middle3.yaml
--- tests/data/nested-fail/templates/test.template_middle3.yaml 2025-01-16 00:25:03
+++ tests/data/fn-foreach-resource-fail/templates/test.template_middle3.yaml    2025-01-16 17:43:32
@@ -4,7 +4,7 @@
     Default: aws-quickstart
     Type: String
   QSS3KeyPrefix:
-    Default: nested-fail/
+    Default: fn-foreach-resource-fail/
     Type: String
 Resources:
   ChildStack:

@andrew-glenn
Copy link
Collaborator

/do-e2e-tests

Copy link

End to end test has been scheduled

Copy link

E2E tests in progress

Copy link

@aws-ia-automator-prod aws-ia-automator-prod bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E2E tests completed successfully

@andrew-glenn
Copy link
Collaborator

@LaikaN57 I assumed you were done since linting was happy and you added a test case.

Please confirm and I'll give this a review over coffee in the morning. Generally I'm encouraged because linting and integ is happy.

@@ -14,3 +14,17 @@ def test_init(self):
template = templates["taskcat-json"]
self.assertEqual(1, len(template.children))
self.assertEqual(4, len(template.descendents))


class TestCfnTemplateForEachResource(unittest.TestCase):
Copy link
Contributor Author

@LaikaN57 LaikaN57 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially a copy-paste of the above but we just change out the dataset we are running against.

Sorry if this is rudimentary. I am not a great test writer and defaulted back to just pattern matching. However, I do believe we have pretty complete coverage sans the implicit else on the if child_name statement.

@LaikaN57
Copy link
Contributor Author

@andrew-glenn yup, im happy if ur happy!

@andrew-glenn
Copy link
Collaborator

I'm merging this in. A new release should be cut later today.

@andrew-glenn andrew-glenn merged commit fc5fcff into aws-ia:main Jan 17, 2025
4 checks passed
@LaikaN57 LaikaN57 deleted the fix-res-fnforeach branch January 17, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fn::ForEach replicated resources crashes
2 participants