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

AWS::Serverless::StateMachine is broken #3518

Closed
animaxcg opened this issue Jul 18, 2024 · 2 comments · Fixed by #3519
Closed

AWS::Serverless::StateMachine is broken #3518

animaxcg opened this issue Jul 18, 2024 · 2 comments · Fixed by #3519

Comments

@animaxcg
Copy link

animaxcg commented Jul 18, 2024

CloudFormation Lint Version

1.7.0

What operating system are you using?

macos/unix

Describe the bug

AWS::Serverless::StateMachine linting breaks on valid cfn syntax in between 1.5.0 and 1.7.0
3 issues I found:

  1. W1032 warning message syntax is wrong: does not match '^arn:aws:([a-z]|-)+:([a-... aws arns for cn and gov clouds aren't arn:aws

  2. W1032 is given despite it fitting the format given: first example fails on !Sub Fn::Sub:

  3. 2nd fails on Catch block but gives no concrete location of the error and it makes zero sense in the context of the template

Expected behavior

I am not sure I have the syntax 100% optimal but it worked in passed 1.X of cfn-lint and still deploys fine.
Error output to be fixed for warning the show it's checking valid arn I'd and not assume the main aws partition only
Better error if the Catch is indeed an error describe what is wrong and fixing the bug that thinks text is an int

Reproduction template

Transform:
- AWS::Serverless-2016-10-31

Parameters:
  MyLoggroupArn:
    Type: String
Resources:
  resCloneOrderStateMachine:
    Type: AWS::Serverless::StateMachine
    Properties:
      Role:
        Fn::Sub: arn:${AWS::Partition}:iam::${AWS::AccountId}:role/MyRole
      Name: myStateMachine
      Logging:
        Destinations:
          - CloudWatchLogsLogGroup:
              LogGroupArn: !Ref MyLoggroupArn
        Level: ALL
        IncludeExecutionData: True
      Definition: # Example from https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html
        StartAt: FirstState
        States:
          # Happy Path Entrypoint
          FirstState:
            Type: Task
            Resource: arn:aws:lambda:some-region-1:123456789012:function:myFunction #works
            Next: ChoiceState
          ChoiceState:
            Type: Choice
            Choices:
            - Variable: "$.foo"
              NumericEquals: 1
              Next: FirstMatchState
            - Variable: "$.foo"
              NumericEquals: 2
              Next: SecondMatchState
            Default: DefaultState
          FirstMatchState:
            Type: Task
            Resource: #Breaks with W1032
              Fn::Sub: arn:aws:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction 
            Next: NextState
          SecondMatchState:
            Type: Task
            Resource: !Sub arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction #Breaks with W1032
            Next: NextState
          DefaultState:
            Type: Fail
            Error: DefaultStateError
            Cause: No Matches!
          NextState:
            Type: Task
            Resource: arn:aws:lambda:some-region-1:123456789012:function:myFunction
            End: true```



```AWSTemplateFormatVersion: "2010-09-09"
Transform:
- AWS::Serverless-2016-10-31

Parameters:
  MyLoggroupArn:
    Type: String
Resources:
  resCloneOrderStateMachine:
    Type: AWS::Serverless::StateMachine
    Properties:
      Role:
        Fn::Sub: arn:${AWS::Partition}:iam::${AWS::AccountId}:role/MyRole
      Name: myStateMachine
      Logging:
        Destinations:
          - CloudWatchLogsLogGroup:
              LogGroupArn: !Ref MyLoggroupArn
        Level: ALL
        IncludeExecutionData: True
      Definition: # Example from https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html
        StartAt: FirstState
        States:
          # Happy Path Entrypoint
          FirstState:
            Type: Task
            Resource: arn:aws:lambda:some-region-1:123456789012:function:myFunction #works
            Next: ChoiceState
          ChoiceState:
            Type: Choice
            Choices:
            - Variable: "$.foo"
              NumericEquals: 1
              Next: FirstMatchState
            - Variable: "$.foo"
              NumericEquals: 2
              Next: SecondMatchState
            Default: DefaultState
          FirstMatchState:
            Type: Task
            Resource: #Breaks with W1032
              Fn::Sub: arn:aws:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction 
            Next: NextState
          SecondMatchState:
            Type: Task
            Resource: !Sub arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction #Breaks with W1032
            Next: NextState
          DefaultState:
            Type: Fail
            Error: DefaultStateError
            Cause: No Matches!
          NextState:
            Type: Task
            Resource: arn:aws:lambda:some-region-1:123456789012:function:myFunction
            Catch:
              - ErrorEquals:
                  [
                    "States.All",
                    "BadRequestError",
                    "Error",
                    "Lambda.Unknown",
                    "Lambda.TooManyRequestsException",
                    "States.DataLimitExceeded",
                    "States.Runtime"
                  ]
                # Next: "rollback"
                ResultPath: "$.error"
              - ErrorEquals: ["NotFoundError"]
                # Next: "finalize"
                ResultPath: "$.error"
                End: True
            End: true```
            
            
            
            
@kddejong
Copy link
Contributor

I'm going to drop the DefinitionString support on this rule. Little bit confused on what SAM is doing here but shouldn't change the result being a valid template and we are creating false positives. It will be hard to undo what SAM is doing here to do proper validation. Going to remove the false positive for now and approach the other part later.

@kddejong
Copy link
Contributor

This gets translated into

"resCloneOrderStateMachine": {
   "Properties": {
    "DefinitionString": {
     "Fn::Join": [
      "\n",
      [
       "{",
       "    \"StartAt\": \"FirstState\",",
       "    \"States\": {",
       "        \"ChoiceState\": {",
       "            \"Choices\": [",
       "                {",
       "                    \"Next\": \"FirstMatchState\",",
       "                    \"NumericEquals\": 1,",
       "                    \"Variable\": \"$.foo\"",
       "                },",
       "                {",
       "                    \"Next\": \"SecondMatchState\",",
       "                    \"NumericEquals\": 2,",
       "                    \"Variable\": \"$.foo\"",
       "                }",
       "            ],",
       "            \"Default\": \"DefaultState\",",
       "            \"Type\": \"Choice\"",
       "        },",
       "        \"DefaultState\": {",
       "            \"Cause\": \"No Matches!\",",
       "            \"Error\": \"DefaultStateError\",",
       "            \"Type\": \"Fail\"",
       "        },",
       "        \"FirstMatchState\": {",
       "            \"Next\": \"NextState\",",
       "            \"Resource\": \"${definition_substitution_1}\",",
       "            \"Type\": \"Task\"",
       "        },",
       "        \"FirstState\": {",
       "            \"Next\": \"ChoiceState\",",
       "            \"Resource\": \"arn:aws:lambda:some-region-1:123456789012:function:myFunction\",",
       "            \"Type\": \"Task\"",
       "        },",
       "        \"NextState\": {",
       "            \"Catch\": [",
       "                {",
       "                    \"ErrorEquals\": [",
       "                        \"States.All\",",
       "                        \"BadRequestError\",",
       "                        \"Error\",",
       "                        \"Lambda.Unknown\",",
       "                        \"Lambda.TooManyRequestsException\",",
       "                        \"States.DataLimitExceeded\",",
       "                        \"States.Runtime\"",
       "                    ],",
       "                    \"ResultPath\": \"$.error\"",
       "                },",
       "                {",
       "                    \"End\": true,",
       "                    \"ErrorEquals\": [",
       "                        \"NotFoundError\"",
       "                    ],",
       "                    \"ResultPath\": \"$.error\"",
       "                }",
       "            ],",
       "            \"End\": true,",
       "            \"Resource\": \"arn:aws:lambda:some-region-1:123456789012:function:myFunction\",",
       "            \"Type\": \"Task\"",
       "        },",
       "        \"SecondMatchState\": {",
       "            \"Next\": \"NextState\",",
       "            \"Resource\": \"${definition_substitution_2}\",",
       "            \"Type\": \"Task\"",
       "        }",
       "    }",
       "}"
      ]
     ]
    },
    "DefinitionSubstitutions": {
     "definition_substitution_1": {
      "Fn::Sub": "arn:aws:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction"
     },
     "definition_substitution_2": {
      "Fn::Sub": "arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:myFunction"
     }
    },

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 a pull request may close this issue.

2 participants